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?
