Hi Johannes,
Thanks for the reply.
I would like to start by explaining why we prioritized the operating
class.
In IEEE Std 802.11-2020 11.38.4 (Channel switching methods for a VHT
BSS), there is a note about operating class and WBCS Element:
“NOTE 2—The indicated operating class within the Extended Channel
Switch Announcement element identifies the bandwidth and the relative
position of the primary 20 MHz and secondary 20 MHz channels. Hence a
Wide Bandwidth Channel Switch subelement is unnecessary when the
Extended Channel Switch Announcement element is used for a channel
switch to a 40 MHz bandwidth.”
Although it only limits the switch to a 40 MHz bandwidth, we thought it
is applicable in 80/80+80/160 MHz bandwidth. So in the version 1, we
preferred to use the operating class than the WBCS Element.
Also, we agree your suggestion that WBCS Element has higher priority
than operating class. Our reason is, when channel switch to a bandwidth
wider than 40 MHz, WBCS Element is mandatory while ECSA (which contains
operating class) is not.
(Note that either CSA or ECSA will be sent in a channel switch.)
BTW, I would also like to clarify one thing. We did this upstream work
solely based on the standard and with the intention of contributing to
mac80211. In fact, we developed and tested with the upstream driver
(mt76) for AP and STA solution together. Before doing upstream, we used
the MTK AP (proprietary solution) to do IOT tests and indeed found some
problems, which were already reported and fixed.
So, we were not aware of what was going on to MTK AP until we used it
to do an IOT test. We didn’t do it with the intention of covering the
broken WBCS element sent by MTK AP.
For other replies, please see the inline. (we deleted some of your
replies since we just change as you suggested)
In summary, we’ll modify the code and submit a new version.
Best,
Michael
On Fri, 2023-12-01 at 20:06 +0100, Johannes Berg wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi,
>
> So looking at this ... I'm not sure I want the operating class
> parsing?
>
> On Wed, 2023-11-29 at 13:43 +0800, Michael-CY Lee wrote:
> > The new Wi-Fi Standard (IEEE P80211be D4.1) specifies that the Wide
> > Bandwidth Channel Switch (WBCS) Element subfields have the same
> > definitions as VHT operation information if the operating band is
> not
> > S1G.
>
> Actually that's already in REVme, no?
>
> "If the New Operating Class field in the frame [...] does not
> indicate
> an S1G band, the subfields New Channel Width, New Channel Center
> Frequency Segment 0 and New Channel Center Frequency Segment 1 have
> the
> same definition, respectively, as Channel Width, Channel Center
> Frequency Segment 0, Channel Center Freqauency Segment 1 in the VHT
> Operation Information field, described in Table 9-313 (VHT Operation
> Information subfields)."
Yes and actually it also shows up in IEEE Std 802.11-2020. I’ll modify
the description to make it clear.
>
> > The problem comes when the BSS is in 6 GHz band, the STA parses the
> WBCS
> > Element by ieee80211_chandef_vht_oper(), which checks the
> capabilities for
> > HT/VHT mode, not HE/EHT mode.
>
> OK, but that's an implementation issue, we can make it look at HE
> capabilities too, for 6 GHz?
yes, that’s what this patch intends to do.
>
> > Also, it adds the way to use op_class in ECSA
> > Element to build a new chandef.
>
> Not sure why that?
I explained it above.
>
> > In summary, the new steps for STA to handle CSA event are:
> > 1. build the new chandef from the CSA-related Elements.
> > (CSA, ECSA, WBCS, etc.)
>
> Actually that's not what you do? The logic is more like
>
> - if BWI present: use only that
> - if operating class/channel can be used: use only that
> - if WBCS is present: use only that
> - otherwise: use (ext) chanswitch element info from before
>
>
> Given that I just got a report of an MTK AP that has a broken WCBS
> element, I'm not sure I'm happy with that logic ;-)
>
> Seems to me the operating class use should maybe be further down the
> list, and perhaps (if it's there) validate the other elements against
> it, to make sure the AP isn't confused?
>
> So perhaps better:
> - use, in this order: BWI, WBCS, ECSA, CSA (according to the mode
> we parse as, and our own capabilities)
> - if present, check that operating class agrees
>
> no?
we agree the order. AS for the check for operating class, we think it’s
a little useless.
Image the case that the WBCS Element and the operating class indicate
two different chandef, but both are valid, which one should we trust,
or neither?
Our answer is that WBCS Element is worth trusting, since it is
mandatory in a channel switch to bandwidth wider than 40 MHz, while
operating class isn’t.
The operating class can be use in the case that the WBCS Element is
missing or indicates a wrong chandef. In other word, the operating
class has lower priority than WBCS Element.
What do you think?
>
> > +break;
> > +case IEEE80211_VHT_CHANWIDTH_80P80MHZ:
> > +chandef->width = NL80211_CHAN_WIDTH_80P80;
> > +chandef->center_freq1 = cf0;
> > +chandef->center_freq2 = cf1;
>
> and not sure I remember well, but this one too? at least going by
> this:
yes, it is also deprecated.
>
> > +ieee80211_conn_flags_t conn_flags,
> > +u32 vht_cap_info,
> > +struct cfg80211_chan_def *chandef)
> > +{
> > +u32 control_freq, center_freq1, center_freq2;
> > +enum nl80211_chan_width chan_width;
> > +struct ieee80211_ht_operation *ht_oper = NULL;
> > +struct ieee80211_vht_operation *vht_oper = NULL;
>
> No point initializing those to NULL?
>
> > +if (conn_flags & (IEEE80211_CONN_DISABLE_HT |
> > + IEEE80211_CONN_DISABLE_40MHZ)) {
> > +chandef->chan = NULL;
> > +return 0;
> > +}
> > +
> > +control_freq = chandef->chan->center_freq;
> > +center_freq1 = chandef->center_freq1;
> > +center_freq2 = chandef->center_freq2;
> > +chan_width = chandef->width;
> > +
> > +ht_oper = kzalloc(sizeof(*ht_oper), GFP_KERNEL);
> > +if (!ht_oper)
> > +return -ENOMEM;
>
> Not sure I see value in putting this on the heap, it's tiny?
>
> > +vht_oper = kzalloc(sizeof(*vht_oper), GFP_KERNEL);
>
> same here
>
> > +if (!vht_oper) {
> > +kfree(ht_oper);
> > +return -ENOMEM;
>
> and if you have these gone, you no longer need a return value either,
> which is nice.
>
> > +static inline int
>
> same here
>
> > +validate_chandef_by_6ghz_he_eht_oper(struct ieee80211_sub_if_data
> *sdata,
> > + ieee80211_conn_flags_t conn_flags,
> > + struct cfg80211_chan_def *chandef)
> > +{
> > +u32 size, control_freq, center_freq1, center_freq2;
> > +enum nl80211_chan_width chan_width;
> > +struct ieee80211_he_operation *he_oper = NULL;
> > +struct ieee80211_eht_operation *eht_oper = NULL;
>
> same here about =NULL, and for the allocations too
We will change the HT/VHT operation to static allocation in the
function. However, the size of HE/EHT operation are variable, so it can
only be dynamically allocated.
>
> > +case NL80211_CHAN_WIDTH_80P80:
> > +he_6ghz_oper->control =
> > +IEEE80211_HE_6GHZ_OPER_CTRL_CHANWIDTH_160MHZ;
>
> Is that right? Do HE/EHT even still do 80+80?
According to the Standard, 80+80 MHz bandwidth still exists in HE mode.
The function ieee80211_chandef_he_6ghz_oper() handles this case.
However, from our experience, there are few commercial APs that support
80+80 MHz bandwidth in HE mode. We can remove it, what do you think?
>
> > +/* parse one of the Elements to build a new chandef */
>
> except you don't really, as discussed above
>
> I'd actually kind of like to have these validated against each other,
> but that's for another day, and we don't build the strictest
> implementation.
>
> Though I probably will make the implementation optionally stricter in
> some places like this, and enable that for all testing/certification
> in
> the future.
we agree that the validation can be done in the future.
>
> > +} else if (!ieee80211_operating_class_to_chandef(new_op_class,
> new_chan,
> > + &new_chandef)) {
> > +if (wide_bw_chansw_ie)
> > +wbcs_ie_to_chandef(wide_bw_chansw_ie, &new_chandef);
> > +else
> > +new_chandef = csa_ie->chandef;
>
> So like I said above, this starts to ignore WBCS if you have things
> from
> operating class, why? Is the only reason it doesn't work against your
> broken AP now? ;-)
Like I explained above, we did find and report some problems during the
tests. But it's definitely not the reason.
>
> > if (elems->max_channel_switch_time)
> > csa_ie->max_switch_time =
> > (elems->max_channel_switch_time[0] << 0) |
> > -(elems->max_channel_switch_time[1] << 8) |
> > +(elems->max_channel_switch_time[1] << 8) |
> >
>
> No need, I guess, but hey, doesn't matter much. How'd you find this
> anyway? :)
Just saw it accidently. Anyway, since it’s harmless, we will drop it.
