diff mbox series

[v3,01/11] cfg80211: use only HE capability to set prohibited flags in 6 GHz

Message ID 1589399105-25472-1-git-send-email-rmanohar@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [v3,01/11] cfg80211: use only HE capability to set prohibited flags in 6 GHz | expand

Commit Message

Rajkumar Manoharan May 13, 2020, 7:44 p.m. UTC 
The prohibited flags to determine whether configured bandwidth
is supported by driver are validated only against HT and VHT capability.
In 6 GHz band, Only HE capability should be validated to find out
given chandef is usable.

Co-developed-by: Vamsi Krishna <vamsin@codeaurora.org>
Signed-off-by: Vamsi Krishna <vamsin@codeaurora.org>
Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
---
 net/wireless/chan.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 4 deletions(-)

Comments

Johannes Berg May 27, 2020, 1:43 p.m. UTC | #1 
Hi,

This is what we have in this area:
https://p.sipsolutions.net/d8e56772a261199a.txt

but I see it's also incomplete.

> +static bool cfg80211_is_6ghz_freq(u32 freq)
> +{
> +	return (freq > 5940 && freq < 7105);
> +}

That doesn't really make sense, I don't want to see those hardcoded
frequencies all over the place.

>  	case NL80211_CHAN_WIDTH_40:
>  		width = 40;
> +		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {

You can check chandef->chan->band instead. (In fact, we did)

> +			if (!he_cap)
> +				return false;
> +			if (!he_cap->has_he_6ghz)
> +				return false;

I'm not sure you should even _get_ here with a 6 GHz channel if you
don't have 6 GHz capability? I mean, why did you register the channel in
the first place then? This seems unnecessarily complex. If the channel
didn't exist, it was rejected long before here.

However, looking at D6.0, maybe we do need some checks of the HE
capability?

> +			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
> +			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G))
> +				return false;

Looks like even D6.0 still changed something in this area...

Evidently our patch just assumed that in 6 GHz all of this is supported,
but the spec doesn't support that theory :-)

Can you respin this with D6.0 taken into account?

johannes
Rajkumar Manoharan May 27, 2020, 11:32 p.m. UTC | #2 
On 2020-05-27 06:43, Johannes Berg wrote:
> Hi,
> 
> This is what we have in this area:
> https://p.sipsolutions.net/d8e56772a261199a.txt
> 
> but I see it's also incomplete.
> 
>> +static bool cfg80211_is_6ghz_freq(u32 freq)
>> +{
>> +	return (freq > 5940 && freq < 7105);
>> +}
> 
> That doesn't really make sense, I don't want to see those hardcoded
> frequencies all over the place.
> 
>>  	case NL80211_CHAN_WIDTH_40:
>>  		width = 40;
>> +		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {
> 
> You can check chandef->chan->band instead. (In fact, we did)
> 
Got it..

>> +			if (!he_cap)
>> +				return false;
>> +			if (!he_cap->has_he_6ghz)
>> +				return false;
> 
> I'm not sure you should even _get_ here with a 6 GHz channel if you
> don't have 6 GHz capability? I mean, why did you register the channel 
> in
> the first place then? This seems unnecessarily complex. If the channel
> didn't exist, it was rejected long before here.
> 
Hmm... Agreed.

> However, looking at D6.0, maybe we do need some checks of the HE
> capability?
> 
>> +			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
>> +			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G))
>> +				return false;
> 
> Looks like even D6.0 still changed something in this area...
> 
> Evidently our patch just assumed that in 6 GHz all of this is 
> supported,
> but the spec doesn't support that theory :-)
> 
IIUC the same bits are applicable for both 5 GHz & 6 GHz. I understand 
the macro doesn't
capture both.

> Can you respin this with D6.0 taken into account?
> 
Let me check again and respin after your series. Does it sound good?

-Rajkumar
Johannes Berg May 28, 2020, 7:41 a.m. UTC | #3 
On Wed, 2020-05-27 at 16:32 -0700, Rajkumar Manoharan wrote:
> 
> > However, looking at D6.0, maybe we do need some checks of the HE
> > capability?
> > 
> > > +			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
> > > +			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G))
> > > +				return false;
> > 
> > Looks like even D6.0 still changed something in this area...
> > 
> > Evidently our patch just assumed that in 6 GHz all of this is 
> > supported,
> > but the spec doesn't support that theory :-)
> > 
> IIUC the same bits are applicable for both 5 GHz & 6 GHz. I understand 
> the macro doesn't capture both.

Yeah, I think you're right. I looked at D6.0 (though there seems to be
D6.1?) but I couldn't quite 

> > Can you respin this with D6.0 taken into account?
> > 
> Let me check again and respin after your series. Does it sound good?

Ok. I'll include our more limited code in the series for now then, and
we can make changes to that after we're on the same page.

Thanks,
johannes
Johannes Berg May 28, 2020, 7:42 a.m. UTC | #4 
Hello brain, meet fingers, they're a bit slower ...

> > > 
> > IIUC the same bits are applicable for both 5 GHz & 6 GHz. I understand 
> > the macro doesn't capture both.
> 
> Yeah, I think you're right. I looked at D6.0 (though there seems to be
> D6.1?) but I couldn't quite 

... couldn't fully understand it in the limited time I had left
yesterday :)

johannes
diff mbox series

Patch

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index fcac5c6366e1..582b487576e1 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -19,6 +19,11 @@  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);
+}
+
 void cfg80211_chandef_create(struct cfg80211_chan_def *chandef,
 			     struct ieee80211_channel *chan,
 			     enum nl80211_channel_type chan_type)
@@ -882,6 +887,7 @@  bool cfg80211_chandef_usable(struct wiphy *wiphy,
 	struct ieee80211_sta_ht_cap *ht_cap;
 	struct ieee80211_sta_vht_cap *vht_cap;
 	struct ieee80211_edmg *edmg_cap;
+	const struct ieee80211_sta_he_cap *he_cap;
 	u32 width, control_freq, cap;
 
 	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
@@ -890,6 +896,7 @@  bool cfg80211_chandef_usable(struct wiphy *wiphy,
 	ht_cap = &wiphy->bands[chandef->chan->band]->ht_cap;
 	vht_cap = &wiphy->bands[chandef->chan->band]->vht_cap;
 	edmg_cap = &wiphy->bands[chandef->chan->band]->edmg_cap;
+	he_cap = ieee80211_get_he_sta_cap(wiphy->bands[chandef->chan->band]);
 
 	if (edmg_cap->channels &&
 	    !cfg80211_edmg_usable(wiphy,
@@ -919,6 +926,16 @@  bool cfg80211_chandef_usable(struct wiphy *wiphy,
 		break;
 	case NL80211_CHAN_WIDTH_40:
 		width = 40;
+		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {
+			if (!he_cap)
+				return false;
+			if (!he_cap->has_he_6ghz)
+				return false;
+			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
+			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G))
+				return false;
+			break;
+		}
 		if (!ht_cap->ht_supported)
 			return false;
 		if (!(ht_cap->cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) ||
@@ -933,24 +950,53 @@  bool cfg80211_chandef_usable(struct wiphy *wiphy,
 		break;
 	case NL80211_CHAN_WIDTH_80P80:
 		cap = vht_cap->cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK;
-		if (cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ)
+		if (!cfg80211_is_6ghz_freq(chandef->center_freq1) &&
+		    cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ)
 			return false;
+		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {
+			if (!he_cap)
+				return false;
+			if (!he_cap->has_he_6ghz)
+				return false;
+			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
+			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G))
+				return false;
+		}
 		/* fall through */
 	case NL80211_CHAN_WIDTH_80:
-		if (!vht_cap->vht_supported)
+		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {
+			if (!he_cap)
+				return false;
+			if (!he_cap->has_he_6ghz)
+				return false;
+			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
+			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G))
+				return false;
+		} else if (!vht_cap->vht_supported) {
 			return false;
+		}
 		prohibited_flags |= IEEE80211_CHAN_NO_80MHZ;
 		width = 80;
 		break;
 	case NL80211_CHAN_WIDTH_160:
+		prohibited_flags |= IEEE80211_CHAN_NO_160MHZ;
+		width = 160;
+		if (cfg80211_is_6ghz_freq(chandef->center_freq1)) {
+			if (!he_cap)
+				return false;
+			if (!he_cap->has_he_6ghz)
+				return false;
+			if (!(he_cap->he_cap_elem.phy_cap_info[0] &
+			      IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G))
+				return false;
+			break;
+		}
 		if (!vht_cap->vht_supported)
 			return false;
 		cap = vht_cap->cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK;
 		if (cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ &&
 		    cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ)
 			return false;
-		prohibited_flags |= IEEE80211_CHAN_NO_160MHZ;
-		width = 160;
 		break;
 	default:
 		WARN_ON_ONCE(1);