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;
Message-ID: <9d9cc5bc50629090375b185a3af2546e506c22d9.camel@sipsolutions.net>
Subject: Re: [PATCH 1/2] nl80211: Commands for FILS discovery and
 unsolicited broadcast probe response
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aloka Dixit <alokad@codeaurora.org>,
        Arend van Spriel <arend.vanspriel@broadcom.com>
Cc:     linux-wireless@vger.kernel.org
Date:   Fri, 12 Feb 2021 10:33:29 +0100
In-Reply-To: <430f10a576b8490f73827f800c87f58c@codeaurora.org> (sfid-20210125_225531_667698_384A7632)
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)
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?

> 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