> -----Original Message----- > From: Martin Kaistra <martin.kaistra@xxxxxxxxxxxxx> > Sent: Wednesday, January 24, 2024 4:27 PM > To: linux-wireless@xxxxxxxxxxxxxxx > Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; Ping-Ke Shih > <pkshih@xxxxxxxxxxx>; Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>; Sebastian Andrzej Siewior > <bigeasy@xxxxxxxxxxxxx> > Subject: [PATCH v2] 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@xxxxxxxxxxxxx> > --- > changes v1->v2: move 'rssi_level' into struct rtl8xxxu_sta_info > v1: https://lore.kernel.org/linux-wireless/20240117145516.497966-1-martin.kaistra@xxxxxxxxxxxxx/ [...] > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 3b954c2fe448f..3820d3c308759 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -4993,8 +4993,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > struct device *dev = &priv->udev->dev; > struct ieee80211_sta *sta; > struct rtl8xxxu_ra_report *rarpt; > + u8 val8, macid; > u32 val32; > - u8 val8; > > rarpt = &priv->ra_report; > > @@ -5004,6 +5004,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > rtl8xxxu_set_linktype(priv, vif->type, rtlvif->port_num); > > if (vif->cfg.assoc) { > + struct rtl8xxxu_sta_info *sta_info; nit: I remember that declaration at beginning of function is preferred. > u32 ramask; > int sgi = 0; > u8 highest_rate; > @@ -5017,6 +5018,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > rcu_read_unlock(); > goto error; > } > + macid = rtl8xxxu_get_macid(priv, sta); > > if (sta->deflink.ht_cap.ht_supported) > dev_info(dev, "%s: HT supported\n", __func__); > @@ -5037,14 +5039,15 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > bw = RATE_INFO_BW_40; > else > bw = RATE_INFO_BW_20; > + > + sta_info = (struct rtl8xxxu_sta_info *)sta->drv_priv; > + sta_info->rssi_level = RTL8XXXU_RATR_STA_INIT; For AP mode, we should do this as well at rtl8xxxu_sta_add() before calling rtl8xxxu_refresh_rate_mask()? > rcu_read_unlock(); > > rtl8xxxu_update_ra_report(rarpt, highest_rate, sgi, bw); > > - priv->rssi_level = RTL8XXXU_RATR_STA_INIT; > - > priv->fops->update_rate_mask(priv, ramask, 0, sgi, > - bw == RATE_INFO_BW_40, 0); > + bw == RATE_INFO_BW_40, macid); > > rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); > > @@ -6317,6 +6320,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; nit: Here checks bssid, addr1 and beacon frame. For me, if you want to combine some of them, would it be reasonable to combine bssid and add1? > + > + sta = ieee80211_find_sta_by_ifaddr(priv->hw, hdr->addr2, > + vif->addr); > + if (!sta) > + return; > + > + sta_info = (struct rtl8xxxu_sta_info *)sta->drv_priv; > + ewma_rssi_add(&sta_info->avg_rssi, -rx_status->signal); > +} > + > +static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr) Would you like to add a ieee80211_get_BSSID() to ieee80211.h in separated patch? But I wonder if it is enough to check addr1 only? > +{ > + __le16 fc = hdr->frame_control; > + u8 *bssid; > + > + if (ieee80211_has_tods(fc)) > + bssid = hdr->addr1; > + else if (ieee80211_has_fromds(fc)) > + bssid = hdr->addr2; > + else > + bssid = hdr->addr3; > + > + return bssid; > +} > + > +static void rtl8xxxu_rx_addr_match(struct rtl8xxxu_priv *priv, > + struct ieee80211_rx_status *rx_status, > + struct ieee80211_hdr *hdr) > +{ > + struct rtl8xxxu_rx_addr_match_data data = {}; > + > + if (ieee80211_is_ctl(hdr->frame_control)) > + return; > + > + data.priv = priv; > + data.hdr = hdr; > + data.rx_status = rx_status; > + data.bssid = get_hdr_bssid(hdr); > + > + rtl8xxxu_iterate_vifs_atomic(priv, rtl8xxxu_rx_addr_match_iter, &data); > +} > + > int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) > { > struct ieee80211_hw *hw = priv->hw; > @@ -6376,18 +6449,26 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) > skb_queue_tail(&priv->c2hcmd_queue, skb); > schedule_work(&priv->c2hcmd_work); > } else { > + struct ieee80211_hdr *hdr; > + > phy_stats = (struct rtl8723au_phy_stats *)skb->data; > > skb_pull(skb, drvinfo_sz + desc_shift); > > skb_trim(skb, pkt_len); > > - if (rx_desc->phy_stats) > + hdr = (struct ieee80211_hdr *)skb->data; > + if (rx_desc->phy_stats) { > priv->fops->parse_phystats( > priv, rx_status, phy_stats, > rx_desc->rxmcs, > - (struct ieee80211_hdr *)skb->data, > + hdr, > rx_desc->crc32 || rx_desc->icverr); > + if (!rx_desc->crc32 && !rx_desc->icverr) > + rtl8xxxu_rx_addr_match(priv, > + rx_status, > + hdr); This function name isn't clear, because it doesn't just match rx addr, but it is to update RSSI by rx_status for corresponding station. > + } > > rx_status->mactime = rx_desc->tsfl; > rx_status->flag |= RX_FLAG_MACTIME_START; [...]
