[rt2x00-users] [PATCHv2] rt2x00: rework tx status handling in rt2800pci

Ivo Van Doorn ivdoorn at gmail.com
Sat Sep 11 20:31:11 UTC 2010


>> > +               queue = rt2x00queue_get_queue(rt2x00dev, qid);
>> > +               if (unlikely(queue == NULL)) {
>> > +                       /*
>> > +                        * The queue is NULL, this shouldn't happen. Stop
>> > +                        * processing here without dropping the tx status
>> > +                        * report.
>> > +                        */
>> > +                       break;
>> > +               }
>>
>> Why don't we drop the TX status? Apparently the PID was incorrect,
>> so we won't be able to match it against a valid queue later either.
>
> Yeah, basically this shoud never happen since we've validated the PID already
> above. So, maybe we should just add a WARN_ON here?

WARN_ON() is overkill here, it is not a code bug but a HW bug.
I would say a WARNING() to just add a single line to the log (rather the the
full stack trace).

>>
>> > +               if (rt2x00queue_empty(queue)) {
>> > +                       /*
>> > +                        * The queue is empty. Stop processing here without
>> > +                        * dropping the tx status report.
>> > +                        */
>> > +                       break;
>> > +               }
>>
>> Why don't we want to drop the tx status report? The queue is empty,
>> so apparently we received a TX status report from a frame which has
>> already been completed. reusing this TX status report only means that
>> we know for a 100% that it will not be matched to the correct frame.
>
> Yeah, you're right. This check should also not trigger at all, so I'm fine
> with dropping the status here, and maybe also adding a WARN?

Same here, just a WARNING() to the log is sufficient.

>> The rest of this function is equal to the rt2800lib
>
> Unfortunately, not 100%. In rt2800lib there's the loop that drops tx status
> reports when ack, wcid and pid don't match. In case of rt2800pci this check
> sometimes triggers _but_ the number of tx'ed frames is exactly the same as
> received tx status reports. So, in the rt2800pci case we don't want to drop
> status reports as they are indeed valid but may be reordered or something.
> I'm not sure if reordering really happens or if it is maybe related to
> retransmissions of failed frames in an aggregate or something but I know for
> sure that we get exactly as many tx status reports as frames put in the tx
> queue. And that's why dropping tx status reports in case of a mismatch doesn't
> work for rt2800pci (the status fifo would fill up).
>
> But yes, we can merge parts of both txdone functions into a helper (with
> rt2800usb using the status drop loop and rt2800pci not using it).

Well reordering should _never_ happen, and the legacy driver at no point
assumes that it will. So I don't think we need to keep that in mind.

>> txdone variant, so perhaps this function belongs in rt2800lib, where we find
>> someway to prevent code duplication.
>> Personally I think it will be worth investigating if the rt2800usb code can also
>> make use of the kfifo which would prevent the code-path difference
>
> Sure, we could do that. Not sure if it would help us for rt2800usb though. But
> we can do that in a separate patch to not grow the patch too much. At least I'd
> prefer that as otherwise the patcn gets unreadable very fast. And because of
> the difference explained above we might not be able to merge them completely
> (at least not yet).

Ok.

>> Additionally, could you check if this patch is also possible for rt61pci?
>> That one is also suffering from lost TX status reports, and might the same fix.
>
> Sure, should be possible, however I don't have a device here for testing ;)
> but we can do that in a separate patch afterwards. Would be cool if that
> would help rt61pci as well ...

If you come up with a patch, then perhaps Luis (added to CC) can test :)

>> >  /*
>> > @@ -797,6 +964,20 @@ static int rt2800pci_probe_hw(struct rt2x00_dev *rt2x00dev)
>> >         */
>> >        rt2x00dev->rssi_offset = DEFAULT_RSSI_OFFSET;
>> >
>> > +       /*
>> > +        * Allocate txstatus fifo and tasklet, we use a size of 512 for the
>> > +        * kfifo which is big enough to store 512/4=128 tx status reports.
>> > +        * In the worst case (tx status for all tx queues gets reported before
>> > +        * we've got a chance to handle them) 24*4=384 tx status reports need
>> > +        * to be cached.
>> > +        */
>> > +       retval = kfifo_alloc(&rt2x00dev->txstatus_fifo, 512, GFP_KERNEL);
>> > +       if (retval)
>> > +               return retval;
>> > +
>> > +       tasklet_init(&rt2x00dev->txstatus_tasklet, rt2800pci_txstatus_tasklet,
>> > +                    (unsigned long)rt2x00dev);
>> > +
>>
>> If we can also make use of this in rt61pci, this can be moved into rt2x00lib.
>
> Ok, but maybe we should introduce a flag that only allocates the fifo in case it
> is needed (or maybe the drivers can report a fifo size to rt2x00lib, and in case
> they report 0 we don't allocate it).

Yeah, that flag would be essential. Not sure about the kfifo size
though, I think
we can just enforce a single size for now.

>> Especially since you free this memory inside rt2x00lib.I am not a big fan of
>> freeing stuff in module X which was allocated in Y.
>
> Yeah, that's not nice, I agree, however, the free didn't affect drivers not
> allocating/initializing the fifo and tasklet :)
>
> Ok, so I'm going to respin the patch in accordance to your comments. Hopefully
> I can do that soon.

Thanks.

Ivo



More information about the users mailing list