Re: [ath9k-devel] [RFC] ath9k: fix tx queue selection

[Felix Fietkau <nbd@openwrt.org>]

On 2010-11-03 5:27 PM, Björn Smedman wrote:
>>> It comes down to this: either we look at the header qos when we select
>>> the queue (so the above cannot happen) or we relay on mac80211 to set
>>> the header qos and the skb queue mapping in a certain way. If we
>>> choose the later I vote for a BUG_ON(txctl->txq != tid->ac->txq) in
>>> ath_tx_send_ampdu().
>> For regular QoS data frames (and no other frames ever hit the
>> aggregation code) there is only one possible way to map tid -> ac ->
>> queue. I did review those code paths, and I believe them to be safe.
>> If you want, we can add a WARN_ON_ONCE later, but definitely no BUG_ON.
> 
> I've briefly looked through the IEEE Std 802.11e-2005. There is a
> clear requirement that "There shall be no reordering of unicast MSDUs
> with the same TID value and addressed to the same destination" in
> analog to what Hulmut pointed out earlier. Other than that the only
> reference I can find is that: "The MAC data service for QSTAs shall
> incorporate a TID with each MA-UNITDATA.request service. This TID
> associates the MSDU with the AC or TS queue for the indicated
> traffic." Why are you sure there is only one way to map tid -> ac ->
> queue? I don't think it's hard to come up with a case where you want
> to map differently (e.g. depending on RA or even TA).
Take a look at Table 9-1 on page 253 (PDF page 301) in 802.11-2007.

> Ok, regardless. So lets say there is a bug in mac80211 that allows a
> "mismatch" between header qos tid and skb queue mapping to occur
> (which in fact there is because this happens all the time with my
> frame injection heavy app). Then it's ok for ath9k to screw up the
> locking, possibly corrupt data and so on, silently?
I don't see potential for locking issues or data corruption here, even
if such a bug were to show up.

> Other than that I guess that it's basically an argument about
> aesthetics, and you may very well be right. All I know is that I've
> been following ath9k development now for almost two years and I'm
> amazed by the severity of bugs that are still found, and I guess yet
> to be found. We're dma:ing all over the place, deadlocking queues and
> so on, on a regular basis, or at least we where 3 months ago. After
> each one of these is fixed the attitude seems to be "now everything is
> perfect and suggesting there could be some more problems or will be in
> the future is just plain rude". Then yet another is found...
I'm not saying we should assume that everything is always fine, but I do
object to adding defensive code against made up scenarios of potential
bugs that "might" be introduced at some point in the future.

> If you relay on something so fragile as the contents of frame data
> "matching" skb_get_queue_mapping() I think you owe me at least a
> WARN_ON_ONCE before you start corrupting memory. ;)
The assumption that I make is not just about some random field in the
frame contents. I'm assuming that ieee80211_select_queue() makes a sane
decision that matches the description in the standard, and that the
network stack preserves the decision that this function made.
And besides - it's not like part of ath9k that cares about the TID is
going to live on for much longer :)

- Felix