> -----Original Message----- > From: Martin Kaistra <martin.kaistra@xxxxxxxxxxxxx> > Sent: Monday, January 29, 2024 9:33 PM > To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx > Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; Bitterblue Smith > <rtl8821cerfe2@xxxxxxxxx>; Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > Subject: Re: [PATCH v2] wifi: rtl8xxxu: update rate mask per sta > > Am 26.01.24 um 03:41 schrieb Ping-Ke Shih: > > > > > >> -----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. > > I will move it to the beginning of the function. > > > > >> 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()? > > refresh_rate_mask is called from sta_add with force == true, so a new value is > set there to sta_info->rssi_level regardless of the previous value, but it can't > hurt to have a proper initialisation. rtl8xxxu_refresh_rate_mask() uses sta_info->rssi_level to choose threshold to transition rssi_level that can affect rate_bitmap, so it would be safer to give it an initial value. Luckily, RTL8XXXU_RATR_STA_INIT is 0, so nothing is changed. > > > >> + > >> + 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? > > I just saw that there already is a ieee80211_get_bssid() in net/mac80211/util.c. > Only problem is that it needs enum nl80211_iftype type as parameter and I > currently do not have that available in rtl8xxxu_parse_rxdesc16().. Maybe, ieee80211_get_bssid_of_hdr(), or just keep it as it was. No good idea. > >> + 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. > > Does rtl8xxxu_rx_update_rssi() sound better to you? That is better.
