Received-SPF: pass (google.com: domain of linux-wireless+bounces-2823-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f4501322-c9e4-449b-9c81-f64a6d2172fc@quicinc.com>
Date: Tue, 30 Jan 2024 20:14:47 +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>
CC: Johannes Berg <johannes.berg@intel.com>
References: <20240129203544.ef7258d5790d.Idafe22e41621757458d4960659b9621853f7104d@changeid>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <20240129203544.ef7258d5790d.Idafe22e41621757458d4960659b9621853f7104d@changeid>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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?

In the chandef identical check we have -

return (chandef1->chan == chandef2->chan &&
	chandef1->width == chandef2->width && ....
	``````````````````````````````````
So the width would not match hence false would be returned.

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?

> also useful for testing, since it's something that an
> AP may do and the client needs to be prepared. Simply
> allow it.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   net/mac80211/cfg.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 8f647e28e354..c92acbf7b002 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3874,10 +3874,6 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>   	if (sdata->wdev.cac_started)
>   		return -EBUSY;
>   
> -	if (cfg80211_chandef_identical(&chanreq.oper,
> -				       &sdata->vif.bss_conf.chanreq.oper))
> -		return -EINVAL;
> -
>   	if (chanreq.oper.punctured && !sdata->vif.bss_conf.eht_support)
>   		return -EINVAL;
>