| Message ID | 1609816120-9411-3-git-send-email-wgong@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: set NSS correct for supported HE-MCS and NSS set | expand |
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 <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?
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
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,
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(+)