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: <733b35861bbed8d3b7cf8625d3bf59f1c34e08da.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:47:48 +0100
In-Reply-To: <1598bf6d-1d0b-3753-73fa-2568d27d60b6@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>
         <26ef0d67db3af16853915568585767b13a73ef9a.camel@sipsolutions.net>
         <1598bf6d-1d0b-3753-73fa-2568d27d60b6@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 12:43 -0800, Aloka Dixit wrote:
> >=20
> > > Hence the
> > > current design, at least ath11k, assumes that BSS_CHANGED_FILS_DISCOV=
ERY
> > > and BSS_CHANGED_UNSOL_BCAST_PROBE_RESP "not being set", when beacon i=
s
> > > changed, means disable these features.
> > > What do you think?
> >=20
> > 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???
> >=20
> "max_interval" was be used as the enable/disable knob for these=20
> features. Non-zero =3D enable, zero =3D disable.
> But the side effect of that is if NL80211 does not receive these=20
> 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?

> I can think of following:
> (1) max_interval cannot be the enable/disable knob because then every=20
> code path in the userspace would still need to set it to non-zero to=20
> continue transmission. Are you okay with having extra members in enum=20
> nl80211_fils_discovery_attributes to ENABLE/DISABLE? I think that is=20
> what you suggested in your comment for the next patch in this series as=
=20
> well.
>=20
> (2) If the template needs changing for any reason then the userspace=20
> will be responsible to send a new one. Until then the driver will=20
> continue the transmission with existing template and interval unless=20
> DISABLE is used.
>=20

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.

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