| Message ID | 20230727174100.11721-1-quic_alokad@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Additional processing in beacon updates (v8) | expand |
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>
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!
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.
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 (¶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