Received-SPF: pass (google.com: domain of linux-wireless+bounces-2824-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <fa1b179cd0d4de0c23e967d623f71314efc90235.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: allow CSA to same channel
From: Johannes Berg <johannes@sipsolutions.net>
To: Aditya Kumar Singh <quic_adisi@quicinc.com>, 
	linux-wireless@vger.kernel.org
Date: Tue, 30 Jan 2024 15:53:10 +0100
In-Reply-To: <f4501322-c9e4-449b-9c81-f64a6d2172fc@quicinc.com>
References: 
	<20240129203544.ef7258d5790d.Idafe22e41621757458d4960659b9621853f7104d@changeid>
	 <f4501322-c9e4-449b-9c81-f64a6d2172fc@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-01-30 at 20:14 +0530, Aditya Kumar Singh wrote:
> On 1/30/24 01:05, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >=20
> > This could be used e.g. for temporarily sending quiet
> > (mode=3D1 in CSA/ECSA), or updating bandwidth. This is
>=20
> I know the intent here (from the other thread), but using the phrase _or=
=20
> updating bandwidth_ is probably not correct since currently without this=
=20
> change also, just changing the bandwidth is possible, isn't it?

Err, yes, indeed... bad commit message.

> Also, bringing a part of the discussion we had in the other thread -
>=20
>  >>> I'm thinking about removing that identical() check entirely - you
>  >>> might want to switch to the same channel with quiet=3D1. At least fo=
r
>  >>> testing that'd be really useful, and I don't think it really serves
>  >>> any purpose to forbid it.
>  >
>  >> Yeah, we can do. But is there any actual use case? Also, what if some
>  >> notorious user space application simply sends NL command without even
>  >> quiet=3D1? There should be some check I guess?
>  >
>  > I'm not sure we care much about a broken userspace application running
>  > with root privileges breaking something here? :-)
>  >
>  > And at least for testing it's very useful to be able to do that. Agree
>  > that identical channel and quiet=3D=3D0 doesn't make _sense_, but even
>  > then I'm not sure there's a lot of value in not permitting it. With
>  > quiet=3D=3D1 at least it does make some sense still though, and we're
>  > currently not allowing it, hence my patch (to be able to test
>  > scenarios like that we saw elsewhere.)
>=20
> Agreed to your point. So in that case, should we skip the identical=20
> check only when quiet=3D1?

We could, though I even now have a test (not posted yet) that's using
this, but I could rewrite it to actually switch bandwidth.

I'm just not sure it's worth the extra complexity? What are we actually
saving ourselves from by doing it? Clearly we have to handle this case,
and whether or not it's quiet shouldn't really matter to the underlying
code?

IOW, yeah, do have that information so we could just add

	!params->block_tx &&

to the condition, but I'm not really sure what the value in that would
be, other than some kind of accurate reflecting of how we think today
CSA should be used?

But now that I think about it, Jouni mentioned yesterday that REVme D5.0
is getting language to allow capability changes during CSA, so that'd be
another thing to check for, you might (eventually) want to actually do a
CSA to the same channel to change capabilities, without quiet?


Anyway, even without that, I think the check doesn't really help for
anything. I'm not convinced the kernel needs to enforce a rule that
"userspace doesn't do something useless"? From a kernel POV it doesn't
matter if you can do it or not.

johannes