v8: Patches #1, #2 are new in this version which allow resetting
the interval to 0 once set to non-zero which was not possible earlier.
No functional changes to the remaining three patches from v7 here:
https://patchwork.kernel.org/project/linux-wireless/list/?series=693804&state=%2A&archive=both
v7: Resolved conflicts with MLO code changes.
FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP. The configurations
may get changed whenever there is a change in beacon, e. g. when
a channel switch operation completes the new templates will be sent
by the userspace which reflects the new channel bandwidth. Process the
attributes for these features in NL80211_CMD_SET_BEACON as well.
- Replace the check for interval (for both features) with a new flag
'update' which is set only when the userspace requests an update to
the configuration. This allows the interval to be set to 0 and
templates deleted which wasn't allowed earlier as the attributes got
processed only if the interval was non-zero.
- 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 (5):
wifi: nl80211: fixes to FILS discovery updates
wifi: mac80211: fixes in FILS discovery updates
wifi: cfg80211: modify prototype for change_beacon
wifi: nl80211: additions to NL80211_CMD_SET_BEACON
wifi: mac80211: additions to 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 | 6 +-
include/uapi/linux/nl80211.h | 11 +-
net/mac80211/cfg.c | 102 ++++++++++--------
net/wireless/nl80211.c | 47 ++++++--
net/wireless/rdev-ops.h | 2 +-
net/wireless/trace.h | 54 +++++-----
13 files changed, 159 insertions(+), 91 deletions(-)
base-commit: b2090d93d4b6f1c72a9793d5a171806b8468b7cb
--
2.39.0
FILS discovery and unsolicited broadcast probe response templates
need to be updated along with beacon templates in some cases such as
the channel switch operation. Add the missing implementation.
Signed-off-by: Aloka Dixit <[email protected]>
---
v8: No changes.
net/wireless/nl80211.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5e087534a12e..bb1ab78d3f27 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6239,6 +6239,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 &&
@@ -6260,6 +6261,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.39.0
FILS discovery configuration gets updated only if the maximum interval
is set to a non-zero value, hence there is no way to reset this value
to 0 once set. Replace the check for interval with a new flag which is
set only if the configuration should be updated.
Add similar changes for the unsolicited broadcast probe response handling.
Signed-off-by: Aloka Dixit <[email protected]>
---
v8: New patch in this series.
net/mac80211/cfg.c | 75 +++++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index e7ac24603892..dcf152114652 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -984,25 +984,28 @@ static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
struct fils_discovery_data *new, *old = NULL;
struct ieee80211_fils_discovery *fd;
- if (!params->tmpl || !params->tmpl_len)
- return -EINVAL;
+ if (!params->update)
+ return 0;
fd = &link_conf->fils_discovery;
fd->min_interval = params->min_interval;
fd->max_interval = params->max_interval;
old = sdata_dereference(link->u.ap.fils_discovery, sdata);
- new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
- if (!new)
- return -ENOMEM;
- new->len = params->tmpl_len;
- memcpy(new->data, params->tmpl, params->tmpl_len);
- rcu_assign_pointer(link->u.ap.fils_discovery, new);
-
if (old)
kfree_rcu(old, rcu_head);
- return 0;
+ RCU_INIT_POINTER(link->u.ap.fils_discovery, NULL);
+ if (params->tmpl && params->tmpl_len) {
+ new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+ new->len = params->tmpl_len;
+ memcpy(new->data, params->tmpl, params->tmpl_len);
+ rcu_assign_pointer(link->u.ap.fils_discovery, new);
+ }
+
+ return BSS_CHANGED_FILS_DISCOVERY;
}
static int
@@ -1013,23 +1016,26 @@ ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
{
struct unsol_bcast_probe_resp_data *new, *old = NULL;
- if (!params->tmpl || !params->tmpl_len)
- return -EINVAL;
+ if (!params->update)
+ return 0;
- old = sdata_dereference(link->u.ap.unsol_bcast_probe_resp, sdata);
- new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
- if (!new)
- return -ENOMEM;
- new->len = params->tmpl_len;
- memcpy(new->data, params->tmpl, params->tmpl_len);
- rcu_assign_pointer(link->u.ap.unsol_bcast_probe_resp, new);
+ link_conf->unsol_bcast_probe_resp_interval = params->interval;
+ old = sdata_dereference(link->u.ap.unsol_bcast_probe_resp, sdata);
if (old)
kfree_rcu(old, rcu_head);
- link_conf->unsol_bcast_probe_resp_interval = params->interval;
+ RCU_INIT_POINTER(link->u.ap.unsol_bcast_probe_resp, NULL);
+ if (params->tmpl && params->tmpl_len) {
+ new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+ new->len = params->tmpl_len;
+ memcpy(new->data, params->tmpl, params->tmpl_len);
+ rcu_assign_pointer(link->u.ap.unsol_bcast_probe_resp, new);
+ }
- return 0;
+ return BSS_CHANGED_UNSOL_BCAST_PROBE_RESP;
}
static int ieee80211_set_ftm_responder_params(
@@ -1460,23 +1466,18 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
if (err < 0)
goto error;
- if (params->fils_discovery.max_interval) {
- err = ieee80211_set_fils_discovery(sdata,
- ¶ms->fils_discovery,
- link, link_conf);
- if (err < 0)
- goto error;
- changed |= BSS_CHANGED_FILS_DISCOVERY;
- }
+ err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery,
+ link, link_conf);
+ if (err < 0)
+ goto error;
+ changed |= err;
- if (params->unsol_bcast_probe_resp.interval) {
- err = ieee80211_set_unsol_bcast_probe_resp(sdata,
- ¶ms->unsol_bcast_probe_resp,
- link, link_conf);
- if (err < 0)
- goto error;
- changed |= BSS_CHANGED_UNSOL_BCAST_PROBE_RESP;
- }
+ err = ieee80211_set_unsol_bcast_probe_resp(sdata,
+ ¶ms->unsol_bcast_probe_resp,
+ link, link_conf);
+ if (err < 0)
+ goto error;
+ changed |= err;
err = drv_start_ap(sdata->local, sdata, link_conf);
if (err) {
--
2.39.0
Hi Aloka,
> Please review this series.
Yes, sorry for the delay.
> I know this version is too late to recollect the context of earlier
> patches but hopefully following is helpful.
>
> Versions 1 to 7 tried to fix this issue - FILS discovery transmission
> stops after CSA. I had tried to fix it in mac80211 which did not set
> BSS_CHANGED_FILS_DISCOVERY unless new parameters were sent by user-space.
>
> For v7, you mentioned that while the flag=0 indicates that FILS
> configurations did not change, it does not indicate that it got deleted
> so the driver should decide depending on the existing configuration and
> not depend only on the flag. I have already validated this ath12k patch
> which fixes the above issue, without cfg80211 and mac80211 patches in
> this series:
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> And I have changed this series to let the user-space give 'interval=0'
> as the parameter which was basically a no-op earlier. This way the
> transmission can be stopped explicitly and include the additional
> processing in the change_beacon from the previous versions which was
> anyway required.
>
Yep, thanks a lot!
I've applied the series since I was rebasing it on the current tree with
the locking changes and while that wasn't hard, I didn't want to
needlessly double the work and have you do it for a resend as well.
I've made some small tweaks and fixes, so please take a look at it, I
hope I didn't mess anything up.
Also, I'd like you to send a follow-up patch that updates the
documentation: now that we pass the whole settings to change_beacon(), I
think we need to document - perhaps as part of the kernel-doc for struct
cfg80211_ap_settings - which of the parameters are actually changing
there. Right now given your patches, it's clear that only beacon,
unsol_bcast_probe_resp and fils_discovery are (currently) allowed to
change.
Alternatively, maybe we should indeed change the prototype again and
introduce a new struct cfg80211_ap_update that contains only the
parameters that change?
That feels a bit harder, but really it isn't by that much - in mac80211
ieee80211_set_fils_discovery() etc. already take the sub-parameters
(¶ms->fils_discovery), so not a problem there, and in nl80211 we
could as well pass struct cfg80211_fils_discovery directly to
nl80211_parse_fils_discovery() rather than the entire struct
cfg80211_ap_settings, which wouldn't be a massive change.
I think maybe I even prefer the latter if I'm looking at it now, but I'm
not sure I'm not missing something from earlier discussions on this.
What do you think?
johannes
On 9/13/2023 3:26 AM, Johannes Berg wrote:
> Hi Aloka,
>
> I've applied the series since I was rebasing it on the current tree with
> the locking changes and while that wasn't hard, I didn't want to
> needlessly double the work and have you do it for a resend as well.
>
> I've made some small tweaks and fixes, so please take a look at it, I
> hope I didn't mess anything up.
>
Thanks!
> Also, I'd like you to send a follow-up patch that updates the
> documentation: now that we pass the whole settings to change_beacon(), I
> think we need to document - perhaps as part of the kernel-doc for struct
> cfg80211_ap_settings - which of the parameters are actually changing
> there. Right now given your patches, it's clear that only beacon,
> unsol_bcast_probe_resp and fils_discovery are (currently) allowed to
> change.
>
>
> Alternatively, maybe we should indeed change the prototype again and
> introduce a new struct cfg80211_ap_update that contains only the
> parameters that change?
>
> That feels a bit harder, but really it isn't by that much - in mac80211
> ieee80211_set_fils_discovery() etc. already take the sub-parameters
> (¶ms->fils_discovery), so not a problem there, and in nl80211 we
> could as well pass struct cfg80211_fils_discovery directly to
> nl80211_parse_fils_discovery() rather than the entire struct
> cfg80211_ap_settings, which wouldn't be a massive change.
>
>
> I think maybe I even prefer the latter if I'm looking at it now, but I'm
> not sure I'm not missing something from earlier discussions on this.
>
> What do you think?
>
> johannes
The second option will take couple of weeks due to current work load.
How about I do the first option, kernel-doc, until then?
Thanks.
On Mon, 2023-09-18 at 15:29 -0700, Aloka Dixit wrote:
> > Alternatively, maybe we should indeed change the prototype again and
> > introduce a new struct cfg80211_ap_update that contains only the
> > parameters that change?
> >
> > That feels a bit harder, but really it isn't by that much - in mac80211
> > ieee80211_set_fils_discovery() etc. already take the sub-parameters
> > (¶ms->fils_discovery), so not a problem there, and in nl80211 we
> > could as well pass struct cfg80211_fils_discovery directly to
> > nl80211_parse_fils_discovery() rather than the entire struct
> > cfg80211_ap_settings, which wouldn't be a massive change.
> >
> >
> > I think maybe I even prefer the latter if I'm looking at it now, but I'm
> > not sure I'm not missing something from earlier discussions on this.
>
> The second option will take couple of weeks due to current work load.
Since you didn't seem to be opposed, I just did the second thing myself
now, it wasn't really hard. :)
johannes