Re: [PATCH] mac80211: Ensure vif queues are operational after start

[Ben Greear <greearb@candelatech.com>]

On 9/16/22 9:38 AM, Alexander Wetzel wrote:
> On 15.09.22 22:39, Ben Greear wrote:
> 
>>  From reading the original patch description, it was to stop an NPE when AP was stopped.  I have been testing
>> this patch below and it fixes the problems I saw with multiple vdevs.  I was worried that
>> the code in the 'list_for_each_entry_rcu(sta, &local->sta_list, list) {' might still need to run
>> to keep everything in sync (and my patch allows that to happen), but I do not know if that is true or not.
>>
> 
> I'm not sure if it's still save to wake the queues for the vif we are tearing down and assumed the intend was to skip those, too.
> 
> But it looks like all stations for the vif are deleted prior to setting sdata->bss = NULL, so the outcome should be the same.
> 
> Your solution removes potentially set IEEE80211_TXQ_STOP_NETIF_TX flags, reducing the risk that a queue restart during vif setup ends up with inoperable queues.
> But the only way to set IEEE80211_TXQ_STOP_NETIF_TX seems to be during ieee80211_tx_dequeue(). Which should not be possible to be called as long as 
> SDATA_STATE_RUNNING was never set for the vif.
> 
> But I'm on thin ice here:-)

I spent two days debugging this and have only barest understanding of how all of the atf/fq/txq logic
is supposed to work.

But, I think my test case was a bit different from yours, and in my test case, before my patch,
it failed 100% of the time:

create two station vdevs on same (mtk7916) radio
admin one up (this works)
admin second one up (this fails, tx path is hung, because sdata->vif.txqs_stopped[ac] was true).

In general, I'd like to keep start/stop state as in-sync everywhere as possible, and I think my patch
might be better at that than yours since it goes through the sta list (and, maybe too, I'm completely
wrong about that).

Potentially, static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
should just be called each time a vdev goes admin up.
But since my original test case failed so reliably, the __ieee80211_wake_txqs method must not actually be
called when secondary vdevs go admin up.  I am not sure if that is per design or some other
bug.

Hoping the 2-3 people who understand this logic well will chime in :)

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com