On 11/23/2022 12:56 AM, Johannes Berg wrote:
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 ...
Yes, to my understanding, I think it's more appropriate to define it
like you in "ieee80211_bss_conf".
Well Aloka is back - Aloka maybe you have some comments?
Added few comments below.
I can somewhat understand why you might want puncturing to be part of
the chandef for an AP, especially for configuration, though I'm honestly
not sure what the use cases are; perhaps some other use for that
bandwidth (perhaps cellular?), but then does that really mean we should
refuse configurations that aren't in line with it?
If we put puncturing into the chandef that might not even mean we
*refuse* configurations, it would initially just mean we need another
channel context (if supported, otherwise refused) when we add two.
But given that the primary use case seems to be different use for the
part of the spectrum that's being punctured - maybe we should set it up
in a wholly different way and have some way of "carving out" reserved
spectrum?
The primary use case from AP side is trying to get maximum bandwidth in
a noisy environment. Without puncturing AP has to downgrade from 80 MHz
to to 40/20 Mhz but now it can use 60 Mhz if only 20 Mhz in the given
channel width has a lot of interference.
As puncturing bitmap is tightly coupled with the channel and bandwidth,
it made sense to put it in chandef and the channel switch case was
covered inherently because it uses chandef.
But I see that doesn't work for client wanting to connect to two APs in
multi-BSS case.
So you could, say, reserve channel 40, and then to have an 80 MHz AP on
channels 36-48 you'd have to puncture 40 there. We wouldn't have to
store it as part of the chandef/chanctx because the validation is now
done against the reservation; or we could even store it as part of the
chandef/chanctx but say it only applies to beaconing modes or such, and
not handle it as part of the chandef equality comparison.
Unless we know for sure that the punctured bandwidth is going to be used
for some other purpose, the above mentioned approach seems little
complicated and unnecessary.
And really that's the thing that matters to the client here, the client
never has a choice, so if it's part of the equality comparison on the
client, then the client might not be able to simultaneously connect to
two BSSes (or need two chanctxs to do it) if both have 80 MHz on channel
36, but one has puncturing and the other doesn't. That doesn't really
make sense IMHO.
If we treat the puncturing as a more global "reserved spectrum" case,
then we can still put the puncturing into the chandef, say it applies to
beaconing modes only (I suppose AP is the only relevant one, perhaps
someone will care about mesh in the future?), not have it as part of the
== comparison. All APs would have to adhere to that.
I agree that cfg80211_chandef_identical() should return true if the
bitmap is different but other basic channel parameters are same for AP
mode too.
That still leaves a corner case of concurrent client + AP functionality
which can only be handled if hardware can deal with different puncturing
settings per interface? Which again argues for *not* storing it as part
of the chandef, I'd say.
Muna sent a AP mode follow-up based on this current patch -
https://patchwork.kernel.org/project/linux-wireless/list/?series=701342&state=%2A&archive=both
Please note that we missed to include some compilation error fixes while
sending, will send a new version soon.
This approach needs little extra NL80211 attribute processing in case of
channel switch along with start_ap() and SET_INTERFACE but it works.
Thanks.
