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

[Stanislaw Gruszka <sgruszka@redhat.com>]

Hi Ivo

On Sat, Aug 06, 2011 at 01:06:51PM +0200, Ivo Van Doorn wrote:
> On Thu, Aug 4, 2011 at 2:46 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > -static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
> > +static int rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
> >  {
> >        struct data_queue *queue;
> >        struct queue_entry *entry;
> >        u32 reg;
> >        u8 qid;
> >
> > -       while (kfifo_get(&rt2x00dev->txstatus_fifo, &reg)) {
> > +       while (kfifo_peek(&rt2x00dev->txstatus_fifo, &reg)) {
> 
> I'm not too sure about this change, why do you need to do kfifo_peek
> and add gotos to the end of the while-loop to remove the item from the queue?
> There is no condition in which the obtained value from kfifo-peek
> will require it to be read again later (because when the value couldn't be
> handled we are throwing it away anyway using kfifo_skip).

There is new case (see below) where it is needed. I can get rid of goto,
that will make code a bit cleaner. There is place for optimization, mainly
make tx_status fifo per queue, but for now I just want to fix kernel crashes.

> >                /* TX_STA_FIFO_PID_QUEUE is a 2-bit field, thus
> >                 * qid is guaranteed to be one of the TX QIDs
> > @@ -517,25 +517,39 @@ static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
> >                if (unlikely(!queue)) {
> >                        WARNING(rt2x00dev, "Got TX status for an unavailable "
> >                                           "queue %u, dropping\n", qid);
> > -                       continue;
> > +                       goto next_reg;
> >                }
> >
> >                /*
> >                 * Inside each queue, we process each entry in a chronological
> >                 * order. We first check that the queue is not empty.
> >                 */
> > -               entry = NULL;
> > -               while (!rt2x00queue_empty(queue)) {
> > +               while (1) {
> > +                       entry = NULL;
> > +
> > +                       if (rt2x00queue_empty(queue))
> > +                               break;
> > +
> >                        entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
> > +
> > +                       if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
> > +                           !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags)) {
> > +                               WARNING(rt2x00dev, "Data pending for entry %u"
> > +                                       "in queue %u\n", entry->entry_idx, qid);
> > +                               return 1;

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.

Stanislaw