[PATCH v3 0/2] mac80211: set NSS correct for supported HE-MCS and NSS set

[PATCH v3 0/2] mac80211: set NSS correct for supported HE-MCS and NSS set

[Wen Gong]

The NSS number should be the intersection of peer and own capbility.

v3: added "mac80211: remove NSS number of 160MHz if not support 160MHz for HE"

v2: fix build warning

Wen Gong (2):
  mac80211: remove NSS number of 160MHz if not support 160MHz for HE
  mac80211: do intersection with he mcs and nss set of peer and own

 net/mac80211/he.c          | 94 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  5 ++
 net/mac80211/vht.c         |  9 +++-
 3 files changed, 107 insertions(+), 1 deletion(-)

-- 
2.23.0

[PATCH v3 1/2] mac80211: remove NSS number of 160MHz if not support 160MHz for HE

[Wen Gong]

When it does not support 160MHz in HE phy capabilities information,
it should not treat the NSS number of 160MHz as a valid number,
otherwise the final NSS will be set to 0.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
 net/mac80211/vht.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/vht.c b/net/mac80211/vht.c
index d87e020715b3..d872f1d4d946 100644
--- a/net/mac80211/vht.c
+++ b/net/mac80211/vht.c
@@ -484,6 +484,7 @@ enum ieee80211_sta_rx_bandwidth ieee80211_sta_cur_vht_bw(struct sta_info *sta)
 void ieee80211_sta_set_rx_nss(struct sta_info *sta)
 {
 	u8 ht_rx_nss = 0, vht_rx_nss = 0, he_rx_nss = 0, rx_nss;
+	bool support_160;
 
 	/* if we received a notification already don't overwrite it */
 	if (sta->sta.rx_nss)
@@ -514,7 +515,13 @@ void ieee80211_sta_set_rx_nss(struct sta_info *sta)
 			}
 		}
 
-		he_rx_nss = min(rx_mcs_80, rx_mcs_160);
+		support_160 = !!(he_cap->he_cap_elem.phy_cap_info[0] &
+			  IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G);
+
+		if (support_160)
+			he_rx_nss = min(rx_mcs_80, rx_mcs_160);
+		else
+			he_rx_nss = rx_mcs_80;
 	}
 
 	if (sta->sta.ht_cap.ht_supported) {
-- 
2.23.0

[PATCH v3 2/2] mac80211: do intersection with he mcs and nss set of peer and own

[Wen Gong]

For VHT capbility, it has intersection of mcs and nss for peer in
function ieee80211_vht_cap_ie_to_sta_vht_cap. For HE capbility,
it does not have intersection.

This patch is do intersection for HE capbility.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
 net/mac80211/he.c          | 94 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  5 ++
 2 files changed, 99 insertions(+)

diff --git a/net/mac80211/he.c b/net/mac80211/he.c
index cc26f239838b..1850f9899726 100644
--- a/net/mac80211/he.c
+++ b/net/mac80211/he.c
@@ -52,6 +52,59 @@ ieee80211_update_from_he_6ghz_capa(const struct ieee80211_he_6ghz_capa *he_6ghz_
 	sta->sta.he_6ghz_capa = *he_6ghz_capa;
 }
 
+void
+ieee80211_he_mcs_disable(u16 *he_mcs)
+{
+	u32 i;
+
+	for (i = 0; i < 8; i++)
+		*he_mcs |= cpu_to_le16(IEEE80211_HE_MCS_NOT_SUPPORTED << i * 2);
+}
+
+void
+ieee80211_he_mcs_intersection(u16 *he_own_rx, u16 *he_peer_rx,
+			      u16 *he_own_tx, u16 *he_peer_tx)
+{
+	u32 i;
+	u16 own_rx, own_tx, peer_rx, peer_tx;
+
+	for (i = 0; i < 8; i++) {
+		own_rx = le16_to_cpu(*he_own_rx);
+		own_rx = (own_rx >> i * 2) & IEEE80211_HE_MCS_NOT_SUPPORTED;
+
+		own_tx = le16_to_cpu(*he_own_tx);
+		own_tx = (own_tx >> i * 2) & IEEE80211_HE_MCS_NOT_SUPPORTED;
+
+		peer_rx = le16_to_cpu(*he_peer_rx);
+		peer_rx = (peer_rx >> i * 2) & IEEE80211_HE_MCS_NOT_SUPPORTED;
+
+		peer_tx = le16_to_cpu(*he_peer_tx);
+		peer_tx = (peer_tx >> i * 2) & IEEE80211_HE_MCS_NOT_SUPPORTED;
+
+		if (peer_tx != IEEE80211_HE_MCS_NOT_SUPPORTED) {
+			if (own_rx == IEEE80211_HE_MCS_NOT_SUPPORTED)
+				peer_tx = IEEE80211_HE_MCS_NOT_SUPPORTED;
+			else if (own_rx < peer_tx)
+				peer_tx = own_rx;
+		}
+
+		if (peer_rx != IEEE80211_HE_MCS_NOT_SUPPORTED) {
+			if (own_tx == IEEE80211_HE_MCS_NOT_SUPPORTED)
+				peer_rx = IEEE80211_HE_MCS_NOT_SUPPORTED;
+			else if (own_tx < peer_rx)
+				peer_rx = own_tx;
+		}
+
+		*he_peer_rx &=
+			~cpu_to_le16(IEEE80211_HE_MCS_NOT_SUPPORTED << i * 2);
+		*he_peer_rx |= cpu_to_le16(peer_rx << i * 2);
+
+		*he_peer_tx &=
+			~cpu_to_le16(IEEE80211_HE_MCS_NOT_SUPPORTED << i * 2);
+		*he_peer_tx |= cpu_to_le16(peer_tx << i * 2);
+	}
+}
+
 void
 ieee80211_he_cap_ie_to_sta_he_cap(struct ieee80211_sub_if_data *sdata,
 				  struct ieee80211_supported_band *sband,
@@ -60,10 +113,12 @@ ieee80211_he_cap_ie_to_sta_he_cap(struct ieee80211_sub_if_data *sdata,
 				  struct sta_info *sta)
 {
 	struct ieee80211_sta_he_cap *he_cap = &sta->sta.he_cap;
+	struct ieee80211_sta_he_cap own_he_cap = sband->iftype_data->he_cap;
 	struct ieee80211_he_cap_elem *he_cap_ie_elem = (void *)he_cap_ie;
 	u8 he_ppe_size;
 	u8 mcs_nss_size;
 	u8 he_total_size;
+	bool own_160, peer_160, own_80p80, peer_80p80;
 
 	memset(he_cap, 0, sizeof(*he_cap));
 
@@ -101,6 +156,45 @@ ieee80211_he_cap_ie_to_sta_he_cap(struct ieee80211_sub_if_data *sdata,
 
 	if (sband->band == NL80211_BAND_6GHZ && he_6ghz_capa)
 		ieee80211_update_from_he_6ghz_capa(he_6ghz_capa, sta);
+
+	ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_80,
+				      &he_cap->he_mcs_nss_supp.rx_mcs_80,
+				      &own_he_cap.he_mcs_nss_supp.tx_mcs_80,
+				      &he_cap->he_mcs_nss_supp.tx_mcs_80);
+
+	own_160 = !!(own_he_cap.he_cap_elem.phy_cap_info[0] &
+		  IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G);
+	peer_160 = !!(he_cap->he_cap_elem.phy_cap_info[0] &
+		  IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G);
+
+	if (peer_160 && own_160) {
+		ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_160,
+					      &he_cap->he_mcs_nss_supp.rx_mcs_160,
+					      &own_he_cap.he_mcs_nss_supp.tx_mcs_160,
+					      &he_cap->he_mcs_nss_supp.tx_mcs_160);
+	} else if (peer_160 && !own_160) {
+		ieee80211_he_mcs_disable(&he_cap->he_mcs_nss_supp.rx_mcs_160);
+		ieee80211_he_mcs_disable(&he_cap->he_mcs_nss_supp.tx_mcs_160);
+		he_cap->he_cap_elem.phy_cap_info[0] &=
+			~IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G;
+	}
+
+	own_80p80 = !!(own_he_cap.he_cap_elem.phy_cap_info[0] &
+		  IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G);
+	peer_80p80 = !!(he_cap->he_cap_elem.phy_cap_info[0] &
+		  IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G);
+
+	if (peer_80p80 && own_80p80) {
+		ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_80p80,
+					      &he_cap->he_mcs_nss_supp.rx_mcs_80p80,
+					      &own_he_cap.he_mcs_nss_supp.tx_mcs_80p80,
+					      &he_cap->he_mcs_nss_supp.tx_mcs_80p80);
+	} else if (peer_80p80 && !own_80p80) {
+		ieee80211_he_mcs_disable(&he_cap->he_mcs_nss_supp.rx_mcs_80p80);
+		ieee80211_he_mcs_disable(&he_cap->he_mcs_nss_supp.tx_mcs_80p80);
+		he_cap->he_cap_elem.phy_cap_info[0] &=
+			~IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G;
+	}
 }
 
 void
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2a21226fb518..93c8e8d0b9e3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1915,6 +1915,11 @@ ieee80211_sta_rx_bw_to_chan_width(struct sta_info *sta);
 
 /* HE */
 void
+ieee80211_he_mcs_disable(u16 *he_mcs);
+void
+ieee80211_he_mcs_intersection(u16 *he_own_rx, u16 *he_peer_rx,
+			      u16 *he_own_tx, u16 *he_peer_tx);
+void
 ieee80211_he_cap_ie_to_sta_he_cap(struct ieee80211_sub_if_data *sdata,
 				  struct ieee80211_supported_band *sband,
 				  const u8 *he_cap_ie, u8 he_cap_len,
-- 
2.23.0

Re: [PATCH v3 2/2] mac80211: do intersection with he mcs and nss set of peer and own

[Johannes Berg]

Hi,


I had done a bunch of fixups to this patch, but the zero-day build robot
correctly reports that:

> +	ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_80,
> +				      &he_cap->he_mcs_nss_supp.rx_mcs_80,
> +				      &own_he_cap.he_mcs_nss_supp.tx_mcs_80,
> +				      &he_cap->he_mcs_nss_supp.tx_mcs_80);

the &own_he_cap... parts here will take an __le16 pointer to a possibly
unaligned variable - any thoughts how we could fix that?

johannes

Re: [PATCH v3 2/2] mac80211: do intersection with he mcs and nss set of peer and own

[Wen Gong]

On 2021-09-28 21:02, Johannes Berg wrote:
> Hi,
> 
> 
> I had done a bunch of fixups to this patch, but the zero-day build 
> robot
> correctly reports that:
> 
>> +	ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_80,
>> +				      &he_cap->he_mcs_nss_supp.rx_mcs_80,
>> +				      &own_he_cap.he_mcs_nss_supp.tx_mcs_80,
>> +				      &he_cap->he_mcs_nss_supp.tx_mcs_80);
> 
> the &own_he_cap... parts here will take an __le16 pointer to a possibly
> unaligned variable - any thoughts how we could fix that?
> 
Hi Johannes,

Add "__packed" before the "__le16 *" should solve this warning by my 
understand like this:

diff --git a/net/mac80211/he.c b/net/mac80211/he.c
index c05af7018f79..960fea9646b0 100644
--- a/net/mac80211/he.c
+++ b/net/mac80211/he.c
@@ -52,7 +52,7 @@ ieee80211_update_from_he_6ghz_capa(const struct 
ieee80211_he_6ghz_capa *he_6ghz_
         sta->sta.he_6ghz_capa = *he_6ghz_capa;
  }

-static void ieee80211_he_mcs_disable(__le16 *he_mcs)
+static void ieee80211_he_mcs_disable(__packed __le16 *he_mcs)
  {
         u32 i;

@@ -60,8 +60,8 @@ static void ieee80211_he_mcs_disable(__le16 *he_mcs)
                 *he_mcs |= cpu_to_le16(IEEE80211_HE_MCS_NOT_SUPPORTED << 
i * 2);
  }

-static void ieee80211_he_mcs_intersection(__le16 *he_own_rx, __le16 
*he_peer_rx,
-                                         __le16 *he_own_tx, __le16 
*he_peer_tx)
+static void ieee80211_he_mcs_intersection(__packed __le16 *he_own_rx, 
__packed __le16 *he_peer_rx,
+                                         __packed __le16 *he_own_tx, 
__packed __le16 *he_peer_tx)
  {
         u32 i;
         u16 own_rx, own_tx, peer_rx, peer_tx;



net/mac80211/he.c:158:33: warning: taking address of packed member 
'rx_mcs_80' of class or structure 'ieee80211_he_mcs_nss_supp' may result 
in an unaligned pointer value [-Waddress-of-packed-member]

> johannes

Re: [PATCH v3 2/2] mac80211: do intersection with he mcs and nss set of peer and own

[Kalle Valo]

Wen Gong <wgong@codeaurora.org> writes:

> On 2021-09-28 21:02, Johannes Berg wrote:
>> Hi,
>>
>>
>> I had done a bunch of fixups to this patch, but the zero-day build
>> robot
>> correctly reports that:
>>
>>> +	ieee80211_he_mcs_intersection(&own_he_cap.he_mcs_nss_supp.rx_mcs_80,
>>> +				      &he_cap->he_mcs_nss_supp.rx_mcs_80,
>>> +				      &own_he_cap.he_mcs_nss_supp.tx_mcs_80,
>>> +				      &he_cap->he_mcs_nss_supp.tx_mcs_80);
>>
>> the &own_he_cap... parts here will take an __le16 pointer to a possibly
>> unaligned variable - any thoughts how we could fix that?
>>
> Hi Johannes,
>
> Add "__packed" before the "__le16 *" should solve this warning by my
> understand like this:
>
> diff --git a/net/mac80211/he.c b/net/mac80211/he.c
> index c05af7018f79..960fea9646b0 100644
> --- a/net/mac80211/he.c
> +++ b/net/mac80211/he.c
> @@ -52,7 +52,7 @@ ieee80211_update_from_he_6ghz_capa(const struct
> ieee80211_he_6ghz_capa *he_6ghz_
>         sta->sta.he_6ghz_capa = *he_6ghz_capa;
>  }
>
> -static void ieee80211_he_mcs_disable(__le16 *he_mcs)
> +static void ieee80211_he_mcs_disable(__packed __le16 *he_mcs)
>  {
>         u32 i;
>
> @@ -60,8 +60,8 @@ static void ieee80211_he_mcs_disable(__le16 *he_mcs)
>                 *he_mcs |= cpu_to_le16(IEEE80211_HE_MCS_NOT_SUPPORTED
> << i * 2);
>  }
>
> -static void ieee80211_he_mcs_intersection(__le16 *he_own_rx, __le16
> *he_peer_rx,
> -                                         __le16 *he_own_tx, __le16
> *he_peer_tx)
> +static void ieee80211_he_mcs_intersection(__packed __le16 *he_own_rx,
> __packed __le16 *he_peer_rx,
> +                                         __packed __le16 *he_own_tx,
> __packed __le16 *he_peer_tx)
>  {
>         u32 i;
>         u16 own_rx, own_tx, peer_rx, peer_tx;
>
>
>
> net/mac80211/he.c:158:33: warning: taking address of packed member
> 'rx_mcs_80' of class or structure 'ieee80211_he_mcs_nss_supp' may
> result in an unaligned pointer value [-Waddress-of-packed-member]

I don't know what Johannes thinks, but to me that looks like an ugly
hack. Wouldn't use get_unaligned() or similar be cleaner?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3 2/2] mac80211: do intersection with he mcs and nss set of peer and own

[Johannes Berg]

On Fri, 2021-10-01 at 09:32 +0300, Kalle Valo wrote:
> > 
> > Add "__packed" before the "__le16 *" should solve this warning by my
> > understand like this:

[snip]
> > 
> > -static void ieee80211_he_mcs_disable(__le16 *he_mcs)
> > +static void ieee80211_he_mcs_disable(__packed __le16 *he_mcs)
> > 

[snip]

> I don't know what Johannes thinks, but to me that looks like an ugly
> hack. Wouldn't use get_unaligned() or similar be cleaner?
> 
Well, then we've have to pass an untyped pointer (void *), which I guess
is fine? Since we do all kinds of le16_to_cpu() with it anyway, that'd
just become get_unaligned_le16().

That's probably the better choice.

But regardless, would the __packed even *work*? __attribute__((packed))
is documented as:

   This attribute, attached to a struct, union, or C++ class type
   definition, specifies that each of its members (other than zero-width
   bit-fields) is placed to minimize the memory required. This is
   equivalent to specifying the packed attribute on each of the members.
   
   When attached to an enum definition, the packed attribute indicates that
   the smallest integral type should be used. Specifying the -fshort-enums
   flag on the command line is equivalent to specifying the packed
   attribute on all enum definitions. 
   
   [snip example]
   
   You may only specify the packed attribute on the definition of an enum,
   struct, union, or class, not on a typedef that does not also define the
   enumerated type, structure, union, or class. 

So I'm not convinced it would actually *do* anything here at all, in the
proposed context?

johannes