Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1598bf6d-1d0b-3753-73fa-2568d27d60b6@quicinc.com>
Date:   Thu, 19 Jan 2023 12:43:58 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v7 2/3] nl80211: additional processing in
 NL80211_CMD_SET_BEACON
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
References: <20221109214720.6097-1-quic_alokad@quicinc.com>
 <20221109214720.6097-3-quic_alokad@quicinc.com>
 <0b42cfb9f0effce61b33c41ec9067b3747fade5c.camel@sipsolutions.net>
 <9ff00d88-6d12-8ee5-aa25-a5e82879d25c@quicinc.com>
 <26ef0d67db3af16853915568585767b13a73ef9a.camel@sipsolutions.net>
From:   Aloka Dixit <quic_alokad@quicinc.com>
In-Reply-To: <26ef0d67db3af16853915568585767b13a73ef9a.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/19/2023 12:12 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 11:40 -0800, Aloka Dixit wrote:
>> On 1/18/2023 7:57 AM, Johannes Berg wrote:
>>> On Wed, 2022-11-09 at 13:47 -0800, Aloka Dixit wrote:
>>>> FILS discovery and unsolicited broadcast probe response transmissions
>>>> are configured as part of NL80211_CMD_START_AP, however both stop
>>>> after userspace uses the NL80211_CMD_SET_BEACON command as these
>>>> attributes are not processed in that command.
>>>
>>> That seems odd. Why would that *stop*? Nothing in the driver should
>>> actually update it to _remove_ it during change_beacon()?
>>>> Are you sure you didn't mean to "just" say "however both cannot be
>>> updated as these attributes ..."?
>>>
>>> johannes
>>
>> FILS discovery has primary channel, center frequency in the frame format
>> which should be changed depending on why the beacon changed.
> 
> Hmm? Sure, the frequency is important, but beacon can change for so many
> other reasons? Even just the greenfield bit in HT would cause a beacon
> change as far as cfg80211/mac80211 are concerned.
> 
>> Hence the
>> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY
>> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is
>> changed, means disable these features.
>> What do you think?
> 
> I think that makes no sense? If mac80211 didn't clear struct
> ieee80211_bss_conf::fils_discovery, then surely it should stick around
> even if the beacon changes???
> 
"max_interval" was be used as the enable/disable knob for these 
features. Non-zero = enable, zero = disable.
But the side effect of that is if NL80211 does not receive these 
attributes then by default the interval is set to 0.

> Surely you can't be right - that'd basically mean the whole feature is
> useless right now because even the greenfield update or similar that
> basically *always* happens in hostapd would disable it, since the beacon
> changes and we don't have these patches?
>Pretty much, yeah.

>> Should I send a follow-up with a different commit log?
>>
> 
> Well seems like we need to first figure out the correct semantics here,
> and possibly fix things...
> 
> johannes

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.

Thanks.