batman-adv: Correct sparse warning about different lock contexts for basic block

Message ID 1288166545-8252-1-git-send-email-sven.eckelmann@gmx.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Sven Eckelmann Oct. 27, 2010, 8:02 a.m. UTC
  sparse noticed that if_list_lock is taken in some situations with bottom
halfes disabled in some SMP kernels and that we must always lock it with
spin_lock_bh.

Signed-off-by: Sven Eckelmann <sven.eckelmann@gmx.de>
---
 batman-adv/hard-interface.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
  

Comments

Sven Eckelmann Oct. 28, 2010, 9:49 a.m. UTC | #1
On Wednesday 27 October 2010 10:02:25 Sven Eckelmann wrote:
> sparse noticed that if_list_lock is taken in some situations with bottom
> halfes disabled in some SMP kernels and that we must always lock it with
> spin_lock_bh.

I would like to reject that patch by myself. Not that it does something bad, 
but more that it does not make any sense what sparse says. I've looked through 
the 2.6.32 smp code and couldn't locate the real problem. So if anyone has a 
opinion or can show were the actual problem is... please tell me.

My current test was done like that (on debian squeeze i386):
-----------------------------------------------------------
$ cd $MY_v2.6.32
$ make mrproper
$ make allnoconfig
$ grep -v 'CONFIG_MODULES is not set' .config > .config.tmp; mv .config.tmp \
 .config
$ grep -v 'CONFIG_NET is not set' .config > .config.tmp; mv .config.tmp \
 .config
$ grep -v 'CONFIG_SMP is not set' .config > .config.tmp; mv .config.tmp \
 .config
$ grep -v 'CONFIG_MODULE_UNLOAD is not set' .config > .config.tmp; mv \
 .config.tmp .config
$ echo 'CONFIG_MODULES=y' >> .config
$ echo 'CONFIG_NET=y' >> .config
$ echo 'CONFIG_SMP=y' >> .config
$ echo 'CONFIG_MODULE_UNLOAD=y' >> .config
$ echo 'xy'|make menuconfig
$ make prepare
$ make modules


$ mkdir test
$ cd test
$ cat << EOF > Makefile
obj-m += test.o
test-y += main.o
EOF
$ cat << EOF > main.c
#include <linux/module.h>
#include <linux/spinlock.h>

static DEFINE_SPINLOCK(testlock);

static int __init test_init(void)
{
	spin_lock(&testlock);
	spin_unlock(&testlock);
	return 0;
}

static void __exit test_exit(void)
{
}

module_init(test_init);
module_exit(test_exit);
EOF


$ make CC=cgcc -C $MY_v2.6.32/linux-next M=$(pwd) modules          
make: Entering directory `$MY_v2.6.32'
  CC [M]  $MY_v2.6.32/testmodule/main.o
$MY_v2.6.32/test/main.c:6:19: warning: context imbalance in 'test_init' - 
wrong count at exit
  LD [M]  $MY_v2.6.32/test/test.o
  Building modules, stage 2.
  MODPOST 1 modules
  LD [M]  $MY_v2.6.32/test/test.ko
make: Leaving directory `$MY_v2.6.32'
-----------------------------------------------------------

The sparse version was v0.4.3.

I will post a question regarding that problem on the sparse mailinglist

Best regards,
	Sven
  

Patch

diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index 37f0f8b..b5514e3 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -441,9 +441,9 @@  static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 
 	check_known_mac_addr(batman_if->net_dev->dev_addr);
 
-	spin_lock(&if_list_lock);
+	spin_lock_bh(&if_list_lock);
 	list_add_tail_rcu(&batman_if->list, &if_list);
-	spin_unlock(&if_list_lock);
+	spin_unlock_bh(&if_list_lock);
 
 	/* extra reference for return */
 	kref_get(&batman_if->refcount);
@@ -478,11 +478,11 @@  void hardif_remove_interfaces(void)
 	struct batman_if *batman_if, *batman_if_tmp;
 
 	rtnl_lock();
-	spin_lock(&if_list_lock);
+	spin_lock_bh(&if_list_lock);
 	list_for_each_entry_safe(batman_if, batman_if_tmp, &if_list, list) {
 		hardif_remove_interface(batman_if);
 	}
-	spin_unlock(&if_list_lock);
+	spin_unlock_bh(&if_list_lock);
 	rtnl_unlock();
 }
 
@@ -508,9 +508,9 @@  static int hard_if_event(struct notifier_block *this,
 		hardif_deactivate_interface(batman_if);
 		break;
 	case NETDEV_UNREGISTER:
-		spin_lock(&if_list_lock);
+		spin_lock_bh(&if_list_lock);
 		hardif_remove_interface(batman_if);
-		spin_unlock(&if_list_lock);
+		spin_unlock_bh(&if_list_lock);
 		break;
 	case NETDEV_CHANGEMTU:
 		if (batman_if->soft_iface)