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@xxxxxxxxxxxxxxxx/ 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
