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

[Felix Fietkau <nbd@openwrt.org>]

On 2010-11-03 6:31 PM, Björn Smedman wrote:
> 2010/11/3 Felix Fietkau <nbd@openwrt.org>:
>> On 2010-11-03 5:27 PM, Björn Smedman wrote:
>>> 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.
> 
> Then I think this is the only point we really disagree on. :)
> 
> It goes like this. When we get to ath_tx_start_dma() there already is
> a txq assigned (passed as txctl->txq) and we lock that txq. Then, if
> it's aggregation data we look up the tid:
> 
>                 an = (struct ath_node *)tx_info->control.sta->drv_priv;
>                 tid = ATH_AN_2_TID(an, bf->bf_tidno);
> 
> Notice how bf->bf_tidno is used. This contains the TID from the 802.11
> header qos field. That means tid->ac->txq may not be the same as
> txctl->txq if there is a mismatch between frame data and skb queue
> mapping. Now we call ath_tx_send_ampdu() which presumes the txq (and
> therefore the associated tid) is already locked and starts fiddling
> with e.g. tid->buf_q, in this case without holding
> tid->ac->txq->axq_lock. This is racy e.g. against ath_draintxq() /
> ath_txq_drain_pending_buffers() which does not know about this madness
> and locks the correct txq.
Hmm, I guess you have a point there ;)

>> 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.
> 
> I can see your point. I don't want lots of defensive stuff (like what
> you removed in your patch). But I still feel the balance is wrong.
> Take one recent case for example: We're not 100% sure we can always
> stop RX dma. In fact, a few weeks ago we weren't even sure what we
> didn't start it when we weren't supposed to. Yet for some reason there
> seems to be a consensus it is a good idea to keep ds_data of all those
> dma descriptors pointing at arbitrary kernel data. I realize it takes
> some time and adds some clutter to do "ds_data = 0". I also understand
> it does not help in all cases. But I think it's a reasonable
> precaution under the circumstances. It's like in medicine, patients
> will die but when they do you want to be able to say "we did
> everything we could". ;)
Actually, when dealing with hardware pointers, I'm not sure setting them
to 0 makes things any better, since 0 still points to a physical RAM
location :)

- Felix