Re: [PATCH 2.5.3] wavelan_cs.c : new WE api

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

On Mon, Feb 04, 2002 at 09:36:18PM -0500, Jeff Garzik wrote:
> Comments pertaining to all three of wavelan, wavelan_cs, and netwave_cs:
> * wv_splhi should really just be spin_lock_irqsave.  calling
> spin_lock_irqsave with 'flags' from another function is non-portable. 
> doing so to an inline function is just barely portable, and is
> discouraged :)

	We will have to agree to disagree. I won't oppose a patch (as
long as the driver still works after it).

> * I still see a couple save_flags/restore_flags in there...

	Of course, I haven't removed them, just moved them around.

	Old code (simplified) :
---------------------------
xxx_ioctl()
{
	save_flags();
	switch(cmd) {
		[...]
		copy_from_user(extra, ...);
		outsb(..., extra);
		[...]
	}
	restore_flags();
}
----------------------------
	Alan told me that this is a no-no.

	New code :
-----------------------
xxx_set_xxx(... , char *extra)
{
	save_flags();
	[...]
	outsb(..., extra);
	[...]
	restore_flags();
}
-----------------------
	copy_to/from_user is handled before reaching the wireless
handler. What is nice is that the new API *enforce* the proper
behaviour.

	Actually, you may want to add that to the list of cleanups for
the kernel janitors, check that all copy_to/from_user in device ioctl
functions are not done with irq disabled. Actually, I think it was
mostly in Wireless drivers...

> otherwise looks ok to me.

	Good. If I understood the new "official" procedure from Linus
himself, you are supposed to forward my patches to hin ;-)

> Jeff Garzik

	Have fun...

	Jean