Re: [PATCH v8 0/5] Additional processing in beacon updates (v8)

[Johannes Berg <johannes@sipsolutions.net>]

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