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
