On 04/24, Arend van Spriel wrote:
> On 4/24/2025 1:46 PM, Johannes Berg wrote:
> > On Thu, 2025-04-24 at 13:36 +0200, Arend van Spriel wrote:
> > > On 4/24/2025 12:22 PM, Johannes Berg wrote:
> > > > On Thu, 2025-04-24 at 11:50 +0200, Arend van Spriel wrote:
> > > > >
> > > > > Looked at other drivers implementing this callback and here are the results:
> > > > >
> > > > > [wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
> > > > > [wilc1000] change_bss(): worse! it accepts everything and does nothing.
> > > > > [rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
> > > >
> > > > OK, though I guess other drivers being bad doesn't mean this one should
> > > > be :)
> > >
> > > Sure. I am on your team in this. Can you recommend a plan of attack
> > > here? Should we add a mechanism to expose what BSS parameter changes the
> > > driver can handle similar to what is used for struct
> > > station_info::bss_params?
> >
> > I don't even know off the top of my head what's there and what isn't,
> > just thought the code looked odd. I guess mac80211 just mostly supports
> > all of them anyway.
> >
> > Today the change is simply rejected by cfg80211 with -EOPNOTSUPP:
> >
> > static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
> > {
> > ...
> > if (!rdev->ops->change_bss)
> > return -EOPNOTSUPP;
> >
> >
> > why not keep doing that for everything but ap_isolate?
> >
> > Oh, I see what you mean, there's no way to keep that updated since you'd
> > have to check each value for being changed and reject that ...
> >
> > Hmm, yeah, that's not great. I guess ideally then we'd have a bitmap of
> > what changed, and what the driver can support?
> >
> > enum {
> > CFG80211_BSS_PARAM_CHANGE_CTS_PROT = BIT(0),
> > CFG80211_BSS_PARAM_CHANGE_SHORT_PRE = BIT(1),
> > CFG80211_BSS_PARAM_CHANGE_SHORT_SLOT = BIT(2),
> > ...
> > CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE = BIT(N),
> > ...
> > };
> >
> > and
> >
> > struct bss_parameters {
> > int link_id;
> > u32 changed;
> > ...
> > }
> >
> > and then this driver can just do
> >
> > if (params->changed & ~CFG80211_BSS_PARAM_CHANGE_AP_ISOLATE)
> > return -EOPNOTSUPP;
> >
> > or so?
IMHO, In CFG80211, if we introduce a bitmap to track which BSS Parameter
is changed by the userspace in the SET_BSS operation and then used this
bitmap while handling the request, then the WLAN Driver would reject the
operation with "EOPTNOTSUPP", instead of doing AP Isolation, because it
does not support setting the other BSS params in the request.
For Example, considering hostapd (iwd doesn't support SET BSS operation)
if the user only enabled the config file param "ap_isolate", but didn't
explicitly set "preamble", hostapd would implicitly set default value
(0 - LONG_PREAMBLE) in the preamble param while sending the SET_BSS
operation request to CFG80211.
CFG80211 would then mark the bit corresponding to the SHORT_PREAMBLE BSS
param as "changed" in the bitmap. WLAN Driver on receiving this SET_BSS
request, would then have to reject the entire operation without enabling
the user requested "ap_isolation" param, because of the preamble param
that is not even explicitly requested by the user.
In a way, we can consider this as an implementation specific behaviour
of the userspace application. But what i'm trying to convey is that the
userspace currently does not have a mechanism to know which subset of
BSS parameters are supported by the WLAN Drivers, so it is enabling
all the params while doing the SET_BSS operation. This increases the
chances for the entire SET_BSS operation getting rejected,
> > Not sure how that'd be related to station_info::bss_param, that's just
> > output from the driver.
>
> It kinda looked similar. At least there is a bitmap in place for it that
> is a subset of the enum you listed above.
I guess, you are referring to the enum bss_param_flags. Since it is
currently being used by drivers to set the fields specific to STA BSS
while returning the STATION info to cfg80211, we may not be able to
extended this enum with the new AP specific BSS params like ap_isolate.
enum bss_param_flags {
BSS_PARAM_FLAGS_CTS_PROT = BIT(0),
BSS_PARAM_FLAGS_SHORT_PREAMBLE = BIT(1),
BSS_PARAM_FLAGS_SHORT_SLOT_TIME = BIT(2)
};
Ideally renaming this from "bss_param_flags" -> "sta_bss_param_flags"
would be more appropriate, because these flags are only being used by
the struct member "station_info::bss::flags".
> I was wondering if we should convey the bitmask to usesr-space
> beforehand in struct wiphy.
Yes, if the WLAN driver is able to advertise the list of AP BSS params
that can be set bv userpace, then userspace can attempt to configure
only the BSS params that the driver can support, increasing the chances
for the SET_BSS operation to become successful.
And I can see the following two NL80211 feature flags already existing
which corresponds to the members in the AP/P2P-GO BSS Parameter struct.
@NL80211_FEATURE_P2P_GO_CTWIN: P2P GO implementation supports
CT Window setting
@NL80211_FEATURE_P2P_GO_OPPPS: P2P GO implementation supports
opportunistic powersave
However, for other AP BSS Parammeters, we don't have the corresponding
NL80211 feature flags.
Will wait for Johannes's opinion on this.
Regards,
Gokul
