On Wed, 2022-11-02 at 19:40 +0100, Alexander Wetzel wrote:
>
> +++ b/net/mac80211/sta_info.c
> @@ -114,6 +114,28 @@ static int link_sta_info_hash_del(struct ieee80211_local *local,
> link_sta_rht_params);
> }
>
> +static void num_sta_ps_dec_for_mc_txq(struct ieee80211_sub_if_data *sdata,
> + atomic_t *num_sta_ps)
passing the pointer to the atomic_t variable seems a bit odd here, maybe
pass the 'ps' pointer here:
> + /* can call wake_tx_queue for vif.txq */
> + num_sta_ps_dec_for_mc_txq(sdata, &ps->num_sta_ps);
instead?
Also maybe that comment should be a couple of lines above on the
local_bh_disable() since that's the context requirement for that?
> @@ -1046,8 +1070,8 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
> if (ignore_for_tim & ieee80211_ac_to_qos_mask[ac])
> continue;
>
> - indicate_tim |= !skb_queue_empty(&sta->tx_filtered[ac]) ||
> - !skb_queue_empty(&sta->ps_tx_buf[ac]);
> + indicate_tim |= !skb_queue_empty(&sta->tx_filtered[ac]);
This seems odd, we must indicate the TIM whenever we have *any* traffic
pending to that STA? IOW also if there's anything on the TXQs?
> @@ -1553,23 +1547,11 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>
> BUILD_BUG_ON(BITS_TO_LONGS(IEEE80211_NUM_TIDS) > 1);
> sta->driver_buffered_tids = 0;
> - sta->txq_buffered_tids = 0;
>
> if (!ieee80211_hw_check(&local->hw, AP_LINK_PS))
> drv_sta_notify(local, sdata, STA_NOTIFY_AWAKE, &sta->sta);
>
> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> - if (!sta->sta.txq[i] || !txq_has_queue(sta->sta.txq[i]))
> - continue;
> -
> - schedule_and_wake_txq(local, to_txq_info(sta->sta.txq[i]));
> - }
> -
> - skb_queue_head_init(&pending);
> -
> - /* sync with ieee80211_tx_h_unicast_ps_buf */
> - spin_lock(&sta->ps_lock);
> - /* Send all buffered frames to the station */
> + /* Send all filtered frames to the station */
> for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
> int count = skb_queue_len(&pending), tmp;
>
> @@ -1579,16 +1561,39 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> tmp = skb_queue_len(&pending);
> filtered += tmp - count;
> count = tmp;
> -
> - spin_lock_irqsave(&sta->ps_tx_buf[ac].lock, flags);
> - skb_queue_splice_tail_init(&sta->ps_tx_buf[ac], &pending);
> - spin_unlock_irqrestore(&sta->ps_tx_buf[ac].lock, flags);
> - tmp = skb_queue_len(&pending);
> - buffered += tmp - count;
> }
>
> + skb_queue_head_init(&pending);
> ieee80211_add_pending_skbs(local, &pending);
It seems to me you moved the skb_queue_head_init() erroneously, it's
still needed for the loop for the filtered frames?
If you reinit here you probably leak, and if it's not inited before you
probably crash?
> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> + if (!sta->sta.txq[i])
> + continue;
> +
> + txqi = to_txq_info(sta->sta.txq[i]);
> + clear_bit(IEEE80211_TXQ_STOP_PS, &txqi->flags);
> + }
Should we really do this before pending are transmitted? Actually I'm a
bit confused about ieee80211_add_pending_skbs() now - does that mean
those frames don't go to the driver via TXQs?
johannes
