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: <26ef0d67db3af16853915568585767b13a73ef9a.camel@sipsolutions.net>
Subject: Re: [PATCH v7 2/3] nl80211: additional processing in
 NL80211_CMD_SET_BEACON
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aloka Dixit <quic_alokad@quicinc.com>,
        linux-wireless@vger.kernel.org
Date:   Thu, 19 Jan 2023 21:12:06 +0100
In-Reply-To: <9ff00d88-6d12-8ee5-aa25-a5e82879d25c@quicinc.com>
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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.
> >=20
> > 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 ..."?
> >=20
> > johannes
>=20
> FILS discovery has primary channel, center frequency in the frame format=
=20
> which should be changed depending on why the beacon changed.=C2=A0

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=20
> current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOVERY=
=20
> and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon is=
=20
> 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???

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?

> Should I send a follow-up with a different commit log?
>=20

Well seems like we need to first figure out the correct semantics here,
and possibly fix things...

johannes