On Tue, 2021-11-30 at 12:52 +0100, Johannes Berg wrote: > On Tue, 2021-11-30 at 12:32 +0100, Toke Høiland-Jørgensen wrote: > > Luca Coelho <luca@xxxxxxxxx> writes: > > > > > On Mon, 2021-11-29 at 14:54 +0100, Toke Høiland-Jørgensen wrote: > > > > Luca Coelho <luca@xxxxxxxxx> writes: > > > > > > > > > From: Johannes Berg <johannes.berg@xxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxxxxxxx > > > > > > > > > > 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@xxxxxxxxxxxxxxxxx> > > > > > Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx> > > > > > Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx> > > > > > > > > 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. Thanks, guys! Johannes, do you want me to send v2 or do you want to add the tag yourself? There is one more patch that is broken (ugh, sorry) that I need to fix anyway... -- Cheers, Luca.
