| Message ID | 35165caf-337c-4da0-b55c-c1a7081a1456@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: rtlwifi: Fix setting the basic rates | expand |
> -----Original Message----- > From: Bitterblue Smith <rtl8821cerfe2@gmail.com> > Sent: Sunday, February 18, 2024 4:20 AM > To: linux-wireless@vger.kernel.org > Cc: Ping-Ke Shih <pkshih@realtek.com>; Larry Finger <Larry.Finger@lwfinger.net> > Subject: [PATCH] wifi: rtlwifi: Fix setting the basic rates Though driver sets register via enum HW_VAR_BASIC_RATE, but actually it uses supported rates as ACK and RTS rates, so I think it would be clearer to mention "fix setting of RTS rate" in subject. Others look good to me. > > RTL8192CU transmits RTS frames at 48M instead of the expected 24M. > This is because rtlwifi never writes REG_INIRTS_RATE_SEL, because when > rtl_op_bss_info_changed() is called with BSS_CHANGED_BASIC_RATES (and > BSS_CHANGED_BSSID) it calls ieee80211_find_sta(), which returns NULL, > and the code skips over the part that handles BSS_CHANGED_BASIC_RATES. > > A bit later rtl_op_bss_info_changed() is called with BSS_CHANGED_ASSOC. > At this point ieee80211_find_sta() works, so set the basic rates from > here. > > Some of the code from BSS_CHANGED_BSSID which needs ieee80211_find_sta() > was already duplicated under BSS_CHANGED_ASSOC, so delete it from > BSS_CHANGED_BSSID. > > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > > I'm not sure if this is enough. Should we also handle > BSS_CHANGED_BASIC_RATES? But bss_conf->basic_rates is only 0xf (CCK > rates only) and the out-of-tree Realtek drivers want to use the 6, 12, > and 24M rates as well. If ieee80211_find_sta() returns NULL, how can we > know if OFDM rates are supported? > > I'm also not sure if it's okay to set the basic rates later than > originally intended, but it's still better than never. bss_conf->basic_rates is from AP beacon basically, and only the supported rates with 0x80 bit are basic rates, which is minimum rates requirement to the AP. Thus, I think it is not suitable to consider basic rates as RTS rate. Ping-Ke
On Mon, 2024-02-19 at 07:37 +0000, Ping-Ke Shih wrote: > > > > I'm not sure if this is enough. Should we also handle > > BSS_CHANGED_BASIC_RATES? But bss_conf->basic_rates is only 0xf (CCK > > rates only) and the out-of-tree Realtek drivers want to use the 6, 12, > > and 24M rates as well. If ieee80211_find_sta() returns NULL, how can we > > know if OFDM rates are supported? This whole find_sta is a bit questionable - basic rates are from the BSS configuration anyway? > > I'm also not sure if it's okay to set the basic rates later than > > originally intended, but it's still better than never. > > bss_conf->basic_rates is from AP beacon basically, and only the supported rates > with 0x80 bit are basic rates, which is minimum rates requirement to the AP. > Thus, I think it is not suitable to consider basic rates as RTS rate. > But you have to consider them? Control response rates are very precisely defined in the spec, see 10.6.6.5.2 "Selection of a rate or MCS". I also have a bunch of explanations about that in the iwlmvm driver in drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c around line 360. johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Monday, February 19, 2024 6:04 PM > To: Ping-Ke Shih <pkshih@realtek.com>; Bitterblue Smith <rtl8821cerfe2@gmail.com>; > linux-wireless@vger.kernel.org > Cc: Larry Finger <Larry.Finger@lwfinger.net> > Subject: Re: [PATCH] wifi: rtlwifi: Fix setting the basic rates > > I also have a bunch of explanations about that in the iwlmvm driver in > drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c around line 360. Thanks for the explanations. If no OFDM rates contained in basic rate, mandatory rate (6M, 12M and 24M) will be added. This is the point I also missed. Bitterblue, please use the same logic as iwlwifi to see if it works as expected. Ping-Ke
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index 2e60a6991ca1..7a68c528bcd2 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -1139,6 +1139,15 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw *hw, mac->mode = WIRELESS_MODE_N_24G; else mac->mode = WIRELESS_MODE_N_5G; + + mac->ht_enable = true; + + /* + * for cisco 1252 bw20 it's wrong + * if (ht_cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) { + * mac->bw_40 = true; + * } + * */ } if (sta->deflink.vht_cap.vht_supported) { @@ -1146,13 +1155,35 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw *hw, mac->mode = WIRELESS_MODE_AC_5G; else mac->mode = WIRELESS_MODE_AC_24G; + + mac->vht_enable = true; } - if (vif->type == NL80211_IFTYPE_STATION) + if (vif->type == NL80211_IFTYPE_STATION) { + struct rtl_sta_info *sta_entry; + + sta_entry = (struct rtl_sta_info *)sta->drv_priv; + /* Just station needs it, because ibss & ap mode + * will set in sta_add, and will be NULL here. + */ + sta_entry->wireless_mode = mac->mode; + rtlpriv->cfg->ops->update_rate_tbl(hw, sta, 0, true); + } + + /* for 5G must << RATE_6M_INDEX = 4, + * because 5G have no cck rate*/ + if (rtlhal->current_bandtype == BAND_ON_5G) + mac->basic_rates = sta->deflink.supp_rates[1] << 4; + else + mac->basic_rates = sta->deflink.supp_rates[0]; + rcu_read_unlock(); + rtlpriv->cfg->ops->set_hw_reg(hw, HW_VAR_BASIC_RATE, + (u8 *)(&mac->basic_rates)); + /* to avoid AP Disassociation caused by inactivity */ rtlpriv->cfg->ops->set_hw_reg(hw, HW_VAR_KEEP_ALIVE, @@ -1266,9 +1297,6 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw *hw, } if (changed & BSS_CHANGED_BSSID) { - u32 basic_rates; - struct ieee80211_sta *sta = NULL; - rtlpriv->cfg->ops->set_hw_reg(hw, HW_VAR_BSSID, (u8 *)bss_conf->bssid); @@ -1277,73 +1305,6 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw *hw, mac->vendor = PEER_UNKNOWN; memcpy(mac->bssid, bss_conf->bssid, ETH_ALEN); - - rcu_read_lock(); - sta = ieee80211_find_sta(vif, (u8 *)bss_conf->bssid); - if (!sta) { - rcu_read_unlock(); - goto out; - } - - if (rtlhal->current_bandtype == BAND_ON_5G) { - mac->mode = WIRELESS_MODE_A; - } else { - if (sta->deflink.supp_rates[0] <= 0xf) - mac->mode = WIRELESS_MODE_B; - else - mac->mode = WIRELESS_MODE_G; - } - - if (sta->deflink.ht_cap.ht_supported) { - if (rtlhal->current_bandtype == BAND_ON_2_4G) - mac->mode = WIRELESS_MODE_N_24G; - else - mac->mode = WIRELESS_MODE_N_5G; - } - - if (sta->deflink.vht_cap.vht_supported) { - if (rtlhal->current_bandtype == BAND_ON_5G) - mac->mode = WIRELESS_MODE_AC_5G; - else - mac->mode = WIRELESS_MODE_AC_24G; - } - - /* just station need it, because ibss & ap mode will - * set in sta_add, and will be NULL here */ - if (vif->type == NL80211_IFTYPE_STATION) { - struct rtl_sta_info *sta_entry; - - sta_entry = (struct rtl_sta_info *)sta->drv_priv; - sta_entry->wireless_mode = mac->mode; - } - - if (sta->deflink.ht_cap.ht_supported) { - mac->ht_enable = true; - - /* - * for cisco 1252 bw20 it's wrong - * if (ht_cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) { - * mac->bw_40 = true; - * } - * */ - } - - if (sta->deflink.vht_cap.vht_supported) - mac->vht_enable = true; - - if (changed & BSS_CHANGED_BASIC_RATES) { - /* for 5G must << RATE_6M_INDEX = 4, - * because 5G have no cck rate*/ - if (rtlhal->current_bandtype == BAND_ON_5G) - basic_rates = sta->deflink.supp_rates[1] << 4; - else - basic_rates = sta->deflink.supp_rates[0]; - - mac->basic_rates = basic_rates; - rtlpriv->cfg->ops->set_hw_reg(hw, HW_VAR_BASIC_RATE, - (u8 *)(&basic_rates)); - } - rcu_read_unlock(); } out: mutex_unlock(&rtlpriv->locks.conf_mutex);
RTL8192CU transmits RTS frames at 48M instead of the expected 24M. This is because rtlwifi never writes REG_INIRTS_RATE_SEL, because when rtl_op_bss_info_changed() is called with BSS_CHANGED_BASIC_RATES (and BSS_CHANGED_BSSID) it calls ieee80211_find_sta(), which returns NULL, and the code skips over the part that handles BSS_CHANGED_BASIC_RATES. A bit later rtl_op_bss_info_changed() is called with BSS_CHANGED_ASSOC. At this point ieee80211_find_sta() works, so set the basic rates from here. Some of the code from BSS_CHANGED_BSSID which needs ieee80211_find_sta() was already duplicated under BSS_CHANGED_ASSOC, so delete it from BSS_CHANGED_BSSID. Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- I'm not sure if this is enough. Should we also handle BSS_CHANGED_BASIC_RATES? But bss_conf->basic_rates is only 0xf (CCK rates only) and the out-of-tree Realtek drivers want to use the 6, 12, and 24M rates as well. If ieee80211_find_sta() returns NULL, how can we know if OFDM rates are supported? I'm also not sure if it's okay to set the basic rates later than originally intended, but it's still better than never. --- drivers/net/wireless/realtek/rtlwifi/core.c | 103 ++++++-------------- 1 file changed, 32 insertions(+), 71 deletions(-)