Received-SPF: pass (google.com: domain of linux-wireless+bounces-2825-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <14f9e510-8d69-41af-a68d-78d1e2dffe2e@quicinc.com>
Date: Tue, 30 Jan 2024 20:29:01 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: allow CSA to same channel
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
References: <20240129203544.ef7258d5790d.Idafe22e41621757458d4960659b9621853f7104d@changeid>
 <f4501322-c9e4-449b-9c81-f64a6d2172fc@quicinc.com>
 <fa1b179cd0d4de0c23e967d623f71314efc90235.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <fa1b179cd0d4de0c23e967d623f71314efc90235.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 1/30/24 20:23, Johannes Berg wrote:
> 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>
>>>
>>> This could be used e.g. for temporarily sending quiet
>>> (mode=1 in CSA/ECSA), or updating bandwidth. This is
>>
>> I know the intent here (from the other thread), but using the phrase _or
>> updating bandwidth_ is probably not correct since currently without this
>> 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 -
>>
>>   >>> I'm thinking about removing that identical() check entirely - you
>>   >>> might want to switch to the same channel with quiet=1. At least for
>>   >>> 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=1? 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==0 doesn't make _sense_, but even
>>   > then I'm not sure there's a lot of value in not permitting it. With
>>   > quiet==1 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.)
>>
>> Agreed to your point. So in that case, should we skip the identical
>> check only when quiet=1?
> 
> 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?
> 

Oh! In that case yeah it is pointless. We can remove the check from 
kernel completely then.

> 
> 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.
> 

Yeah makes sense :) Fine then. Let's remove it.