mbox series

[v8,0/5] Additional processing in beacon updates (v8)

Message ID 20230727174100.11721-1-quic_alokad@quicinc.com (mailing list archive)
Headers show
Series Additional processing in beacon updates (v8) | expand

Message

Aloka Dixit July 27, 2023, 5:40 p.m. UTC
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

Comments

Jeff Johnson Aug. 16, 2023, 6:38 p.m. UTC | #1
On 7/27/2023 10:40 AM, Aloka Dixit wrote:
> 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
for the series:

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Aloka Dixit Aug. 17, 2023, 6:04 p.m. UTC | #2
On 8/16/2023 11:38 AM, Jeff Johnson wrote:
> On 7/27/2023 10:40 AM, Aloka Dixit wrote:
>> 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
> for the series:
> 
> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> 

Thanks for the review!
Aloka Dixit Sept. 6, 2023, 5:52 p.m. UTC | #3
On 8/17/2023 11:04 AM, Aloka Dixit wrote:
> On 8/16/2023 11:38 AM, Jeff Johnson wrote:
>> On 7/27/2023 10:40 AM, Aloka Dixit wrote:
>>> 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.
>>>
>>> base-commit: b2090d93d4b6f1c72a9793d5a171806b8468b7cb
>> for the series:
>>
>> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>>
> 
> Thanks for the review!


Hello Johannes,

Please review this series.

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/20230905174324.25296-1-quic_alokad@quicinc.com/

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.

Thanks.
Johannes Berg Sept. 13, 2023, 10:26 a.m. UTC | #4
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/20230905174324.25296-1-quic_alokad@quicinc.com/
> 
> 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
(&params->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
Aloka Dixit Sept. 18, 2023, 10:29 p.m. UTC | #5
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
> (&params->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.
Johannes Berg Sept. 25, 2023, 7:17 a.m. UTC | #6
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
> > (&params->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