Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Aloka Dixit <alokad@codeaurora.org>]

On 2021-02-12 01:33, Johannes Berg wrote:
> On Mon, 2021-01-25 at 13:52 -0800, Aloka Dixit wrote:
>> 
>> FILS discovery and especially unsolicited probe response templates are
>> big. Sometimes send_and_recv() returns error due to memory
>> unavailability during wpa_driver_nl80211_set_ap() depending on how 
>> many
>> interfaces, which elements are added.
> 
> What? Where do you get errors from? Netlink even supports vmalloc now, 
> I
> believe, so the kernel really shouldn't care?
> 
The error was shown on hostapd side during attribute building when
16 interfaces were added on one radio and unsolicited broadcast
probe response feature was enabled.
It was resolved with a separate command for this feature instead of
being a part of NL80211_CMD_START_AP.

>> Moving these to separate commands
>> resolves this issue along with more control over the time interval
>> during run-time.
> 
> I tend to agree with Arend though, we have NL80211_CMD_SET_BEACON and
> since NL80211_CMD_NEW_BEACON was renamed to NL80211_CMD_START_AP to 
> more
> accurately say what it does, I think generalizing "AP modifications" by
> renaming NL80211_CMD_SET_BEACON to NL80211_CMD_UPDATE_AP (or such) 
> would
> make a lot of sense.
> 
> Let's not conflate the two issues here.
> 
> 1) memory issues - need to understand better
> 
> 2) update is needed - I'd say SET_BEACON/UPDATE_AP would be a better 
> way
>    than pulling everything into separate commands. Updates can be
>    partial too, after all, if you include only the changed attributes,
>    and that might even address case 1? I.e. why wouldn't userspace be
>    able to do UPDATE_AP multiple times, just like with your patch it
>    would do NL80211_CMD_SET_FILS_DISCOVERY
>    and NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP?
> 
> 
> Also, technically this constitutes an API break. One that perhaps 
> nobody
> cares about yet, but surely somebody already has hostapd versions that
> use NL80211_ATTR_FILS_DISCOVERY or NL80211_ATTR_UNSOL_BCAST_PROBE_RESP?
> After all, you don't want to tell me you never tested this code ;-)
> 
> johannes

I myself added the hostapd changes corresponding to the original kernel 
code,
This patch-set doesn't remove these attributes so, if accepted,
current hostapd calls will become no-op but won't break any other
functionality until I send the next version for separate commands for 
hostapd.

On a different note, a line was missed in nl80211_fils_discovery_policy
definition from my side in the original change so included it in this
patch-set.
Should I send a separate patch for that while we discuss regarding the 
separate
command?
Thanks.