On Mon, 2025-01-27 at 17:26 +0100, Alexander Wetzel wrote:
>
> +++ b/net/mac80211/iface.c
> @@ -1124,11 +1124,63 @@ static void ieee80211_sdata_init(struct ieee80211_local *local,
> * MLD connection, we get a separate allocation for it.
> */
> ieee80211_link_init(sdata, -1, &sdata->deflink, &sdata->vif.bss_conf);
> +
> + for (int i = 0; i < NUM_NL80211_BANDS; i++) {
> + struct ieee80211_supported_band *sband;
> +
> + sband = local->hw.wiphy->bands[i];
> + sdata->rc_rateidx_mask[i] =
> + sband ? (1 << sband->n_bitrates) - 1 : 0;
Feels like this code move should perhaps be in a separate refactoring
patch?
> +static void ieee80211_vif_txq_init(struct ieee80211_sub_if_data *sdata,
> + int txq_offset, int txq_size, int num_queues)
> +{
> + void *buffer = (char *)sdata + txq_offset;
That seems ... awkward at best, my first instinct would be to say this
must be wrong.
Do we really need to do this dynamically just to save two a bit of
memory for VLAN and (virtual) monitor interfaces?
Would probably be better to use pointers to a TXQ array instead of
embedding them then, or otherwise just embed enough for all kinds of
interfaces and just not use them.
> -void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
> +static void ieee80211_purge_txqs(struct ieee80211_sub_if_data *sdata)
> {
> - struct ieee80211_txq *txq = sdata->vif.txq[IEEE80211_VIF_TXQ_MULTICAST];
> + struct sta_info *sta;
>
> + list_for_each_entry(sta, &sdata->local->sta_list, list) {
> + if (sdata != sta->sdata)
> + continue;
> + ieee80211_purge_sta_txqs(sta);
> + }
> +
> + for (int i = IEEE80211_VIF_TXQ_MULTICAST;
> + i <= IEEE80211_VIF_TXQ_FALLBACK;
> + i++) {
> + if (sdata->vif.txq[i])
> + ieee80211_txq_purge(sdata->local,
> + to_txq_info(sdata->vif.txq[i]));
> + }
> +
> + synchronize_rcu();
You should keep the synchronize_rcu() outside this function. It's
expensive, and
> @@ -2300,6 +2342,8 @@ void ieee80211_remove_interfaces(struct ieee80211_local *local)
> list_for_each_entry_safe(sdata, tmp, &unreg_list, list) {
> bool netdev = sdata->dev;
>
> + ieee80211_purge_txqs(sdata);
> +
> /*
> * Remove IP addresses explicitly, since the notifier will
> * skip the callbacks if wdev->registered is false, since
can be amortized across all interfaces in this case.
> @@ -3803,7 +3818,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif = txq->vif;
> int q = vif->hw_queue[txq->ac];
> unsigned long flags;
> - bool q_stopped;
> + int qsr;
Should be "unsigned long qsr", to match queue_stop_reasons, I guess.
> + /* Drop off-channel frames if queues are stopped for any reason
> + * other than off-channel operation. Never queue them.
> + */
> + if (unlikely(txq->tid == IEEE80211_TXQ_NOQUEUE)) {
> + WARN(1, "mac80211: Drop noqueue TX. qsr=%i\n", qsr);
> + ieee80211_txq_purge(local, txqi);
> + return NULL;
> + }
if (WARN_ON(...)) { ... }
Perhaps better just "if (WARN_ON_ONCE())", since it's in TX.
> +static void __ieee80211_wake_txq(struct ieee80211_local *local,
> + struct ieee80211_txq *txq)
> +{
> + struct txq_info *txqi = to_txq_info(txq);
> + struct fq *fq = &local->fq;
> +
> + if (WARN_ON(!txq))
> + return;
> + if (test_and_clear_bit(IEEE80211_TXQ_DIRTY, &txqi->flags)) {
> + spin_unlock(&fq->lock);
> + drv_wake_tx_queue(local, txqi);
> + spin_lock(&fq->lock);
> + }
> +}
I really don't like dropping locks in the middle - if that's *really*
needed there better be a very good reason and a good comment indicating
why it's safe, including a comment in the caller that says this happens.
> + /* %IEEE80211_VIF_TXQ_NOQUEUE must be ignored here */
No need for % outside kernel-doc.
> +
> + if (ac == IEEE80211_AC_VO) {
> + __ieee80211_wake_txq(local,
> + vif->txq[IEEE80211_VIF_TXQ_FALLBACK]);
> + }
nit: no need for braces
Why is VO special, not sure I understand the logic of checking the AC?
> + if (ac == IEEE80211_AC_BE && vif->txq[IEEE80211_VIF_TXQ_MULTICAST] &&
> + (!ps || !atomic_read(&ps->num_sta_ps)))
> + __ieee80211_wake_txq(local,
> + vif->txq[IEEE80211_VIF_TXQ_MULTICAST]);
Also here, why BE?
> list_for_each_entry_rcu(sta, &local->sta_list, list) {
> if (sdata != sta->sdata)
> continue;
>
> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> + /* IEEE80211_TXQ_NOQUEUE must be ignored here. */
some of these comments should probably say _why_ too
johannes
