[rt2x00-users] [RFC 2/2] rt2x00: Fix tx status reporting for reordered frames in rt2800pci

[Gertjan van Wingerde]

On Thu, Mar 31, 2011 at 8:47 AM, Helmut Schaa
<helmut.schaa at googlemail.com> wrote:
> Am Mittwoch, 30. März 2011 schrieb Gertjan van Wingerde:
>> On 03/29/11 22:48, Helmut Schaa wrote:
>> > Am Dienstag, 29. März 2011 schrieb Gertjan van Wingerde:
>> >> On 03/29/11 14:02, Helmut Schaa wrote:
>> >> I guess your testing does show that we need to be able to handle out-of-order
>> >> status reports, but before we do so we better make very sure that a missing TX status
>> >> cannot cause issues.
>> > That's the problem :) I don't see how we can find out if a TX status was missed.
>> >
>> > Maybe implementing a watchdog and dropping a frame if it didn't get a tx status
>> > would make sense then?
>>
>> Maybe we can do without a watchdog. But we do need something so that we can handle the
>> queue entries for which no TX status was received.
>> Perhaps we can change the code that emits the error message below to instead of just
>> complaining also clean up the entry and report the frame as having TX status unknown
>> and clean it up, so that the queue entry can be reused.
>> However, that is a major change to the way TX queues are currently handled.
>
> Interesting idea. Might be feasible but indeed would change the tx queue
> handling quite a lot.
>
>> >> With this patch I can see the reports about the following error pop up:
>> >>     Arrived at non-free entry in the non-full queue %d.
>> > I did quite some testing and never ended up in such a situation but I agree that
>> > the TX status handling is a weak point in the driver currently ...
>>
>> OK. But I still wouldn't like to see this error pop up again.
>> It took us ages to get rid of it.
>
> Sure, I can understand that :)
>
>> >>>  drivers/net/wireless/rt2x00/rt2800pci.c   |   81 +++++++++++++++++++++++++++-
>> >>>  drivers/net/wireless/rt2x00/rt2x00queue.h |    4 ++
>> >>>  2 files changed, 82 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
>> >>> index 59316e9..0be8744 100644
>> >>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> >>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> >>> @@ -717,10 +717,68 @@ static void rt2800pci_wakeup(struct rt2x00_dev *rt2x00dev)
>> >>>   rt2800_config(rt2x00dev, &libconf, IEEE80211_CONF_CHANGE_PS);
>> >>>  }
>> >>>
>> >>> +static bool rt2800pci_txdone_entry_check(struct queue_entry *entry, u32 status)
>> >>> +{
>> >>> + __le32 *txwi;
>> >>> + u32 word;
>> >>> + int wcid, tx_wcid;
>> >>> +
>> >>> + wcid = rt2x00_get_field32(status, TX_STA_FIFO_WCID);
>> >>> +
>> >>> + txwi = rt2800_drv_get_txwi(entry);
>> >>> + rt2x00_desc_read(txwi, 1, &word);
>> >>> + tx_wcid = rt2x00_get_field32(word, TXWI_W1_WIRELESS_CLI_ID);
>> >>> +
>> >>> + return (tx_wcid == wcid);
>> >>> +}
>> >> Is there any reason why you cannot use the already existing rt2800_txdone_entry_check
>> >> function?
>> > Because I only want to check the wcid, not the other fields. But I agree, we might be
>> > able to share some more code.
>>
>> To be honest, I think the other checks that are in rt2800_txdone_entry_check are useful for
>> rt2800pci as well. So I would rather see us share more code than creating more differences
>> between rt2800usb and rt2800pci.
>
> I already tried once to use the generic txdone_check function in rt2800pci and
> it always messed up the tx status handling due to strange side effects like
> having incorrect PIDs in the TX_STA_FIFO when sending AMPDUs.
>
> When adding some prints to the tx status handling code in case of wcid!=tx_wcid
> the messages always appear pairwise:
>
> Something like
> wcid=33, tx_wcid=34
> wcid=33, tx_wcid=34
> wcid=33, tx_wcid=34
> wcid=34, tx_wcid=33
> wcid=34, tx_wcid=33
> wcid=34, tx_wcid=33
>
> So, the number of tx status reports per wcid at least stays correct.
>
> I'm not entirely sure if this is true for the ack subfield or the aggr
> subfield as well due to my previous observations in this regard. It seems as
> if the hardware sometimes will aggregate frames even if they are meant to
> not get aggregated. So, checking aggr!=tx_aggr would most likely break the
> already fragile tx status handling.

Hmm, but that would mean that we already have issues with rt2800usb, as there
is no reason to assume that the behaviour of the USB chipsets would be different
in this matter.

To me, all the more reason to properly flesh this out in a common way between
rt2800pci and rt2800usb. I really see no reason why the handling of a
TX_STA_FIFO
value would have to be different between the two.

---
Gertjan