Re: [PATCH 15/16] mac80211: agg-tx: don't schedule_and_wake_txq() under sta->lock

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2021-11-30 at 12:32 +0100, Toke Høiland-Jørgensen wrote:
>> Luca Coelho <luca@coelho.fi> writes:
>> 
>> > On Mon, 2021-11-29 at 14:54 +0100, Toke Høiland-Jørgensen wrote:
>> > > Luca Coelho <luca@coelho.fi> writes:
>> > > 
>> > > > From: Johannes Berg <johannes.berg@intel.com>
>> > > > 
>> > > > When we call ieee80211_agg_start_txq(), that will in turn call
>> > > > schedule_and_wake_txq(). Called from ieee80211_stop_tx_ba_cb()
>> > > > this is done under sta->lock, which leads to certain circular
>> > > > lock dependencies, as reported by Chris Murphy:
>> > > > https://lore.kernel.org/r/CAJCQCtSXJ5qA4bqSPY=oLRMbv-irihVvP7A2uGutEbXQVkoNaw@mail.gmail.com
>> > > > 
>> > > > In general, ieee80211_agg_start_txq() is usually not called
>> > > > with sta->lock held, only in this one place. But it's always
>> > > > called with sta->ampdu_mlme.mtx held, and that's therefore
>> > > > clearly sufficient.
>> > > > 
>> > > > Change ieee80211_stop_tx_ba_cb() to also call it without the
>> > > > sta->lock held, by factoring it out of ieee80211_remove_tid_tx()
>> > > > (which is only called in this one place).
>> > > > 
>> > > > This breaks the locking chain and makes it less likely that
>> > > > we'll have similar locking chain problems in the future.
>> > > > 
>> > > > Reported-by: Chris Murphy <lists@colorremedies.com>
>> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>> > > 
>> > > Does this need a fixes: tag?
>> > 
>> > Hi Toke,
>> > 
>> > Neither Johannes nor Chris pointed to any specific patch that this
>> > commit is fixing.  If you know the exact commit that broke this, I can
>> > add the tag and send v2.
>> 
>> Well, it looks like the code you're changing comes all the way back from:
>> ba8c3d6f16a1 ("mac80211: add an intermediate software queue implementation")
>> 
>> Maybe Johannes can comment on whether it's appropriate to include this,
>> or if the code changed too much in the meantime...
>> 
>
> I think that probably makes sense, and I guess I can include that tag
> when I apply it.
>
> I suspect the reason I didn't do it internally (we have a different tag
> though, so that's no excuse) is that there we didn't care until iwlwifi
> actually gained TXQ support.

Right, makes sense - thanks! :)

-Toke