Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <73a4e63e16bffb69cd9b62fd904b926dd5278fbf.camel@sipsolutions.net>
Subject: Re: [PATCH 02/10] cfg80211: validate 6 GHz chandef
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Rajkumar Manoharan <rmanohar@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org
Date:   Wed, 29 Apr 2020 16:26:48 +0200
In-Reply-To: <1587768108-25248-3-git-send-email-rmanohar@codeaurora.org>
References: <1587768108-25248-1-git-send-email-rmanohar@codeaurora.org>
         <1587768108-25248-3-git-send-email-rmanohar@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 (3.34.4-1.fc31) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, 2020-04-24 at 15:41 -0700, Rajkumar Manoharan wrote:
> 
> +static inline bool
> +cfg80211_chandef_is_6ghz(const struct cfg80211_chan_def *chandef)
> +{
> +	return (chandef->center_freq1 > 5940 && chandef->center_freq1 < 7105);
> +}

Seems like this

> +++ b/net/wireless/chan.c
> @@ -19,6 +19,29 @@ static bool cfg80211_valid_60g_freq(u32 freq)
>  	return freq >= 58320 && freq <= 70200;
>  }
>  
> +static bool cfg80211_is_6ghz_freq(u32 freq)
> +{
> +	return (freq > 5940 && freq < 7105);
> +}

should use this, by also exposing it, or something.

> +static enum nl80211_chan_width cfg80211_chan_to_bw_6ghz(u8 idx)
> +{
> +	/* channels: 1, 5, 9, 13... */
> +	if ((idx & 0x3) == 0x1)
> +		return NL80211_CHAN_WIDTH_20;
> +	/* channels 3, 11, 19... */
> +	if ((idx & 0x7) == 0x3)
> +		return NL80211_CHAN_WIDTH_40;
> +	/* channels 7, 23, 39.. */
> +	if ((idx & 0xf) == 0x7)
> +		return NL80211_CHAN_WIDTH_80;
> +	/* channels 15, 47, 79...*/
> +	if ((idx & 0x1f) == 0xf)
> +		return NL80211_CHAN_WIDTH_160;
> +
> +	return NL80211_CHAN_WIDTH_20;
> +}

We haven't really done that for anything else - is that really
necessary?

> +static bool cfg80211_6ghz_chandef_valid(const struct cfg80211_chan_def *chandef)
> +{
> +	enum nl80211_chan_width bw;
> +	int chan_idx;
> +
> +	if (!cfg80211_is_6ghz_freq(chandef->center_freq1))
> +		return false;

this is kinda pointless,

> @@ -213,6 +255,10 @@ bool cfg80211_chandef_valid(const struct cfg80211_chan_def *chandef)
>  	    !cfg80211_edmg_chandef_valid(chandef))
>  		return false;
>  
> +	if (cfg80211_chandef_is_6ghz(chandef) &&
> +	    !cfg80211_6ghz_chandef_valid(chandef))
> +		return false;

You only get there if it was in range ...

Not sure about this whole patch, it seems a bit pointless?

johannes