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 <[email protected]>
---
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
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
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
Wen Gong <[email protected]> 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
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