On Mon, 2022-11-21 at 15:29 +0800, Kang Yang wrote:
> Hi:
> 1.Do you have any new version about this RFC patch?
Not really, no.
> 2.I have some questions about this patch:
(a couple of blank lines and some trimming of the context really would
help ... please try next time)
> > +static u16
> > +ieee80211_extract_dis_subch_bmap(const struct ieee80211_eht_operation *eht_oper,
> > + struct cfg80211_chan_def *chandef, u16 bitmap)
> > +{
> > + int sta_center_freq = ieee80211_channel_to_frequency(eht_oper->ccfs,
> > + chandef->chan->band);
> > + u32 center_freq = chandef->chan->center_freq;
> The shift is calculated by the difference of old and new channel center
> frequency.The new channel center frequency should be
> "chandef->center_freq1" after BW negotitaion.
> "chandef->chan->center_freq" is the primary channel frequency.
Yeah I think we did fix a couple of bugs in this area later.
> > + u8 sta_bw = 20 * BIT(u8_get_bits(eht_oper->chan_width,
> > + IEEE80211_EHT_OPER_CHAN_WIDTH));
> > + u8 bw = 20 * BIT(ieee80211_chan_width_to_rx_bw(chandef->width));
> > + int sta_start_freq = sta_center_freq - sta_bw / 2;
> > + int start_freq = center_freq - bw / 2;
> > + u16 shift = (start_freq - sta_start_freq) / 20;
> > + u16 mask = BIT(sta_bw / 20) - 1;
> The mask is used to extra the valid bit according to the new BW,
> but current algorithm is using the old bandwidth.
:)
> > + while (chandef->width > NL80211_CHAN_WIDTH_40) {
> > + extracted =
> > + ieee80211_extract_dis_subch_bmap(eht_oper, chandef,
> > + bitmap);
> > +
> > + if (ieee80211_valid_disable_subchannel_bitmap(&bitmap,
> > + chandef->width))
> Here extract the bitmap according new negotiated BW. After extracting,
> check whether it is valid.
> I think you should use "&extracted" instead of "&bitmap"
Yeah I guess that makes sense.
I don't know what fixes we have and what version of this patch this is.
> > +static bool ieee80211_config_puncturing(struct ieee80211_sub_if_data *sdata,
> > + const struct ieee80211_eht_operation *eht_oper,
> > + u64 *changed)
> > +{
> > + u16 bitmap, extracted;
> > + u8 bw;
> > +
> > + if (!u8_get_bits(eht_oper->present_bm,
> > + IEEE80211_EHT_OPER_DISABLED_SUBCHANNEL_BITMAP_PRESENT))
> > + bitmap = 0;
> > + else
> > + bitmap = get_unaligned_le16(eht_oper->disable_subchannel_bitmap);
> > +
> Should check initial bitmap here.
What do you mean by "initial" here?
> > + extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
> > + &sdata->vif.bss_conf.chandef,
> > + bitmap);
> > +
> > + /* accept if there are no changes */
> > + if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
> > + extracted == sdata->vif.bss_conf.eht_puncturing)
> > + return true;
> > +
> > + bw = u8_get_bits(eht_oper->chan_width, IEEE80211_EHT_OPER_CHAN_WIDTH);
> > +
> > + if (!ieee80211_valid_disable_subchannel_bitmap(&bitmap, bw)) {
> > + sdata_info(sdata,
> > + "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
> > + sdata->u.mgd.associated->bssid, bitmap, bw);
> > + return false;
> > + }
> The initial bitmap received from the AP is checked here.
> I think it should be carried out before the extraction above.
Ah, yes I guess that makes sense.
Anyway the more fundamental thing we have to figure out here (and thanks
for bringing this back) is how we treat the puncturing - QCOM's AP-side
puncturing patch treated it as part of the chandef, but that's not
working well for client side ...
johannes
