| Message ID | 20230928055022.9670-2-quic_kangyang@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | dynamically update puncturing bitmap | expand |
On Thu, 2023-09-28 at 13:50 +0800, Kang Yang wrote: > > +static enum nl80211_chan_width > +ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw) > +{ > + switch (bw) { > + case IEEE80211_STA_RX_BW_20: > + return NL80211_CHAN_WIDTH_20; So for a while now I was actually not responding to this because I was scratching my head over how this function ever could be needed or make sense ... > static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, > const struct ieee80211_eht_operation *eht_oper, > u64 *changed) > { > + struct cfg80211_chan_def rx_chandef = link->conf->chandef; > u16 bitmap = 0, extracted; > + u8 bw = 0; > > if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) && > (eht_oper->params & > @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, > const u8 *disable_subchannel_bitmap = info->optional; > > bitmap = get_unaligned_le16(disable_subchannel_bitmap); > + bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH); > + rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw); But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_* is a purely internal API, has nothing to do with the spec. All this might even be "accidentally correct", but it really isn't right at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*. More generally though, I don't even understand the change. > + if (rx_chandef.width == NL80211_CHAN_WIDTH_80) > + rx_chandef.center_freq1 = > + ieee80211_channel_to_frequency(info->ccfs0, > + rx_chandef.chan->band); > + else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 || > + rx_chandef.width == NL80211_CHAN_WIDTH_320) > + rx_chandef.center_freq1 = > + ieee80211_channel_to_frequency(info->ccfs1, > + rx_chandef.chan->band); > + } > + > + if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, > + &rx_chandef)) { > + link_info(link, > + "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n", > + link->u.mgd.bssid, > + bitmap, > + rx_chandef.width); > + return false; > } > > extracted = ieee80211_extract_dis_subch_bmap(eht_oper, // I've filled in the context here in the patch > &link->conf->chandef, > bitmap); > > /* accept if there are no changes */ > if (!(*changed & BSS_CHANGED_BANDWIDTH) && > extracted == link->conf->eht_puncturing) > return true; but ... ieee80211_extract_dis_subch_bmap actually already takes the bandwidth from eht_oper into account! > - if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, > - &link->conf->chandef)) { So are you saying that the real bug is that we're missing to update the link->conf->chandef with the EHT operation from the assoc response? But you didn't fix that issue ... so not sure? johannes
On 10/18/2023 7:39 PM, Johannes Berg wrote: >> static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, >> const struct ieee80211_eht_operation *eht_oper, >> u64 *changed) >> { >> + struct cfg80211_chan_def rx_chandef = link->conf->chandef; >> u16 bitmap = 0, extracted; >> + u8 bw = 0; >> >> if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) && >> (eht_oper->params & >> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, >> const u8 *disable_subchannel_bitmap = info->optional; >> >> bitmap = get_unaligned_le16(disable_subchannel_bitmap); >> + bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH); >> + rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw); > > But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_* > is a purely internal API, has nothing to do with the spec. > > All this might even be "accidentally correct", but it really isn't right > at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are > IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*. > Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will replace them. > > > More generally though, I don't even understand the change. During assoc, downgrade may happen in func ieee80211_config_bw(). In this situation, the bandwidth in beacon and the bandwidth after downgrade(chandef->width, maybe i should call it local bandwidth during below context, will use this name in next version) during assoc will be different. The change is based on this point. > >> + if (rx_chandef.width == NL80211_CHAN_WIDTH_80) >> + rx_chandef.center_freq1 = >> + ieee80211_channel_to_frequency(info->ccfs0, >> + rx_chandef.chan->band); >> + else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 || >> + rx_chandef.width == NL80211_CHAN_WIDTH_320) >> + rx_chandef.center_freq1 = >> + ieee80211_channel_to_frequency(info->ccfs1, >> + rx_chandef.chan->band); >> + } >> + >> + if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, >> + &rx_chandef)) { Here i change you code cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef) As described above, downgrade may happen before enter ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef may be different with bandwidth in beacon. Here, the bitmap you used is from eht_oper in beacon, but the bandwidth you used is local bandwidth. They are not match. This is the first issue. >> + link_info(link, >> + "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n", >> + link->u.mgd.bssid, >> + bitmap, >> + rx_chandef.width); >> + return false; >> } >> >> extracted = ieee80211_extract_dis_subch_bmap(eht_oper, > // I've filled in the context here in the patch Here is move the cfg80211_valid_disable_subchannel_bitmap() before the ieee80211_extract_dis_subch_bmap(), cause i think check should done before extract. This is the second issue i said(perhaps not a issue). >> &link->conf->chandef, >> bitmap); >> >> /* accept if there are no changes */ >> if (!(*changed & BSS_CHANGED_BANDWIDTH) && >> extracted == link->conf->eht_puncturing) >> return true; > > but ... ieee80211_extract_dis_subch_bmap actually already takes the > bandwidth from eht_oper into account! > Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from eht_oper into account, and the local_bw in this func is the local bandwidth i discuss. You already notice the difference between bandwidth from eht_oper and local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you should also take it into account when you use cfg80211_valid_disable_subchannel_bitmap(), right? BTW, do you want to verify the bitmap from eht_oper, or the extracted bitmap? Anyway, whether you want to verify the bitmap from eht_oper or extracted bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and bandwidth must correspond. >> - if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, >> - &link->conf->chandef)) { > > So are you saying that the real bug is that we're missing to update the > link->conf->chandef with the EHT operation from the assoc response? > > But you didn't fix that issue ... so not sure? I have described my patch with the comments above, maybe i should make my commit log more coherent. Besides, this is you first version, i made some comments on Nov. 21, 2022, 7:29 a.m. Maybe you already forget them. https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/ > > johannes >
Due to Johannes's refactor of Puncturing bitmap, this patchset can be abandoned now. Will prepare a new patch about puncturing bitmap for ath12k. On 10/19/2023 11:25 AM, Kang Yang wrote: > > > On 10/18/2023 7:39 PM, Johannes Berg wrote: >>> static bool ieee80211_config_puncturing(struct ieee80211_link_data >>> *link, >>> const struct ieee80211_eht_operation *eht_oper, >>> u64 *changed) >>> { >>> + struct cfg80211_chan_def rx_chandef = link->conf->chandef; >>> u16 bitmap = 0, extracted; >>> + u8 bw = 0; >>> if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) && >>> (eht_oper->params & >>> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct >>> ieee80211_link_data *link, >>> const u8 *disable_subchannel_bitmap = info->optional; >>> bitmap = get_unaligned_le16(disable_subchannel_bitmap); >>> + bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH); >>> + rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw); >> >> But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_* >> is a purely internal API, has nothing to do with the spec. >> >> All this might even be "accidentally correct", but it really isn't right >> at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are >> IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*. >> > > > > Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will > replace them. > > >> >> >> More generally though, I don't even understand the change. > > > During assoc, downgrade may happen in func ieee80211_config_bw(). In > this situation, the bandwidth in beacon and the bandwidth after > downgrade(chandef->width, maybe i should call it local bandwidth during > below context, will use this name in next version) during assoc will be > different. > > The change is based on this point. > > >> >>> + if (rx_chandef.width == NL80211_CHAN_WIDTH_80) >>> + rx_chandef.center_freq1 = >>> + ieee80211_channel_to_frequency(info->ccfs0, >>> + rx_chandef.chan->band); >>> + else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 || >>> + rx_chandef.width == NL80211_CHAN_WIDTH_320) >>> + rx_chandef.center_freq1 = >>> + ieee80211_channel_to_frequency(info->ccfs1, >>> + rx_chandef.chan->band); >>> + } >>> + >>> + if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, >>> + &rx_chandef)) { > > > Here i change you code > cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to > cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef) > > As described above, downgrade may happen before enter > ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef > may be different with bandwidth in beacon. > > Here, the bitmap you used is from eht_oper in beacon, but the bandwidth > you used is local bandwidth. They are not match. This is the first issue. > > >>> + link_info(link, >>> + "Got an invalid disable subchannel bitmap from AP %pM: >>> bitmap = 0x%x, bw = 0x%x. disconnect\n", >>> + link->u.mgd.bssid, >>> + bitmap, >>> + rx_chandef.width); >>> + return false; >>> } >>> extracted = ieee80211_extract_dis_subch_bmap(eht_oper, >> // I've filled in the context here in the patch > > > Here is move the cfg80211_valid_disable_subchannel_bitmap() before the > ieee80211_extract_dis_subch_bmap(), cause i think check should done > before extract. > > This is the second issue i said(perhaps not a issue). > > > >>> >>> &link->conf->chandef, >>> bitmap); >>> >>> /* accept if there are no changes */ >>> if (!(*changed & BSS_CHANGED_BANDWIDTH) && >>> extracted == link->conf->eht_puncturing) >>> return true; >> >> but ... ieee80211_extract_dis_subch_bmap actually already takes the >> bandwidth from eht_oper into account! > > Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from > eht_oper into account, and the local_bw in this func is the local > bandwidth i discuss. > > You already notice the difference between bandwidth from eht_oper and > local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you > should also take it into account when you use > cfg80211_valid_disable_subchannel_bitmap(), right? > > BTW, do you want to verify the bitmap from eht_oper, or the extracted > bitmap? > > Anyway, whether you want to verify the bitmap from eht_oper or extracted > bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and > bandwidth must correspond. > > > >>> - if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, >>> - &link->conf->chandef)) { >> >> So are you saying that the real bug is that we're missing to update the >> link->conf->chandef with the EHT operation from the assoc response? >> >> But you didn't fix that issue ... so not sure? > > > I have described my patch with the comments above, maybe i should make > my commit log more coherent. > > > Besides, this is you first version, i made some comments on Nov. 21, > 2022, 7:29 a.m. Maybe you already forget them. > https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/ > > >> >> johannes >> >
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index f93eb38ae0b8..16e15ced28a5 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -5670,11 +5670,33 @@ static bool ieee80211_rx_our_beacon(const u8 *tx_bssid, return ether_addr_equal(tx_bssid, bss->transmitted_bss->bssid); } +static enum nl80211_chan_width +ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw) +{ + switch (bw) { + case IEEE80211_STA_RX_BW_20: + return NL80211_CHAN_WIDTH_20; + case IEEE80211_STA_RX_BW_40: + return NL80211_CHAN_WIDTH_40; + case IEEE80211_STA_RX_BW_80: + return NL80211_CHAN_WIDTH_80; + case IEEE80211_STA_RX_BW_160: + return NL80211_CHAN_WIDTH_160; + case IEEE80211_STA_RX_BW_320: + return NL80211_CHAN_WIDTH_320; + default: + WARN_ON(1); + return NL80211_CHAN_WIDTH_20; + } +} + static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, const struct ieee80211_eht_operation *eht_oper, u64 *changed) { + struct cfg80211_chan_def rx_chandef = link->conf->chandef; u16 bitmap = 0, extracted; + u8 bw = 0; if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) && (eht_oper->params & @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, const u8 *disable_subchannel_bitmap = info->optional; bitmap = get_unaligned_le16(disable_subchannel_bitmap); + bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH); + rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw); + + if (rx_chandef.width == NL80211_CHAN_WIDTH_80) + rx_chandef.center_freq1 = + ieee80211_channel_to_frequency(info->ccfs0, + rx_chandef.chan->band); + else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 || + rx_chandef.width == NL80211_CHAN_WIDTH_320) + rx_chandef.center_freq1 = + ieee80211_channel_to_frequency(info->ccfs1, + rx_chandef.chan->band); + } + + if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, + &rx_chandef)) { + link_info(link, + "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n", + link->u.mgd.bssid, + bitmap, + rx_chandef.width); + return false; } extracted = ieee80211_extract_dis_subch_bmap(eht_oper, @@ -5695,16 +5739,6 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link, extracted == link->conf->eht_puncturing) return true; - if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap, - &link->conf->chandef)) { - link_info(link, - "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n", - link->u.mgd.bssid, - bitmap, - link->conf->chandef.width); - return false; - } - ieee80211_handle_puncturing_bitmap(link, eht_oper, bitmap, changed); return true; }
Puncturing bitmap and bandwidth is included in beacon's EHT Operation element. After receiving beacon, mac80211 will verify if they are match. But the bandwidth used for verification is incorrect. Because bandwidth in link->conf->chandef is a negotiated bandwidth, it may be downgraded. Should use bandwidth included in beacon. Otherwise when bandwidth downgrade occurs, even if the received values match, an error may be returned. Also, checking if bitmap and bandwidth match should be done before extraction. But here extract first and then check. So fix these two issues. Fixes: aa87cd8b3573 ("wifi: mac80211: mlme: handle EHT channel puncturing") Signed-off-by: Kang Yang <quic_kangyang@quicinc.com> --- net/mac80211/mlme.c | 54 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 10 deletions(-)