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
