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/[email protected]/
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/[email protected]/
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 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/[email protected]/
> 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.
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