| Message ID | 20230906103458.24092-4-quic_wgong@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: change to match driver to support MLO connection | expand |
On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: > Currently for MLO connection, only deflink's rx_nss is set to correct > value. The others links' rx_nss of struct ieee80211_link_sta is > value 0 in ieee80211_set_associated(), because they are not pass into > ieee80211_sta_set_rx_nss() in mac80211 except the deflink in > rate_control_rate_init(). This leads driver get NSS = 0 for other links. > Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), > then the other links' rx_nss will be set to the correct value. This is pretty much true, but I also think it's problematic the way you phrase it. Software rate control is pretty much, at least currently, _not_ supported for MLO (and I don't really see how to support it, if firmware picks the link to transmit on, as it probably should). Thus, I'm not even sure we should be calling rate_control_rate_init(). Clearly we do today, but it's also obviously wrong for everything except the call to ieee80211_sta_set_rx_nss(). So while I agree that there's a problem with the RX NSS, I disagree that this patch is the right way to fix it. Yes, it also fairly obviously fixes the problem, but it just makes an existing design problem worse. Please change change the overall design here so that ieee80211_sta_set_rx_nss() isn't related to rate control at all. johannes
On 9/13/2023 5:04 PM, Johannes Berg wrote: > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: >> Currently for MLO connection, only deflink's rx_nss is set to correct >> value. The others links' rx_nss of struct ieee80211_link_sta is >> value 0 in ieee80211_set_associated(), because they are not pass into >> ieee80211_sta_set_rx_nss() in mac80211 except the deflink in >> rate_control_rate_init(). This leads driver get NSS = 0 for other links. >> Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), >> then the other links' rx_nss will be set to the correct value. > This is pretty much true, but I also think it's problematic the way you > phrase it. Software rate control is pretty much, at least currently, > _not_ supported for MLO (and I don't really see how to support it, if > firmware picks the link to transmit on, as it probably should). > > Thus, I'm not even sure we should be calling rate_control_rate_init(). > Clearly we do today, but it's also obviously wrong for everything except > the call to ieee80211_sta_set_rx_nss(). > > So while I agree that there's a problem with the RX NSS, I disagree that > this patch is the right way to fix it. Yes, it also fairly obviously > fixes the problem, but it just makes an existing design problem worse. > > Please change change the overall design here so that > ieee80211_sta_set_rx_nss() isn't related to rate control at all. > > johannes So should I delete ieee80211_sta_set_rx_nss() in rate_control_rate_init(), and add it into ieee80211_assoc_config_link() as you said before here? https://lore.kernel.org/linux-wireless/ca0f6ea2d78538ffb6640f2e56d65c89c86f5221.camel@sipsolutions.net/ I checked the git log, ieee80211_sta_set_rx_nss() is added into rate_control_rate_init() here for VHT, so is it correct to delete ieee80211_sta_set_rx_nss() in rate_control_rate_init()? https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?id=8921d04e8df7475d733d853564bdb001e83bf33f >
On 9/13/2023 5:04 PM, Johannes Berg wrote: > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: >> Currently for MLO connection, only deflink's rx_nss is set to correct >> value. The others links' rx_nss of struct ieee80211_link_sta is >> value 0 in ieee80211_set_associated(), because they are not pass into >> ieee80211_sta_set_rx_nss() in mac80211 except the deflink in >> rate_control_rate_init(). This leads driver get NSS = 0 for other links. >> Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), >> then the other links' rx_nss will be set to the correct value. > This is pretty much true, but I also think it's problematic the way you > phrase it. Software rate control is pretty much, at least currently, > _not_ supported for MLO (and I don't really see how to support it, if > firmware picks the link to transmit on, as it probably should). I searched all the folder wireless, I only found 5 places have the handler of rate_init below, and the functions are all empty. Is it means no driver support rate_init currently? drivers/net/wireless/realtek/rtlwifi/rc.c:304: .rate_init = rtl_rate_init, drivers/net/wireless/intel/iwlwifi/mvm/rs.c:4098: .rate_init = rs_rate_init_ops, drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3276: .rate_init = rs_rate_init_stub, drivers/net/wireless/intel/iwlegacy/4965-rs.c:2779: .rate_init = il4965_rs_rate_init_stub, drivers/net/wireless/intel/iwlegacy/3945-rs.c:867: .rate_init = il3945_rs_rate_init_stub, [...]
On Fri, 2023-09-15 at 15:53 +0800, Wen Gong wrote: > On 9/13/2023 5:04 PM, Johannes Berg wrote: > > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: > > > Currently for MLO connection, only deflink's rx_nss is set to correct > > > value. The others links' rx_nss of struct ieee80211_link_sta is > > > value 0 in ieee80211_set_associated(), because they are not pass into > > > ieee80211_sta_set_rx_nss() in mac80211 except the deflink in > > > rate_control_rate_init(). This leads driver get NSS = 0 for other links. > > > Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), > > > then the other links' rx_nss will be set to the correct value. > > This is pretty much true, but I also think it's problematic the way you > > phrase it. Software rate control is pretty much, at least currently, > > _not_ supported for MLO (and I don't really see how to support it, if > > firmware picks the link to transmit on, as it probably should). > > > > Thus, I'm not even sure we should be calling rate_control_rate_init(). > > Clearly we do today, but it's also obviously wrong for everything except > > the call to ieee80211_sta_set_rx_nss(). > > > > So while I agree that there's a problem with the RX NSS, I disagree that > > this patch is the right way to fix it. Yes, it also fairly obviously > > fixes the problem, but it just makes an existing design problem worse. > > > > Please change change the overall design here so that > > ieee80211_sta_set_rx_nss() isn't related to rate control at all. > > > > johannes > So should I delete ieee80211_sta_set_rx_nss() in rate_control_rate_init(), > and add it into ieee80211_assoc_config_link() as you said before here? > https://lore.kernel.org/linux-wireless/ca0f6ea2d78538ffb6640f2e56d65c89c86f5221.camel@sipsolutions.net/ I think that would make sense. After all, rate_control_rate_init() is related to the software rate control which isn't really supported with MLD, and the NSS init is unrelated, it's just updating a piece of per (link) station data. > I checked the git log, ieee80211_sta_set_rx_nss() is added into > rate_control_rate_init() here for VHT, so is it correct to delete > ieee80211_sta_set_rx_nss() in rate_control_rate_init()? > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?id=8921d04e8df7475d733d853564bdb001e83bf33f > > Well we'll have to call it appropriately when rate_control_rate_init() is called today, and then the new places in your patch, I guess. But I per the above that makes more sense semantically, since we don't support software rate control on link stations. johannes
On Fri, 2023-09-15 at 16:43 +0800, Wen Gong wrote: > On 9/13/2023 5:04 PM, Johannes Berg wrote: > > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: > > > Currently for MLO connection, only deflink's rx_nss is set to correct > > > value. The others links' rx_nss of struct ieee80211_link_sta is > > > value 0 in ieee80211_set_associated(), because they are not pass into > > > ieee80211_sta_set_rx_nss() in mac80211 except the deflink in > > > rate_control_rate_init(). This leads driver get NSS = 0 for other links. > > > Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), > > > then the other links' rx_nss will be set to the correct value. > > This is pretty much true, but I also think it's problematic the way you > > phrase it. Software rate control is pretty much, at least currently, > > _not_ supported for MLO (and I don't really see how to support it, if > > firmware picks the link to transmit on, as it probably should). > I searched all the folder wireless, I only found 5 places have the > handler of rate_init below, > and the functions are all empty. Is it means no driver support rate_init > currently? > drivers/net/wireless/realtek/rtlwifi/rc.c:304: .rate_init = > rtl_rate_init, > drivers/net/wireless/intel/iwlwifi/mvm/rs.c:4098: .rate_init = > rs_rate_init_ops, > drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3276: .rate_init = > rs_rate_init_stub, > drivers/net/wireless/intel/iwlegacy/4965-rs.c:2779: .rate_init = > il4965_rs_rate_init_stub, > drivers/net/wireless/intel/iwlegacy/3945-rs.c:867: .rate_init = > il3945_rs_rate_init_stub, > [...] > Well, many other drivers use minstrel, and the _stub ones in intel are atually not doing anything. So most drivers would use net/mac80211/rc80211_minstrel_ht.c: .rate_init = minstrel_ht_rate_init, here. johannes
On 9/26/2023 5:42 PM, Johannes Berg wrote: > On Fri, 2023-09-15 at 15:53 +0800, Wen Gong wrote: >> On 9/13/2023 5:04 PM, Johannes Berg wrote: >>> On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: >>>> Currently for MLO connection, only deflink's rx_nss is set to correct >>>> value. The others links' rx_nss of struct ieee80211_link_sta is >>>> value 0 in ieee80211_set_associated(), because they are not pass into >>>> ieee80211_sta_set_rx_nss() in mac80211 except the deflink in >>>> rate_control_rate_init(). This leads driver get NSS = 0 for other links. >>>> Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), >>>> then the other links' rx_nss will be set to the correct value. >>> This is pretty much true, but I also think it's problematic the way you >>> phrase it. Software rate control is pretty much, at least currently, >>> _not_ supported for MLO (and I don't really see how to support it, if >>> firmware picks the link to transmit on, as it probably should). >>> >>> Thus, I'm not even sure we should be calling rate_control_rate_init(). >>> Clearly we do today, but it's also obviously wrong for everything except >>> the call to ieee80211_sta_set_rx_nss(). >>> >>> So while I agree that there's a problem with the RX NSS, I disagree that >>> this patch is the right way to fix it. Yes, it also fairly obviously >>> fixes the problem, but it just makes an existing design problem worse. >>> >>> Please change change the overall design here so that >>> ieee80211_sta_set_rx_nss() isn't related to rate control at all. >>> >>> johannes >> So should I delete ieee80211_sta_set_rx_nss() in rate_control_rate_init(), >> and add it into ieee80211_assoc_config_link() as you said before here? >> https://lore.kernel.org/linux-wireless/ca0f6ea2d78538ffb6640f2e56d65c89c86f5221.camel@sipsolutions.net/ > I think that would make sense. After all, rate_control_rate_init() is > related to the software rate control which isn't really supported with > MLD, and the NSS init is unrelated, it's just updating a piece of per > (link) station data. > >> I checked the git log, ieee80211_sta_set_rx_nss() is added into >> rate_control_rate_init() here for VHT, so is it correct to delete >> ieee80211_sta_set_rx_nss() in rate_control_rate_init()? >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?id=8921d04e8df7475d733d853564bdb001e83bf33f > Well we'll have to call it appropriately when rate_control_rate_init() > is called today, and then the new places in your patch, I guess. > > But I per the above that makes more sense semantically, since we don't > support software rate control on link stations. > > johannes OK. So I will send new version patch to remove the ieee80211_sta_set_rx_nss() in rate_control_rate_init() as you said.
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 1679d5011fb6..0a2fb660fa00 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -4354,6 +4354,8 @@ static bool ieee80211_assoc_config_link(struct ieee80211_link_data *link, bss_conf->assoc_capability = capab_info; ret = true; + + ieee80211_sta_set_rx_nss(link_sta); out: kfree(elems); kfree(bss_ies);
Currently for MLO connection, only deflink's rx_nss is set to correct value. The others links' rx_nss of struct ieee80211_link_sta is value 0 in ieee80211_set_associated(), because they are not pass into ieee80211_sta_set_rx_nss() in mac80211 except the deflink in rate_control_rate_init(). This leads driver get NSS = 0 for other links. Add the ieee80211_sta_set_rx_nss() in ieee80211_assoc_config_link(), then the other links' rx_nss will be set to the correct value. Signed-off-by: Wen Gong <quic_wgong@quicinc.com> --- net/mac80211/mlme.c | 2 ++ 1 file changed, 2 insertions(+)