[PATCH] (1/11) Irda dongle module owner support

[PATCH] (1/11) Irda dongle module owner support

[Stephen Hemminger]

IRDA dongle interface needed to be converted to have an owner field
to avoid races on module unload.

Eliminated the use of hashbin locking because the dongle control
code needed to do it's own locking to avoid races.   This also closed
the race between find and insert. 

The find/insert hashbin race may be a general problem with all the IRDA
hashbin stuff. IMHO the hashbin stuff should be replaced, it is full
of dead incomplete code and done better by the list macros.

diff -Nru a/include/net/irda/irda_device.h b/include/net/irda/irda_device.h
--- a/include/net/irda/irda_device.h	Thu Oct  2 14:51:18 2003
+++ b/include/net/irda/irda_device.h	Thu Oct  2 14:51:18 2003
@@ -128,6 +128,7 @@
 	void (*close)(dongle_t *dongle);
 	int  (*reset)(struct irda_task *task);
 	int  (*change_speed)(struct irda_task *task);
+	struct module *owner;
 };
 
 /* 
diff -Nru a/net/irda/irda_device.c b/net/irda/irda_device.c
--- a/net/irda/irda_device.c	Thu Oct  2 14:51:18 2003
+++ b/net/irda/irda_device.c	Thu Oct  2 14:51:18 2003
@@ -58,6 +58,7 @@
 static void __irda_task_delete(struct irda_task *task);
 
 static hashbin_t *dongles = NULL;
+static spinlock_t dongle_lock = SPIN_LOCK_UNLOCKED;
 static hashbin_t *tasks = NULL;
 
 const char *infrared_mode[] = {
@@ -85,7 +86,7 @@
 
 int __init irda_device_init( void)
 {
-	dongles = hashbin_new(HB_LOCK);
+	dongles = hashbin_new(HB_NOLOCK);
 	if (dongles == NULL) {
 		printk(KERN_WARNING "IrDA: Can't allocate dongles hashbin!\n");
 		return -ENOMEM;
@@ -424,25 +425,35 @@
 dongle_t *irda_device_dongle_init(struct net_device *dev, int type)
 {
 	struct dongle_reg *reg;
-	dongle_t *dongle;
+	dongle_t *dongle = NULL;
 
 	ASSERT(dev != NULL, return NULL;);
+	ASSERT(!in_interrupt(), return NULL;);
+
+	spin_lock(&dongle_lock);
+	reg = hashbin_find(dongles, type, NULL);
 
 #ifdef CONFIG_KMOD
-	ASSERT(!in_interrupt(), return NULL;);
 	/* Try to load the module needed */
-	request_module("irda-dongle-%d", type);
+	if (!reg && capable(CAP_SYS_MODULE)) {
+		spin_unlock(&dongle_lock);
+	
+		request_module("irda-dongle-%d", type);
+		
+		spin_lock(&dongle_lock);
+		reg = hashbin_find(dongles, type, NULL);
+	}
 #endif
 
-	if (!(reg = hashbin_lock_find(dongles, type, NULL))) {
-		ERROR("IrDA: Unable to find requested dongle\n");
-		return NULL;
+	if (!reg || !try_module_get(reg->owner) ) {
+		ERROR("IrDA: Unable to find requested dongle type %x\n", type);
+		goto out;
 	}
 
 	/* Allocate dongle info for this instance */
 	dongle = kmalloc(sizeof(dongle_t), GFP_KERNEL);
 	if (!dongle)
-		return NULL;
+		goto out;
 
 	memset(dongle, 0, sizeof(dongle_t));
 
@@ -450,6 +461,8 @@
 	dongle->issue = reg;
 	dongle->dev = dev;
 
+ out:
+	spin_unlock(&dongle_lock);
 	return dongle;
 }
 
@@ -461,7 +474,7 @@
 	ASSERT(dongle != NULL, return -1;);
 
 	dongle->issue->close(dongle);
-
+	module_put(dongle->issue->owner);
 	kfree(dongle);
 
 	return 0;
@@ -472,14 +485,16 @@
  */
 int irda_device_register_dongle(struct dongle_reg *new)
 {
+	spin_lock(&dongle_lock);
 	/* Check if this dongle has been registered before */
-	if (hashbin_lock_find(dongles, new->type, NULL)) {
-		MESSAGE("%s: Dongle already registered\n", __FUNCTION__);
-                return 0;
-        }
-
-	/* Insert IrDA dongle into hashbin */
-	hashbin_insert(dongles, (irda_queue_t *) new, new->type, NULL);
+	if (hashbin_find(dongles, new->type, NULL)) {
+		MESSAGE("%s: Dongle type %x already registered\n", 
+			__FUNCTION__, new->type);
+        } else {
+		/* Insert IrDA dongle into hashbin */
+		hashbin_insert(dongles, (irda_queue_t *) new, new->type, NULL);
+	}
+	spin_unlock(&dongle_lock);
 
         return 0;
 }
@@ -494,11 +509,11 @@
 {
 	struct dongle *node;
 
+	spin_lock(&dongle_lock);
 	node = hashbin_remove(dongles, dongle->type, NULL);
-	if (!node) {
+	if (!node) 
 		ERROR("%s: dongle not found!\n", __FUNCTION__);
-		return;
-	}
+	spin_unlock(&dongle_lock);
 }
 
 /*

Re: [PATCH] (1/11) Irda dongle module owner support

[Jean Tourrilhes]

On Thu, Oct 02, 2003 at 03:20:26PM -0700, Stephen Hemminger wrote:
>
> IRDA dongle interface needed to be converted to have an owner field
> to avoid races on module unload.

	Yep, this code was broken. At this point, we were supposed to
use the new dongle stuff of Martin, wich I think is correct, but it
didn't happened.

> Eliminated the use of hashbin locking because the dongle control
> code needed to do it's own locking to avoid races.   This also closed
> the race between find and insert. 

	Yep, that's the right approach.

> The find/insert hashbin race may be a general problem with all the IRDA
> hashbin stuff.

	This is clearly commented in the hashbin code (big block of
comments). Note that this problem is not a problem of hashbin
themselves, because there is only so much you can do there, but more
about how you use hashbins.
	This is why over the last year a lot of critical IrDA code has
migrated to HB_NOLOCK or/and use external locking, and therefore the
situation is not as bad as it looks. Do a "grep hb_spinlock *" to
confirm this (or check my web page) ;-)
	For the reason mentionned above, the dongle code and the task
code were not upgraded.

> IMHO the hashbin stuff should be replaced, it is full
> of dead incomplete code and done better by the list macros.

	I somehow agree with that (check my comments on
hashbin.c). However, as the majority of locking issues have been
addressed during 2.5.X, it's not as critical, and as long as it
works...

> +static spinlock_t dongle_lock = SPIN_LOCK_UNLOCKED;

	The usual IrDA convention is to reuse &dongle->hb_spinlock
rather than adding a new variable. Less bloat.

> +	spin_lock(&dongle_lock);

	I wonder if you need to lock BH as well. I'm not sure if all
the dongles call are guaranteed to come from user space. You don't want to introduce nasty deadlocks ;-)

> -	ASSERT(!in_interrupt(), return NULL;);

	Hum... My recollections is that calling request_module with
interrupt disable was guaranteed to crash. But that was with the "old"
module code.
	I would prefer if you leave this stuff in, it helps debugging.

	Have fun...

	Jean

Re: [PATCH] (1/11) Irda dongle module owner support

[David S. Miller]

Jean noted some problems he has with these changes, in particular
the hashbin locking bits.

So I'm going to wait until that stuff is resolved since all the other
IRDA patches in this series depend upon this first dongle module owner
infrastructure one.

Or did I misinterpret what happened?

[PATCH] (1/11) Irda dongle module owner support (revised)

[Stephen Hemminger]

Revised version of the dongle module owner patch, incorporating Jean's comments.
This replaces the original patch.  It provides an owner field an appropriate 
ref counting for IRDA dongles.

Changes since list version:
	s/dongle_lock/dongles->hb_spinlock/
	get lock on device_cleanup
	replace ASSERT() about in_interrupt with might_sleep().

In case your curious about locking, the callers are:
	module_init -> irda_device_init
	module_exit -> irda_device_cleanup
	irport_net_ioctl -> irda_device_dongle_init
	module_init -> XXX_dongle_init -> irda_device_register_dongle
	module_exit -> XXX_dongle_exit -> irda_device_unregister_dongle

In other words, no interrupt or BH access to the dongle list.
Obviously, hashing this list is overkill, but "when in Rome"...

--- linux-2.5/net/irda/irda_device.c	2003-10-01 13:40:11.000000000 -0700
+++ irda-2.5/net/irda/irda_device.c	2003-10-03 10:03:52.497879915 -0700
@@ -85,7 +85,7 @@ static void irda_task_timer_expired(void
 
 int __init irda_device_init( void)
 {
-	dongles = hashbin_new(HB_LOCK);
+	dongles = hashbin_new(HB_NOLOCK);
 	if (dongles == NULL) {
 		printk(KERN_WARNING "IrDA: Can't allocate dongles hashbin!\n");
 		return -ENOMEM;
@@ -109,7 +109,9 @@ void __exit irda_device_cleanup(void)
 	IRDA_DEBUG(4, "%s()\n", __FUNCTION__);
 
 	hashbin_delete(tasks, (FREE_FUNC) __irda_task_delete);
+	spin_lock(&dongles->hb_spinlock);
 	hashbin_delete(dongles, NULL);
+	spin_unlock(&dongles->hb_spinlock);
 }
 
 /*
@@ -424,25 +426,34 @@ int irda_device_txqueue_empty(struct net
 dongle_t *irda_device_dongle_init(struct net_device *dev, int type)
 {
 	struct dongle_reg *reg;
-	dongle_t *dongle;
+	dongle_t *dongle = NULL;
 
-	ASSERT(dev != NULL, return NULL;);
+	might_sleep();
+
+	spin_lock(&dongles->hb_spinlock);
+	reg = hashbin_find(dongles, type, NULL);
 
 #ifdef CONFIG_KMOD
-	ASSERT(!in_interrupt(), return NULL;);
 	/* Try to load the module needed */
-	request_module("irda-dongle-%d", type);
+	if (!reg && capable(CAP_SYS_MODULE)) {
+		spin_unlock(&dongles->hb_spinlock);
+	
+		request_module("irda-dongle-%d", type);
+		
+		spin_lock(&dongles->hb_spinlock);
+		reg = hashbin_find(dongles, type, NULL);
+	}
 #endif
 
-	if (!(reg = hashbin_lock_find(dongles, type, NULL))) {
-		ERROR("IrDA: Unable to find requested dongle\n");
-		return NULL;
+	if (!reg || !try_module_get(reg->owner) ) {
+		ERROR("IrDA: Unable to find requested dongle type %x\n", type);
+		goto out;
 	}
 
 	/* Allocate dongle info for this instance */
 	dongle = kmalloc(sizeof(dongle_t), GFP_KERNEL);
 	if (!dongle)
-		return NULL;
+		goto out;
 
 	memset(dongle, 0, sizeof(dongle_t));
 
@@ -450,6 +461,8 @@ dongle_t *irda_device_dongle_init(struct
 	dongle->issue = reg;
 	dongle->dev = dev;
 
+ out:
+	spin_unlock(&dongles->hb_spinlock);
 	return dongle;
 }
 
@@ -461,7 +474,7 @@ int irda_device_dongle_cleanup(dongle_t 
 	ASSERT(dongle != NULL, return -1;);
 
 	dongle->issue->close(dongle);
-
+	module_put(dongle->issue->owner);
 	kfree(dongle);
 
 	return 0;
@@ -472,14 +485,16 @@ int irda_device_dongle_cleanup(dongle_t 
  */
 int irda_device_register_dongle(struct dongle_reg *new)
 {
+	spin_lock(&dongles->hb_spinlock);
 	/* Check if this dongle has been registered before */
-	if (hashbin_lock_find(dongles, new->type, NULL)) {
-		MESSAGE("%s: Dongle already registered\n", __FUNCTION__);
-                return 0;
-        }
-
-	/* Insert IrDA dongle into hashbin */
-	hashbin_insert(dongles, (irda_queue_t *) new, new->type, NULL);
+	if (hashbin_find(dongles, new->type, NULL)) {
+		MESSAGE("%s: Dongle type %x already registered\n", 
+			__FUNCTION__, new->type);
+        } else {
+		/* Insert IrDA dongle into hashbin */
+		hashbin_insert(dongles, (irda_queue_t *) new, new->type, NULL);
+	}
+	spin_unlock(&dongles->hb_spinlock);
 
         return 0;
 }
@@ -494,11 +509,11 @@ void irda_device_unregister_dongle(struc
 {
 	struct dongle *node;
 
+	spin_lock(&dongles->hb_spinlock);
 	node = hashbin_remove(dongles, dongle->type, NULL);
-	if (!node) {
+	if (!node) 
 		ERROR("%s: dongle not found!\n", __FUNCTION__);
-		return;
-	}
+	spin_unlock(&dongles->hb_spinlock);
 }
 
 /*

Re: [PATCH] (1/11) Irda dongle module owner support (revised)

[Jean Tourrilhes]

On Fri, Oct 03, 2003 at 10:21:33AM -0700, Stephen Hemminger wrote:
> Revised version of the dongle module owner patch, incorporating Jean's comments.
> This replaces the original patch.  It provides an owner field an appropriate 
> ref counting for IRDA dongles.
> 
> Changes since list version:
> 	s/dongle_lock/dongles->hb_spinlock/
> 	get lock on device_cleanup
> 	replace ASSERT() about in_interrupt with might_sleep().

	Perfect ! Thanks for the quick turnaround, and sorry for the
extra work.

> In case your curious about locking, the callers are:
> 	module_init -> irda_device_init
> 	module_exit -> irda_device_cleanup
> 	irport_net_ioctl -> irda_device_dongle_init
> 	module_init -> XXX_dongle_init -> irda_device_register_dongle
> 	module_exit -> XXX_dongle_exit -> irda_device_unregister_dongle
> 
> In other words, no interrupt or BH access to the dongle list.
> Obviously, hashing this list is overkill,

	Ok.

> but "when in Rome"...

	Eat some pasta ?
	Maintaining others people code is very much like archeological
excavation, sometimes you find treasures buried in the various
stratas, but most of the time...

	Have fun...

	Jean

Re: [PATCH] (1/11) Irda dongle module owner support (revised)

[David S. Miller]

On Fri, 3 Oct 2003 10:21:33 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> Revised version of the dongle module owner patch, incorporating Jean's comments.
> This replaces the original patch.  It provides an owner field an appropriate 
> ref counting for IRDA dongles.

I applied this, but where's the necessary IRDA header file change
that adds the 'owner' field to the dongle_reg structure?

It's missing from this patch.

Please send me that, and then I assume that I can just suck in all
of your individual IRDA driver patches from yesterday?

Re: [PATCH] (1/11) Irda dongle module owner support

[Jean Tourrilhes]

On Thu, Oct 02, 2003 at 03:20:26PM -0700, Stephen Hemminger wrote:
> IRDA dongle interface needed to be converted to have an owner field
> to avoid races on module unload.
> 
> Eliminated the use of hashbin locking because the dongle control
> code needed to do it's own locking to avoid races.   This also closed
> the race between find and insert. 
> 
> The find/insert hashbin race may be a general problem with all the IRDA
> hashbin stuff. IMHO the hashbin stuff should be replaced, it is full
> of dead incomplete code and done better by the list macros.

	Hi,

	More testing on 2.6.0-test6-bk8 on my SMP box...
	After modprobing irport, a modprobe of the actisys module
hangs forever.

	Conditions : irport was not yet attached/up. I had irdadump
running and an irda-usb active, and I had just rmmod sir_dev.
	I can see that modprobe is taking 100% cpu :
---------------------------------------
 1786 pts/1    R      4:11 modprobe actisys
---------------------------------------
	I can't kill it !
	No messages in the log.
---------------------------------------
# cat /proc/modules 
actisys 4032 1 - Loading 0xd0859000
irport 12672 0 - Live 0xd0884000
irda 182260 5 actisys,irport,irnet,irda_usb, Live 0xd08d3000
---------------------------------------

	After a clean reboot, I just tried to modprobe actisys even
before modprobing irport. Same result :
--------------------------------
# cat /proc/modules 
actisys 4032 1 - Loading 0xd0859000
irda 182260 1 actisys, Live 0xd08d3000
--------------------------------

	Any tip for debugging that ?

	Thanks...

	Jean