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

[Johannes Stezenbach]

Hi Ivo,

On Sun, Dec 12, 2010 at 02:36:39PM +0100, Ivo Van Doorn wrote:
> Hi,
> 
> > Trying to fix the "TX status report missed" warnings
> > by reading the TX_STA_FIFO entries as quickly as possible.
> > The TX_STA_FIFO is too small in hardware, thus reading
> > it only from the workqueue is too slow and entries get lost.
> 
> And this patch reduces the number of warnings that you get?
> I mean I am not using rt2800usb on embedded hardware, and
> I get the warning as well...

Yes, with this patch there are almost no warnings anymore,
even when running iperf TX load on the embedded board.

> Just a thought, as I am not really seeing it happening in the
> patch here. But if we have rt2800usb using the FIFO queue,
> we can move stuff into rt2800lib again since some code can probably
> be shared between rt2800pci and rt2800usb.

OK, I'll check if the code can be shared.  It looked to me like Helmut's
changes make it less likely that something can be shared, but I did not
really check in detail since both his and my code are still
in development.

> > 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.

> > +                       WARNING(rt2x00dev, "TX status FIFO overrun,"
> > +                               "drop tx status report.\n");
> 
> Small nitpick, place add a space after the comma. ;)

OK

> > +               else {
> > +                       /* immediately try to read the next FIFO entry */
> > +                       rt2x00usb_register_read_async(rt2x00dev, TX_STA_FIFO,
> > +                                                     rt2800usb_tx_sta_fifo_read_completed);
> > +                       ieee80211_queue_work(rt2x00dev->hw, &rt2x00dev->txdone_work);
> 
> I think the queue_work command should be moved 1 level up in this if-statement.
> If we read 10 valid TX_STATUS reports, we don't want to schedule the
> work structure 10 times.
> So perhaps it is better to only schedule when TX_STA_FIFO_VALID is
> false _and_ we
> have items in the fifo queue.

Yeah, that sounds like a good idea.  Will test.


Thanks
Johannes