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@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
OK. So I will send new version patch to remove the
ieee80211_sta_set_rx_nss() in rate_control_rate_init() as you said.
