[rt2x00-users] [PATCH 3/3] rt2800usb: read TX_STA_FIFO asynchronously

[Johannes Stezenbach]

Hi,

On Sun, Dec 12, 2010 at 04:50:22PM +0100, Ivo Van Doorn wrote:
> 
> >> > TODO:
> >> >  - add a list for the URBs in flight so we can
> >> >   kill them on device unplug.  rtl818x uses struct usb_anchor
> >> >   to do that, but I suppose I cannot put USB stuff into
> >> >   rt2x00.h.  How about adding a "void *priv;" to struct rt2x00_dev?
> >>
> >> But at the moment we are already killing all urbs which are in the air.
> >> We don't need any fancy stuff for that, since we have a queue,
> >> with index pointers pointing exactly to the URBs which are in the air.
> >>
> >> During unplug the rt2x00usb will loop through the queue entries,
> >> and kill all pending urbs.
> >
> > Ah, yes, but the URBs submitted by rt2x00usb_register_read_async()
> > are not on any queue, so currently they cannot be killed.
> > I've seen it crash on unplug so it needs to be fixed.
> 
> Ah ok. But aren't the URB's not returned automatically as well
> and they then fail with the ENODEV URB status.

I think you are right, and with the current code I cannot reproduce
the Oops on unplug I saw earlier anymore.  It might be that I missed
to add the urb_status check in an early version of the patch.

However, I think there is a potential problem:

static void rt2800usb_tx_sta_fifo_read_completed(struct rt2x00_dev *rt2x00dev,
						 int urb_status, u32 tx_status)
{
	if (urb_status) {
		WARNING(rt2x00dev, "rt2x00usb_register_read_async failed: %d\n", urb_status);
		return;
	}

The WARNING accesses wiphy_name((__dev)->hw->wiphy) which I'm not sure
is still valid when this callback is executed on unplug.  However, in my current
unplug tests the warning prints fine:

[ 3360.287425] phy0 -> rt2800usb_tx_sta_fifo_read_completed: Warning - rt2x00usb_register_read_async failed: -71

Should I keep it or use a plain printk()?


Thanks
Johannes