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

[Jean Tourrilhes <jt@bougret.hpl.hp.com>]

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