Re: [PATCH] rt2x00: rt2800usb: fix races in tx queue

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Mon, Aug 08, 2011 at 10:45:25PM +0200, Gertjan van Wingerde wrote:
> >> Here is part of code where we exit the loop (and whole function) and do
> >> not remove head "reg" from tx_status fifo - and read it again when
> >> _txdone work is called next time.
> > 
> > Well but for what reason would we want to read the register again? If
> > we found an status report
> > for a queue which does not have pending items, then in this change it
> > would mean that the
> > status report is intended for a TX frame which has yet to be enqueued
> > to the hardware.
> > 
> > Obviously this means a mismatch between the TX status report and the
> > actual frame to which it
> > is being connected.
> 
> Hmm, if I understood the patch description correctly, then there may be a race in the code,
> where we actually read the TX status report before we have had the chance to handle the TX done
> URB interrupt. As far as I understood it, it are not spurious TX status reports for frames that
> were never submitted to the HW.
> If that is really the case then it is quite reasonable to wait a little bit until the TX done
> code has been executed and then process the TX status report again. 
> However, we must be damn sure that this is really what happens.
> 
> Stanislaw, please confirm (or deny) that my understanding is correct.


In this loop

                while (!rt2x00queue_empty(queue)) {
                        entry = rt2x00queue_get_entry(queue,Q_INDEX_DONE);
                        if (rt2800usb_txdone_entry_check(entry, reg))
                                break;
                }
we can process entry that is currently added to tx queue and nulify skb.

This can happen if we return false from rt2800usb_txdone_entry_check(),
so only when reg does not match ((wcid != tx_wcid) || (ack != tx_ack) ||
(pid != tx_pid)), or previous entry is ENTRY_DATA_IO_FAILED.

Stanislaw