[OLPC-devel] libertas driver usb issues

[Marcelo Tosatti]

Dan,

On Mon, Jun 26, 2006 at 07:58:52AM -0400, Dan Williams wrote:
> Hi,
> 
> One thing I noticed over the weekend was that you can't hot-unplug the
> card without rmmod-ding the driver first.  Otherwise, you get a NULL
> pointer dereference in this call chain:
> 
> wlan_cleanup_module()
> sbi_unregister()
> PrepareAndSendCommand()
>   BUG kernel NULL pointer dereference
> 
> Here's the problem;  to me, it looks like there's some really dodgy
> stuff going on in if_usb.c WRT static global variables there.  For
> instance, at the top see:
> 
> static wlan_private *pwlanpriv;
> *) The _only_ reason pwlanpriv is kept around as a global is so that
> command in sbi_unregister() can get run, which resets the card when the
> module gets unplugged. 
> 
> static struct usb_card_rec usb_cardp;
> *) usb_cardp gets kept around to preserve the interface abstraction
> between wlan_main.c and if_usb.c, presumably because you can use other
> interfaces like CF/MMC or whatever.
> 
> The reason the pointer deref happens above is that if you remove the
> card before rmmod-ding the driver, a bunch of hot-unplug stuff
> (correctly) gets run, which pretty much invalidates the stored global
> pwlanpriv.  Thus, when sbi_unregister() runs at module exit, there's no
> card to reset.  This should probably get moved to the card probe stage
> or something.

Right. There's also the problem that, on the OLPC boards, the firmware
download is not working reliably. We probably have to reset the chip
somehow.

> These globals should probably be removed and refactored into the
> device's usb_intfdata() (which is actually already used to store
> _another_ wlan_private structure for the card).  Some work necessary
> here.
> 
> Furthermore, it doesn't look like we could ever attach > 1 marvel USB
> device and have this driver work?

Right, there are several global status variables used for all present
devices (!).

The plan is to remove command processing from the main thread, and      
perform it via schedule_work(), removing the global state variables 
such as "usb_int_cause" etc.

Had not noticed pwlanpriv before though.