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
