diff mbox series

[01/13] wifi: cfg80211: Add provision to advertise multiple radio in one wiphy

Message ID 20240328072916.1164195-2-quic_periyasa@quicinc.com (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show
Series wifi: Add multi physical hardware iface combination support | expand

Commit Message

Karthikeyan Periyasamy March 28, 2024, 7:29 a.m. UTC
From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], the prerequisite for MLO support
in cfg80211/mac80211 is that all the links participating in MLO must
belong to the same wiphy/ieee80211_hw. To meet this expectation, some
drivers may need to group multiple discrete hardware, each acting as a
link in MLO, under one wiphy. Though most of the hardware abstractions must
be handled within the driver itself, it may be required to have some sort
of mapping while describing interface combination capabilities for each of
the underlying hardware. Add an advertisement provision for drivers
supporting such MLO mode of operation.

Capability advertisement such as the number of supported channels for each
physical hardware has been identified to support some of the common use
cases.

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-2-quic_vthiagar@quicinc.com/

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
Co-developed-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 include/net/cfg80211.h |  24 ++++++++++
 net/wireless/core.c    | 100 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

Comments

Johannes Berg March 28, 2024, 7:46 a.m. UTC | #1
That's a big set, not sure I can review it all at once :)


> +struct ieee80211_chans_per_hw {
> +	u32 n_chans;
> +	struct ieee80211_channel chans[];

That should probably use __counted_by() these days.

> + * @hw_chans: list of the channels supported by every constituent underlying
> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
> + *	one wiphy can advertise the list of channels supported by each physical
> + *	hardware in this list. Underlying hardware specific channel list can be
> + *	used while describing interface combination for each of them.

I'd expect there to be a limit on channels being within a single band on
a single "hardware"?

> + * @num_hw: number of underlying hardware for which the channels list are

"number of [...] hardware" sounds odd to me, perhaps 'devices'?

> + *	advertised in @hw_chans, 0 if multi hardware is not support. Expect >2
> + *	if multi hardware is support.

>1, I guess, not >2 - and ==1 makes no sense really, maybe it should say
that?

> +static bool
> +cfg80211_hw_chans_in_supported_list(struct wiphy *wiphy,
> +				    const struct ieee80211_chans_per_hw *chans)
> +{
> +	enum nl80211_band band;
> +	struct ieee80211_supported_band *sband;
> +	int i, j;
> +
> +	for (i = 0; i < chans->n_chans; i++) {
> +		bool found = false;

So if there should be a limit on single band per HW then probably this
check could be written more efficiently: finding the band first based on
the first channel, and then checking all the channels exist within the
band.

johannes
Vasanthakumar Thiagarajan March 29, 2024, 2:11 p.m. UTC | #2
On 3/28/2024 1:16 PM, Johannes Berg wrote:
> That's a big set, not sure I can review it all at once :)
> 
> 
>> +struct ieee80211_chans_per_hw {
>> +	u32 n_chans;
>> +	struct ieee80211_channel chans[];
> 
> That should probably use __counted_by() these days.
> 
>> + * @hw_chans: list of the channels supported by every constituent underlying
>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>> + *	one wiphy can advertise the list of channels supported by each physical
>> + *	hardware in this list. Underlying hardware specific channel list can be
>> + *	used while describing interface combination for each of them.
> 
> I'd expect there to be a limit on channels being within a single band on
> a single "hardware"?
> 

There are ath12k hardware supporting multiple band which need to be 
registered under one mac80211_hw/wiphy. This design is to support such 
hardware. I agree, it is adding complexities. I was thinking if this can 
be done in steps , first limiting to single band for a single hardware 
then extending it with multiple bands later but that may bring in 
different set of challenges...


Vasanth
Johannes Berg March 29, 2024, 2:30 p.m. UTC | #3
On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
> > 
> > > + * @hw_chans: list of the channels supported by every constituent underlying
> > > + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
> > > + *	one wiphy can advertise the list of channels supported by each physical
> > > + *	hardware in this list. Underlying hardware specific channel list can be
> > > + *	used while describing interface combination for each of them.
> > 
> > I'd expect there to be a limit on channels being within a single band on
> > a single "hardware"?
> > 
> 
> There are ath12k hardware supporting multiple band which need to be 
> registered under one mac80211_hw/wiphy. This design is to support such 
> hardware.

Oh OK, that was something that I didn't have in mind any more, or never
knew or paid attention to.

> I agree, it is adding complexities. I was thinking if this can 
> be done in steps , first limiting to single band for a single hardware 
> then extending it with multiple bands later but that may bring in 
> different set of challenges...

Probably not point, yeah.

johannes
Ben Greear March 29, 2024, 2:47 p.m. UTC | #4
On 3/29/24 07:30, Johannes Berg wrote:
> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>
>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>> + *	one wiphy can advertise the list of channels supported by each physical
>>>> + *	hardware in this list. Underlying hardware specific channel list can be
>>>> + *	used while describing interface combination for each of them.
>>>
>>> I'd expect there to be a limit on channels being within a single band on
>>> a single "hardware"?
>>>
>>
>> There are ath12k hardware supporting multiple band which need to be
>> registered under one mac80211_hw/wiphy. This design is to support such
>> hardware.
> 
> Oh OK, that was something that I didn't have in mind any more, or never
> knew or paid attention to.

Would it work to leave the phy reporting pretty much as it is now, but add
a 'associated_peer_radios' list section, so that each phy could report the phys
associated with it?  Then user-space, driver, mac80211 etc could look up the
other phys as needed to get a full picture?

At least with mtk 7996, it appears there will be 3 single-band phys can can be used
independently, but I assume when they have MLO support, those three will be logically
grouped.

Thanks,
Ben
Karthikeyan Periyasamy April 1, 2024, 4:19 a.m. UTC | #5
On 3/28/2024 1:16 PM, Johannes Berg wrote:
> That's a big set, not sure I can review it all at once :)
>
>
>> +struct ieee80211_chans_per_hw {
>> +	u32 n_chans;
>> +	struct ieee80211_channel chans[];
> That should probably use __counted_by() these days.

sure, will address this comment in the next version of the patch.


>> + * @num_hw: number of underlying hardware for which the channels list are
> "number of [...] hardware" sounds odd to me, perhaps 'devices'?

sure, will address this comment in the next version of the patch.


>> + *	advertised in @hw_chans, 0 if multi hardware is not support. Expect >2
>> + *	if multi hardware is support.
>> 1, I guess, not >2 - and ==1 makes no sense really, maybe it should say
> that?

sure, will address this comment in the next version of the patch.
Johannes Berg April 10, 2024, 7:56 a.m. UTC | #6
On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
> On 3/29/24 07:30, Johannes Berg wrote:
> > On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
> > > > 
> > > > > + * @hw_chans: list of the channels supported by every constituent underlying
> > > > > + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
> > > > > + *	one wiphy can advertise the list of channels supported by each physical
> > > > > + *	hardware in this list. Underlying hardware specific channel list can be
> > > > > + *	used while describing interface combination for each of them.
> > > > 
> > > > I'd expect there to be a limit on channels being within a single band on
> > > > a single "hardware"?
> > > > 
> > > 
> > > There are ath12k hardware supporting multiple band which need to be
> > > registered under one mac80211_hw/wiphy. This design is to support such
> > > hardware.
> > 
> > Oh OK, that was something that I didn't have in mind any more, or never
> > knew or paid attention to.
> 
> Would it work to leave the phy reporting pretty much as it is now, but add
> a 'associated_peer_radios' list section, so that each phy could report the phys
> associated with it?  Then user-space, driver, mac80211 etc could look up the
> other phys as needed to get a full picture?
> 

There's not really a good way to _do_ that short of creating multiple
wiphys, but that causes _massive_ complexity in the stack (both cfg80211
and mac80211) so we rejected it years ago.

johannes
Karthikeyan Periyasamy April 10, 2024, 9:08 a.m. UTC | #7
On 3/28/2024 1:16 PM, Johannes Berg wrote:
> That's a big set, not sure I can review it all at once :)
> 

I plan to separate the code refactoring changes from this patchset. Do 
you have any concerns?
Johannes Berg April 10, 2024, 9:09 a.m. UTC | #8
On Wed, 2024-04-10 at 14:38 +0530, Karthikeyan Periyasamy wrote:
> 
> On 3/28/2024 1:16 PM, Johannes Berg wrote:
> > That's a big set, not sure I can review it all at once :)
> > 
> 
> I plan to separate the code refactoring changes from this patchset. Do 
> you have any concerns?
> 
If you split it up? I don't see why.

johannes
Karthikeyan Periyasamy April 10, 2024, 9:15 a.m. UTC | #9
On 4/10/2024 2:39 PM, Johannes Berg wrote:
> On Wed, 2024-04-10 at 14:38 +0530, Karthikeyan Periyasamy wrote:
>>
>> On 3/28/2024 1:16 PM, Johannes Berg wrote:
>>> That's a big set, not sure I can review it all at once :)
>>>
>>
>> I plan to separate the code refactoring changes from this patchset. Do
>> you have any concerns?
>>
> If you split it up? I don't see why.
> 

Based on the comments, the patch count for the next version is 
increasing. I believe it would be beneficial to have the refactor 
patches in a separate submission before sending the next version.
Ben Greear April 10, 2024, 2:37 p.m. UTC | #10
On 4/10/24 00:56, Johannes Berg wrote:
> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>> On 3/29/24 07:30, Johannes Berg wrote:
>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>
>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>> + *	one wiphy can advertise the list of channels supported by each physical
>>>>>> + *	hardware in this list. Underlying hardware specific channel list can be
>>>>>> + *	used while describing interface combination for each of them.
>>>>>
>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>> a single "hardware"?
>>>>>
>>>>
>>>> There are ath12k hardware supporting multiple band which need to be
>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>> hardware.
>>>
>>> Oh OK, that was something that I didn't have in mind any more, or never
>>> knew or paid attention to.
>>
>> Would it work to leave the phy reporting pretty much as it is now, but add
>> a 'associated_peer_radios' list section, so that each phy could report the phys
>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>> other phys as needed to get a full picture?
>>
> 
> There's not really a good way to _do_ that short of creating multiple
> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
> and mac80211) so we rejected it years ago.

I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
look like a single phy?

For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
cases, but I guess there will be also some way to treat them as a single entity when using MLO.

For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
at least somewhat like a single entity (while also concurrently being able to act as individual
wiphys so that one can do a mix of MLO and non MLO sta/AP.)

Maybe I'm missing the entire point of the ath12k patches though...

Thanks,
Ben

> 
> johannes
>
Johannes Berg April 10, 2024, 3:42 p.m. UTC | #11
On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
> On 4/10/24 00:56, Johannes Berg wrote:
> > On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
> > > On 3/29/24 07:30, Johannes Berg wrote:
> > > > On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
> > > > > > 
> > > > > > > + * @hw_chans: list of the channels supported by every constituent underlying
> > > > > > > + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
> > > > > > > + *	one wiphy can advertise the list of channels supported by each physical
> > > > > > > + *	hardware in this list. Underlying hardware specific channel list can be
> > > > > > > + *	used while describing interface combination for each of them.
> > > > > > 
> > > > > > I'd expect there to be a limit on channels being within a single band on
> > > > > > a single "hardware"?
> > > > > > 
> > > > > 
> > > > > There are ath12k hardware supporting multiple band which need to be
> > > > > registered under one mac80211_hw/wiphy. This design is to support such
> > > > > hardware.
> > > > 
> > > > Oh OK, that was something that I didn't have in mind any more, or never
> > > > knew or paid attention to.
> > > 
> > > Would it work to leave the phy reporting pretty much as it is now, but add
> > > a 'associated_peer_radios' list section, so that each phy could report the phys
> > > associated with it?  Then user-space, driver, mac80211 etc could look up the
> > > other phys as needed to get a full picture?
> > > 
> > 
> > There's not really a good way to _do_ that short of creating multiple
> > wiphys, but that causes _massive_ complexity in the stack (both cfg80211
> > and mac80211) so we rejected it years ago.
> 
> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
> look like a single phy?

Correct.

> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
> cases

No, I don't see why, and if you want that we wouldn't support it anyway,
you'd have to have a module option or something to decide which way to
go.

But it really ought to not be needed - the point of these patches is to
give userspace enough information to know how to (and where) to create
separate BSSes, with or without MLO between them.

> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
> at least somewhat like a single entity

Yes.

> (while also concurrently being able to act as individual
> wiphys so that one can do a mix of MLO and non MLO sta/AP.)

No.

johannes
Ben Greear April 10, 2024, 4:23 p.m. UTC | #12
On 4/10/24 08:42, Johannes Berg wrote:
> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>> On 4/10/24 00:56, Johannes Berg wrote:
>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>
>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>> + *	one wiphy can advertise the list of channels supported by each physical
>>>>>>>> + *	hardware in this list. Underlying hardware specific channel list can be
>>>>>>>> + *	used while describing interface combination for each of them.
>>>>>>>
>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>> a single "hardware"?
>>>>>>>
>>>>>>
>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>> hardware.
>>>>>
>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>> knew or paid attention to.
>>>>
>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>> other phys as needed to get a full picture?
>>>>
>>>
>>> There's not really a good way to _do_ that short of creating multiple
>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>> and mac80211) so we rejected it years ago.
>>
>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>> look like a single phy?
> 
> Correct.
> 
>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>> cases
> 
> No, I don't see why, and if you want that we wouldn't support it anyway,
> you'd have to have a module option or something to decide which way to
> go.
> 
> But it really ought to not be needed - the point of these patches is to
> give userspace enough information to know how to (and where) to create
> separate BSSes, with or without MLO between them.

If phy0 told user-space that phy1 and phy2 were 'mlo peers', and phy1 and phy2
gave similar mapping, couldn't user-space just notice the peer group and then
have all the info it needed without the bulk of the patch set in question?

So then if hostapd wants to have 3 radios in an mlo grouping, it can do so.

And if instead it wants three old-style wifi-6 AP interfaces, it could do that
as well.  I don't see why it would need any module option, and I also do not
see why it could not do both behaviours concurrently (one BSSID being MLO, second one
being non MLO, for instance).

>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>> at least somewhat like a single entity
> 
> Yes.
> 
>> (while also concurrently being able to act as individual
>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
> 
> No.

How will you allow all three bands/phys to host bssids that can talk to
wifi-6 and earlier stations if there is only a single phy seen by hostapd?

I definitely want to put STA vdevs on the three individual 7996 phys and have them
be able to talk to non MLO APs. This currently works.

I suspect other use cases, such as meshing with non MLO APs may also want
this sort of ability.

Thanks,
Ben
Jeff Johnson April 10, 2024, 4:43 p.m. UTC | #13
On 4/10/2024 9:23 AM, Ben Greear wrote:
> On 4/10/24 08:42, Johannes Berg wrote:
>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>
>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>> + *	one wiphy can advertise the list of channels supported by each physical
>>>>>>>>> + *	hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>> + *	used while describing interface combination for each of them.
>>>>>>>>
>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>> a single "hardware"?
>>>>>>>>
>>>>>>>
>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>> hardware.
>>>>>>
>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>> knew or paid attention to.
>>>>>
>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>> other phys as needed to get a full picture?
>>>>>
>>>>
>>>> There's not really a good way to _do_ that short of creating multiple
>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>> and mac80211) so we rejected it years ago.
>>>
>>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>>> look like a single phy?
>>
>> Correct.
>>
>>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>>> cases
>>
>> No, I don't see why, and if you want that we wouldn't support it anyway,
>> you'd have to have a module option or something to decide which way to
>> go.
>>
>> But it really ought to not be needed - the point of these patches is to
>> give userspace enough information to know how to (and where) to create
>> separate BSSes, with or without MLO between them.
> 
> If phy0 told user-space that phy1 and phy2 were 'mlo peers', and phy1 and phy2
> gave similar mapping, couldn't user-space just notice the peer group and then
> have all the info it needed without the bulk of the patch set in question?
> 
> So then if hostapd wants to have 3 radios in an mlo grouping, it can do so.
> 
> And if instead it wants three old-style wifi-6 AP interfaces, it could do that
> as well.  I don't see why it would need any module option, and I also do not
> see why it could not do both behaviours concurrently (one BSSID being MLO, second one
> being non MLO, for instance).
> 
>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>>> at least somewhat like a single entity
>>
>> Yes.
>>
>>> (while also concurrently being able to act as individual
>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>
>> No.
> 
> How will you allow all three bands/phys to host bssids that can talk to
> wifi-6 and earlier stations if there is only a single phy seen by hostapd?
> 
> I definitely want to put STA vdevs on the three individual 7996 phys and have them
> be able to talk to non MLO APs. This currently works.
> 
> I suspect other use cases, such as meshing with non MLO APs may also want
> this sort of ability.
> 
> Thanks,
> Ben
> 

Ben, the patches we have posted so far allow ath12k to either have each phy
assigned to a separate wiphy (legacy operation) or have multiple phys assigned
to a single wiphy (required for MLO operation). In an upcoming patch we'll
introduce a DT-driven mechanism to perform the mapping of phys to wiphys. So
the goal is to be able to support both modes of operation.

/jeff

/jeff
Maxime Bizon April 10, 2024, 6:01 p.m. UTC | #14
On Wed, 2024-04-10 at 09:23 -0700, Ben Greear wrote:


Hello Ben,

> How will you allow all three bands/phys to host bssids that can talk
> to wifi-6 and earlier stations if there is only a single phy seen by
> hostapd?

Unless I'm mistaken (I did not try the patches), the single-phy becomes
multi-concurrent-AP capable (described via netlink iface combination).

So you can still create multiple devices (AP type wlanX) and have the
same behavior as previously.

If I'm wrong, then I agree with you, the proposed solution is a
regression.
Maxime Bizon April 10, 2024, 6:08 p.m. UTC | #15
On Wed, 2024-04-10 at 09:43 -0700, Jeff Johnson wrote:


Hello Jeff,

> Ben, the patches we have posted so far allow ath12k to either have
> each phy assigned to a separate wiphy (legacy operation) or have
> multiple phys assigned to a single wiphy (required for MLO
> operation). In an upcoming patch we'll introduce a DT-driven

When the physical wiphy are grouped in a logical single wiphy, can we
still do "legacy" operations on the individual physical wiphy ? 

For example, starting a 5Ghz AP in ax-only mode, and at the same time
creating a STA interface on 2.4GHz ?

If not that's a regression. Changing DT (imply reboot) does not really
fix the issue.
Ben Greear April 10, 2024, 9:03 p.m. UTC | #16
On 4/10/24 08:42, Johannes Berg wrote:
> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>> On 4/10/24 00:56, Johannes Berg wrote:
>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>
>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>> + *	hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>> + *	one wiphy can advertise the list of channels supported by each physical
>>>>>>>> + *	hardware in this list. Underlying hardware specific channel list can be
>>>>>>>> + *	used while describing interface combination for each of them.
>>>>>>>
>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>> a single "hardware"?
>>>>>>>
>>>>>>
>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>> hardware.
>>>>>
>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>> knew or paid attention to.
>>>>
>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>> other phys as needed to get a full picture?
>>>>
>>>
>>> There's not really a good way to _do_ that short of creating multiple
>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>> and mac80211) so we rejected it years ago.
>>
>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>> look like a single phy?
> 
> Correct.
> 
>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>> cases
> 
> No, I don't see why, and if you want that we wouldn't support it anyway,
> you'd have to have a module option or something to decide which way to
> go.
> 
> But it really ought to not be needed - the point of these patches is to
> give userspace enough information to know how to (and where) to create
> separate BSSes, with or without MLO between them.
> 
>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>> at least somewhat like a single entity
> 
> Yes.
> 
>> (while also concurrently being able to act as individual
>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
> 
> No.

Hello Johannes,

Is there any design document for the combined phy approach somewhere publicly available?

It is hard to understand the over all goals by just reading patches as they show up on
the public mailing lists...

Thanks,
Ben
Vasanthakumar Thiagarajan April 11, 2024, 4:26 p.m. UTC | #17
On 4/10/2024 11:38 PM, Maxime Bizon wrote:
> 
> On Wed, 2024-04-10 at 09:43 -0700, Jeff Johnson wrote:
> 
> 
> Hello Jeff,
> 
>> Ben, the patches we have posted so far allow ath12k to either have
>> each phy assigned to a separate wiphy (legacy operation) or have
>> multiple phys assigned to a single wiphy (required for MLO
>> operation). In an upcoming patch we'll introduce a DT-driven
> 
> When the physical wiphy are grouped in a logical single wiphy, can we
> still do "legacy" operations on the individual physical wiphy ?
> 

Please note that there will be only one wiphy when all the radios are
grouped together. However, there can be interfaces working concurrently
on each underlying radio advertised in this composite wiphy through
the proposed interface combination capability extensions.

> For example, starting a 5Ghz AP in ax-only mode, and at the same time
> creating a STA interface on 2.4GHz ?
> 

Yes, such use cases continue to be supported in single wiphy mode.

> If not that's a regression. Changing DT (imply reboot) does not really
> fix the issue.
>   

This is not the case really.

Vasanth
Maxime Bizon April 11, 2024, 4:39 p.m. UTC | #18
On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:

Hello,

Thanks for making it clear,


> > For example, starting a 5Ghz AP in ax-only mode, and at the same
> > time
> > creating a STA interface on 2.4GHz ?

> Yes, such use cases continue to be supported in single wiphy mode.

Understood, but I see some corner cases.


For example, assume two bands AP hardware, 2.4GHz and 5GHz.

Previously:
  - phy0 is 2.4Ghz
  - phy1 is 5Ghz
  - iw phy phy0 interface create wlan0 type managed
  - iw dev wlan0 scan

=> will only scan 2.4Ghz


With single phy approach:
  - phy0 is 2.4Ghz + 5Ghz concurrent
  - # iw phy phy0 interface create wlan0 type managed
  - # iw dev wlan0 scan

=> will scan both bands ?

  - <starting from previous state>
  - # iw phy phy0 interface create wlan1 type __ap
  - # hostapd -i wlan1 <config>
  - # iw dev wlan0 scan

=> what will happen then ?


Same goes for hostapd ACS, I assume specifying freqlist becomes
mandatory or we can't predict which band the AP will be on ?
Vasanthakumar Thiagarajan April 12, 2024, 3:50 a.m. UTC | #19
On 4/11/2024 10:09 PM, Maxime Bizon wrote:
> 
> On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:
> 
> Hello,
> 
> Thanks for making it clear,
> 
> 
>>> For example, starting a 5Ghz AP in ax-only mode, and at the same
>>> time
>>> creating a STA interface on 2.4GHz ?
> 
>> Yes, such use cases continue to be supported in single wiphy mode.
> 
> Understood, but I see some corner cases.
> 
> 
> For example, assume two bands AP hardware, 2.4GHz and 5GHz.
> 
> Previously:
>    - phy0 is 2.4Ghz
>    - phy1 is 5Ghz
>    - iw phy phy0 interface create wlan0 type managed
>    - iw dev wlan0 scan
> 
> => will only scan 2.4Ghz
> 
> 
> With single phy approach:
>    - phy0 is 2.4Ghz + 5Ghz concurrent
>    - # iw phy phy0 interface create wlan0 type managed
>    - # iw dev wlan0 scan
> 
> => will scan both bands ?
> 

Yes, both the bands will be scanned if freq list is not given

>    - <starting from previous state>
>    - # iw phy phy0 interface create wlan1 type __ap
>    - # hostapd -i wlan1 <config>
>    - # iw dev wlan0 scan
> 
> => what will happen then ?
>

Scan will be carried out on all the radios including the one on which AP interface is 
running. Scan with freq list can be used not to disturb the radio of AP interface.

> > Same goes for hostapd ACS, I assume specifying freqlist becomes
> mandatory or we can't predict which band the AP will be on ?
> 

If no freq list is given, ACS will run through all the bands and select a channel from any 
of the bands. But this can be optimized to do ACS and channels selection on a particular 
band using channellist/freqlist hostapd configurations.

Vasanth
Vasanthakumar Thiagarajan April 12, 2024, 4:11 a.m. UTC | #20
On 4/11/2024 2:33 AM, Ben Greear wrote:
> On 4/10/24 08:42, Johannes Berg wrote:
>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>
>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>
>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>> a single "hardware"?
>>>>>>>>
>>>>>>>
>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>> hardware.
>>>>>>
>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>> knew or paid attention to.
>>>>>
>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>> other phys as needed to get a full picture?
>>>>>
>>>>
>>>> There's not really a good way to _do_ that short of creating multiple
>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>> and mac80211) so we rejected it years ago.
>>>
>>> I thought the problem ath12k is trying to fix is that there are currently multiple phys 
>>> (radios) that needed to be made to
>>> look like a single phy?
>>
>> Correct.
>>
>>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual 
>>> radios for non-MLO use
>>> cases
>>
>> No, I don't see why, and if you want that we wouldn't support it anyway,
>> you'd have to have a module option or something to decide which way to
>> go.
>>
>> But it really ought to not be needed - the point of these patches is to
>> give userspace enough information to know how to (and where) to create
>> separate BSSes, with or without MLO between them.
>>
>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used 
>>> independently.
>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to 
>>> start acting
>>> at least somewhat like a single entity
>>
>> Yes.
>>
>>> (while also concurrently being able to act as individual
>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>
>> No.
> 
> Hello Johannes,
> 
> Is there any design document for the combined phy approach somewhere publicly available?
> 
> It is hard to understand the over all goals by just reading patches as they show up on
> the public mailing lists...
> 

Hi Ben,

I dont think there is a document for this composite phy approach. But we try to clarify
as much as possible in the commit log and kernel-doc. Pls let us know the area which
is more appropriate to be clarified in the path.

Vasanth
Nicolas Escande April 12, 2024, 7:38 a.m. UTC | #21
On Fri Apr 12, 2024 at 5:50 AM CEST, Vasanthakumar Thiagarajan wrote:
>
>
> On 4/11/2024 10:09 PM, Maxime Bizon wrote:
> > 
> > On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:
> > 
> > Hello,
> > 
> > Thanks for making it clear,
> > 
> > 
> >>> For example, starting a 5Ghz AP in ax-only mode, and at the same
> >>> time
> >>> creating a STA interface on 2.4GHz ?
> > 
> >> Yes, such use cases continue to be supported in single wiphy mode.
> > 
> > Understood, but I see some corner cases.
> > 
> > 
> > For example, assume two bands AP hardware, 2.4GHz and 5GHz.
> > 
> > Previously:
> >    - phy0 is 2.4Ghz
> >    - phy1 is 5Ghz
> >    - iw phy phy0 interface create wlan0 type managed
> >    - iw dev wlan0 scan
> > 
> > => will only scan 2.4Ghz
> > 
> > 
> > With single phy approach:
> >    - phy0 is 2.4Ghz + 5Ghz concurrent
> >    - # iw phy phy0 interface create wlan0 type managed
> >    - # iw dev wlan0 scan
> > 
> > => will scan both bands ?
> > 
>
> Yes, both the bands will be scanned if freq list is not given
>
> >    - <starting from previous state>
> >    - # iw phy phy0 interface create wlan1 type __ap
> >    - # hostapd -i wlan1 <config>
> >    - # iw dev wlan0 scan
> > 
> > => what will happen then ?
> >
>
> Scan will be carried out on all the radios including the one on which AP interface is 
> running. Scan with freq list can be used not to disturb the radio of AP interface.
>
> > > Same goes for hostapd ACS, I assume specifying freqlist becomes
> > mandatory or we can't predict which band the AP will be on ?
> > 
>
> If no freq list is given, ACS will run through all the bands and select a channel from any 
> of the bands. But this can be optimized to do ACS and channels selection on a particular 
> band using channellist/freqlist hostapd configurations.
Hello,

And in a (very unlikely) case that there are two underlying radios that can
operate on the same frequencies, how would freqlist enable us to really select
the underlying radio ?  

Thanks
>
> Vasanth
Vasanthakumar Thiagarajan April 12, 2024, 8:01 a.m. UTC | #22
On 4/12/2024 1:08 PM, Nicolas Escande wrote:
> On Fri Apr 12, 2024 at 5:50 AM CEST, Vasanthakumar Thiagarajan wrote:
>>
>>
>> On 4/11/2024 10:09 PM, Maxime Bizon wrote:
>>>
>>> On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:
>>>
>>> Hello,
>>>
>>> Thanks for making it clear,
>>>
>>>
>>>>> For example, starting a 5Ghz AP in ax-only mode, and at the same
>>>>> time
>>>>> creating a STA interface on 2.4GHz ?
>>>
>>>> Yes, such use cases continue to be supported in single wiphy mode.
>>>
>>> Understood, but I see some corner cases.
>>>
>>>
>>> For example, assume two bands AP hardware, 2.4GHz and 5GHz.
>>>
>>> Previously:
>>>     - phy0 is 2.4Ghz
>>>     - phy1 is 5Ghz
>>>     - iw phy phy0 interface create wlan0 type managed
>>>     - iw dev wlan0 scan
>>>
>>> => will only scan 2.4Ghz
>>>
>>>
>>> With single phy approach:
>>>     - phy0 is 2.4Ghz + 5Ghz concurrent
>>>     - # iw phy phy0 interface create wlan0 type managed
>>>     - # iw dev wlan0 scan
>>>
>>> => will scan both bands ?
>>>
>>
>> Yes, both the bands will be scanned if freq list is not given
>>
>>>     - <starting from previous state>
>>>     - # iw phy phy0 interface create wlan1 type __ap
>>>     - # hostapd -i wlan1 <config>
>>>     - # iw dev wlan0 scan
>>>
>>> => what will happen then ?
>>>
>>
>> Scan will be carried out on all the radios including the one on which AP interface is
>> running. Scan with freq list can be used not to disturb the radio of AP interface.
>>
>>>> Same goes for hostapd ACS, I assume specifying freqlist becomes
>>> mandatory or we can't predict which band the AP will be on ?
>>>
>>
>> If no freq list is given, ACS will run through all the bands and select a channel from any
>> of the bands. But this can be optimized to do ACS and channels selection on a particular
>> band using channellist/freqlist hostapd configurations.
> Hello,
> 
> And in a (very unlikely) case that there are two underlying radios that can
> operate on the same frequencies, how would freqlist enable us to really select
> the underlying radio ?
> 

This can not be supported in this approach. As only the radios participating in MLO are 
supposed to be combined under one wiphy, not sure if we have real use case to combine 
radios of same frequencies.

Vasanth
James Dutton April 12, 2024, noon UTC | #23
On Fri, 12 Apr 2024 at 09:02, Vasanthakumar Thiagarajan
<quic_vthiagar@quicinc.com> wrote:
> On 4/12/2024 1:08 PM, Nicolas Escande wrote:
> > On Fri Apr 12, 2024 at 5:50 AM CEST, Vasanthakumar Thiagarajan wrote:
> >> On 4/11/2024 10:09 PM, Maxime Bizon wrote:
> >>>
> >>> On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:
> >>>
> >>> Hello,
> >>>
> >>> Thanks for making it clear,
> >>>
> >>>
> >>>>> For example, starting a 5Ghz AP in ax-only mode, and at the same
> >>>>> time
> >>>>> creating a STA interface on 2.4GHz ?
> >>>
> >>>> Yes, such use cases continue to be supported in single wiphy mode.
> >>>
> >>> Understood, but I see some corner cases.
> >>>
> >>>
> >>> For example, assume two bands AP hardware, 2.4GHz and 5GHz.
> >>>
> >>> Previously:
> >>>     - phy0 is 2.4Ghz
> >>>     - phy1 is 5Ghz
> >>>     - iw phy phy0 interface create wlan0 type managed
> >>>     - iw dev wlan0 scan
> >>>
> >>> => will only scan 2.4Ghz
> >>>
> >>>
> >>> With single phy approach:
> >>>     - phy0 is 2.4Ghz + 5Ghz concurrent
> >>>     - # iw phy phy0 interface create wlan0 type managed
> >>>     - # iw dev wlan0 scan
> >>>
.> >>> => will scan both bands ?
> >>>
> >>
> >> Yes, both the bands will be scanned if freq list is not given
> >>
> >>>     - <starting from previous state>
> >>>     - # iw phy phy0 interface create wlan1 type __ap
> >>>     - # hostapd -i wlan1 <config>
> >>>     - # iw dev wlan0 scan
> >>>
> >>> => what will happen then ?
> >>>
> >>
> >> Scan will be carried out on all the radios including the one on which AP interface is
> >> running. Scan with freq list can be used not to disturb the radio of AP interface.
> >>
> >>>> Same goes for hostapd ACS, I assume specifying freqlist becomes
> >>> mandatory or we can't predict which band the AP will be on ?
> >>>
> >>
> >> If no freq list is given, ACS will run through all the bands and select a channel from any
> >> of the bands. But this can be optimized to do ACS and channels selection on a particular
> >> band using channellist/freqlist hostapd configurations.
> > Hello,
> >
> > And in a (very unlikely) case that there are two underlying radios that can
> > operate on the same frequencies, how would freqlist enable us to really select
> > the underlying radio ?
> >
>
> This can not be supported in this approach. As only the radios participating in MLO are
> supposed to be combined under one wiphy, not sure if we have real use case to combine
> radios of same frequencies.
>
> Vasanth
>

Looking at this discussion, I think it would really be helped with
some architectural diagrams describing the various combinations of
components in an RF chain.

Let us take a single example.
We have antennas, analogue front end, and ADCs (Analogue to digital converters)
The features of those two are:
1)  Antennas can be optimised for particular frequencies.
2) The analogue front end has many different parameters, but for this
discussion, the important one is:
a) frequencies it can tune to.
b) instantaneous bandwidth. I.e. When tuned to a particular frequency,
what is the bandwidth around that frequency that one can receive.
3) ADC
a) The Sample rate

Once the RF sample are in the digital domain one has the ADC ->
DIgital Processing -> Output data
If we assume that the ADC is set to receive the full instantaneous
bandwidth, the digital processing can do a number of things.
1) process the full instantaneous bandwidth into a single channel of data.
2) process the instantaneous bandwidth into multiple sub bands, or
multiple channels of data.

There is also the control endpoint that controls all these components.
There can be multiple front ends for each ADC. multiple ADCs per
Digital Processing.

A higher level process (maybe hostapd) can then speak to multiple
Digital Processing and ADC, RF Front end components and then somehow
has to manage and coordinate them all.

All these capabilities and varying arrangement of the various
components need to be reported up to a higher level, in a common way,
that can handle all possible arrangements.
Once the higher level process has all this information, it can then
manage to do everything necessary for Multi-Link operation (MLO).

So, to answer some of the questions in this thread.
Scanning: The high level process should know what capabilities are
available and what can be done in parallel or not. So it should be
able to convert a request from a user, into a more detailed request
down towards the hardware.
I.e. User asks for scan.  High level process tells which bits of the
hardware should do the scanning and across which frequencies.
While it might sound complicated, when implemented correctly, it is
just a matter of iterating over a  tree with a search match criteria.

My expertise is in managing far more complex RF hardware and not Wifi
specifically, but the concepts for managing the hardware across all
the available frequencies is the same. I am just offering a
perspective of where the problems discussed here have already been
solved in another domain.
Ben Greear April 12, 2024, 2:08 p.m. UTC | #24
On 4/11/24 21:11, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 4/11/2024 2:33 AM, Ben Greear wrote:
>> On 4/10/24 08:42, Johannes Berg wrote:
>>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>>
>>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>>
>>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>>> a single "hardware"?
>>>>>>>>>
>>>>>>>>
>>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>>> hardware.
>>>>>>>
>>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>>> knew or paid attention to.
>>>>>>
>>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>>> other phys as needed to get a full picture?
>>>>>>
>>>>>
>>>>> There's not really a good way to _do_ that short of creating multiple
>>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>>> and mac80211) so we rejected it years ago.
>>>>
>>>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>>>> look like a single phy?
>>>
>>> Correct.
>>>
>>>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>>>> cases
>>>
>>> No, I don't see why, and if you want that we wouldn't support it anyway,
>>> you'd have to have a module option or something to decide which way to
>>> go.
>>>
>>> But it really ought to not be needed - the point of these patches is to
>>> give userspace enough information to know how to (and where) to create
>>> separate BSSes, with or without MLO between them.
>>>
>>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>>>> at least somewhat like a single entity
>>>
>>> Yes.
>>>
>>>> (while also concurrently being able to act as individual
>>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>>
>>> No.
>>
>> Hello Johannes,
>>
>> Is there any design document for the combined phy approach somewhere publicly available?
>>
>> It is hard to understand the over all goals by just reading patches as they show up on
>> the public mailing lists...
>>
> 
> Hi Ben,
> 
> I dont think there is a document for this composite phy approach. But we try to clarify
> as much as possible in the commit log and kernel-doc. Pls let us know the area which
> is more appropriate to be clarified in the path.
> 
> Vasanth

I am worried that the whole approach has problems that would be better solved with different
architecture.  I understand that someone has made a decision to go with the combined approach,
and I am sure they have reasons.  It would be good to see some details about how this combined
approach can work in lots of different use cases, including with un-modified user-space, and
including what changes *are* required in user-space to keep current features and control working
with combined wiphy approach.

My over-all concerns are that I feel user-space is still going to need to understand the individual
underlying phys and be able to read/modify them individually.  Older radios will continue to have single phy
mappings, so that must be supported pretty much forever.  So it seems there is going to be a huge amount
of duplicated code up and down the stack and user-space.

Having your team grind on a large patch set that turns out to have fundamental flaws would be
a huge waste of time for all involved.

Thanks,
Ben
Vasanthakumar Thiagarajan April 12, 2024, 2:31 p.m. UTC | #25
On 4/12/2024 7:38 PM, Ben Greear wrote:
> On 4/11/24 21:11, Vasanthakumar Thiagarajan wrote:
>>
>>
>> On 4/11/2024 2:33 AM, Ben Greear wrote:
>>> On 4/10/24 08:42, Johannes Berg wrote:
>>>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>>>
>>>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>>>
>>>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>>>> a single "hardware"?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>>>> hardware.
>>>>>>>>
>>>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>>>> knew or paid attention to.
>>>>>>>
>>>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>>>> other phys as needed to get a full picture?
>>>>>>>
>>>>>>
>>>>>> There's not really a good way to _do_ that short of creating multiple
>>>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>>>> and mac80211) so we rejected it years ago.
>>>>>
>>>>> I thought the problem ath12k is trying to fix is that there are currently multiple 
>>>>> phys (radios) that needed to be made to
>>>>> look like a single phy?
>>>>
>>>> Correct.
>>>>
>>>>> For dual and tri-concurrent radios, I think we will need them to look like 3 
>>>>> individual radios for non-MLO use
>>>>> cases
>>>>
>>>> No, I don't see why, and if you want that we wouldn't support it anyway,
>>>> you'd have to have a module option or something to decide which way to
>>>> go.
>>>>
>>>> But it really ought to not be needed - the point of these patches is to
>>>> give userspace enough information to know how to (and where) to create
>>>> separate BSSes, with or without MLO between them.
>>>>
>>>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used 
>>>>> independently.
>>>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to 
>>>>> start acting
>>>>> at least somewhat like a single entity
>>>>
>>>> Yes.
>>>>
>>>>> (while also concurrently being able to act as individual
>>>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>>>
>>>> No.
>>>
>>> Hello Johannes,
>>>
>>> Is there any design document for the combined phy approach somewhere publicly available?
>>>
>>> It is hard to understand the over all goals by just reading patches as they show up on
>>> the public mailing lists...
>>>
>>
>> Hi Ben,
>>
>> I dont think there is a document for this composite phy approach. But we try to clarify
>> as much as possible in the commit log and kernel-doc. Pls let us know the area which
>> is more appropriate to be clarified in the path.
>>
>> Vasanth
> 
> I am worried that the whole approach has problems that would be better solved with different
> architecture.


If you see a better approach, please feel free to propose one (preferably some RFC) to 
solve the problem.

   I understand that someone has made a decision to go with the combined
> approach,
> and I am sure they have reasons.  It would be good to see some details about how this 
> combined
> approach can work in lots of different use cases, including with un-modified user-space,

Unmodified user space sees all bands from same radio. I guess, driver can probably provide
some configuration knob to turn this off so that everything works a before but will not
be able to operate in MLO. Please note that, user space has to updated to get MLO
support anyway.

  and
> including what changes *are* required in user-space to keep current features and control 
> working
> with combined wiphy approach.
> 
> My over-all concerns are that I feel user-space is still going to need to understand the 
> individual
> underlying phys and be able to read/modify them individually.  Older radios will continue 
> to have single phy
> mappings, so that must be supported pretty much forever.  So it seems there is going to be 
> a huge amount
> of duplicated code up and down the stack and user-space.
> 

Not sure why there should be any duplication, perhaps when corresponding user space
(hostapd) changes will clarify most of these concerns.

> Having your team grind on a large patch set that turns out to have fundamental flaws would be
> a huge waste of time for all involved.
> 

As said, please feel free to propose an alternate solution to address the issue.

Vasanth
Vasanthakumar Thiagarajan April 12, 2024, 2:39 p.m. UTC | #26
On 4/12/2024 5:30 PM, James Dutton wrote:
> On Fri, 12 Apr 2024 at 09:02, Vasanthakumar Thiagarajan
> <quic_vthiagar@quicinc.com> wrote:
>> On 4/12/2024 1:08 PM, Nicolas Escande wrote:
>>> On Fri Apr 12, 2024 at 5:50 AM CEST, Vasanthakumar Thiagarajan wrote:
>>>> On 4/11/2024 10:09 PM, Maxime Bizon wrote:
>>>>>
>>>>> On Thu, 2024-04-11 at 21:56 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Thanks for making it clear,
>>>>>
>>>>>
>>>>>>> For example, starting a 5Ghz AP in ax-only mode, and at the same
>>>>>>> time
>>>>>>> creating a STA interface on 2.4GHz ?
>>>>>
>>>>>> Yes, such use cases continue to be supported in single wiphy mode.
>>>>>
>>>>> Understood, but I see some corner cases.
>>>>>
>>>>>
>>>>> For example, assume two bands AP hardware, 2.4GHz and 5GHz.
>>>>>
>>>>> Previously:
>>>>>      - phy0 is 2.4Ghz
>>>>>      - phy1 is 5Ghz
>>>>>      - iw phy phy0 interface create wlan0 type managed
>>>>>      - iw dev wlan0 scan
>>>>>
>>>>> => will only scan 2.4Ghz
>>>>>
>>>>>
>>>>> With single phy approach:
>>>>>      - phy0 is 2.4Ghz + 5Ghz concurrent
>>>>>      - # iw phy phy0 interface create wlan0 type managed
>>>>>      - # iw dev wlan0 scan
>>>>>
> .> >>> => will scan both bands ?
>>>>>
>>>>
>>>> Yes, both the bands will be scanned if freq list is not given
>>>>
>>>>>      - <starting from previous state>
>>>>>      - # iw phy phy0 interface create wlan1 type __ap
>>>>>      - # hostapd -i wlan1 <config>
>>>>>      - # iw dev wlan0 scan
>>>>>
>>>>> => what will happen then ?
>>>>>
>>>>
>>>> Scan will be carried out on all the radios including the one on which AP interface is
>>>> running. Scan with freq list can be used not to disturb the radio of AP interface.
>>>>
>>>>>> Same goes for hostapd ACS, I assume specifying freqlist becomes
>>>>> mandatory or we can't predict which band the AP will be on ?
>>>>>
>>>>
>>>> If no freq list is given, ACS will run through all the bands and select a channel from any
>>>> of the bands. But this can be optimized to do ACS and channels selection on a particular
>>>> band using channellist/freqlist hostapd configurations.
>>> Hello,
>>>
>>> And in a (very unlikely) case that there are two underlying radios that can
>>> operate on the same frequencies, how would freqlist enable us to really select
>>> the underlying radio ?
>>>
>>
>> This can not be supported in this approach. As only the radios participating in MLO are
>> supposed to be combined under one wiphy, not sure if we have real use case to combine
>> radios of same frequencies.
>>
>> Vasanth
>>
> 
> Looking at this discussion, I think it would really be helped with
> some architectural diagrams describing the various combinations of
> components in an RF chain.
> 
> Let us take a single example.
> We have antennas, analogue front end, and ADCs (Analogue to digital converters)
> The features of those two are:
> 1)  Antennas can be optimised for particular frequencies.
> 2) The analogue front end has many different parameters, but for this
> discussion, the important one is:
> a) frequencies it can tune to.
> b) instantaneous bandwidth. I.e. When tuned to a particular frequency,
> what is the bandwidth around that frequency that one can receive.
> 3) ADC
> a) The Sample rate
> 
> Once the RF sample are in the digital domain one has the ADC ->
> DIgital Processing -> Output data
> If we assume that the ADC is set to receive the full instantaneous
> bandwidth, the digital processing can do a number of things.
> 1) process the full instantaneous bandwidth into a single channel of data.
> 2) process the instantaneous bandwidth into multiple sub bands, or
> multiple channels of data.
> 
> There is also the control endpoint that controls all these components.
> There can be multiple front ends for each ADC. multiple ADCs per
> Digital Processing.
> 
> A higher level process (maybe hostapd) can then speak to multiple
> Digital Processing and ADC, RF Front end components and then somehow
> has to manage and coordinate them all.
> 
> All these capabilities and varying arrangement of the various
> components need to be reported up to a higher level, in a common way,
> that can handle all possible arrangements.
> Once the higher level process has all this information, it can then
> manage to do everything necessary for Multi-Link operation (MLO).
> 

Exactly, that is what this patch series is trying to do. Advertising
per-underlying radio's capabilities to upper layer.

> So, to answer some of the questions in this thread.
> Scanning: The high level process should know what capabilities are
> available and what can be done in parallel or not. So it should be
> able to convert a request from a user, into a more detailed request
> down towards the hardware.
> I.e. User asks for scan.  High level process tells which bits of the
> hardware should do the scanning and across which frequencies.
> While it might sound complicated, when implemented correctly, it is
> just a matter of iterating over a  tree with a search match criteria.
> 

Current implementation does not change anything in scan function
wrt a wiphy. So there will be only one scan at anytime for a wiphy.
Possible enhancements can be discussed and added in future.

Vasanth
Ben Greear April 12, 2024, 3:58 p.m. UTC | #27
On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 4/12/2024 7:38 PM, Ben Greear wrote:
>> On 4/11/24 21:11, Vasanthakumar Thiagarajan wrote:
>>>
>>>
>>> On 4/11/2024 2:33 AM, Ben Greear wrote:
>>>> On 4/10/24 08:42, Johannes Berg wrote:
>>>>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>>>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>>>>
>>>>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>>>>
>>>>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>>>>> a single "hardware"?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>>>>> hardware.
>>>>>>>>>
>>>>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>>>>> knew or paid attention to.
>>>>>>>>
>>>>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>>>>> other phys as needed to get a full picture?
>>>>>>>>
>>>>>>>
>>>>>>> There's not really a good way to _do_ that short of creating multiple
>>>>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>>>>> and mac80211) so we rejected it years ago.
>>>>>>
>>>>>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>>>>>> look like a single phy?
>>>>>
>>>>> Correct.
>>>>>
>>>>>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>>>>>> cases
>>>>>
>>>>> No, I don't see why, and if you want that we wouldn't support it anyway,
>>>>> you'd have to have a module option or something to decide which way to
>>>>> go.
>>>>>
>>>>> But it really ought to not be needed - the point of these patches is to
>>>>> give userspace enough information to know how to (and where) to create
>>>>> separate BSSes, with or without MLO between them.
>>>>>
>>>>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>>>>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>>>>>> at least somewhat like a single entity
>>>>>
>>>>> Yes.
>>>>>
>>>>>> (while also concurrently being able to act as individual
>>>>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>>>>
>>>>> No.
>>>>
>>>> Hello Johannes,
>>>>
>>>> Is there any design document for the combined phy approach somewhere publicly available?
>>>>
>>>> It is hard to understand the over all goals by just reading patches as they show up on
>>>> the public mailing lists...
>>>>
>>>
>>> Hi Ben,
>>>
>>> I dont think there is a document for this composite phy approach. But we try to clarify
>>> as much as possible in the commit log and kernel-doc. Pls let us know the area which
>>> is more appropriate to be clarified in the path.
>>>
>>> Vasanth
>>
>> I am worried that the whole approach has problems that would be better solved with different
>> architecture.
> 
> 
> If you see a better approach, please feel free to propose one (preferably some RFC) to solve the problem.
> 
>    I understand that someone has made a decision to go with the combined
>> approach,
>> and I am sure they have reasons.  It would be good to see some details about how this combined
>> approach can work in lots of different use cases, including with un-modified user-space,
> 
> Unmodified user space sees all bands from same radio. I guess, driver can probably provide
> some configuration knob to turn this off so that everything works a before but will not
> be able to operate in MLO. Please note that, user space has to updated to get MLO
> support anyway.
> 
>   and
>> including what changes *are* required in user-space to keep current features and control working
>> with combined wiphy approach.
>>
>> My over-all concerns are that I feel user-space is still going to need to understand the individual
>> underlying phys and be able to read/modify them individually.  Older radios will continue to have single phy
>> mappings, so that must be supported pretty much forever.  So it seems there is going to be a huge amount
>> of duplicated code up and down the stack and user-space.
>>
> 
> Not sure why there should be any duplication, perhaps when corresponding user space
> (hostapd) changes will clarify most of these concerns.
> 
>> Having your team grind on a large patch set that turns out to have fundamental flaws would be
>> a huge waste of time for all involved.
>>
> 
> As said, please feel free to propose an alternate solution to address the issue.

I do not know the particulars of your driver or driver's needs, but at high level:

*  Leave existing wiphy mapping as is.
*  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys.  Sort of
    like bridges on top of Eth ports.
*  The combined wiphy would report underlying wiphy's capabilities (for instance, a combined wiphy on top of
    3 single band phys would report itself as tri-band).
*  The combined wiphy would add new netlink field listing of its underlying wiphys.  User-space wanting to control the combined
    wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd set the underlying 2.4g
    wiphy to channel 6)
*  This should require very minimal changes to user space, except of course for new code to actually utilize
    the new combined wiphy.
*  MLO links and any other logic that needs the combined view would live on the combined wiphy (I understand
    from Johannes' comments this is a needed feature.)
*  Or user can ignore that combined wiphy entirely and directly use underlying wiphys like we use them currently
    for sniffers, stations, aps, combinations thereof.
*  Advanced use case could allow combined wiphy to use subset of underlying radios (add combined wiphy on 2.4 and 5g, use 6g for
    dedicated mesh backhaul/whatever).
*  Interesting logic would be needed to deal with constraints to properly share the underlying resources (you could not
    add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that also uses 2.4 radio most likely,
    for instance).  But I think that logic has to be written
    either way and is not overly worse with this approach.

Thanks,
Ben


> 
> Vasanth
>
Ben Greear April 13, 2024, 3:40 p.m. UTC | #28
On 4/12/24 08:58, Ben Greear wrote:
> On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:

>> As said, please feel free to propose an alternate solution to address the issue.
> 
> I do not know the particulars of your driver or driver's needs, but at high level:
> 
> *  Leave existing wiphy mapping as is.
> *  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys.  Sort of
>     like bridges on top of Eth ports.
> *  The combined wiphy would report underlying wiphy's capabilities (for instance, a combined wiphy on top of
>     3 single band phys would report itself as tri-band).
> *  The combined wiphy would add new netlink field listing of its underlying wiphys.  User-space wanting to control the combined
>     wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd set the underlying 2.4g
>     wiphy to channel 6)
> *  This should require very minimal changes to user space, except of course for new code to actually utilize
>     the new combined wiphy.
> *  MLO links and any other logic that needs the combined view would live on the combined wiphy (I understand
>     from Johannes' comments this is a needed feature.)
> *  Or user can ignore that combined wiphy entirely and directly use underlying wiphys like we use them currently
>     for sniffers, stations, aps, combinations thereof.
> *  Advanced use case could allow combined wiphy to use subset of underlying radios (add combined wiphy on 2.4 and 5g, use 6g for
>     dedicated mesh backhaul/whatever).
> *  Interesting logic would be needed to deal with constraints to properly share the underlying resources (you could not
>     add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that also uses 2.4 radio most likely,
>     for instance).  But I think that logic has to be written
>     either way and is not overly worse with this approach.

I had some further thoughts on this approach:

*  If someone has 2 QCA radio cards, and each radio card has 3 phys, would it be possible to have a 6-link MLO
    setup?

*  Could two be200 be combined into a multi-channel concurrent MLO setup with this approach?


*  For multi-phy arrangements like QCA ath12k and MTK7996, I think the default configuration when the driver
    loads should just be the physical phys by themselves (as mt7996, at least, does it now).  This would be fully backwards compatible with
    current user-space and operation.  But the phys would have netlink attributes that lets user-space
    know combined phys (cphys?) can be created on top of them.  User-space that knows about MLO can then
    create the cphy(s) as wanted and build sta/vap/whatever on top of the cphys.

*  For radios like be200 that are already designed to show a single phy to userspace, they would not
    need any significant change.

Thanks,
Ben
Vasanthakumar Thiagarajan April 14, 2024, 3:52 p.m. UTC | #29
On 4/12/2024 9:28 PM, Ben Greear wrote:
> On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:
>>
>>
>> On 4/12/2024 7:38 PM, Ben Greear wrote:
>>> On 4/11/24 21:11, Vasanthakumar Thiagarajan wrote:
>>>>
>>>>
>>>> On 4/11/2024 2:33 AM, Ben Greear wrote:
>>>>> On 4/10/24 08:42, Johannes Berg wrote:
>>>>>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>>>>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>>>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>>>>>
>>>>>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>>>>>> a single "hardware"?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>>>>>> hardware.
>>>>>>>>>>
>>>>>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>>>>>> knew or paid attention to.
>>>>>>>>>
>>>>>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>>>>>> other phys as needed to get a full picture?
>>>>>>>>>
>>>>>>>>
>>>>>>>> There's not really a good way to _do_ that short of creating multiple
>>>>>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>>>>>> and mac80211) so we rejected it years ago.
>>>>>>>
>>>>>>> I thought the problem ath12k is trying to fix is that there are currently multiple 
>>>>>>> phys (radios) that needed to be made to
>>>>>>> look like a single phy?
>>>>>>
>>>>>> Correct.
>>>>>>
>>>>>>> For dual and tri-concurrent radios, I think we will need them to look like 3 
>>>>>>> individual radios for non-MLO use
>>>>>>> cases
>>>>>>
>>>>>> No, I don't see why, and if you want that we wouldn't support it anyway,
>>>>>> you'd have to have a module option or something to decide which way to
>>>>>> go.
>>>>>>
>>>>>> But it really ought to not be needed - the point of these patches is to
>>>>>> give userspace enough information to know how to (and where) to create
>>>>>> separate BSSes, with or without MLO between them.
>>>>>>
>>>>>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used 
>>>>>>> independently.
>>>>>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to 
>>>>>>> start acting
>>>>>>> at least somewhat like a single entity
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> (while also concurrently being able to act as individual
>>>>>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>>>>>
>>>>>> No.
>>>>>
>>>>> Hello Johannes,
>>>>>
>>>>> Is there any design document for the combined phy approach somewhere publicly available?
>>>>>
>>>>> It is hard to understand the over all goals by just reading patches as they show up on
>>>>> the public mailing lists...
>>>>>
>>>>
>>>> Hi Ben,
>>>>
>>>> I dont think there is a document for this composite phy approach. But we try to clarify
>>>> as much as possible in the commit log and kernel-doc. Pls let us know the area which
>>>> is more appropriate to be clarified in the path.
>>>>
>>>> Vasanth
>>>
>>> I am worried that the whole approach has problems that would be better solved with 
>>> different
>>> architecture.
>>
>>
>> If you see a better approach, please feel free to propose one (preferably some RFC) to 
>> solve the problem.
>>
>>    I understand that someone has made a decision to go with the combined
>>> approach,
>>> and I am sure they have reasons.  It would be good to see some details about how this 
>>> combined
>>> approach can work in lots of different use cases, including with un-modified user-space,
>>
>> Unmodified user space sees all bands from same radio. I guess, driver can probably provide
>> some configuration knob to turn this off so that everything works a before but will not
>> be able to operate in MLO. Please note that, user space has to updated to get MLO
>> support anyway.
>>
>>   and
>>> including what changes *are* required in user-space to keep current features and 
>>> control working
>>> with combined wiphy approach.
>>>
>>> My over-all concerns are that I feel user-space is still going to need to understand 
>>> the individual
>>> underlying phys and be able to read/modify them individually.  Older radios will 
>>> continue to have single phy
>>> mappings, so that must be supported pretty much forever.  So it seems there is going to 
>>> be a huge amount
>>> of duplicated code up and down the stack and user-space.
>>>
>>
>> Not sure why there should be any duplication, perhaps when corresponding user space
>> (hostapd) changes will clarify most of these concerns.
>>
>>> Having your team grind on a large patch set that turns out to have fundamental flaws 
>>> would be
>>> a huge waste of time for all involved.
>>>
>>
>> As said, please feel free to propose an alternate solution to address the issue.
> 
> I do not know the particulars of your driver or driver's needs, but at high level:
> > *  Leave existing wiphy mapping as is.
> *  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys.  
> Sort of
>     like bridges on top of Eth ports.
> *  The combined wiphy would report underlying wiphy's capabilities (for instance, a 
> combined wiphy on top of
>     3 single band phys would report itself as tri-band).
> *  The combined wiphy would add new netlink field listing of its underlying wiphys.  
> User-space wanting to control the combined
>     wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd 
> set the underlying 2.4g
>     wiphy to channel 6)
> *  This should require very minimal changes to user space, except of course for new code 
> to actually utilize
>     the new combined wiphy.
> *  MLO links and any other logic that needs the combined view would live on the combined 
> wiphy (I understand

Having MLO links on top of thins combined wiphy will not work because there is no 
difference in the way MLO and legacy interfaces are created. More over, at a given
time same STA can be either MLO or legacy based on the AP the STA is connected with.

>     from Johannes' comments this is a needed feature.)
> *  Or user can ignore that combined wiphy entirely and directly use underlying wiphys like 
> we use them currently

But old user space will get confused with this new combined wiphy...

>     for sniffers, stations, aps, combinations thereof.
> *  Advanced use case could allow combined wiphy to use subset of underlying radios (add 
> combined wiphy on 2.4 and 5g, use 6g for
>     dedicated mesh backhaul/whatever).

Not sure having parallel path one for legacy and the other one for advanced use cases
may not be a clean solution...

> *  Interesting logic would be needed to deal with constraints to properly share the 
> underlying resources (you could not
>     add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that 
> also uses 2.4 radio most likely,
>     for instance).  But I think that logic has to be written
>     either way and is not overly worse with this approach.
> 

Anyway, similar approach was considered before coming up the with the one that
this patch series implements. Due to complexities in cfg80211 and mac80211 all
options building MLO on top of multiple wiphy was rejected. Im not planning
to look into such approaches. Instead, the approach implemented in this patch
series has gone into extensive testings with corresponding changes in ath12k
driver and hostapd. Other than few items (concurrent scan is one additional
not mentioned explicitly) as mentioned in the cover letter to do list, things
look fine. But all these pending items can be addressed in follow-on patches.
We dont really see much complexities in hostapd implementations , we'll post
them once the kernel part is concluded.

Other than ability to run interface on multiple supported bands concurrently,
this patches series does not change anything else.

Vasanth
Vasanthakumar Thiagarajan April 14, 2024, 4:02 p.m. UTC | #30
On 4/13/2024 9:10 PM, Ben Greear wrote:
> On 4/12/24 08:58, Ben Greear wrote:
>> On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:
> 
>>> As said, please feel free to propose an alternate solution to address the issue.
>>
>> I do not know the particulars of your driver or driver's needs, but at high level:
>>
>> *  Leave existing wiphy mapping as is.
>> *  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys.  
>> Sort of
>>     like bridges on top of Eth ports.
>> *  The combined wiphy would report underlying wiphy's capabilities (for instance, a 
>> combined wiphy on top of
>>     3 single band phys would report itself as tri-band).
>> *  The combined wiphy would add new netlink field listing of its underlying wiphys.  
>> User-space wanting to control the combined
>>     wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd 
>> set the underlying 2.4g
>>     wiphy to channel 6)
>> *  This should require very minimal changes to user space, except of course for new code 
>> to actually utilize
>>     the new combined wiphy.
>> *  MLO links and any other logic that needs the combined view would live on the combined 
>> wiphy (I understand
>>     from Johannes' comments this is a needed feature.)
>> *  Or user can ignore that combined wiphy entirely and directly use underlying wiphys 
>> like we use them currently
>>     for sniffers, stations, aps, combinations thereof.
>> *  Advanced use case could allow combined wiphy to use subset of underlying radios (add 
>> combined wiphy on 2.4 and 5g, use 6g for
>>     dedicated mesh backhaul/whatever).
>> *  Interesting logic would be needed to deal with constraints to properly share the 
>> underlying resources (you could not
>>     add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that 
>> also uses 2.4 radio most likely,
>>     for instance).  But I think that logic has to be written
>>     either way and is not overly worse with this approach.
> 
> I had some further thoughts on this approach:
> 
> *  If someone has 2 QCA radio cards, and each radio card has 3 phys, would it be possible 
> to have a 6-link MLO
>     setup?
> 

As long as supported frequencies of the radios are not overlapping , it is technically 
possible to register 6 radios under one wiphy

> *  Could two be200 be combined into a multi-channel concurrent MLO setup with this approach?
> 

By nature, MLO device is multi-channel concurrent. Not quite sure about the
be200 capability.

> 
> *  For multi-phy arrangements like QCA ath12k and MTK7996, I think the default 
> configuration when the driver
>     loads should just be the physical phys by themselves (as mt7996, at least, does it 
> now).  This would be fully backwards compatible with
>     current user-space and operation. 

There can be configuration knobs in the driver to register it differently...

  But the phys would have netlink attributes that
> lets user-space
>     know combined phys (cphys?) can be created on top of them.  User-space that knows 
> about MLO can then
>     create the cphy(s) as wanted and build sta/vap/whatever on top of the cphys.
>

Not quite positive on the combined_phy+legacy_phy design as mentioned in the previous mail.

> *  For radios like be200 that are already designed to show a single phy to userspace, they 
> would not
>     need any significant change.

As mentioned, it is not lot of changes in hostapd/wpa_s. We'll post them once kernel
part is concluded.

Vasanth
Ben Greear April 15, 2024, 1:53 p.m. UTC | #31
On 4/14/24 08:52, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 4/12/2024 9:28 PM, Ben Greear wrote:
>> On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:
>>>
>>>
>>> On 4/12/2024 7:38 PM, Ben Greear wrote:
>>>> On 4/11/24 21:11, Vasanthakumar Thiagarajan wrote:
>>>>>
>>>>>
>>>>> On 4/11/2024 2:33 AM, Ben Greear wrote:
>>>>>> On 4/10/24 08:42, Johannes Berg wrote:
>>>>>>> On Wed, 2024-04-10 at 07:37 -0700, Ben Greear wrote:
>>>>>>>> On 4/10/24 00:56, Johannes Berg wrote:
>>>>>>>>> On Fri, 2024-03-29 at 07:47 -0700, Ben Greear wrote:
>>>>>>>>>> On 3/29/24 07:30, Johannes Berg wrote:
>>>>>>>>>>> On Fri, 2024-03-29 at 19:41 +0530, Vasanthakumar Thiagarajan wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> + * @hw_chans: list of the channels supported by every constituent underlying
>>>>>>>>>>>>>> + *    hardware. Drivers abstracting multiple discrete hardware (radio) under
>>>>>>>>>>>>>> + *    one wiphy can advertise the list of channels supported by each physical
>>>>>>>>>>>>>> + *    hardware in this list. Underlying hardware specific channel list can be
>>>>>>>>>>>>>> + *    used while describing interface combination for each of them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'd expect there to be a limit on channels being within a single band on
>>>>>>>>>>>>> a single "hardware"?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> There are ath12k hardware supporting multiple band which need to be
>>>>>>>>>>>> registered under one mac80211_hw/wiphy. This design is to support such
>>>>>>>>>>>> hardware.
>>>>>>>>>>>
>>>>>>>>>>> Oh OK, that was something that I didn't have in mind any more, or never
>>>>>>>>>>> knew or paid attention to.
>>>>>>>>>>
>>>>>>>>>> Would it work to leave the phy reporting pretty much as it is now, but add
>>>>>>>>>> a 'associated_peer_radios' list section, so that each phy could report the phys
>>>>>>>>>> associated with it?  Then user-space, driver, mac80211 etc could look up the
>>>>>>>>>> other phys as needed to get a full picture?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There's not really a good way to _do_ that short of creating multiple
>>>>>>>>> wiphys, but that causes _massive_ complexity in the stack (both cfg80211
>>>>>>>>> and mac80211) so we rejected it years ago.
>>>>>>>>
>>>>>>>> I thought the problem ath12k is trying to fix is that there are currently multiple phys (radios) that needed to be made to
>>>>>>>> look like a single phy?
>>>>>>>
>>>>>>> Correct.
>>>>>>>
>>>>>>>> For dual and tri-concurrent radios, I think we will need them to look like 3 individual radios for non-MLO use
>>>>>>>> cases
>>>>>>>
>>>>>>> No, I don't see why, and if you want that we wouldn't support it anyway,
>>>>>>> you'd have to have a module option or something to decide which way to
>>>>>>> go.
>>>>>>>
>>>>>>> But it really ought to not be needed - the point of these patches is to
>>>>>>> give userspace enough information to know how to (and where) to create
>>>>>>> separate BSSes, with or without MLO between them.
>>>>>>>
>>>>>>>> For instance, mt7996 currently reports 3 single-band wiphys, and each can be used independently.
>>>>>>>> But assuming it starts supporting MLO, then those 3 single band wiphys will need to start acting
>>>>>>>> at least somewhat like a single entity
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> (while also concurrently being able to act as individual
>>>>>>>> wiphys so that one can do a mix of MLO and non MLO sta/AP.)
>>>>>>>
>>>>>>> No.
>>>>>>
>>>>>> Hello Johannes,
>>>>>>
>>>>>> Is there any design document for the combined phy approach somewhere publicly available?
>>>>>>
>>>>>> It is hard to understand the over all goals by just reading patches as they show up on
>>>>>> the public mailing lists...
>>>>>>
>>>>>
>>>>> Hi Ben,
>>>>>
>>>>> I dont think there is a document for this composite phy approach. But we try to clarify
>>>>> as much as possible in the commit log and kernel-doc. Pls let us know the area which
>>>>> is more appropriate to be clarified in the path.
>>>>>
>>>>> Vasanth
>>>>
>>>> I am worried that the whole approach has problems that would be better solved with different
>>>> architecture.
>>>
>>>
>>> If you see a better approach, please feel free to propose one (preferably some RFC) to solve the problem.
>>>
>>>    I understand that someone has made a decision to go with the combined
>>>> approach,
>>>> and I am sure they have reasons.  It would be good to see some details about how this combined
>>>> approach can work in lots of different use cases, including with un-modified user-space,
>>>
>>> Unmodified user space sees all bands from same radio. I guess, driver can probably provide
>>> some configuration knob to turn this off so that everything works a before but will not
>>> be able to operate in MLO. Please note that, user space has to updated to get MLO
>>> support anyway.
>>>
>>>   and
>>>> including what changes *are* required in user-space to keep current features and control working
>>>> with combined wiphy approach.
>>>>
>>>> My over-all concerns are that I feel user-space is still going to need to understand the individual
>>>> underlying phys and be able to read/modify them individually.  Older radios will continue to have single phy
>>>> mappings, so that must be supported pretty much forever.  So it seems there is going to be a huge amount
>>>> of duplicated code up and down the stack and user-space.
>>>>
>>>
>>> Not sure why there should be any duplication, perhaps when corresponding user space
>>> (hostapd) changes will clarify most of these concerns.
>>>
>>>> Having your team grind on a large patch set that turns out to have fundamental flaws would be
>>>> a huge waste of time for all involved.
>>>>
>>>
>>> As said, please feel free to propose an alternate solution to address the issue.
>>
>> I do not know the particulars of your driver or driver's needs, but at high level:
>> > *  Leave existing wiphy mapping as is.
>> *  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys. Sort of
>>     like bridges on top of Eth ports.
>> *  The combined wiphy would report underlying wiphy's capabilities (for instance, a combined wiphy on top of
>>     3 single band phys would report itself as tri-band).
>> *  The combined wiphy would add new netlink field listing of its underlying wiphys. User-space wanting to control the combined
>>     wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd set the underlying 2.4g
>>     wiphy to channel 6)
>> *  This should require very minimal changes to user space, except of course for new code to actually utilize
>>     the new combined wiphy.
>> *  MLO links and any other logic that needs the combined view would live on the combined wiphy (I understand
> 
> Having MLO links on top of thins combined wiphy will not work because there is no difference in the way MLO and legacy interfaces are created. More over, at a 
> given
> time same STA can be either MLO or legacy based on the AP the STA is connected with.

Your comment makes no sense to me.  Of course it could be made to work, but maybe it is more
difficult that I imagine.  It is possible to make STA vdevs to use older wifi features than
what the local radio and the peer radio support, and you can have one STA be /ac and another be
/n on the same radio.  It should be possible to have one MLO capable STA, one /ax, one /ac, one /n,
etc...  I do this now with some modest changes to supplicant and the kernel.  But if the
underlying radios are obscured, I am not sure it can work without much more extensive
changes.

>>     from Johannes' comments this is a needed feature.)
>> *  Or user can ignore that combined wiphy entirely and directly use underlying wiphys like we use them currently
> 
> But old user space will get confused with this new combined wiphy...

I think new combined phy should not even exist by default, user-space would build it
stacked on top of physical phys if user-space is modified to use MLO.  Old user-space
would just see 3 single-band phys.

>>     for sniffers, stations, aps, combinations thereof.
>> *  Advanced use case could allow combined wiphy to use subset of underlying radios (add combined wiphy on 2.4 and 5g, use 6g for
>>     dedicated mesh backhaul/whatever).
> 
> Not sure having parallel path one for legacy and the other one for advanced use cases
> may not be a clean solution...

I don't see it as parallel paths, more as stacked paths.  Underlying phys stay mostly the
same as today, but the combined phy would know how to manage the underlying phys and
act a bit like a pass-through phy.

>> *  Interesting logic would be needed to deal with constraints to properly share the underlying resources (you could not
>>     add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that also uses 2.4 radio most likely,
>>     for instance).  But I think that logic has to be written
>>     either way and is not overly worse with this approach.
>>
> 
> Anyway, similar approach was considered before coming up the with the one that
> this patch series implements. Due to complexities in cfg80211 and mac80211 all
> options building MLO on top of multiple wiphy was rejected. Im not planning
> to look into such approaches. Instead, the approach implemented in this patch
> series has gone into extensive testings with corresponding changes in ath12k
> driver and hostapd. Other than few items (concurrent scan is one additional
> not mentioned explicitly) as mentioned in the cover letter to do list, things
> look fine. But all these pending items can be addressed in follow-on patches.
> We dont really see much complexities in hostapd implementations , we'll post
> them once the kernel part is concluded.
> 
> Other than ability to run interface on multiple supported bands concurrently,
> this patches series does not change anything else.

Ok, I'll experiment with my preferred approach when mt7996 MLO driver patches
are available.

Thanks,
Ben


> 
> Vasanth
>
Ben Greear April 15, 2024, 1:59 p.m. UTC | #32
On 4/14/24 09:02, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 4/13/2024 9:10 PM, Ben Greear wrote:
>> On 4/12/24 08:58, Ben Greear wrote:
>>> On 4/12/24 07:31, Vasanthakumar Thiagarajan wrote:
>>
>>>> As said, please feel free to propose an alternate solution to address the issue.
>>>
>>> I do not know the particulars of your driver or driver's needs, but at high level:
>>>
>>> *  Leave existing wiphy mapping as is.
>>> *  Allow adding new combined wiphy(s) on top of groups of underlying physical wiphys. Sort of
>>>     like bridges on top of Eth ports.
>>> *  The combined wiphy would report underlying wiphy's capabilities (for instance, a combined wiphy on top of
>>>     3 single band phys would report itself as tri-band).
>>> *  The combined wiphy would add new netlink field listing of its underlying wiphys. User-space wanting to control the combined
>>>     wiphy would generally configure the underlying phys (to set 2.4g channel to 6, you'd set the underlying 2.4g
>>>     wiphy to channel 6)
>>> *  This should require very minimal changes to user space, except of course for new code to actually utilize
>>>     the new combined wiphy.
>>> *  MLO links and any other logic that needs the combined view would live on the combined wiphy (I understand
>>>     from Johannes' comments this is a needed feature.)
>>> *  Or user can ignore that combined wiphy entirely and directly use underlying wiphys like we use them currently
>>>     for sniffers, stations, aps, combinations thereof.
>>> *  Advanced use case could allow combined wiphy to use subset of underlying radios (add combined wiphy on 2.4 and 5g, use 6g for
>>>     dedicated mesh backhaul/whatever).
>>> *  Interesting logic would be needed to deal with constraints to properly share the underlying resources (you could not
>>>     add 16 ap bssid to 2.4 wiphy and also add 16 other ones to the combined wiphy that also uses 2.4 radio most likely,
>>>     for instance).  But I think that logic has to be written
>>>     either way and is not overly worse with this approach.
>>
>> I had some further thoughts on this approach:
>>
>> *  If someone has 2 QCA radio cards, and each radio card has 3 phys, would it be possible to have a 6-link MLO
>>     setup?
>>
> 
> As long as supported frequencies of the radios are not overlapping , it is technically possible to register 6 radios under one wiphy

Couldn't you have MLO links where one is on 5ghz channel 36 and second is on 5ghz channel 149?
Underlying radios could support entire 5ghz band, but would use non-overlapping channels.

> 
>> *  Could two be200 be combined into a multi-channel concurrent MLO setup with this approach?
>>
> 
> By nature, MLO device is multi-channel concurrent. Not quite sure about the
> be200 capability.

be200 is eMLSR, I'm wondering if two be200 could be combined to make an STR MLMR
link.

> 
>>
>> *  For multi-phy arrangements like QCA ath12k and MTK7996, I think the default configuration when the driver
>>     loads should just be the physical phys by themselves (as mt7996, at least, does it now).  This would be fully backwards compatible with
>>     current user-space and operation. 
> 
> There can be configuration knobs in the driver to register it differently...

This is where I'd like to dynamically create them in user-space.

> 
>   But the phys would have netlink attributes that
>> lets user-space
>>     know combined phys (cphys?) can be created on top of them.  User-space that knows about MLO can then
>>     create the cphy(s) as wanted and build sta/vap/whatever on top of the cphys.
>>
> 
> Not quite positive on the combined_phy+legacy_phy design as mentioned in the previous mail.
> 
>> *  For radios like be200 that are already designed to show a single phy to userspace, they would not
>>     need any significant change.
> 
> As mentioned, it is not lot of changes in hostapd/wpa_s. We'll post them once kernel
> part is concluded.

Thanks,
Ben

> 
> Vasanth
>
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2e2be4fd2bb6..dde129e61b60 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5394,6 +5394,18 @@  struct wiphy_iftype_akm_suites {
 
 #define CFG80211_HW_TIMESTAMP_ALL_PEERS	0xffff
 
+/**
+ * struct ieee80211_chans_per_hw - supported channels as per the
+ * underlying physical hardware configuration
+ *
+ * @n_chans: number of channels in @chans
+ * @chans: list of channels supported by the constituent hardware
+ */
+struct ieee80211_chans_per_hw {
+	u32 n_chans;
+	struct ieee80211_channel chans[];
+};
+
 /**
  * struct wiphy - wireless hardware description
  * @mtx: mutex for the data (structures) of this device
@@ -5610,6 +5622,15 @@  struct wiphy_iftype_akm_suites {
  *	A value of %CFG80211_HW_TIMESTAMP_ALL_PEERS indicates the driver
  *	supports enabling HW timestamping for all peers (i.e. no need to
  *	specify a mac address).
+ *
+ * @hw_chans: list of the channels supported by every constituent underlying
+ *	hardware. Drivers abstracting multiple discrete hardware (radio) under
+ *	one wiphy can advertise the list of channels supported by each physical
+ *	hardware in this list. Underlying hardware specific channel list can be
+ *	used while describing interface combination for each of them.
+ * @num_hw: number of underlying hardware for which the channels list are
+ *	advertised in @hw_chans, 0 if multi hardware is not support. Expect >2
+ *	if multi hardware is support.
  */
 struct wiphy {
 	struct mutex mtx;
@@ -5760,6 +5781,9 @@  struct wiphy {
 
 	u16 hw_timestamp_max_peers;
 
+	struct ieee80211_chans_per_hw **hw_chans;
+	u8 num_hw;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 3fb1b637352a..119937d0f2e0 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -690,6 +690,103 @@  static int wiphy_verify_combinations(struct wiphy *wiphy)
 	return 0;
 }
 
+static int cfg80211_check_hw_chans(const struct ieee80211_chans_per_hw *chans1,
+				   const struct ieee80211_chans_per_hw *chans2)
+{
+	int i, j;
+
+	if (!chans1 || !chans2)
+		return -EINVAL;
+
+	if (!chans1->n_chans || !chans2->n_chans)
+		return -EINVAL;
+
+	/* for now same channel is not allowed in more than one
+	 * physical hardware.
+	 */
+	for (i = 0; i < chans1->n_chans; i++)
+		for (j = 0; j < chans2->n_chans; j++)
+			if (chans1->chans[i].center_freq ==
+			    chans2->chans[j].center_freq)
+				return -EINVAL;
+	return 0;
+}
+
+static bool
+cfg80211_hw_chans_in_supported_list(struct wiphy *wiphy,
+				    const struct ieee80211_chans_per_hw *chans)
+{
+	enum nl80211_band band;
+	struct ieee80211_supported_band *sband;
+	int i, j;
+
+	for (i = 0; i < chans->n_chans; i++) {
+		bool found = false;
+
+		for (band = 0; band < NUM_NL80211_BANDS; band++) {
+			sband = wiphy->bands[band];
+			if (!sband)
+				continue;
+			for (j = 0; j < sband->n_channels; j++) {
+				if (chans->chans[i].center_freq ==
+				    sband->channels[j].center_freq) {
+					found = true;
+					break;
+				}
+			}
+
+			if (found)
+				break;
+		}
+
+		if (!found)
+			return false;
+	}
+
+	return true;
+}
+
+static int cfg80211_validate_per_hw_chans(struct wiphy *wiphy)
+{
+	int i, j;
+	int ret;
+
+	if (!wiphy->num_hw)
+		return 0;
+
+	if (!wiphy->hw_chans)
+		return -EINVAL;
+
+	/* advertisement of supported channels in wiphy->bands should be
+	 * sufficient when physical hardware is one.
+	 */
+	if (wiphy->num_hw < 2)
+		return -EINVAL;
+
+	for (i = 0; i < wiphy->num_hw; i++) {
+		for (j = i + 1; j < wiphy->num_hw; j++) {
+			const struct ieee80211_chans_per_hw *hw_chans1;
+			const struct ieee80211_chans_per_hw *hw_chans2;
+
+			hw_chans1 = wiphy->hw_chans[i];
+			hw_chans2 = wiphy->hw_chans[j];
+			ret = cfg80211_check_hw_chans(hw_chans1, hw_chans2);
+			if (ret)
+				return ret;
+		}
+	}
+
+	for (i = 0; i < wiphy->num_hw; i++) {
+		const struct ieee80211_chans_per_hw *hw_chans;
+
+		hw_chans = wiphy->hw_chans[i];
+		if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 int wiphy_register(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
@@ -927,6 +1024,9 @@  int wiphy_register(struct wiphy *wiphy)
 		return -EINVAL;
 	}
 
+	if (WARN_ON(cfg80211_validate_per_hw_chans(&rdev->wiphy)))
+		return -EINVAL;
+
 	for (i = 0; i < rdev->wiphy.n_vendor_commands; i++) {
 		/*
 		 * Validate we have a policy (can be explicitly set to