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: <5accd9bb-b172-be8f-2045-a55b2695b2d4@quicinc.com>
Date:   Thu, 19 Jan 2023 13:19:59 -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>
 <1598bf6d-1d0b-3753-73fa-2568d27d60b6@quicinc.com>
 <733b35861bbed8d3b7cf8625d3bf59f1c34e08da.camel@sipsolutions.net>
From:   Aloka Dixit <quic_alokad@quicinc.com>
In-Reply-To: <733b35861bbed8d3b7cf8625d3bf59f1c34e08da.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/19/2023 12:47 PM, Johannes Berg wrote:
> On Thu, 2023-01-19 at 12:43 -0800, Aloka Dixit wrote:
>>>
>>>> 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.
> 
> 
> But it doesn't change in bss_conf if you send change beacon, at least
> not before these patches?
> 
> Or am I confusing myself?
> 

Your understanding is correct, however, without these patches there is a 
cascading effect.
-> NL80211: If the attribute is not sent by user-space/processed in this 
layer then cfg80211_ap_settings->fils_discovery->max_interval is 0 
(default).
-> MAC80211: max_interval=0, hence BSS_CHANGED_FILS_DISCOVERY is not set
-> ath11k: BSS_CHANGED_FILS_DISCOVERY is not set hence driver doesn't 
configure/re-configure. Unless target doesn't receive it every beacon 
change, it disables it.

I can make changes up to the driver to fix this part.

>> 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.
>>
> 
> Were those meant to be mutually exclusive, because it doesn't seem like
> that to me? I think (2) must be what happens now, before these patches,
> because I don't see where it would be changed? Like I said above.
> 

Not exclusive. I meant I can make both the changes above to not have the 
above mentioned cascading effect

> I agree that we'd now need an explicit way to indicate "disable", but we
> could for example say you disable by adding a nested
> NL80211_ATTR_FILS_DISCOVERY attribute without any of the sub-attributes,
> which logically kind of makes sense - you're changing
> NL80211_ATTR_FILS_DISCOVERY, but you're not including a new set of
> parameters, so logically that means disable?
> 
> johannes

Sure, will update nl80211_parse_fils_discovery() to allow this case and 
reset all to 0/null etc.

I can start a new series with all the changes, and include current 
patches last.

Will that work?