[PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Lachlan Hodges]

mac80211 does not support TX/RX rate reporting for S1G bandwidths and
as a result WARNs occur during validation as the default value of 20MHz
(0) is always higher then the highest S1G bandwidth of 16MHz. This
prevents association from occuring in some instances.

Skip bandwidth validation for S1G interfaces to avoid spurious WARNs and
ensure a clean association process as there is no support for S1G TX/RX
bandwidths.

Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
---
This is kind of a bandaid fix (hence why RFC), but implementing proper
native rate control support for S1G within minstrel and mac80211 is not
pretty (lack of flag space, ordering of RX bandwidth enum, aliasing with
VHT flags and a myriad of other issues). If an S1G driver is submitted it
would be considered a "modern" driver and would probably contain its own
rate control. There has been some S1G hostap work recently submitted [1]
though it's only STA side though this will need to be fixed in order for
proper tests to be run.

hwsim tests were run, with the following errors occuring on the tip
31dc1462a Define QCA_NL80211_VENDOR_SUBCMD_GET_COEX_STATS. All dpp
errors are related to some OpenSSL python error and the bss config
file test fails to open some trace file.

dpp_config_connector_error_ext_sign
dpp_config_connector_error_too_short_timestamp
dpp_config_connector_error_invalid_timestamp_date
dpp_config_connector_error_invalid_timestamp
dpp_config_connector_error_invalid_time_zone
dpp_config_connector_error_invalid_time_zone_2
dpp_config_connector_error_expired_2
dpp_config_connector_error_expired_1
dpp_config_connector_error_expired_4
dpp_config_connector_error_expired_3
dpp_config_connector_error_expired_5
dpp_config_connector_error_expired_6
dpp_config_connector_error_missing_group_id
dpp_config_connector_error_no_groups
dpp_config_connector_error_empty_groups
dpp_config_connector_error_missing_net_role
dpp_config_connector_error_net_access_key_mismatch
dpp_config_connector_error_missing_net_access_key
ap_bss_config_file

[1] https://patchwork.ozlabs.org/project/hostap/cover/cover.1759334464.git.james@teledatics.com/
---
 drivers/net/wireless/virtual/mac80211_hwsim.c | 33 ++++++++++---------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index dde6a622ff72..e3688beb086b 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -2096,23 +2096,26 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
 				       txi->control.rates,
 				       ARRAY_SIZE(txi->control.rates));
 
-	for (i = 0; i < ARRAY_SIZE(txi->control.rates); i++) {
-		u16 rflags = txi->control.rates[i].flags;
-		/* initialize to data->bw for 5/10 MHz handling */
-		enum nl80211_chan_width bw = data->bw;
+	if (channel->band != NL80211_BAND_S1GHZ) {
+		for (i = 0; i < ARRAY_SIZE(txi->control.rates); i++) {
+			u16 rflags = txi->control.rates[i].flags;
+			/* initialize to data->bw for 5/10 MHz handling */
+			enum nl80211_chan_width bw = data->bw;
 
-		if (txi->control.rates[i].idx == -1)
-			break;
+			if (txi->control.rates[i].idx == -1)
+				break;
 
-		if (rflags & IEEE80211_TX_RC_40_MHZ_WIDTH)
-			bw = NL80211_CHAN_WIDTH_40;
-		else if (rflags & IEEE80211_TX_RC_80_MHZ_WIDTH)
-			bw = NL80211_CHAN_WIDTH_80;
-		else if (rflags & IEEE80211_TX_RC_160_MHZ_WIDTH)
-			bw = NL80211_CHAN_WIDTH_160;
+			if (rflags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+				bw = NL80211_CHAN_WIDTH_40;
+			else if (rflags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+				bw = NL80211_CHAN_WIDTH_80;
+			else if (rflags & IEEE80211_TX_RC_160_MHZ_WIDTH)
+				bw = NL80211_CHAN_WIDTH_160;
 
-		if (WARN_ON(hwsim_get_chanwidth(bw) > hwsim_get_chanwidth(confbw)))
-			return;
+			if (WARN_ON(hwsim_get_chanwidth(bw) >
+				    hwsim_get_chanwidth(confbw)))
+				return;
+		}
 	}
 
 	if (skb->len >= 24 + 8 &&
@@ -2677,7 +2680,7 @@ mac80211_hwsim_sta_rc_update(struct ieee80211_hw *hw,
 
 		link_sta = rcu_dereference(sta->link[link_id]);
 
-		if (!link_sta)
+		if (!link_sta || link_sta->s1g_cap.s1g)
 			continue;
 
 		switch (link_sta->bandwidth) {
-- 
2.43.0

Re: [PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Johannes Berg]

On Wed, 2025-10-08 at 12:40 +1100, Lachlan Hodges wrote:
> mac80211 does not support TX/RX rate reporting for S1G bandwidths and
> as a result WARNs occur during validation as the default value of 20MHz
> (0) is always higher then the highest S1G bandwidth of 16MHz. This
> prevents association from occuring in some instances.
> 
> Skip bandwidth validation for S1G interfaces to avoid spurious WARNs and
> ensure a clean association process as there is no support for S1G TX/RX
> bandwidths.
> 
> Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
> ---
> This is kind of a bandaid fix (hence why RFC), but implementing proper
> native rate control support for S1G within minstrel and mac80211 is not
> pretty (lack of flag space, ordering of RX bandwidth enum, aliasing with
> VHT flags and a myriad of other issues).

Yeah that makes sense. It might be more feasible with the table-based
approach (sta_rate_tbl_update), but there's not a lot of sense in adding
support for S1G to minstrel (or adding another algorithm) when no driver
would ever use it.

And a lot of the issues would still remain for feedback etc.

> If an S1G driver is submitted it
> would be considered a "modern" driver and would probably contain its own
> rate control. There has been some S1G hostap work recently submitted [1]
> though it's only STA side though this will need to be fixed in order for
> proper tests to be run.

I'm not sure really to be considered "modern" you need driver/firmware
rate control, but I'd assume a new driver were to do that anyway :)

So overall it seems reasonable to mostly ignore the rate control issue,
but maybe it'd make more sense to be "cleaner" about it, maybe
separating out something like HAS_RATE_CONTROL to be per station, i.e.
letting drivers somehow (not) set WLAN_STA_RATE_CONTROL?

And removing the checks here in tx() seems a bit odd, why should that
even be necessary? Surely _something_ would still need to be done here,
even if only to generate some sensible radiotap headers for the virtual
sniffer interface?

johannes

Re: [PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Lachlan Hodges]

> > If an S1G driver is submitted it
> > would be considered a "modern" driver and would probably contain its own
> > rate control. There has been some S1G hostap work recently submitted [1]
> > though it's only STA side though this will need to be fixed in order for
> > proper tests to be run.
> 
> I'm not sure really to be considered "modern" you need driver/firmware
> rate control, but I'd assume a new driver were to do that anyway :)
> 
> So overall it seems reasonable to mostly ignore the rate control issue,
> but maybe it'd make more sense to be "cleaner" about it, maybe
> separating out something like HAS_RATE_CONTROL to be per station, i.e.
> letting drivers somehow (not) set WLAN_STA_RATE_CONTROL?

I do agree - I have some initial rough thoughts -

Separating out HAS_RATE_CONTROL to be per station rather then per wiphy
seems to be a major change for just this single case? Now Im not sure what
Wi-Fi8 entails (heh) but I also understand it's best to do this properly rather
then some S1G specific hack.

It seems that introducing a flags parameter into the ieee80211_link_sta where
drivers can apply the inverse i.e LINK_STA_NO_RATE_CONTROL or something similar
that before drv_link_sta_rc_update() is called the flag can be tested, something
like, for example within rate_control_rate_update():

[...]
	if (sta->uploaded && !(link_sta->pub->flags & LINK_STA_NO_RC))
		drv_link_sta_rc_update(local, sta->sdata, link_sta->pub,
				       changed);
[...]

Then w.r.t setting the flag, we could introduce a new function that gets
called within mac80211_hwsim_sta_add() called mac80211_hwsim_link_sta_set_flags()
or something where it could enumerate the links of the ieee80211_sta and
apply the flag to the link with an S1G band channel context?

The reason for it being driver-facing is that any future upstream S1G drivers
may make use of the .sta_rc_update() callback.

Anyways, I probably need to do a bit more reading to see how best this can
be done.

> And removing the checks here in tx() seems a bit odd, why should that
> even be necessary? Surely _something_ would still need to be done here,
> even if only to generate some sensible radiotap headers for the virtual
> sniffer interface?

I think this can be left as is for now, still need to look into the radiotap
generation for S1G within hwsim.

lachlan

Re: [PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Johannes Berg]

On Tue, 2025-10-21 at 14:36 +1100, Lachlan Hodges wrote:
> > > If an S1G driver is submitted it
> > > would be considered a "modern" driver and would probably contain its own
> > > rate control. There has been some S1G hostap work recently submitted [1]
> > > though it's only STA side though this will need to be fixed in order for
> > > proper tests to be run.
> > 
> > I'm not sure really to be considered "modern" you need driver/firmware
> > rate control, but I'd assume a new driver were to do that anyway :)
> > 
> > So overall it seems reasonable to mostly ignore the rate control issue,
> > but maybe it'd make more sense to be "cleaner" about it, maybe
> > separating out something like HAS_RATE_CONTROL to be per station, i.e.
> > letting drivers somehow (not) set WLAN_STA_RATE_CONTROL?
> 
> I do agree - I have some initial rough thoughts -
> 
> Separating out HAS_RATE_CONTROL to be per station rather then per wiphy
> seems to be a major change for just this single case? 

Yeah, that's true.

> Now Im not sure what
> Wi-Fi8 entails (heh) but I also understand it's best to do this properly rather
> then some S1G specific hack.

WiFi8/UHR isn't going to change anything in this area, it's not really
any different from WiFi7/EHT wrt. this.

> It seems that introducing a flags parameter into the ieee80211_link_sta where
> drivers can apply the inverse i.e LINK_STA_NO_RATE_CONTROL or something similar
> that before drv_link_sta_rc_update() is called the flag can be tested, something
> like, for example within rate_control_rate_update():
> 
> [...]
> 	if (sta->uploaded && !(link_sta->pub->flags & LINK_STA_NO_RC))
> 		drv_link_sta_rc_update(local, sta->sdata, link_sta->pub,
> 				       changed);
> [...]
> 
> Then w.r.t setting the flag, we could introduce a new function that gets
> called within mac80211_hwsim_sta_add() called mac80211_hwsim_link_sta_set_flags()
> or something where it could enumerate the links of the ieee80211_sta and
> apply the flag to the link with an S1G band channel context?
> 
> The reason for it being driver-facing is that any future upstream S1G drivers
> may make use of the .sta_rc_update() callback.

But the .sta_rc_update() call is desired/needed either way (if
implemented) - the issue is about the _other_ side of this, "if (ref &&
ref->ops->rate_update)", no?

So I'd rather think we need something like

static int sta_prepare_rate_control(struct ieee80211_local *local,
                                    struct sta_info *sta, gfp_t gfp)
{
        if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL))
                return 0;

+	if (something new here)
+		return 0;
+
        sta->rate_ctrl = local->rate_ctrl;
        sta->rate_ctrl_priv = rate_control_alloc_sta(sta->rate_ctrl,
                                                     sta, gfp);
        if (!sta->rate_ctrl_priv)
                return -ENOMEM;

        return 0;
}


However, an issue is that the "something new here" can't check a per-STA
flag because this happens before the driver is actually told about the
station.

I suppose the easiest way, if a bit special to hwsim, would be to add a
specific flag for S1G, so that "something new here" becomes just

	on_s1g && ieee80211_hw_check(&local->hw, HAS_S1G_RATE_CONTROL)

(where on_s1g needs the sdata passed to the function and the chanctx
checked)

Then hwsim can sort it out or even just call rate_control_send_low() for
that all the time (which has a TODO for S1G buried, but that probably
needs addressing either way eventually.)

johannes

Re: [PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Lachlan Hodges]

> > It seems that introducing a flags parameter into the ieee80211_link_sta where
> > drivers can apply the inverse i.e LINK_STA_NO_RATE_CONTROL or something similar
> > that before drv_link_sta_rc_update() is called the flag can be tested, something
> > like, for example within rate_control_rate_update():
> > 
> > [...]
> > 	if (sta->uploaded && !(link_sta->pub->flags & LINK_STA_NO_RC))
> > 		drv_link_sta_rc_update(local, sta->sdata, link_sta->pub,
> > 				       changed);
> > [...]
> > 
> > Then w.r.t setting the flag, we could introduce a new function that gets
> > called within mac80211_hwsim_sta_add() called mac80211_hwsim_link_sta_set_flags()
> > or something where it could enumerate the links of the ieee80211_sta and
> > apply the flag to the link with an S1G band channel context?
> > 
> > The reason for it being driver-facing is that any future upstream S1G drivers
> > may make use of the .sta_rc_update() callback.
> 
> But the .sta_rc_update() call is desired/needed either way (if
> implemented) - the issue is about the _other_ side of this, "if (ref &&
> ref->ops->rate_update)", no?

So doing some reading, when rate control is instantiated every S1G frame
is sent at the low rate. Additionally, data->bw is accurate in this case
within mac80211_hwsim_tx unlike the sta_rc_update callback. Then since
the only rate flags used is IEEE80211_TX_RC_S1G_MCS, the Tx bandwidth
remains at data->bw. So I think for now, we can leave this as-is.

I also have some hwsim tests functioning - testing the various channel
permutations (soon to be hopefully in hostapd =)) and this part seems fine,
the problem is the link sta bandwidth validation which does indeed
cause issues with hwsim association.

> So I'd rather think we need something like
> 
> static int sta_prepare_rate_control(struct ieee80211_local *local,
>                                     struct sta_info *sta, gfp_t gfp)
> {
>         if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL))
>                 return 0;
> 
> +	if (something new here)
> +		return 0;
> +
>         sta->rate_ctrl = local->rate_ctrl;
>         sta->rate_ctrl_priv = rate_control_alloc_sta(sta->rate_ctrl,
>                                                      sta, gfp);
>         if (!sta->rate_ctrl_priv)
>                 return -ENOMEM;
> 
>         return 0;
> }

So as mentioned above, I think for now this probably isn't needed? Also same
for the comment w.r.t hwsim manually sending rate_control_send_low() since
the rate subsystem is already doing that?

> However, an issue is that the "something new here" can't check a per-STA
> flag because this happens before the driver is actually told about the
> station.
> 
> I suppose the easiest way, if a bit special to hwsim, would be to add a
> specific flag for S1G, so that "something new here" becomes just
> 
> 	on_s1g && ieee80211_hw_check(&local->hw, HAS_S1G_RATE_CONTROL)
> 
> (where on_s1g needs the sdata passed to the function and the chanctx
> checked)
> 
> Then hwsim can sort it out or even just call rate_control_send_low() for
> that all the time (which has a TODO for S1G buried, but that probably
> needs addressing either way eventually.)

Now with all this said, this still doesn't fix the issue regarding the
link_sta rc update ? Since link_sta->bandwidth will always be 20MHz for an
S1G interface. This is why I brought up the link sta flags which I believe
could actually just go in the STA since MLO is prohibited for carriers < 1GHz.

lachlan

Re: [PATCH RFC wireless-next] wifi: mac80211_hwsim: dont perform rate validation for S1G

[Lachlan Hodges]

Hi Johannes,

I am revisiting this thread again with some fresh eyes;

> > It seems that introducing a flags parameter into the ieee80211_link_sta where
> > drivers can apply the inverse i.e LINK_STA_NO_RATE_CONTROL or something similar
> > that before drv_link_sta_rc_update() is called the flag can be tested, something
> > like, for example within rate_control_rate_update():
> > 
> > [...]
> > 	if (sta->uploaded && !(link_sta->pub->flags & LINK_STA_NO_RC))
> > 		drv_link_sta_rc_update(local, sta->sdata, link_sta->pub,
> > 				       changed);
> > [...]
> > 
> > Then w.r.t setting the flag, we could introduce a new function that gets
> > called within mac80211_hwsim_sta_add() called mac80211_hwsim_link_sta_set_flags()
> > or something where it could enumerate the links of the ieee80211_sta and
> > apply the flag to the link with an S1G band channel context?
> > 
> > The reason for it being driver-facing is that any future upstream S1G drivers
> > may make use of the .sta_rc_update() callback.
> 
> But the .sta_rc_update() call is desired/needed either way (if
> implemented) - the issue is about the _other_ side of this, "if (ref &&
> ref->ops->rate_update)", no?

The actual call into .sta_rc_update() isn't the issue here from mac80211,
but rather the call from mac80211_hwsim_sta_add(). Just for a refresher
on this main idea of this thread - is that during the hwsim tests that
I've made for hostapd the WARN is hit in mac80211_hwsim_sta_rc_update()
since there is no understanding of S1G widths.

But actually looking at this again, even during normal S1G operation
at the current point .sta_rc_update() is never called since there's no
channel switching support and only identical chanctxs are allowed.
In the future you are right we would like this to be supported, but
that is a fair chunk of work down the road ^.^.

Anyways a potentially nicer solution is to only call sta_rc_update()
for non-S1G chanctxs. Something like this:

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim_main.c b/drivers/net/wireless/virtual/mac80211_hwsim_main.c
index 3a0c4366dfdb..a0e81282d42a 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim_main.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim_main.c
@@ -2788,9 +2788,21 @@ static int mac80211_hwsim_sta_add(struct ieee80211_hw *hw,
                                  struct ieee80211_sta *sta)
 {
        struct hwsim_sta_priv *sp = (void *)sta->drv_priv;
+       struct ieee80211_chanctx_conf *conf;

        hwsim_check_magic(vif);
        hwsim_set_sta_magic(sta);
+
+       /*
+        * Don't perform the RC update for S1G chandefs. Since S1G cannot
+        * exist during MLO operation we can take the vif->bss_conf chandef.
+        */
+       scoped_guard(rcu) {
+               conf = rcu_dereference(vif->bss_conf.chanctx_conf);
+               if (conf && cfg80211_chandef_is_s1g(&conf->def))
+                       return 0;
+       }
+
        mac80211_hwsim_sta_rc_update(hw, vif, &sta->deflink, 0);

        if (sta->valid_links) {

I do agree that this is still somewhat hacky, but I think it's a bit
clearer now maybe? I'm not sure. This sort of allows us to get some
basic hwsim infra up and running without introducing a new flag for
S1G rate control and such. I've run the hwsim tests and it seems to
not break any of the MLO tests but I am not an expert there so yea.

Let me know your thoughts, can always explore some other options.

lachlan