RE: [PATCH] wifi: rtl8xxxu: update rate mask per sta

[Ping-Ke Shih <pkshih@realtek.com>]

> -----Original Message-----
> From: Martin Kaistra <martin.kaistra@linutronix.de>
> Sent: Wednesday, January 17, 2024 10:55 PM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ping-Ke Shih
> <pkshih@realtek.com>; Bitterblue Smith <rtl8821cerfe2@gmail.com>; Sebastian Andrzej Siewior
> <bigeasy@linutronix.de>
> Subject: [PATCH] wifi: rtl8xxxu: update rate mask per sta
> 
> Until now, rtl8xxxu_watchdog_callback() only fetches RSSI and updates
> the rate mask in station mode. This means, in AP mode only the default
> rate mask is used.
> 
> In order to have the rate mask reflect the actual connection quality,
> extend rtl8xxxu_watchdog_callback() to iterate over every sta. Like in
> the rtw88 driver, add a function to collect all currently present stas
> and then iterate over a list of copies to ensure no RCU lock problems
> for register access via USB. Remove the existing RCU lock in
> rtl8xxxu_refresh_rate_mask().
> 
> Since the currently used ieee80211_ave_rssi() is only for 'vif', add
> driver-level tracking of RSSI per sta.
> 
> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>

[...]

> @@ -6317,6 +6318,76 @@ static void rtl8188e_c2hcmd_callback(struct work_struct *work)
>         }
>  }
> 
> +#define rtl8xxxu_iterate_vifs_atomic(priv, iterator, data)                     \
> +       ieee80211_iterate_active_interfaces_atomic((priv)->hw,                  \
> +                       IEEE80211_IFACE_ITER_NORMAL, iterator, data)
> +
> +struct rtl8xxxu_rx_addr_match_data {
> +       struct rtl8xxxu_priv *priv;
> +       struct ieee80211_hdr *hdr;
> +       struct ieee80211_rx_status *rx_status;
> +       u8 *bssid;
> +};
> +
> +static void rtl8xxxu_rx_addr_match_iter(void *data, u8 *mac,
> +                                       struct ieee80211_vif *vif)
> +{
> +       struct rtl8xxxu_rx_addr_match_data *iter_data = data;
> +       struct ieee80211_sta *sta;
> +       struct ieee80211_hdr *hdr = iter_data->hdr;
> +       struct rtl8xxxu_priv *priv = iter_data->priv;
> +       struct rtl8xxxu_sta_info *sta_info;
> +       struct ieee80211_rx_status *rx_status = iter_data->rx_status;
> +       u8 *bssid = iter_data->bssid;
> +
> +       if (!ether_addr_equal(vif->bss_conf.bssid, bssid))
> +               return;
> +
> +       if (!(ether_addr_equal(vif->addr, hdr->addr1) ||
> +             ieee80211_is_beacon(hdr->frame_control)))
> +               return;
> +
> +       sta = ieee80211_find_sta_by_ifaddr(priv->hw, hdr->addr2,
> +                                          vif->addr);

Can't we search for 'sta' by rx_desc->mac_id? Then, you don't need a lot of
code to check address. 

> +       if (!sta)
> +               return;
> +
> +       sta_info = (struct rtl8xxxu_sta_info *)sta->drv_priv;
> +       ewma_rssi_add(&sta_info->avg_rssi, -rx_status->signal);
> +}
> +

[...]

> @@ -7119,7 +7203,7 @@ static void rtl8xxxu_refresh_rate_mask(struct rtl8xxxu_priv *priv,
>         u8 go_up_gap = 5;
>         u8 macid = rtl8xxxu_get_macid(priv, sta);
> 
> -       rssi_level = priv->rssi_level;
> +       rssi_level = priv->rssi_level[macid];

Is it possible to move 'rssi_level' into struct rtl8xxxu_sta_info?

>         snr = rtl8xxxu_signal_to_snr(signal);
>         snr_thresh_high = RTL8XXXU_SNR_THRESH_HIGH;
>         snr_thresh_low = RTL8XXXU_SNR_THRESH_LOW;

[...]

> @@ -7329,40 +7411,60 @@ static void rtl8xxxu_track_cfo(struct rtl8xxxu_priv *priv)
>         rtl8xxxu_set_atc_status(priv, abs(cfo_average) >= CFO_TH_ATC);
>  }
> 
> -static void rtl8xxxu_watchdog_callback(struct work_struct *work)
> +static void rtl8xxxu_ra_iter(void *data, struct ieee80211_sta *sta)
>  {
> -       struct ieee80211_vif *vif;
> -       struct rtl8xxxu_priv *priv;
> -       int i;
> +       struct rtl8xxxu_sta_info *sta_info = (struct rtl8xxxu_sta_info *)sta->drv_priv;
> +       struct rtl8xxxu_priv *priv = data;
> +       int signal = -ewma_rssi_read(&sta_info->avg_rssi);

The unit conversion of signal is a little complicated --
from physt to rx_status->signal to sta_info->avg_rssi.

I think you did it well. Just want to confirm have you checked the final result
is equal to before at runtime?

[...]