On Mon, 2023-11-13 at 10:11 +0800, Michael-CY Lee wrote:
>
> + new_chan_width = ieee80211_operating_class_to_chan_width(op_class);
> + if (!ieee80211_operating_class_to_center_freq(op_class, chan,
> + ¢er_freq1,
> + ¢er_freq2)) {
Unless I missed it, I see two places here calling it together, so seems
reasonable to fill in a chandef here instead?
> + new_chan_width = ieee80211_operating_class_to_chan_width(op_class);
> + if (!ieee80211_operating_class_to_center_freq(op_class, chan,
> + ¢er_freq1,
> + ¢er_freq2)) {
Here you have it too.
> + new_chan_width = NL80211_CHAN_WIDTH_20;
> + center_freq1 = chan->center_freq;
And actually you could just have a chandef created with
cfg80211_chandef_create(&chandef, chan, NL80211_CHAN_WIDTH_20)
which mirrors the failure case here, and just not update it when
something like
ieee80211_update_chandef_from_op_class(op_class, &chandef)
returned false (not that I necessarily think that name should be used.)
Or just pass the channel, and make it create one with WIDTH_20 in the
failure case?
ieee80211_create_chandef_from_opclass(chan, op_class, &chandef);
which is is maybe even nicer?
I'm also not quite sure why you're converting to operation elements
first?
johannes
Hi Johannes,
On Fri, 2023-11-24 at 20:31 +0100, Johannes Berg wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Mon, 2023-11-13 at 10:11 +0800, Michael-CY Lee wrote:
> >
> > +new_chan_width =
> ieee80211_operating_class_to_chan_width(op_class);
> > +if (!ieee80211_operating_class_to_center_freq(op_class, chan,
> > + ¢er_freq1,
> > + ¢er_freq2)) {
>
> Unless I missed it, I see two places here calling it together, so
> seems
> reasonable to fill in a chandef here instead?
>
> > +new_chan_width =
> ieee80211_operating_class_to_chan_width(op_class);
> > +if (!ieee80211_operating_class_to_center_freq(op_class, chan,
> > + ¢er_freq1,
> > + ¢er_freq2)) {
>
>
> Here you have it too.
>
>
> > +new_chan_width = NL80211_CHAN_WIDTH_20;
> > +center_freq1 = chan->center_freq;
>
> And actually you could just have a chandef created with
>
> cfg80211_chandef_create(&chandef, chan, NL80211_CHAN_WIDTH_20)
>
> which mirrors the failure case here, and just not update it when
> something like
>
> ieee80211_update_chandef_from_op_class(op_class, &chandef)
>
> returned false (not that I necessarily think that name should be
> used.)
>
> Or just pass the channel, and make it create one with WIDTH_20 in the
> failure case?
>
> ieee80211_create_chandef_from_opclass(chan, op_class, &chandef);
>
> which is is maybe even nicer?
Good suggestion!
We'll decide which one is better.
>
> I'm also not quite sure why you're converting to operation elements
> first?
The old flow also converted the Element to operation elements first,
then it used ieee80211_chandef_vht_oper() to build the new chandef from
operation elements.
We think it's necessary for the case that AP is trying to switch to a
160 MHz bandwidth, while the STA doesn't support the 160 MHz bandwidth.
Just like what had been done during the association,
ieee80211_chandef_vht_oper() checks the STA's capabilities and builds a
valid chandef for the STA. However, even if the STA doesn't support the
160 MHz bandwidth, ieee80211_chandef_vht_oper() doesn't mark the
conn_flags as IEEE80211_CONN_DISABLE_160MHZ, so the same check is
necessary when handling CSA.
Like we had discussed in previous mail, we expected the patch will be
simplified.
In summary, the steps for STA to handling CSA are,
1. parse the new channel information from either operating class or
WBCS Element.
2. convert the channel information into corresponding operation Element
(HT/VHT in 5 GHz band and HE/EHT in 6 GHz band)
3. Build a valid chandef from the operation Element.
>
> johannes
Best,
Micahel
>
Hi,
> The old flow also converted the Element to operation elements first,
> then it used ieee80211_chandef_vht_oper() to build the new chandef from
> operation elements.
>
> We think it's necessary for the case that AP is trying to switch to a
> 160 MHz bandwidth, while the STA doesn't support the 160 MHz bandwidth.
Yeah, you're right, it did before and I suppose it's still easier than
managing two conversions, since some formats are the same.
> Just like what had been done during the association,
> ieee80211_chandef_vht_oper() checks the STA's capabilities and builds a
> valid chandef for the STA. However, even if the STA doesn't support the
> 160 MHz bandwidth, ieee80211_chandef_vht_oper() doesn't mark the
> conn_flags as IEEE80211_CONN_DISABLE_160MHZ, so the same check is
> necessary when handling CSA.
Right.
> Like we had discussed in previous mail, we expected the patch will be
> simplified.
Yeah I just circled back to it for stupid reasons (I guess mostly forgot
to mark in patchwork that you were going to make changes) and looked
again after having looked at all the other code in the series I posted
late last week.
> In summary, the steps for STA to handling CSA are,
> 1. parse the new channel information from either operating class or
> WBCS Element.
> 2. convert the channel information into corresponding operation Element
> (HT/VHT in 5 GHz band and HE/EHT in 6 GHz band)
> 3. Build a valid chandef from the operation Element.
>
Sounds good.
johannes