| Message ID | 20221109214720.6097-3-quic_alokad@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Additional processing in NL80211_CMD_SET_BEACON | expand |
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 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 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: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
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);
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 <quic_alokad@quicinc.com> --- net/wireless/nl80211.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)