v7: Resolved conflicts with MLO code changes.
FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP, however both stop
after userspace uses the NL80211_CMD_SET_BEACON command as these
attributes are not processed as part of this command.
- Modify the local variable in nl80211_set_beacon() and input parameter
to rdev_change_beacon() from type struct cfg80211_beacon_data to
type struct cfg80211_ap_settings to support the new processing.
- Modify ieee80211_change_beacon() to reflect the new input parameter type.
- Modify driver specific functions pointed by change_beacon to reflect
the new input parameter type.
- Add the missing implementation in nl80211 and mac80211 to process
FILS discovery and unsolicited broadcast probe response configurations.
Aloka Dixit (3):
cfg80211: modify prototype for change_beacon
nl80211: additional processing in NL80211_CMD_SET_BEACON
mac80211: additional processing in ieee80211_change_beacon
drivers/net/wireless/ath/ath6kl/cfg80211.c | 4 +-
drivers/net/wireless/ath/wil6210/cfg80211.c | 3 +-
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 4 +-
.../net/wireless/marvell/mwifiex/cfg80211.c | 3 +-
.../wireless/microchip/wilc1000/cfg80211.c | 4 +-
.../net/wireless/quantenna/qtnfmac/cfg80211.c | 4 +-
.../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 6 ++-
include/net/cfg80211.h | 2 +-
net/mac80211/cfg.c | 38 ++++++++++---
net/wireless/nl80211.c | 28 ++++++++--
net/wireless/rdev-ops.h | 2 +-
net/wireless/trace.h | 54 ++++++++++---------
12 files changed, 102 insertions(+), 50 deletions(-)
base-commit: b8f6efccbb9dc0ff5dee7e20d69a4747298ee603
--
2.17.1
FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP, however both stop
after userspace uses the NL80211_CMD_SET_BEACON command as these
attributes are not processed in that command.
Add the missing implementation.
Signed-off-by: Aloka Dixit <[email protected]>
---
net/wireless/nl80211.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4b0b02fc822c..95de9e006444 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6069,6 +6069,7 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_ap_settings *params;
+ struct nlattr *attrs;
int err;
if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
@@ -6089,6 +6090,20 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
if (err)
goto out;
+ attrs = info->attrs[NL80211_ATTR_FILS_DISCOVERY];
+ if (attrs) {
+ err = nl80211_parse_fils_discovery(rdev, attrs, params);
+ if (err)
+ goto out;
+ }
+
+ attrs = info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP];
+ if (attrs) {
+ err = nl80211_parse_unsol_bcast_probe_resp(rdev, attrs, params);
+ if (err)
+ goto out;
+ }
+
wdev_lock(wdev);
err = rdev_change_beacon(rdev, dev, params);
wdev_unlock(wdev);
--
2.17.1
On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
> FILS discovery and unsolicited broadcast probe response transmissions
> are configured as part of NL80211_CMD_START_AP, however both stop
> after userspace uses the NL80211_CMD_SET_BEACON command as these
> attributes are not processed in that command.
That seems odd. Why would that *stop*? Nothing in the driver should
actually update it to _remove_ it during change_beacon()?
Are you sure you didn't mean to "just" say "however both cannot be
updated as these attributes ..."?
johannes
On 1/18/2023 7:57 AM, Johannes Berg wrote:
> On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
>> FILS discovery and unsolicited broadcast probe response transmissions
>> are configured as part of NL80211_CMD_START_AP, however both stop
>> after userspace uses the NL80211_CMD_SET_BEACON command as these
>> attributes are not processed in that command.
>
> That seems odd. Why would that *stop*? Nothing in the driver should
> actually update it to _remove_ it during change_beacon()?
> > Are you sure you didn't mean to "just" say "however both cannot be
> updated as these attributes ..."?
>
> johannes
FILS discovery has primary channel, center frequency in the frame format
which should be changed depending on why the beacon changed. Hence the
current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
changed, means disable these features.
What do you think?
Like you said, we still need this code to update these templates if
provided by the userspace, e.g. in case of channel switch.
Should I send a follow-up with a different commit log?
Thanks.
On Thu, 2023-01-19 at 11:40 -0800, Aloka Dixit wrote:
> On 1/18/2023 7:57 AM, Johannes Berg wrote:
> > On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
> > > FILS discovery and unsolicited broadcast probe response transmissions
> > > are configured as part of NL80211_CMD_START_AP, however both stop
> > > after userspace uses the NL80211_CMD_SET_BEACON command as these
> > > attributes are not processed in that command.
> >
> > That seems odd. Why would that *stop*? Nothing in the driver should
> > actually update it to _remove_ it during change_beacon()?
> > > Are you sure you didn't mean to "just" say "however both cannot be
> > updated as these attributes ..."?
> >
> > johannes
>
> FILS discovery has primary channel, center frequency in the frame format
> which should be changed depending on why the beacon changed.
Hmm? Sure, the frequency is important, but beacon can change for so many
other reasons? Even just the greenfield bit in HT would cause a beacon
change as far as cfg80211/mac80211 are concerned.
> Hence the
> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
> changed, means disable these features.
> What do you think?
I think that makes no sense? If mac80211 didn't clear struct
ieee80211_bss_conf::fils_discovery, then surely it should stick around
even if the beacon changes???
Surely you can't be right - that'd basically mean the whole feature is
useless right now because even the greenfield update or similar that
basically *always* happens in hostapd would disable it, since the beacon
changes and we don't have these patches?
> Should I send a follow-up with a different commit log?
>
Well seems like we need to first figure out the correct semantics here,
and possibly fix things...
johannes
On Thu, 2023-01-19 at 12:43 -0800, Aloka Dixit wrote:
> >
> > > Hence the
> > > current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
> > > and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
> > > changed, means disable these features.
> > > What do you think?
> >
> > I think that makes no sense? If mac80211 didn't clear struct
> > ieee80211_bss_conf::fils_discovery, then surely it should stick around
> > even if the beacon changes???
> >
> "max_interval" was be used as the enable/disable knob for these
> features. Non-zero = enable, zero = disable.
> But the side effect of that is if NL80211 does not receive these
> attributes then by default the interval is set to 0.
But it doesn't change in bss_conf if you send change beacon, at least
not before these patches?
Or am I confusing myself?
> I can think of following:
> (1) max_interval cannot be the enable/disable knob because then every
> code path in the userspace would still need to set it to non-zero to
> continue transmission. Are you okay with having extra members in enum
> nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
> what you suggested in your comment for the next patch in this series as
> well.
>
> (2) If the template needs changing for any reason then the userspace
> will be responsible to send a new one. Until then the driver will
> continue the transmission with existing template and interval unless
> DISABLE is used.
>
Were those meant to be mutually exclusive, because it doesn't seem like
that to me? I think (2) must be what happens now, before these patches,
because I don't see where it would be changed? Like I said above.
I agree that we'd now need an explicit way to indicate "disable", but we
could for example say you disable by adding a nested
NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
which logically kind of makes sense - you're changing
NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
parameters, so logically that means disable?
johannes
On 1/19/2023 12:12 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 11:40 -0800, Aloka Dixit wrote:
>> On 1/18/2023 7:57 AM, Johannes Berg wrote:
>>> On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
>>>> FILS discovery and unsolicited broadcast probe response transmissions
>>>> are configured as part of NL80211_CMD_START_AP, however both stop
>>>> after userspace uses the NL80211_CMD_SET_BEACON command as these
>>>> attributes are not processed in that command.
>>>
>>> That seems odd. Why would that *stop*? Nothing in the driver should
>>> actually update it to _remove_ it during change_beacon()?
>>>> Are you sure you didn't mean to "just" say "however both cannot be
>>> updated as these attributes ..."?
>>>
>>> johannes
>>
>> FILS discovery has primary channel, center frequency in the frame format
>> which should be changed depending on why the beacon changed.
>
> Hmm? Sure, the frequency is important, but beacon can change for so many
> other reasons? Even just the greenfield bit in HT would cause a beacon
> change as far as cfg80211/mac80211 are concerned.
>
>> Hence the
>> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
>> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
>> changed, means disable these features.
>> What do you think?
>
> I think that makes no sense? If mac80211 didn't clear struct
> ieee80211_bss_conf::fils_discovery, then surely it should stick around
> even if the beacon changes???
>
"max_interval" was be used as the enable/disable knob for these
features. Non-zero = enable, zero = disable.
But the side effect of that is if NL80211 does not receive these
attributes then by default the interval is set to 0.
> Surely you can't be right - that'd basically mean the whole feature is
> useless right now because even the greenfield update or similar that
> basically *always* happens in hostapd would disable it, since the beacon
> changes and we don't have these patches?
>Pretty much, yeah.
>> Should I send a follow-up with a different commit log?
>>
>
> Well seems like we need to first figure out the correct semantics here,
> and possibly fix things...
>
> johannes
I can think of following:
(1) max_interval cannot be the enable/disable knob because then every
code path in the userspace would still need to set it to non-zero to
continue transmission. Are you okay with having extra members in enum
nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
what you suggested in your comment for the next patch in this series as
well.
(2) If the template needs changing for any reason then the userspace
will be responsible to send a new one. Until then the driver will
continue the transmission with existing template and interval unless
DISABLE is used.
Thanks.
On 1/19/2023 12:47 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 12:43 -0800, Aloka Dixit wrote:
>>>
>>>> Hence the
>>>> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
>>>> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
>>>> changed, means disable these features.
>>>> What do you think?
>>>
>>> I think that makes no sense? If mac80211 didn't clear struct
>>> ieee80211_bss_conf::fils_discovery, then surely it should stick around
>>> even if the beacon changes???
>>>
>> "max_interval" was be used as the enable/disable knob for these
>> features. Non-zero = enable, zero = disable.
>> But the side effect of that is if NL80211 does not receive these
>> attributes then by default the interval is set to 0.
>
>
> But it doesn't change in bss_conf if you send change beacon, at least
> not before these patches?
>
> Or am I confusing myself?
>
Your understanding is correct, however, without these patches there is a
cascading effect.
-> NL80211: If the attribute is not sent by user-space/processed in this
layer then cfg80211_ap_settings->fils_discovery->max_interval is 0
(default).
-> MAC80211: max_interval=0, hence BSS_CHANGED_FILS_DISCOVERY is not set
-> ath11k: BSS_CHANGED_FILS_DISCOVERY is not set hence driver doesn't
configure/re-configure. Unless target doesn't receive it every beacon
change, it disables it.
I can make changes up to the driver to fix this part.
>> I can think of following:
>> (1) max_interval cannot be the enable/disable knob because then every
>> code path in the userspace would still need to set it to non-zero to
>> continue transmission. Are you okay with having extra members in enum
>> nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
>> what you suggested in your comment for the next patch in this series as
>> well.
>>
>> (2) If the template needs changing for any reason then the userspace
>> will be responsible to send a new one. Until then the driver will
>> continue the transmission with existing template and interval unless
>> DISABLE is used.
>>
>
> Were those meant to be mutually exclusive, because it doesn't seem like
> that to me? I think (2) must be what happens now, before these patches,
> because I don't see where it would be changed? Like I said above.
>
Not exclusive. I meant I can make both the changes above to not have the
above mentioned cascading effect
> I agree that we'd now need an explicit way to indicate "disable", but we
> could for example say you disable by adding a nested
> NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
> which logically kind of makes sense - you're changing
> NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
> parameters, so logically that means disable?
>
> johannes
Sure, will update nl80211_parse_fils_discovery() to allow this case and
reset all to 0/null etc.
I can start a new series with all the changes, and include current
patches last.
Will that work?
Hi Aloka,
Sorry, clearly I dropped the ball on this.
On Thu, 2023-01-19 at 13:19 -0800, Aloka Dixit wrote:
> > But it doesn't change in bss_conf if you send change beacon, at least
> > not before these patches?
> >
> > Or am I confusing myself?
> >
>
> Your understanding is correct, however, without these patches there is a
> cascading effect.
> -> NL80211: If the attribute is not sent by user-space/processed in this
> layer then cfg80211_ap_settings->fils_discovery->max_interval is 0
> (default).
> -> MAC80211: max_interval=0, hence BSS_CHANGED_FILS_DISCOVERY is not set
> -> ath11k: BSS_CHANGED_FILS_DISCOVERY is not set hence driver doesn't
> configure/re-configure. Unless target doesn't receive it every beacon
> change, it disables it.
Hmm. But if max_interval is 0 in bss_conf then the driver might still
look at it even if BSS_CHANGED_FILS_DISCOVERY is not set, for example
for firmware restart? It seems bad to rely on the change flag only for
all this.
> I can make changes up to the driver to fix this part.
Not sure it's a driver change?
> > > I can think of following:
> > > (1) max_interval cannot be the enable/disable knob because then every
> > > code path in the userspace would still need to set it to non-zero to
> > > continue transmission. Are you okay with having extra members in enum
> > > nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is
> > > what you suggested in your comment for the next patch in this series as
> > > well.
> > >
> > > (2) If the template needs changing for any reason then the userspace
> > > will be responsible to send a new one. Until then the driver will
> > > continue the transmission with existing template and interval unless
> > > DISABLE is used.
> > >
> >
> > Were those meant to be mutually exclusive, because it doesn't seem like
> > that to me? I think (2) must be what happens now, before these patches,
> > because I don't see where it would be changed? Like I said above.
> >
>
> Not exclusive. I meant I can make both the changes above to not have the
> above mentioned cascading effect
Right ok.
>
> > I agree that we'd now need an explicit way to indicate "disable", but we
> > could for example say you disable by adding a nested
> > NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
> > which logically kind of makes sense - you're changing
> > NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
> > parameters, so logically that means disable?
> >
> > johannes
>
> Sure, will update nl80211_parse_fils_discovery() to allow this case and
> reset all to 0/null etc.
>
> I can start a new series with all the changes, and include current
> patches last.
>
> Will that work?
I think yes, seems like we have to fix up some things here first.
johannes