Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: alokad=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 16 Feb 2021 14:41:07 -0800
From:   Aloka Dixit <alokad@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     Arend van Spriel <arend.vanspriel@broadcom.com>,
        linux-wireless@vger.kernel.org
Subject: Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited
 broadcast probe response
In-Reply-To: <9d9cc5bc50629090375b185a3af2546e506c22d9.camel@sipsolutions.net>
References: <20210120005229.32582-1-alokad@codeaurora.org>
 <20210120005229.32582-2-alokad@codeaurora.org>
 <ca7281ce-70bb-d1e2-3ec2-f07d91145c04@broadcom.com>
 <430f10a576b8490f73827f800c87f58c@codeaurora.org>
 (sfid-20210125_225531_667698_384A7632)
 <9d9cc5bc50629090375b185a3af2546e506c22d9.camel@sipsolutions.net>
Message-ID: <2fbdef8397607839c73fab66dd05c3c5@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

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.