On 14.03.23 12:20, Alexander Wetzel wrote:
assuming that TXQ drivers actually still call ieee80211_txq_schedule_end() which says it's deprecated.
That even has _bh() so the tasklet can't be running anyway ...
So if the concurrency really is only TX vs. tasklet, then you could even just keep the BHs disabled (in _start spin_unlock only and then in _end local_bh_disable)?
Which may also be the solution for the regression in 6.2: Do it now for ieee80211_handle_wake_tx_queue() and apply this patch to the development tree only.
I'd argue the other way around - do it for all to fix these issues, and then audit drivers such as iwlwifi or even make concurrency here opt-in.
Felix did see some benefits of the concurrency I think though, so he might have a different opinion.
What about handling it that way:
Keep serializing drv_wake_tx_queue(). But use a new spinlock the drivers can opt out from.
No flag set: drv_wake_tx_queue() can be running only once at any time
IEEE80211_HW_CONCURRENT_AC_TX drv_wake_tx_queue() can be running once per AC at any time
IEEE80211_HW_CONCURRENT current behavior.
I think if you really insist on handling this through drv_wake_tx_queue locking, it really shouldn't be done through extra hw flags, because the locking requirements are too different.
I took a quick look at all the drivers that implement iTXQ themselves and what their requirements are.
Only two drivers need changes: - ath10k: needs a per-AC lock, because it does a scheduling round in the wake_tx_queue call. - iwlwifi: needs a global lock, since it touches a common list. None of your proposed locking types are enough for this one.
The rest seem fine to me: - ath9k has a per-hw-queue lock - mt76 only schedules a kthread - rtw88 uses a global lock + workqueue for scheduling - rtw89 uses a (potentially unnecessary) ieee80211_schedule_txq call + workqueue for scheduling
If you want to address this in the least invasive way, add a per-AC lock to ath10k's wake_tx_queue handler, a global lock to iwlwifi, and a per-AC lock inside ieee80211_handle_wake_tx_queue().
That way no locking is needed around the drv_wake_tx_queue calls.
- Felix