[PATCH 00/13] wifi: Add multi physical hardware iface combination support

[PATCH 00/13] wifi: Add multi physical hardware iface combination support

[Karthikeyan Periyasamy]

The prerequisite for MLO support in cfg80211/mac80211 is that all the links
participating in MLO must be from 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 single wiphy. So it required to have some
sort of mapping while describing interface combination capabilities for
each of the underlying physical hardware. This patch set tries to add an
infrastructure to advertise underlying hw specific capabilities like
channel and interface combinations.

Some of the todos

 - More than one concurrent monitor mode support each operating on
   different channels under one ieee80211_hw
 - Mechanism for each underlying radio specific configurations like
   txpower, channel, etc.

RFC series Link: https://lore.kernel.org/linux-wireless/20220920100518.19705-1-quic_vthiagar@quicinc.com/

Harshitha Prem (1):
  wifi: ath12k: Advertise multi hardware iface combination

Karthikeyan Periyasamy (1):
  wifi: ath12k: Introduce iface combination cleanup helper

Vasanthakumar Thiagarajan (11):
  wifi: cfg80211: Add provision to advertise multiple radio in one wiphy
  wifi: nl80211: send underlying multi-hardware channel capabilities to
    user space
  wifi: cfg80211: Refactor the interface combination limit check
  wifi: cfg80211/mac80211: extend iface comb advertisement for
    multi-hardware dev
  wifi: nl80211: Refactor the interface combination limit check
  wifi: nl80211: send iface combination to user space in multi-hardware
    wiphy
  wifi: cfg80211/mac80211: Refactor iface comb iterate callback for
    multi-hardware dev
  wifi: cfg80211: Refactor the iface combination iteration helper
    function
  wifi: cfg80211: Add multi-hardware iface combination support
  wifi: mac80211: expose channel context helper function
  wifi: mac80211: Add multi-hardware support in the iface comb helper

 drivers/net/wireless/ath/ath12k/mac.c | 147 +++++++++++-
 include/net/cfg80211.h                | 175 +++++++++++++-
 include/uapi/linux/nl80211.h          |  78 ++++++-
 net/mac80211/chan.c                   |  35 ++-
 net/mac80211/ieee80211_i.h            |   5 +-
 net/mac80211/main.c                   |  46 ++++
 net/mac80211/util.c                   | 196 ++++++++++++++--
 net/wireless/core.c                   | 294 +++++++++++++++++++----
 net/wireless/nl80211.c                | 154 ++++++++++--
 net/wireless/util.c                   | 321 ++++++++++++++++++++++----
 10 files changed, 1318 insertions(+), 133 deletions(-)


base-commit: d69aef8084cc72df7b0f2583096d9b037c647ec8
-- 
2.34.1

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

[Karthikeyan Periyasamy]

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(+)

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
-- 
2.34.1

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

[Johannes Berg]

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

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

[Vasanthakumar Thiagarajan]

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

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

[Johannes Berg]

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

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Johannes Berg]

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

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

[Ben Greear]

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
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Johannes Berg]

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

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

[Ben Greear]

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 Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Jeff Johnson]

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

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

[Maxime Bizon]

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.
 
-- 
Maxime

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

[Vasanthakumar Thiagarajan]

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

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

[Maxime Bizon]

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 ?

-- 
Maxime

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

[Vasanthakumar Thiagarajan]

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

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

[Nicolas Escande]

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

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

[Vasanthakumar Thiagarajan]

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

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

[James Dutton]

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.

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

[Vasanthakumar Thiagarajan]

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

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

[Maxime Bizon]

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

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Vasanthakumar Thiagarajan]

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

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Vasanthakumar Thiagarajan]

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

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

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Vasanthakumar Thiagarajan]

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

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

[Ben Greear]

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
> 

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Vasanthakumar Thiagarajan]

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

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

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Karthikeyan Periyasamy]

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.

-- 

Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

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

[Karthikeyan Periyasamy]

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?

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

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

[Johannes Berg]

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

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

[Karthikeyan Periyasamy]

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.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], when driver supports multiple
physical hardware under one wiphy, wiphy->num_hw != 0, send per-hardware
supported frequency list to user space. List of frequency are reported
inside an index which identifies the hardware as in wiphy->hw_chans[].
This hardware index will be used in follow up patches to identify the
interface combination capability for each of the underlying physical
hardware abstracted under one wiphy.

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-3-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/uapi/linux/nl80211.h | 28 +++++++++++++++++++++
 net/wireless/nl80211.c       | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f917bc6c9b6f..c53c9f941663 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2856,6 +2856,11 @@ enum nl80211_commands {
  *	%NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs
  *	are used on this connection
  *
+ * @NL80211_ATTR_MULTI_HW: nested attribute to send the hardware specific
+ *	channel capabilities to user space. Drivers registering multiple
+ *	physical hardware under a wiphy can use this attribute,
+ *	see &enum nl80211_multi_hw_mac_attrs.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3401,6 +3406,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_ASSOC_SPP_AMSDU,
 
+	NL80211_ATTR_MULTI_HW,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7999,4 +8006,25 @@ enum nl80211_ap_settings_flags {
 	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
 };
 
+/**
+ * nl80211_multi_hw_attrs - multi-hw attributes
+ *
+ * @NL80211_MULTI_HW_ATTR_INVALID: invalid
+ * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
+ *	for which the supported channel list is advertised. Internally refer
+ *	the index of the wiphy's @hw_chans array.
+ * @NL80211_MULTI_HW_ATTR_FREQS: array of supported center frequencies
+ * @__NL80211_MULTI_HW_ATTR_LAST: internal use
+ * @NL80211_MULTI_HW_ATTR_MAX: maximum multi-hw mac attribute
+ */
+enum nl80211_multi_hw_attrs {
+	__NL80211_MULTI_HW_ATTR_INVALID,
+
+	NL80211_MULTI_HW_ATTR_IDX,
+	NL80211_MULTI_HW_ATTR_FREQS,
+
+	/* keep last */
+	__NL80211_MULTI_HW_ATTR_LAST,
+	NL80211_MULTI_HW_ATTR_MAX = __NL80211_MULTI_HW_ATTR_LAST - 1
+};
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4edba6b0b7b..2a5e395e2e0b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2392,6 +2392,47 @@ static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg)
 	return -ENOBUFS;
 }
 
+static int nl80211_put_multi_hw_support(struct wiphy *wiphy,
+					struct sk_buff *msg)
+{
+	struct nlattr *hw_macs, *hw_mac;
+	struct nlattr *freqs;
+	int i, c;
+
+	if (!wiphy->num_hw)
+		return 0;
+
+	hw_macs = nla_nest_start(msg, NL80211_ATTR_MULTI_HW);
+	if (!hw_macs)
+		return -ENOBUFS;
+
+	for (i = 0; i < wiphy->num_hw; i++) {
+		hw_mac = nla_nest_start(msg, i + 1);
+		if (!hw_mac)
+			return -ENOBUFS;
+
+		if (nla_put_u8(msg, NL80211_MULTI_HW_ATTR_IDX, i))
+			return -ENOBUFS;
+
+		freqs = nla_nest_start(msg,
+				       NL80211_MULTI_HW_ATTR_FREQS);
+		if (!freqs)
+			return -ENOBUFS;
+
+		for (c = 0; c < wiphy->hw_chans[i]->n_chans; c++)
+			if (nla_put_u32(msg, c + 1,
+					wiphy->hw_chans[i]->chans[c].center_freq))
+				return -ENOBUFS;
+
+		nla_nest_end(msg, freqs);
+
+		nla_nest_end(msg, hw_mac);
+	}
+
+	nla_nest_end(msg, hw_macs);
+	return 0;
+}
+
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
@@ -3001,6 +3042,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				rdev->wiphy.hw_timestamp_max_peers))
 			goto nla_put_failure;
 
+		state->split_start++;
+		break;
+	case 17:
+		if (nl80211_put_multi_hw_support(&rdev->wiphy, msg))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;
-- 
2.34.1

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
> +/**
> + * nl80211_multi_hw_attrs - multi-hw attributes
> + *
> + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
> + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
> + *	for which the supported channel list is advertised. Internally refer
> + *	the index of the wiphy's @hw_chans array.

Is there a good reason to expose this? Seems pretty internal to me, and
not sure what userspace would do with it?

> +	for (i = 0; i < wiphy->num_hw; i++) {
> +		hw_mac = nla_nest_start(msg, i + 1);

And you kind of even have it here already ...

> @@ -3001,6 +3042,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>  				rdev->wiphy.hw_timestamp_max_peers))
>  			goto nla_put_failure;
>  
> +		state->split_start++;
> +		break;
> +	case 17:
> +		if (nl80211_put_multi_hw_support(&rdev->wiphy, msg))
> +			goto nla_put_failure;
> 

This could be (or get) pretty big, are you sure it's not needed to push
the splitting down into it?

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Karthikeyan Periyasamy]

On 3/28/2024 1:19 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>> +/**
>> + * nl80211_multi_hw_attrs - multi-hw attributes
>> + *
>> + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
>> + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
>> + *	for which the supported channel list is advertised. Internally refer
>> + *	the index of the wiphy's @hw_chans array.
> Is there a good reason to expose this? Seems pretty internal to me, and
> not sure what userspace would do with it?

Hostapd use this hw index for the channel switch cmd.

The hw index used as a sanity check to identify whether the user 
requested channel fall under the different hw or not.

In split-phy hardware, 5GHz band supported by two physical hardware's. 
First supports 5GHz Low band and second supports 5GHz High band.

In this case, user space cannot use band vise check here to identify 
given channel or freq supported in the given hardware.


>> +	for (i = 0; i < wiphy->num_hw; i++) {
>> +		hw_mac = nla_nest_start(msg, i + 1);
> And you kind of even have it here already ...

Then user and kernel have to make an assumption that implicit index used 
in the life cycle.


>
>> @@ -3001,6 +3042,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>>   				rdev->wiphy.hw_timestamp_max_peers))
>>   			goto nla_put_failure;
>>   
>> +		state->split_start++;
>> +		break;
>> +	case 17:
>> +		if (nl80211_put_multi_hw_support(&rdev->wiphy, msg))
>> +			goto nla_put_failure;
>>
> This could be (or get) pretty big, are you sure it's not needed to push
> the splitting down into it?

ok, can do the advertise for split.


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 15:48 +0530, Karthikeyan Periyasamy wrote:
> On 3/28/2024 1:19 PM, Johannes Berg wrote:
> > On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> > > +/**
> > > + * nl80211_multi_hw_attrs - multi-hw attributes
> > > + *
> > > + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
> > > + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
> > > + *	for which the supported channel list is advertised. Internally refer
> > > + *	the index of the wiphy's @hw_chans array.
> > Is there a good reason to expose this? Seems pretty internal to me, and
> > not sure what userspace would do with it?
> 
> Hostapd use this hw index for the channel switch cmd.

What, where? I don't see that in this patchset? And why??

In any case, unless I just missed it and you're going to tell me to look
at patch N, we don't need it here, and then I'd prefer to keep it an
internal detail until it's needed.

> The hw index used as a sanity check to identify whether the user 
> requested channel fall under the different hw or not.

You mean within hostapd itself? That doesn't make sense, it can derive
that information differently.

> In split-phy hardware, 5GHz band supported by two physical hardware's. 
> First supports 5GHz Low band and second supports 5GHz High band.

In your hardware design anyway, but yeah, I get it.

> In this case, user space cannot use band vise check here to identify 
> given channel or freq supported in the given hardware.

No, but it also doesn't need an index assigned by the kernel for that.

> > > +	for (i = 0; i < wiphy->num_hw; i++) {
> > > +		hw_mac = nla_nest_start(msg, i + 1);
> > And you kind of even have it here already ...
> 
> Then user and kernel have to make an assumption that implicit index used 
> in the life cycle.

Agree that's maybe not a great idea, for all we care this could just use
0 as the index anyway.

Which reminds me ...

Right now, the way you have it, we have the following structure:

NL80211_ATTR_MULTI_HW
- 1
  - NL80211_MULTI_HW_ATTR_IDX: 0
  - NL80211_MULTI_HW_ATTR_FREQS
    - 0: 2412
    - 1: 2417
    ...
- 2
  - NL80211_MULTI_HW_ATTR_IDX: 1
  - NL80211_MULTI_HW_ATTR_FREQS
    - ... as above with 5 GHz etc.
...


Netlink is kind of moving away from nested arrays though:

https://kernel.org/doc/html/latest/userspace-api/netlink/specs.html#multi-attr-arrays
https://kernel.org/doc/html/latest/userspace-api/netlink/genetlink-legacy.html#attribute-type-nests

This talks about families, but maybe we shouldn't read that literally
and do the new style for new arrays in existing families as well, not
just new families.

If we do that, including NL80211_MULTI_HW_ATTR_IDX for illustrative
purposes though I think it should be removed, we'd end up with:

NL80211_ATTR_MULTI_HW
 - NL80211_MULTI_HW_ATTR_IDX: 0
 - NL80211_MULTI_HW_ATTR_FREQ: 2412
 - NL80211_MULTI_HW_ATTR_FREQ: 2417
 ...
NL80211_ATTR_MULTI_HW
 - NL80211_MULTI_HW_ATTR_IDX: 1
 - NL80211_MULTI_HW_ATTR_FREQ: 5180
 - NL80211_MULTI_HW_ATTR_FREQ: 5200
 ...

which _is_ a lot more compact, and removes all the uninteresting mid-
level indexing.

So in that sense, I prefer that, but I'm truly not sure how the (hand-
written) userspace code would deal with that.

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Karthikeyan Periyasamy]

On 3/28/2024 5:31 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 15:48 +0530, Karthikeyan Periyasamy wrote:
>> On 3/28/2024 1:19 PM, Johannes Berg wrote:
>>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>> +/**
>>>> + * nl80211_multi_hw_attrs - multi-hw attributes
>>>> + *
>>>> + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
>>>> + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
>>>> + *	for which the supported channel list is advertised. Internally refer
>>>> + *	the index of the wiphy's @hw_chans array.
>>> Is there a good reason to expose this? Seems pretty internal to me, and
>>> not sure what userspace would do with it?
>> Hostapd use this hw index for the channel switch cmd.
> What, where? I don't see that in this patchset? And why??
>
> In any case, unless I just missed it and you're going to tell me to look
> at patch N, we don't need it here, and then I'd prefer to keep it an
> internal detail until it's needed.
>
>> The hw index used as a sanity check to identify whether the user
>> requested channel fall under the different hw or not.
> You mean within hostapd itself? That doesn't make sense, it can derive
> that information differently.
>
>> In split-phy hardware, 5GHz band supported by two physical hardware's.
>> First supports 5GHz Low band and second supports 5GHz High band.
> In your hardware design anyway, but yeah, I get it.
>
>> In this case, user space cannot use band vise check here to identify
>> given channel or freq supported in the given hardware.
> No, but it also doesn't need an index assigned by the kernel for that.

Yes


>>>> +	for (i = 0; i < wiphy->num_hw; i++) {
>>>> +		hw_mac = nla_nest_start(msg, i + 1);
>>> And you kind of even have it here already ...
>> Then user and kernel have to make an assumption that implicit index used
>> in the life cycle.
> Agree that's maybe not a great idea, for all we care this could just use
> 0 as the index anyway.
>
> Which reminds me ...
>
> Right now, the way you have it, we have the following structure:
>
> NL80211_ATTR_MULTI_HW
> - 1
>    - NL80211_MULTI_HW_ATTR_IDX: 0
>    - NL80211_MULTI_HW_ATTR_FREQS
>      - 0: 2412
>      - 1: 2417
>      ...
> - 2
>    - NL80211_MULTI_HW_ATTR_IDX: 1
>    - NL80211_MULTI_HW_ATTR_FREQS
>      - ... as above with 5 GHz etc.
> ...
>
>
> Netlink is kind of moving away from nested arrays though:
>
> https://kernel.org/doc/html/latest/userspace-api/netlink/specs.html#multi-attr-arrays
> https://kernel.org/doc/html/latest/userspace-api/netlink/genetlink-legacy.html#attribute-type-nests
>
> This talks about families, but maybe we shouldn't read that literally
> and do the new style for new arrays in existing families as well, not
> just new families.
>
> If we do that, including NL80211_MULTI_HW_ATTR_IDX for illustrative
> purposes though I think it should be removed, we'd end up with:
>
> NL80211_ATTR_MULTI_HW
>   - NL80211_MULTI_HW_ATTR_IDX: 0
>   - NL80211_MULTI_HW_ATTR_FREQ: 2412
>   - NL80211_MULTI_HW_ATTR_FREQ: 2417
>   ...
> NL80211_ATTR_MULTI_HW
>   - NL80211_MULTI_HW_ATTR_IDX: 1
>   - NL80211_MULTI_HW_ATTR_FREQ: 5180
>   - NL80211_MULTI_HW_ATTR_FREQ: 5200
>   ...
>
> which _is_ a lot more compact, and removes all the uninteresting mid-
> level indexing.

Can you point to any attribute constructed in this way from kernelspace 
for the reference to explore more ?

-- 

Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> 
> Can you point to any attribute constructed in this way from kernelspace 
> for the reference to explore more ?

I don't have anything directly, looking at the code finds e.g.
devlink_dpipe_entry_ctx_append() but honestly that's really quite
trivial, it just adds that new attribute while iterating whatever list
you have.

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 3/28/2024 9:09 AM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
>>
>> Can you point to any attribute constructed in this way from kernelspace 
>> for the reference to explore more ?
> 
> I don't have anything directly, looking at the code finds e.g.
> devlink_dpipe_entry_ctx_append() but honestly that's really quite
> trivial, it just adds that new attribute while iterating whatever list
> you have.

Note that we are trying to maintain the same structure used by the current
wiphy global advertisement since we actually refactor and reuse the existing code.

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 3/28/2024 9:14 AM, Jeff Johnson wrote:
> On 3/28/2024 9:09 AM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> Can you point to any attribute constructed in this way from kernelspace 
>>> for the reference to explore more ?
>>
>> I don't have anything directly, looking at the code finds e.g.
>> devlink_dpipe_entry_ctx_append() but honestly that's really quite
>> trivial, it just adds that new attribute while iterating whatever list
>> you have.
> 
> Note that we are trying to maintain the same structure used by the current
> wiphy global advertisement since we actually refactor and reuse the existing code.

Need to check myself -- i'm thinking about the interface combo information here

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 09:14 -0700, Jeff Johnson wrote:
> On 3/28/2024 9:09 AM, Johannes Berg wrote:
> > On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> > > 
> > > Can you point to any attribute constructed in this way from kernelspace 
> > > for the reference to explore more ?
> > 
> > I don't have anything directly, looking at the code finds e.g.
> > devlink_dpipe_entry_ctx_append() but honestly that's really quite
> > trivial, it just adds that new attribute while iterating whatever list
> > you have.
> 
> Note that we are trying to maintain the same structure used by the current
> wiphy global advertisement since we actually refactor and reuse the existing code.

Partially, yes. That's not true for the one I was discussing _here_,
notably nl80211_put_multi_hw_support(), however.

It's partially true for patch 6, though even there
nl80211_put_per_hw_iface_combinations() doesn't need to do it that way,
since that's a whole new attribute (NL80211_IFACE_COMB_PER_HW_COMB) and
can define the content anew, as long as the part *inside*
NL80211_IFACE_COMB_PER_HW_COMB_LIMITS remains the same to be able to
call nl80211_put_iface_limits().

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 17:17 +0100, Johannes Berg wrote:
> On Thu, 2024-03-28 at 09:14 -0700, Jeff Johnson wrote:
> > On 3/28/2024 9:09 AM, Johannes Berg wrote:
> > > On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> > > > 
> > > > Can you point to any attribute constructed in this way from kernelspace 
> > > > for the reference to explore more ?
> > > 
> > > I don't have anything directly, looking at the code finds e.g.
> > > devlink_dpipe_entry_ctx_append() but honestly that's really quite
> > > trivial, it just adds that new attribute while iterating whatever list
> > > you have.
> > 
> > Note that we are trying to maintain the same structure used by the current
> > wiphy global advertisement since we actually refactor and reuse the existing code.
> 
> Partially, yes. That's not true for the one I was discussing _here_,
> notably nl80211_put_multi_hw_support(), however.
> 
> It's partially true for patch 6, though even there
> nl80211_put_per_hw_iface_combinations() doesn't need to do it that way,
> since that's a whole new attribute (NL80211_IFACE_COMB_PER_HW_COMB) and
> can define the content anew

I should say "content and how it forms an array in the top-level
message" here.

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jakub Kicinski]

On Thu, 28 Mar 2024 13:01:55 +0100 Johannes Berg wrote:
> If we do that, including NL80211_MULTI_HW_ATTR_IDX for illustrative
> purposes though I think it should be removed, we'd end up with:
> 
> NL80211_ATTR_MULTI_HW
>  - NL80211_MULTI_HW_ATTR_IDX: 0
>  - NL80211_MULTI_HW_ATTR_FREQ: 2412
>  - NL80211_MULTI_HW_ATTR_FREQ: 2417
>  ...
> NL80211_ATTR_MULTI_HW
>  - NL80211_MULTI_HW_ATTR_IDX: 1
>  - NL80211_MULTI_HW_ATTR_FREQ: 5180
>  - NL80211_MULTI_HW_ATTR_FREQ: 5200
>  ...
> 
> which _is_ a lot more compact, and removes all the uninteresting mid-
> level indexing.
> 
> So in that sense, I prefer that, but I'm truly not sure how the (hand-
> written) userspace code would deal with that.

I think the best way today would be two walks:

	for_each_attr() {
		switch (type):
		case THE_A_ARRAY_1:
			cnt1++;
			break;
		case THE_A_ARRAY_2:
			cnt2++;
			break;
	}

	if (cnt1)
		array_1 = calloc();
	cnt1 = 0; /* we'll use it as index in second loop */
	if (cnt2)
		array_2 = calloc();
	cnt2 = 0;

	for_each_attr() {
		/* [ normal parsing, populating array_1[cnt1++] etc. ] */
	}

Compared to "indexed array" the only practical difference I think is
the fact that all attrs are walked. I think you have to count them
either way before parsing.

I was wondering at some point whether we should require that all
multi-attr attributes are grouped together. Or add an explicit "count"
attribute. But couldn't convince myself that such extra rules will
pay off sufficiently with perf and/or ease of use...

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 11:49 -0700, Jakub Kicinski wrote:
> > So in that sense, I prefer that, but I'm truly not sure how the (hand-
> > written) userspace code would deal with that.
> 
> I think the best way today would be two walks:
> 
> 	for_each_attr() {
> 		switch (type):
> 		case THE_A_ARRAY_1:
> 			cnt1++;
> 			break;
> 		case THE_A_ARRAY_2:
> 			cnt2++;
> 			break;
> 	}
> 
> 	if (cnt1)
> 		array_1 = calloc();
> 	cnt1 = 0; /* we'll use it as index in second loop */
> 	if (cnt2)
> 		array_2 = calloc();
> 	cnt2 = 0;
> 
> 	for_each_attr() {
> 		/* [ normal parsing, populating array_1[cnt1++] etc. ] */
> 	}

Yeah, that makes sense.

I'm not sure we even need the calloc() all the time, depends what we're
doing with it, of course.

> Compared to "indexed array" the only practical difference I think is
> the fact that all attrs are walked. I think you have to count them
> either way before parsing.

Right, generally the pattern would be something like

nla_for_each_nested(...)
	n++;

// alloc etc.

idx = 0;
nla_for_each_nested(...)
	array[idx++] = whatever(attr);

or something like that.

So I guess the only thing that changes really is that this now becomes

nla_for_each(...)
	if (type != DESIRED)
		continue;

vs.

nla_for_each_nested(...)


I suppose we could even define a

nla_for_each_type(..., type)

for that.

> I was wondering at some point whether we should require that all
> multi-attr attributes are grouped together. Or add an explicit "count"
> attribute. But couldn't convince myself that such extra rules will
> pay off sufficiently with perf and/or ease of use...

That doesn't seem likely, after all, you'll definitely want to double-
check all that ... Personally, unless you have something super perf
critical, I definitely prefer _not_ having a count like that in the API
because it encourages unsafe code that doesn't do the necessary bounds
checks and then crashes ...

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jakub Kicinski]

On Thu, 28 Mar 2024 19:53:33 +0100 Johannes Berg wrote:
> I suppose we could even define a
> 
> nla_for_each_type(..., type)

That's probably a good idea for the kernel! We already have a bunch of
the loops with the if (type != DESIRED) continue, as you mentioned.

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 11:57 -0700, Jakub Kicinski wrote:
> On Thu, 28 Mar 2024 19:53:33 +0100 Johannes Berg wrote:
> > I suppose we could even define a
> > 
> > nla_for_each_type(..., type)
> 
> That's probably a good idea for the kernel! We already have a bunch of
> the loops with the if (type != DESIRED) continue, as you mentioned.

https://lore.kernel.org/r/20240328203144.b5a6c895fb80.I1869b44767379f204998ff44dd239803f39c23e0@changeid

:P

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Vasanthakumar Thiagarajan]

On 3/28/2024 5:31 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 15:48 +0530, Karthikeyan Periyasamy wrote:
>> On 3/28/2024 1:19 PM, Johannes Berg wrote:
>>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>> +/**
>>>> + * nl80211_multi_hw_attrs - multi-hw attributes
>>>> + *
>>>> + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
>>>> + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
>>>> + *	for which the supported channel list is advertised. Internally refer
>>>> + *	the index of the wiphy's @hw_chans array.
>>> Is there a good reason to expose this? Seems pretty internal to me, and
>>> not sure what userspace would do with it?
>>
>> Hostapd use this hw index for the channel switch cmd.
> 
> What, where? I don't see that in this patchset? And why??
> 
> In any case, unless I just missed it and you're going to tell me to look
> at patch N, we don't need it here, and then I'd prefer to keep it an
> internal detail until it's needed.
> 
>> The hw index used as a sanity check to identify whether the user
>> requested channel fall under the different hw or not.
> 
> You mean within hostapd itself? That doesn't make sense, it can derive
> that information differently.
> 
>> In split-phy hardware, 5GHz band supported by two physical hardware's.
>> First supports 5GHz Low band and second supports 5GHz High band.
> 
> In your hardware design anyway, but yeah, I get it.
> 
>> In this case, user space cannot use band vise check here to identify
>> given channel or freq supported in the given hardware.
> 
> No, but it also doesn't need an index assigned by the kernel for that.
> 

The only purpose of hw index is to link hw_chans to per-hardware 
interface combination capability so that each hardware can be
identified during interface combination checks capability vs
current state. Thinking if we can embed the channel list also
into per-hardware interface combination signaling by giving the
pointer?

Vasanth

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Fri, 2024-03-29 at 19:51 +0530, Vasanthakumar Thiagarajan wrote:
> 
> On 3/28/2024 5:31 PM, Johannes Berg wrote:
> > On Thu, 2024-03-28 at 15:48 +0530, Karthikeyan Periyasamy wrote:
> > > On 3/28/2024 1:19 PM, Johannes Berg wrote:
> > > > On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> > > > > +/**
> > > > > + * nl80211_multi_hw_attrs - multi-hw attributes
> > > > > + *
> > > > > + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
> > > > > + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
> > > > > + *	for which the supported channel list is advertised. Internally refer
> > > > > + *	the index of the wiphy's @hw_chans array.
> > > > Is there a good reason to expose this? Seems pretty internal to me, and
> > > > not sure what userspace would do with it?
> > > 
> > > Hostapd use this hw index for the channel switch cmd.
> > 
> > What, where? I don't see that in this patchset? And why??
> > 
> > In any case, unless I just missed it and you're going to tell me to look
> > at patch N, we don't need it here, and then I'd prefer to keep it an
> > internal detail until it's needed.
> > 
> > > The hw index used as a sanity check to identify whether the user
> > > requested channel fall under the different hw or not.
> > 
> > You mean within hostapd itself? That doesn't make sense, it can derive
> > that information differently.
> > 
> > > In split-phy hardware, 5GHz band supported by two physical hardware's.
> > > First supports 5GHz Low band and second supports 5GHz High band.
> > 
> > In your hardware design anyway, but yeah, I get it.
> > 
> > > In this case, user space cannot use band vise check here to identify
> > > given channel or freq supported in the given hardware.
> > 
> > No, but it also doesn't need an index assigned by the kernel for that.
> > 
> 
> The only purpose of hw index is to link hw_chans to per-hardware 
> interface combination capability so that each hardware can be
> identified during interface combination checks capability vs
> current state. Thinking if we can embed the channel list also
> into per-hardware interface combination signaling by giving the
> pointer?

Maybe? It might mean more allocations where the use is concerned since
it can't be "static const" that way.

I found the code that needs it later, just that Karthikeyan was using
the wrong explanation for it ... I'd hoped he'd understand your own code
better ;-)

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 4/10/2024 12:59 AM, Johannes Berg wrote:
> I found the code that needs it later, just that Karthikeyan was using
> the wrong explanation for it ... I'd hoped he'd understand your own code
> better ;-)

Internally I'm stressing the need to provide sufficient information in these
patches so that you (and I!) can understand the entire scope. Please continue
to let us know when we are failing.

(bcc to our internal development list)

/jeff

[PATCH 03/13] wifi: cfg80211: Refactor the interface combination limit check

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Move the interface combination limit check validation into a helper
function. This will make the iface combination limit validation
functionality more scalable for supporting multiple physical hardware
interface combination.

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>
---
 net/wireless/core.c | 114 +++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 49 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 119937d0f2e0..2b810855a805 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -600,10 +600,69 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 }
 EXPORT_SYMBOL(wiphy_new_nm);
 
+static int
+wiphy_verify_comb_limit(struct wiphy *wiphy,
+			const struct ieee80211_iface_limit *limits,
+			u8 n_limits, u32 bcn_int_min_gcd, u32 *iface_cnt,
+			u16 *all_iftypes)
+{
+	int i;
+
+	for (i = 0; i < n_limits; i++) {
+		u16 types = limits[i].types;
+
+		/* Don't advertise an unsupported type
+		 * in a combination.
+		 */
+		if (WARN_ON((wiphy->interface_modes & types) != types))
+			return -EINVAL;
+
+		if (WARN_ON(!limits[i].max))
+			return -EINVAL;
+
+		/* interface types shouldn't overlap */
+		if (WARN_ON(types & *all_iftypes))
+			return -EINVAL;
+
+		*all_iftypes |= types;
+
+		/* Shouldn't list software iftypes in combinations! */
+		if (WARN_ON(wiphy->software_iftypes & types))
+			return -EINVAL;
+
+		/* Only a single P2P_DEVICE can be allowed */
+		if (WARN_ON(types & BIT(NL80211_IFTYPE_P2P_DEVICE) &&
+			    limits[i].max > 1))
+			return -EINVAL;
+
+		/* Only a single NAN can be allowed */
+		if (WARN_ON(types & BIT(NL80211_IFTYPE_NAN) &&
+			    limits[i].max > 1))
+			return -EINVAL;
+
+		/* This isn't well-defined right now. If you have an
+		 * IBSS interface, then its beacon interval may change
+		 * by joining other networks, and nothing prevents it
+		 * from doing that.
+		 * So technically we probably shouldn't even allow AP
+		 * and IBSS in the same interface, but it seems that
+		 * some drivers support that, possibly only with fixed
+		 * beacon intervals for IBSS.
+		 */
+		if (WARN_ON(types & BIT(NL80211_IFTYPE_ADHOC) &&
+			    bcn_int_min_gcd))
+			return -EINVAL;
+
+		*iface_cnt += limits[i].max;
+	}
+
+	return 0;
+}
+
 static int wiphy_verify_combinations(struct wiphy *wiphy)
 {
 	const struct ieee80211_iface_combination *c;
-	int i, j;
+	int i, ret;
 
 	for (i = 0; i < wiphy->n_iface_combinations; i++) {
 		u32 cnt = 0;
@@ -630,54 +689,11 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 		if (WARN_ON(!c->n_limits))
 			return -EINVAL;
 
-		for (j = 0; j < c->n_limits; j++) {
-			u16 types = c->limits[j].types;
-
-			/* interface types shouldn't overlap */
-			if (WARN_ON(types & all_iftypes))
-				return -EINVAL;
-			all_iftypes |= types;
-
-			if (WARN_ON(!c->limits[j].max))
-				return -EINVAL;
-
-			/* Shouldn't list software iftypes in combinations! */
-			if (WARN_ON(wiphy->software_iftypes & types))
-				return -EINVAL;
-
-			/* Only a single P2P_DEVICE can be allowed */
-			if (WARN_ON(types & BIT(NL80211_IFTYPE_P2P_DEVICE) &&
-				    c->limits[j].max > 1))
-				return -EINVAL;
-
-			/* Only a single NAN can be allowed */
-			if (WARN_ON(types & BIT(NL80211_IFTYPE_NAN) &&
-				    c->limits[j].max > 1))
-				return -EINVAL;
-
-			/*
-			 * This isn't well-defined right now. If you have an
-			 * IBSS interface, then its beacon interval may change
-			 * by joining other networks, and nothing prevents it
-			 * from doing that.
-			 * So technically we probably shouldn't even allow AP
-			 * and IBSS in the same interface, but it seems that
-			 * some drivers support that, possibly only with fixed
-			 * beacon intervals for IBSS.
-			 */
-			if (WARN_ON(types & BIT(NL80211_IFTYPE_ADHOC) &&
-				    c->beacon_int_min_gcd)) {
-				return -EINVAL;
-			}
-
-			cnt += c->limits[j].max;
-			/*
-			 * Don't advertise an unsupported type
-			 * in a combination.
-			 */
-			if (WARN_ON((wiphy->interface_modes & types) != types))
-				return -EINVAL;
-		}
+		ret = wiphy_verify_comb_limit(wiphy, c->limits, c->n_limits,
+					      c->beacon_int_min_gcd,
+					      &cnt, &all_iftypes);
+		if (ret)
+			return ret;
 
 		if (WARN_ON(all_iftypes & BIT(NL80211_IFTYPE_WDS)))
 			return -EINVAL;
-- 
2.34.1

[PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], when a driver combines multiple
discrete hardware under one wiphy, it is required for the driver to be
able to advertise iface combination capabilities per underlying physical
hardware. Each underlying hardware's iface combination is described with
an identifier, which corresponds to the same index used in
wiphy->hw_chans[] to learn the channel capabilities of the respective
hardware. It’s important to note that supporting drivers also signal the
iface combination capabilities that are common for all the hardware
through the existing interface, maintaining backward compatibility with
user space. This commit implements provisions to advertise per-physical
hardware specific iface combination capabilities and includes sanity
checks on the advertised capabilities. The sanity check includes channel
validation against the entire range of DFS frequencies, irrespective of
regulatory configurations.

Example:

Say driver abstracts two discrete hardware under one wiphy,
wiphy->hw_chans[0] supporting 2 GHz and wiphy->hw_chans[1] supporting
5 GHz. Each hardware can operate on only one channel at any given time
but under the wiphy there can be concurrent interfaces on both the radios.
2 GHz hardware supports #STA <= 1, #AP <= 3 total 4 and 5 GHz hardware
supports #STA <= 1, #AP <= 4 total 5

struct ieee80211_iface_limit limits_common[] = {
	{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
	{ .max = 3, .types = BIT(NL80211_IFTYPE_AP), },
};

limits_common[] defines the minimum (common) capability out of all the
underlying hardware specific capabilities. This is reported in the existing
advertisement mechanism. Common max_interfaces across 2 GHz and 5 GHz is 4,
common num_different_channels is 1.

struct ieee80211_iface_limit limits_2ghz[] = {
	{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
	{ .max = 3, .types = BIT(NL80211_IFTYPE_AP), },
};

struct ieee80211_iface_limit limits_5ghz[] = {
	{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
	{ .max = 4, .types = BIT(NL80211_IFTYPE_AP), },
};

struct ieee80211_iface_combination combination = {
	.limits = limits_common,
	.max_interfaces = 4,
	.num_different_channels = 1,
	...
	.freq_range = {
			{
				.hw_chan_idx = 0,
				.limits = limits_2ghz,
				.max_interfaces = 4,
				.num_different_channels = 1,
				.n_limits = ARRAY_SIZE(limits_2ghz),
			},
			{
				.hw_chan_idx = 1,
				.limits = limits_5ghz,
				.max_interfaces = 5,
				.num_different_channels = 1,
				.n_limits = ARRAY_SIZE(limits_5ghz),
			},
		      },
};

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-4-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 | 96 ++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/main.c    | 46 ++++++++++++++++++++
 net/wireless/core.c    | 92 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index dde129e61b60..491d074fe430 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5030,6 +5030,32 @@ struct ieee80211_iface_limit {
 	u16 types;
 };
 
+/**
+ * struct ieee80211_iface_per_hw - hardware specific interface combination
+ *
+ * Drivers registering multiple radios under a single wiphy can advertise
+ * radio specific interface combinations through this structure. Please note
+ * that to maintain the compatibility with the user space which is not aware
+ * of this extension of per-hardware interface combination signaling,
+ * the driver should still advertise it's interface combination (mostly
+ * common minimum capability) using the existing interface combination signaling
+ * method.
+ *
+ * @limits: limits for the given interface type
+ * @num_different_channels: number of different channels which can be active
+ *	concurrently in this hardware
+ * @max_interfaces: maximum number of total interfaces allowed in this group
+ * @n_limits: number of limitations
+ * @hw_chans_idx: index of hardware specific channel list as per wiphy @hw_chans
+ */
+struct ieee80211_iface_per_hw {
+	const struct ieee80211_iface_limit *limits;
+	u32 num_different_channels;
+	u16 max_interfaces;
+	u8 n_limits;
+	u8 hw_chans_idx;
+};
+
 /**
  * struct ieee80211_iface_combination - possible interface combination
  *
@@ -5088,6 +5114,62 @@ struct ieee80211_iface_limit {
  *		.num_different_channels = 2,
  *	};
  *
+ *
+ * 4. Hardware specific interface combination with driver supporting two
+ *    physical HW, first underlying HW supporting 2 GHz band and the other
+ *    supporting 5 GHz band.
+ *
+ *    Allow #STA <= 1, #AP <= 1, channels = 1, total 2 in 2 GHz radio and
+ *
+ *    Allow #STA <= 1, #AP <= 2, channels = 1, total 3 in 5 GHz radio
+ *
+ *    Drivers advertising per-hardware interface combination should also
+ *    advertise a sub-set of capabilities using existing interface mainly for
+ *    maintaining compatibility with the user space which is not aware of the
+ *    new per-hardware advertisement.
+ *
+ *    Sub-set interface combination advertised in the existing infrastructure:
+ *    Allow #STA <= 1, #AP <= 1, channels = 1, total 2
+ *
+ *    .. code-block:: c
+ *
+ *	struct ieee80211_iface_limit limits_overall[] = {
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
+ *	};
+ *	struct ieee80211_iface_limit limits_2ghz[] = {
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
+ *	};
+ *	struct ieee80211_iface_limit limits_5ghz[] = {
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
+ *		{ .max = 2, .types = BIT(NL80211_IFTYPE_AP), },
+ *	};
+ *	struct ieee80211_iface_per_hw hw_combinations[] = {
+ *		{
+ *			.hw_chans_idx = 0,
+ *			.limits = limits_2ghz,
+ *			.num_different_channels = 1,
+ *			.max_interfaces = 2,
+ *			.n_limits = ARRAY_SIZE(limits_2ghz),
+ *		 },
+ *		{
+ *			.hw_chans_idx = 1,
+ *			.limits = limits_5ghz,
+ *			.num_different_channels = 1,
+ *			.max_interfaces = 3,
+ *			.n_limits = ARRAY_SIZE(limits_5ghz),
+ *		 },
+ *	};
+ *	struct ieee80211_iface_combination combination4 = {
+ *		.limits = limits_overall,
+ *		.n_limits = ARRAY_SIZE(limits_overall),
+ *		.max_interfaces = 2,
+ *		.num_different_channels = 1,
+ *		.iface_hw_list = hw_combinations,
+ *		.n_hw_list = ARRAY_SIZE(hw_combinations),
+ *	};
+ *
  */
 struct ieee80211_iface_combination {
 	/**
@@ -5145,6 +5227,20 @@ struct ieee80211_iface_combination {
 	 *   combination must be greater or equal to this value.
 	 */
 	u32 beacon_int_min_gcd;
+
+	/**
+	 * @iface_hw_list:
+	 * This wiphy has multiple underlying radios, describe interface
+	 * combination for each of them, valid only when the driver advertises
+	 * multi-radio presence in wiphy @hw_chans.
+	 */
+	const struct ieee80211_iface_per_hw *iface_hw_list;
+
+	/**
+	 * @n_hw_list:
+	 * number of hardware in @iface_hw_List
+	 */
+	u32 n_hw_list;
 };
 
 struct ieee80211_txrx_stypes {
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 4eaea0a9975b..24765ab5121b 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1083,6 +1083,46 @@ static int ieee80211_init_cipher_suites(struct ieee80211_local *local)
 	return 0;
 }
 
+static int
+ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local,
+				  const struct ieee80211_iface_combination *c)
+{
+	int hw_idx, lmt_idx;
+	u32 hw_idx_bm = 0;
+
+	if (!local->hw.wiphy->num_hw)
+		return -EINVAL;
+
+	if (local->emulate_chanctx)
+		return -EINVAL;
+
+	for (hw_idx = 0; hw_idx < c->n_hw_list; hw_idx++) {
+		const struct ieee80211_iface_per_hw *hl;
+
+		hl = &c->iface_hw_list[hw_idx];
+
+		if (hl->hw_chans_idx >= local->hw.wiphy->num_hw)
+			return -EINVAL;
+
+		/* mac80211 doesn't support more than one IBSS interface right now */
+		for (lmt_idx = 0; lmt_idx < hl->n_limits; lmt_idx++) {
+			const struct ieee80211_iface_limit *limits;
+
+			limits = &hl->limits[lmt_idx];
+			if ((limits->types & BIT(NL80211_IFTYPE_ADHOC)) &&
+			    limits->max > 1)
+				return -EINVAL;
+		}
+
+		if (hw_idx_bm & BIT(hw_idx))
+			return -EINVAL;
+
+		hw_idx_bm |= BIT(hw_idx);
+	}
+
+	return 0;
+}
+
 int ieee80211_register_hw(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -1323,6 +1363,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 			if ((c->limits[j].types & BIT(NL80211_IFTYPE_ADHOC)) &&
 			    c->limits[j].max > 1)
 				return -EINVAL;
+
+		if (!c->n_hw_list)
+			continue;
+
+		if (ieee80211_check_per_hw_iface_comb(local, c))
+			return -EINVAL;
 	}
 
 	local->int_scan_req = kzalloc(sizeof(*local->int_scan_req) +
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 2b810855a805..88de2839ed6b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -600,6 +600,34 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 }
 EXPORT_SYMBOL(wiphy_new_nm);
 
+/**
+ * cfg80211_hw_chans_includes_dfs - check if per-hardware channel includes DFS
+ * @chans: hardware channel list
+ *
+ * Check if the given per-hardware list include channels in DFS range.
+ * Please note the channel is checked against the entire range of DFS
+ * freq in 5 GHz irrespective of regulatory configurations.
+ *
+ * This helper helps for the sanity checks on the channel advertisement from
+ * driver during the hw registration.
+ */
+static bool
+cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
+{
+	int i;
+
+	for (i = 0; i < chans->n_chans; i++) {
+		if (chans->chans[i].band == NL80211_BAND_5GHZ &&
+		    ((chans->chans[i].center_freq >= 5250 &&
+		     chans->chans[i].center_freq <= 5340) ||
+		    (chans->chans[i].center_freq >= 5480 &&
+		     chans->chans[i].center_freq <= 5720)))
+			return true;
+	}
+
+	return false;
+}
+
 static int
 wiphy_verify_comb_limit(struct wiphy *wiphy,
 			const struct ieee80211_iface_limit *limits,
@@ -659,6 +687,63 @@ wiphy_verify_comb_limit(struct wiphy *wiphy,
 	return 0;
 }
 
+static int
+wiphy_verify_comb_per_hw(struct wiphy *wiphy,
+			 const struct ieee80211_iface_combination *comb)
+{
+	int i;
+	u32 hw_idx_bitmap = 0;
+	int ret;
+
+	for (i = 0; i < comb->n_hw_list; i++) {
+		const struct ieee80211_iface_per_hw *hl;
+		const struct ieee80211_chans_per_hw *chans;
+		u32 iface_cnt = 0;
+		u16 all_iftypes = 0;
+
+		hl = &comb->iface_hw_list[i];
+
+		if (hl->hw_chans_idx >= wiphy->num_hw)
+			return -EINVAL;
+
+		if (hw_idx_bitmap & BIT(hl->hw_chans_idx))
+			return -EINVAL;
+
+		hw_idx_bitmap |= BIT(hl->hw_chans_idx);
+		chans = wiphy->hw_chans[hl->hw_chans_idx];
+
+		if (WARN_ON(hl->max_interfaces < 2 &&
+			    (!comb->radar_detect_widths ||
+			     !(cfg80211_hw_chans_includes_dfs(chans)))))
+			return -EINVAL;
+
+		if (WARN_ON(!hl->num_different_channels))
+			return -EINVAL;
+
+		if (WARN_ON(comb->radar_detect_widths &&
+			    cfg80211_hw_chans_includes_dfs(chans) &&
+			    hl->num_different_channels > 1))
+			return -EINVAL;
+
+		if (WARN_ON(!hl->n_limits))
+			return -EINVAL;
+
+		ret = wiphy_verify_comb_limit(wiphy, hl->limits, hl->n_limits,
+					      comb->beacon_int_min_gcd,
+					      &iface_cnt, &all_iftypes);
+		if (ret)
+			return ret;
+
+		if (WARN_ON(all_iftypes & BIT(NL80211_IFTYPE_WDS)))
+			return -EINVAL;
+
+		if (WARN_ON(iface_cnt < comb->max_interfaces))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int wiphy_verify_combinations(struct wiphy *wiphy)
 {
 	const struct ieee80211_iface_combination *c;
@@ -701,6 +786,13 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 		/* You can't even choose that many! */
 		if (WARN_ON(cnt < c->max_interfaces))
 			return -EINVAL;
+
+		/* Do similar validations on the freq range specific interface
+		 * combinations when advertised.
+		 */
+		if (WARN_ON(c->n_hw_list &&
+			    wiphy_verify_comb_per_hw(wiphy, c)))
+			return -EINVAL;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Johannes Berg]

Not sure why this is a combined cfg/mac patch, there doesn't seem to be
all that much need for that? I'd probably rather see this and patch 6
squashed, but cfg/mac kept separate.


On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:

> 
> +static int
> +ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local,
> +				  const struct ieee80211_iface_combination *c)
> +{
> +	int hw_idx, lmt_idx;
> +	u32 hw_idx_bm = 0;

lmt_idx? (also that's only needed in the loop)
bm?

> +	if (!local->hw.wiphy->num_hw)
> +		return -EINVAL;
> +
> +	if (local->emulate_chanctx)
> +		return -EINVAL;
> +
> +	for (hw_idx = 0; hw_idx < c->n_hw_list; hw_idx++) {
> +		const struct ieee80211_iface_per_hw *hl;

hl?

Could have a bit more evocative names :)

I'd rather see 'i' for some iteration thingie  than "lmt_idx" which
means nothing on first reading either, but you start thinking it should
mean something ...

> +static bool
> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
> +{
> +	int i;
> +
> +	for (i = 0; i < chans->n_chans; i++) {
> +		if (chans->chans[i].band == NL80211_BAND_5GHZ &&
> +		    ((chans->chans[i].center_freq >= 5250 &&
> +		     chans->chans[i].center_freq <= 5340) ||
> +		    (chans->chans[i].center_freq >= 5480 &&
> +		     chans->chans[i].center_freq <= 5720)))

???

That's not how this works upstream.

> +		if (WARN_ON(hl->max_interfaces < 2 &&
> +			    (!comb->radar_detect_widths ||
> +			     !(cfg80211_hw_chans_includes_dfs(chans)))))

No need for extra parentheses.

> @@ -701,6 +786,13 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
>  		/* You can't even choose that many! */
>  		if (WARN_ON(cnt < c->max_interfaces))
>  			return -EINVAL;
> +
> +		/* Do similar validations on the freq range specific interface
> +		 * combinations when advertised.
> +		 */
> +		if (WARN_ON(c->n_hw_list &&
> +			    wiphy_verify_comb_per_hw(wiphy, c)))

Don't need the n_hw_list check here, the function just does nothing if
it's 0 anyway.

johannes

Re: [PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Karthikeyan Periyasamy]

On 3/28/2024 6:52 PM, Johannes Berg wrote:
> Not sure why this is a combined cfg/mac patch, there doesn't seem to be
> all that much need for that? I'd probably rather see this and patch 6
> squashed, but cfg/mac kept separate.

Sure, will split into the cfg/mac patches.


>
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>
>> +static int
>> +ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local,
>> +				  const struct ieee80211_iface_combination *c)
>> +{
>> +	int hw_idx, lmt_idx;
>> +	u32 hw_idx_bm = 0;
> lmt_idx? (also that's only needed in the loop)

sure, will move it local to that block


> bm?


bitmap used for hw_id sanity checks.


>
>> +	if (!local->hw.wiphy->num_hw)
>> +		return -EINVAL;
>> +
>> +	if (local->emulate_chanctx)
>> +		return -EINVAL;
>> +
>> +	for (hw_idx = 0; hw_idx < c->n_hw_list; hw_idx++) {
>> +		const struct ieee80211_iface_per_hw *hl;
> hl?
>
> Could have a bit more evocative names :)
>
> I'd rather see 'i' for some iteration thingie  than "lmt_idx" which
> means nothing on first reading either, but you start thinking it should
> mean something ...

sure.


>> +static bool
>> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < chans->n_chans; i++) {
>> +		if (chans->chans[i].band == NL80211_BAND_5GHZ &&
>> +		    ((chans->chans[i].center_freq >= 5250 &&
>> +		     chans->chans[i].center_freq <= 5340) ||
>> +		    (chans->chans[i].center_freq >= 5480 &&
>> +		     chans->chans[i].center_freq <= 5720)))
> ???
>
> That's not how this works upstream.


radar_detect_widths itself not sufficient for the radar specific sanity 
check in the multiple hardware iface combination approach. So include 
this helper for the radar sanity check.

This helper used to check the given channel list present in the 5GHz 
band or not.


>
>> +		if (WARN_ON(hl->max_interfaces < 2 &&
>> +			    (!comb->radar_detect_widths ||
>> +			     !(cfg80211_hw_chans_includes_dfs(chans)))))
> No need for extra parentheses.

sure


>> @@ -701,6 +786,13 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
>>   		/* You can't even choose that many! */
>>   		if (WARN_ON(cnt < c->max_interfaces))
>>   			return -EINVAL;
>> +
>> +		/* Do similar validations on the freq range specific interface
>> +		 * combinations when advertised.
>> +		 */
>> +		if (WARN_ON(c->n_hw_list &&
>> +			    wiphy_verify_comb_per_hw(wiphy, c)))
> Don't need the n_hw_list check here, the function just does nothing if
> it's 0 anyway.

sure, will address all the comments in the next version of the patch


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 05/13] wifi: nl80211: Refactor the interface combination limit check

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Move the nl80211 interface combination limit advertisement into a helper
function. This will make the iface combination limit advertisement
functionality more scalable for supporting multiple physical hardware
interface combination.

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>
---
 net/wireless/nl80211.c | 49 +++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2a5e395e2e0b..37524a61f417 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1622,12 +1622,43 @@ static int nl80211_put_iftypes(struct sk_buff *msg, u32 attr, u16 ifmodes)
 	return -ENOBUFS;
 }
 
+static int
+nl80211_put_iface_limits(struct sk_buff *msg,
+			 const struct ieee80211_iface_limit *limits,
+			 u8 n_limits)
+{
+	int i;
+
+	for (i = 0; i < n_limits; i++) {
+		struct nlattr *nl_limit;
+
+		nl_limit = nla_nest_start_noflag(msg, i + 1);
+		if (!nl_limit)
+			goto nla_put_failure;
+
+		if (nla_put_u32(msg, NL80211_IFACE_LIMIT_MAX,
+				limits[i].max))
+			goto nla_put_failure;
+
+		if (nl80211_put_iftypes(msg, NL80211_IFACE_LIMIT_TYPES,
+					limits[i].types))
+			goto nla_put_failure;
+
+		nla_nest_end(msg, nl_limit);
+	}
+
+	return 0;
+
+nla_put_failure:
+	return -ENOBUFS;
+}
+
 static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 					  struct sk_buff *msg,
 					  bool large)
 {
 	struct nlattr *nl_combis;
-	int i, j;
+	int i;
 
 	nl_combis = nla_nest_start_noflag(msg,
 					  NL80211_ATTR_INTERFACE_COMBINATIONS);
@@ -1649,20 +1680,8 @@ static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 		if (!nl_limits)
 			goto nla_put_failure;
 
-		for (j = 0; j < c->n_limits; j++) {
-			struct nlattr *nl_limit;
-
-			nl_limit = nla_nest_start_noflag(msg, j + 1);
-			if (!nl_limit)
-				goto nla_put_failure;
-			if (nla_put_u32(msg, NL80211_IFACE_LIMIT_MAX,
-					c->limits[j].max))
-				goto nla_put_failure;
-			if (nl80211_put_iftypes(msg, NL80211_IFACE_LIMIT_TYPES,
-						c->limits[j].types))
-				goto nla_put_failure;
-			nla_nest_end(msg, nl_limit);
-		}
+		if (nl80211_put_iface_limits(msg, c->limits, c->n_limits))
+			goto nla_put_failure;
 
 		nla_nest_end(msg, nl_limits);
 
-- 
2.34.1

[PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], add a new nested attribute to the
existing NL80211_CMD_NEW_WIPHY command. This attribute should advertise the
iface combination capability for each underlying physical hardware device.
This is necessary when the driver groups more than one physical hardware
under single wiphy.

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-5-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/uapi/linux/nl80211.h | 50 ++++++++++++++++++++++++++++++-
 net/wireless/nl80211.c       | 58 ++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c53c9f941663..aa0988be1f2a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -6004,6 +6004,10 @@ enum nl80211_iface_limit_attrs {
  * @NL80211_IFACE_COMB_BI_MIN_GCD: u32 attribute specifying the minimum GCD of
  *	different beacon intervals supported by all the interface combinations
  *	in this group (if not present, all beacon intervals be identical).
+ * @NL80211_IFACE_COMB_PER_HW_COMB: nested attribute specifying the interface
+ *	combination for each underlying hardware when multiple hardware are
+ *	registered under one wiphy,
+ *	see &enum nl80211_if_comb_per_hw_comb_attrs.
  * @NUM_NL80211_IFACE_COMB: number of attributes
  * @MAX_NL80211_IFACE_COMB: highest attribute number
  *
@@ -6020,7 +6024,19 @@ enum nl80211_iface_limit_attrs {
  *	numbers = [ #{STA} <= 1, #{P2P-client,P2P-GO} <= 3 ], max = 4
  *	=> allows a STA plus three P2P interfaces
  *
- * The list of these four possibilities could completely be contained
+ *	When describing per-hardware combinations in multi-hardware in
+ *	one wiphy model, the first possibility can further include the finer
+ *	capabilities like below
+ *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
+ *	channels = 1, max = 2
+ *	=> allows a STA plus an AP interface on the underlying hardware mac
+ *	   advertised at index 0 in wiphy @hw_chans array.
+ *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
+ *	channels = 1, max = 3
+ *	=> allows a STA plus two AP interfaces on the underlying hardware mac
+ *	   advertised at index 1 in wiphy @hw_chans array.
+ *
+ * The list of these five possibilities could completely be contained
  * within the %NL80211_ATTR_INTERFACE_COMBINATIONS attribute to indicate
  * that any of these groups must match.
  *
@@ -6039,12 +6055,44 @@ enum nl80211_if_combination_attrs {
 	NL80211_IFACE_COMB_RADAR_DETECT_WIDTHS,
 	NL80211_IFACE_COMB_RADAR_DETECT_REGIONS,
 	NL80211_IFACE_COMB_BI_MIN_GCD,
+	NL80211_IFACE_COMB_PER_HW_COMB,
 
 	/* keep last */
 	NUM_NL80211_IFACE_COMB,
 	MAX_NL80211_IFACE_COMB = NUM_NL80211_IFACE_COMB - 1
 };
 
+/**
+ * enum nl80211_if_comb_per_hw_comb_attrs - per-hardware iface combination
+ * attributes with multi-hw radios in one wiphy model
+ *
+ * @NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC: (reserved)
+ * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
+ *	to the wiphy @hw_chans list for which the iface combination is being
+ *	described.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
+ *	limits for the given interface types, see
+ *	&enum nl80211_iface_limit_attrs.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
+ *	number of interfaces that can be created in this group. This number
+ *	does not apply to the interfaces purely managed in software.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
+ *	number of different channels that can be used in this group.
+ * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
+ * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
+ */
+enum nl80211_if_comb_per_hw_comb_attrs {
+	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
+	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
+	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
+	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
+	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
+
+	/* keep last */
+	NUM_NL80211_IFACE_COMB_PER_HW_COMB,
+	MAX_NL80211_IFACE_COMB_PER_HW_COMB =
+			NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
+};
 
 /**
  * enum nl80211_plink_state - state of a mesh peer link finite state machine
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 37524a61f417..87436924834f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1653,6 +1653,61 @@ nl80211_put_iface_limits(struct sk_buff *msg,
 	return -ENOBUFS;
 }
 
+static int
+nl80211_put_per_hw_iface_combinations(struct wiphy *wiphy, struct sk_buff *msg,
+				      const struct ieee80211_iface_combination *c)
+{
+	struct nlattr *hw_combis;
+	int i;
+
+	if (!wiphy->num_hw)
+		return 0;
+
+	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
+	if (!hw_combis)
+		return -ENOBUFS;
+
+	for (i = 0; i < c->n_hw_list; i++) {
+		struct nlattr *hw_combi, *limits;
+
+		hw_combi = nla_nest_start(msg, i + 1);
+		if (!hw_combi)
+			return -ENOBUFS;
+
+		if (nla_put_u8(msg, NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
+			       c->iface_hw_list[i].hw_chans_idx))
+			return -ENOBUFS;
+
+		limits = nla_nest_start(msg,
+					NL80211_IFACE_COMB_PER_HW_COMB_LIMITS);
+		if (!limits)
+			return -ENOBUFS;
+
+		if (nl80211_put_iface_limits(msg,
+					     c->iface_hw_list[i].limits,
+					     c->iface_hw_list[i].n_limits))
+			return -ENOBUFS;
+
+		nla_nest_end(msg, limits);
+
+		if (nla_put_u32(msg,
+				NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
+				c->iface_hw_list[i].num_different_channels))
+			return -ENOBUFS;
+
+		if (nla_put_u16(msg,
+				NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
+				c->iface_hw_list[i].max_interfaces))
+			return -ENOBUFS;
+
+		nla_nest_end(msg, hw_combi);
+	}
+
+	nla_nest_end(msg, hw_combis);
+
+	return 0;
+}
+
 static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 					  struct sk_buff *msg,
 					  bool large)
@@ -1704,6 +1759,9 @@ static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 				c->beacon_int_min_gcd))
 			goto nla_put_failure;
 
+		if (large && nl80211_put_per_hw_iface_combinations(wiphy, msg, c))
+			goto nla_put_failure;
+
 		nla_nest_end(msg, nl_combi);
 	}
 
-- 
2.34.1

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
> + *	When describing per-hardware combinations in multi-hardware in
> + *	one wiphy model, the first possibility can further include the finer
> + *	capabilities like below

Not sure I'd say "below" rather than e.g. "like this:"

> + *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
> + *	channels = 1, max = 2
> + *	=> allows a STA plus an AP interface on the underlying hardware mac
> + *	   advertised at index 0 in wiphy @hw_chans array.
> + *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
> + *	channels = 1, max = 3
> + *	=> allows a STA plus two AP interfaces on the underlying hardware mac
> + *	   advertised at index 1 in wiphy @hw_chans array.

Have you checked the rst output for this? Seems likely that's not going
to be great with that formatting, but I haven't checked.

> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
> + *	to the wiphy @hw_chans list for which the iface combination is being
> + *	described.

"@hw_chans" doesn't make sense here, this is nl80211, it should refer to
some attribute

but why didn't you just _say_ in the patch 2 discussion that it's used
here ...

> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
> + *	limits for the given interface types, see
> + *	&enum nl80211_iface_limit_attrs.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
> + *	number of interfaces that can be created in this group. This number
> + *	does not apply to the interfaces purely managed in software.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
> + *	number of different channels that can be used in this group.
> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
> + */
> +enum nl80211_if_comb_per_hw_comb_attrs {
> +	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> +	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
> +	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
> +	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
> +	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,

Almost all these attributes duplicate - including their docs -
attributes from enum nl80211_if_combination_attrs. Is it really worth
doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
the different uses of the attribute set?

> +	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
> +	if (!hw_combis)
> +		return -ENOBUFS;
> +
> +	for (i = 0; i < c->n_hw_list; i++) {
> +		struct nlattr *hw_combi, *limits;
> +
> +		hw_combi = nla_nest_start(msg, i + 1);

And of course the array and splitting discussions apply here too.

johannes

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Jeff Johnson]

On 3/28/2024 6:33 AM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
[...]
>> +	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
>> +	if (!hw_combis)
>> +		return -ENOBUFS;
>> +
>> +	for (i = 0; i < c->n_hw_list; i++) {
>> +		struct nlattr *hw_combi, *limits;
>> +
>> +		hw_combi = nla_nest_start(msg, i + 1);
> 
> And of course the array and splitting discussions apply here too.

This is where I meant to say that I believe we are trying to reuse the same
structure and code as the wiphy-global combinations

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Vasanthakumar Thiagarajan]

On 3/28/2024 7:03 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>
>> + *	When describing per-hardware combinations in multi-hardware in
>> + *	one wiphy model, the first possibility can further include the finer
>> + *	capabilities like below
> 
> Not sure I'd say "below" rather than e.g. "like this:"
> 
>> + *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>> + *	channels = 1, max = 2
>> + *	=> allows a STA plus an AP interface on the underlying hardware mac
>> + *	   advertised at index 0 in wiphy @hw_chans array.
>> + *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>> + *	channels = 1, max = 3
>> + *	=> allows a STA plus two AP interfaces on the underlying hardware mac
>> + *	   advertised at index 1 in wiphy @hw_chans array.
> 
> Have you checked the rst output for this? Seems likely that's not going
> to be great with that formatting, but I haven't checked.
> 
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
>> + *	to the wiphy @hw_chans list for which the iface combination is being
>> + *	described.
> 
> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
> some attribute
> 
> but why didn't you just _say_ in the patch 2 discussion that it's used
> here ...
> 
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
>> + *	limits for the given interface types, see
>> + *	&enum nl80211_iface_limit_attrs.
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
>> + *	number of interfaces that can be created in this group. This number
>> + *	does not apply to the interfaces purely managed in software.
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
>> + *	number of different channels that can be used in this group.
>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>> + */
>> +enum nl80211_if_comb_per_hw_comb_attrs {
>> +	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
> 
> Almost all these attributes duplicate - including their docs -
> attributes from enum nl80211_if_combination_attrs. Is it really worth
> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
> the different uses of the attribute set?
> 

I agree, more duplication. So we have to have the per_hw_comb_attrs
defines like below?

enum nl80211_if_comb_per_hw_comb_attrs {
	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX = 			
			NL80211_IFACE_COMB_NUM_CHANNELS + 1,
	/* keep last */
	NUM_NL80211_IFACE_COMB_PER_HW_COMB,
	MAX_NL80211_IFACE_COMB_PER_HW_COMB =
			NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
};

Vasanth

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Karthikeyan Periyasamy]

On 3/29/2024 8:04 PM, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 3/28/2024 7:03 PM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> + *    When describing per-hardware combinations in multi-hardware in
>>> + *    one wiphy model, the first possibility can further include the 
>>> finer
>>> + *    capabilities like below
>>
>> Not sure I'd say "below" rather than e.g. "like this:"
>>
>>> + *    hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>>> + *    channels = 1, max = 2
>>> + *    => allows a STA plus an AP interface on the underlying 
>>> hardware mac
>>> + *       advertised at index 0 in wiphy @hw_chans array.
>>> + *    hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>>> + *    channels = 1, max = 3
>>> + *    => allows a STA plus two AP interfaces on the underlying 
>>> hardware mac
>>> + *       advertised at index 1 in wiphy @hw_chans array.
>>
>> Have you checked the rst output for this? Seems likely that's not going
>> to be great with that formatting, but I haven't checked.
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying 
>>> the index
>>> + *    to the wiphy @hw_chans list for which the iface combination is 
>>> being
>>> + *    described.
>>
>> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
>> some attribute
>>
>> but why didn't you just _say_ in the patch 2 discussion that it's used
>> here ...
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
>>> containing the
>>> + *    limits for the given interface types, see
>>> + *    &enum nl80211_iface_limit_attrs.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
>>> maximum
>>> + *    number of interfaces that can be created in this group. This 
>>> number
>>> + *    does not apply to the interfaces purely managed in software.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
>>> specifying the
>>> + *    number of different channels that can be used in this group.
>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>> + */
>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>
>> Almost all these attributes duplicate - including their docs -
>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>> the different uses of the attribute set?
>>
> 
> I agree, more duplication. So we have to have the per_hw_comb_attrs
> defines like below?
> 
> enum nl80211_if_comb_per_hw_comb_attrs {
>      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>      /* keep last */
>      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

@johannes Berg


Any comments on the vasanth suggested approach ?


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Johannes Berg]

On Wed, 2024-04-10 at 09:40 +0530, Karthikeyan Periyasamy wrote:
> > I agree, more duplication. So we have to have the per_hw_comb_attrs
> > defines like below?
> > 
> > enum nl80211_if_comb_per_hw_comb_attrs {
> >      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> >      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
> >              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
> >      /* keep last */
> >      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
> >      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
> >              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> > };
> > 
> 
> @johannes Berg
> 
> 
> Any comments on the vasanth suggested approach ?

Think about it, do you have any comments? I mean, you've _already_ sent
an email, why not actually say something about the topic?

johannes

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Karthikeyan Periyasamy]

On 3/29/2024 8:04 PM, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 3/28/2024 7:03 PM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> + *    When describing per-hardware combinations in multi-hardware in
>>> + *    one wiphy model, the first possibility can further include the 
>>> finer
>>> + *    capabilities like below
>>
>> Not sure I'd say "below" rather than e.g. "like this:"
>>
>>> + *    hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>>> + *    channels = 1, max = 2
>>> + *    => allows a STA plus an AP interface on the underlying 
>>> hardware mac
>>> + *       advertised at index 0 in wiphy @hw_chans array.
>>> + *    hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>>> + *    channels = 1, max = 3
>>> + *    => allows a STA plus two AP interfaces on the underlying 
>>> hardware mac
>>> + *       advertised at index 1 in wiphy @hw_chans array.
>>
>> Have you checked the rst output for this? Seems likely that's not going
>> to be great with that formatting, but I haven't checked.
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying 
>>> the index
>>> + *    to the wiphy @hw_chans list for which the iface combination is 
>>> being
>>> + *    described.
>>
>> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
>> some attribute
>>
>> but why didn't you just _say_ in the patch 2 discussion that it's used
>> here ...
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
>>> containing the
>>> + *    limits for the given interface types, see
>>> + *    &enum nl80211_iface_limit_attrs.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
>>> maximum
>>> + *    number of interfaces that can be created in this group. This 
>>> number
>>> + *    does not apply to the interfaces purely managed in software.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
>>> specifying the
>>> + *    number of different channels that can be used in this group.
>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>> + */
>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>
>> Almost all these attributes duplicate - including their docs -
>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>> the different uses of the attribute set?
>>
> 
> I agree, more duplication. So we have to have the per_hw_comb_attrs
> defines like below?
> 
> enum nl80211_if_comb_per_hw_comb_attrs {
>      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>      /* keep last */
>      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB, 
shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute 
value will not get conflict to any IFACE combination attributes

  enum nl80211_if_comb_per_hw_comb_attrs {
       NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
       NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
               MAX_NL80211_IFACE_COMB + 1,
       /* keep last */
       NUM_NL80211_IFACE_COMB_PER_HW_COMB,
       MAX_NL80211_IFACE_COMB_PER_HW_COMB =
               NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
};


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Karthikeyan Periyasamy]

On 4/12/2024 10:57 AM, Karthikeyan Periyasamy wrote:
> 
> 
> On 3/29/2024 8:04 PM, Vasanthakumar Thiagarajan wrote:
>>
>>
>> On 3/28/2024 7:03 PM, Johannes Berg wrote:
>>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>>
>>>> + *    When describing per-hardware combinations in multi-hardware in
>>>> + *    one wiphy model, the first possibility can further include 
>>>> the finer
>>>> + *    capabilities like below
>>>
>>> Not sure I'd say "below" rather than e.g. "like this:"
>>>
>>>> + *    hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>>>> + *    channels = 1, max = 2
>>>> + *    => allows a STA plus an AP interface on the underlying 
>>>> hardware mac
>>>> + *       advertised at index 0 in wiphy @hw_chans array.
>>>> + *    hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>>>> + *    channels = 1, max = 3
>>>> + *    => allows a STA plus two AP interfaces on the underlying 
>>>> hardware mac
>>>> + *       advertised at index 1 in wiphy @hw_chans array.
>>>
>>> Have you checked the rst output for this? Seems likely that's not going
>>> to be great with that formatting, but I haven't checked.
>>>
>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying 
>>>> the index
>>>> + *    to the wiphy @hw_chans list for which the iface combination 
>>>> is being
>>>> + *    described.
>>>
>>> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
>>> some attribute
>>>
>>> but why didn't you just _say_ in the patch 2 discussion that it's used
>>> here ...
>>>
>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
>>>> containing the
>>>> + *    limits for the given interface types, see
>>>> + *    &enum nl80211_iface_limit_attrs.
>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving 
>>>> the maximum
>>>> + *    number of interfaces that can be created in this group. This 
>>>> number
>>>> + *    does not apply to the interfaces purely managed in software.
>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
>>>> specifying the
>>>> + *    number of different channels that can be used in this group.
>>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>>> + */
>>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>>
>>> Almost all these attributes duplicate - including their docs -
>>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>>> the different uses of the attribute set?
>>>
>>
>> I agree, more duplication. So we have to have the per_hw_comb_attrs
>> defines like below?
>>
>> enum nl80211_if_comb_per_hw_comb_attrs {
>>      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>>              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>>      /* keep last */
>>      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>>      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>>              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
>> };
>>
> 

Corrected typo

  I agree this approach. Instead of NL80211_IFACE_COMB_NUM_CHANNELS,
  shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute
  value will not get conflict to any IFACE combination attributes

    enum nl80211_if_comb_per_hw_comb_attrs {
         NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
         NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
                 MAX_NL80211_IFACE_COMB + 1,
         /* keep last */
         NUM_NL80211_IFACE_COMB_PER_HW_COMB,
         MAX_NL80211_IFACE_COMB_PER_HW_COMB =
                 NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
  };



-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Johannes Berg]

On Fri, 2024-04-12 at 10:57 +0530, Karthikeyan Periyasamy wrote:
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
> > > > containing the
> > > > + *    limits for the given interface types, see
> > > > + *    &enum nl80211_iface_limit_attrs.
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
> > > > maximum
> > > > + *    number of interfaces that can be created in this group. This 
> > > > number
> > > > + *    does not apply to the interfaces purely managed in software.
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
> > > > specifying the
> > > > + *    number of different channels that can be used in this group.
> > > > + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
> > > > + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
> > > > + */
> > > > +enum nl80211_if_comb_per_hw_comb_attrs {
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
> > > 
> > > Almost all these attributes duplicate - including their docs -
> > > attributes from enum nl80211_if_combination_attrs. Is it really worth
> > > doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
> > > the different uses of the attribute set?
> > > 
> > 
> > I agree, more duplication. So we have to have the per_hw_comb_attrs
> > defines like below?
> > 
> > enum nl80211_if_comb_per_hw_comb_attrs {
> >      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> >      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
> >              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
> >      /* keep last */
> >      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
> >      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
> >              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> > };
> > 
> 
> I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB, 
> shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute 
> value will not get conflict to any IFACE combination attributes
> 
>   enum nl80211_if_comb_per_hw_comb_attrs {
>        NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>        NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>                MAX_NL80211_IFACE_COMB + 1,
>        /* keep last */
>        NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>        MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>                NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

You haven't thought this through - what happens here if something is
added to enum nl80211_if_combination_attrs?

johannes

Re: [PATCH 06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Karthikeyan Periyasamy]

On 4/15/2024 7:57 PM, Johannes Berg wrote:
> On Fri, 2024-04-12 at 10:57 +0530, Karthikeyan Periyasamy wrote:
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute
>>>>> containing the
>>>>> + *    limits for the given interface types, see
>>>>> + *    &enum nl80211_iface_limit_attrs.
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the
>>>>> maximum
>>>>> + *    number of interfaces that can be created in this group. This
>>>>> number
>>>>> + *    does not apply to the interfaces purely managed in software.
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute
>>>>> specifying the
>>>>> + *    number of different channels that can be used in this group.
>>>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>>>> + */
>>>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>>>
>>>> Almost all these attributes duplicate - including their docs -
>>>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>>>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>>>> the different uses of the attribute set?
>>>>
>>>
>>> I agree, more duplication. So we have to have the per_hw_comb_attrs
>>> defines like below?
>>>
>>> enum nl80211_if_comb_per_hw_comb_attrs {
>>>       NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>>       NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>>>               NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>>>       /* keep last */
>>>       NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>>>       MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>>>               NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
>>> };
>>>
>>
>> I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>> shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute
>> value will not get conflict to any IFACE combination attributes
>>
>>    enum nl80211_if_comb_per_hw_comb_attrs {
>>         NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>         NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>>                 MAX_NL80211_IFACE_COMB + 1,
>>         /* keep last */
>>         NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>>         MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>>                 NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
>> };
>>
> 
> You haven't thought this through - what happens here if something is
> added to enum nl80211_if_combination_attrs?
> 

Yeah, it break backward compatibility.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 07/13] wifi: cfg80211/mac80211: Refactor iface comb iterate callback for multi-hardware dev

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Currently, interface combination iterate callback gets notified for each
matching combination. To support multi-physical hardware interface
combination advertisements, the callback subscriber should additionally
receive the hardware index information to indicate which physical hardware
is chosen in the matching combination. Refactor the iterate callback by
introducing an argument for passing the hardware index. This hardware
index is valid for the multi-hardware advertisements and invalid (-1) for
the single hardware combination advertisement.

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 | 2 +-
 net/mac80211/util.c    | 2 +-
 net/wireless/util.c    | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 491d074fe430..246a8c07becf 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -9233,7 +9233,7 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
 int cfg80211_iter_combinations(struct wiphy *wiphy,
 			       struct iface_combination_params *params,
 			       void (*iter)(const struct ieee80211_iface_combination *c,
-					    void *data),
+					    int hw_chan_idx, void *data),
 			       void *data);
 
 /*
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index cda398d8f60d..b1f3b1eb053d 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -4014,7 +4014,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 
 static void
 ieee80211_iter_max_chans(const struct ieee80211_iface_combination *c,
-			 void *data)
+			 int hw_chan_idx, void *data)
 {
 	u32 *max_num_different_channels = data;
 
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2bde8a354631..b60a6a6da744 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2363,13 +2363,13 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 int cfg80211_iter_combinations(struct wiphy *wiphy,
 			       struct iface_combination_params *params,
 			       void (*iter)(const struct ieee80211_iface_combination *c,
-					    void *data),
+					    int hw_chan_idx, void *data),
 			       void *data)
 {
 	const struct ieee80211_regdomain *regdom;
 	enum nl80211_dfs_regions region = 0;
 	int i, j, iftype;
-	int num_interfaces = 0;
+	int num_interfaces = 0, hw_chan_idx = -1;
 	u32 used_iftypes = 0;
 	u32 beacon_int_gcd;
 	bool beacon_int_different;
@@ -2460,7 +2460,7 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		 * supported the requested numbers, so we're good.
 		 */
 
-		(*iter)(c, data);
+		(*iter)(c, hw_chan_idx, data);
  cont:
 		kfree(limits);
 	}
@@ -2471,7 +2471,7 @@ EXPORT_SYMBOL(cfg80211_iter_combinations);
 
 static void
 cfg80211_iter_sum_ifcombs(const struct ieee80211_iface_combination *c,
-			  void *data)
+			  int hw_chan_idx, void *data)
 {
 	int *num = data;
 	(*num)++;
-- 
2.34.1

Re: [PATCH 07/13] wifi: cfg80211/mac80211: Refactor iface comb iterate callback for multi-hardware dev

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
> +	int num_interfaces = 0, hw_chan_idx = -1;

No point in that new variable, just statically pass -1?

And this really is only a cfg80211 patch, with the API updates
propagated where needed, but we don't list all that in the subject.

johannes

[PATCH 08/13] wifi: cfg80211: Refactor the iface combination iteration helper function

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Currently, the iteration helper function retrieves iface types information
from the iface combination parameter and performs iface combination limit
validation. To accommodate per physical hardware iface combinations, the
aforementioned validation and information retrieval need to be executed
for each physical hardware instance. Consequently, relocate the validation
and retrieval operations to a new helper function and scale it to support
per physical hardware iface combinations.

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>
---
 net/wireless/util.c | 135 ++++++++++++++++++++++++++++++++------------
 1 file changed, 98 insertions(+), 37 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index b60a6a6da744..39358b69dd36 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2360,6 +2360,84 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
+static int
+cfg80211_validate_iface_limits(struct wiphy *wiphy,
+			       const int iftype_num[NUM_NL80211_IFTYPES],
+			       struct ieee80211_iface_limit *limits,
+			       u8 n_limits,
+			       u32 *all_iftypes)
+{
+	enum nl80211_iftype iftype;
+	int i;
+
+	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
+		if (cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
+			continue;
+
+		for (i = 0; i < n_limits; i++) {
+			*all_iftypes |= limits[i].types;
+
+			if (!(limits[i].types & BIT(iftype)))
+				continue;
+
+			if (limits[i].max < iftype_num[iftype])
+				return -EINVAL;
+
+			limits[i].max -= iftype_num[iftype];
+		}
+	}
+
+	return 0;
+}
+
+static int
+cfg80211_validate_iface_comb_limits(struct wiphy *wiphy,
+				    struct iface_combination_params *params,
+				    const struct ieee80211_iface_combination *c,
+				    u16 num_interfaces, u32 *all_iftypes)
+{
+	struct ieee80211_iface_limit *limits;
+	int ret = 0;
+
+	if (num_interfaces > c->max_interfaces)
+		return -EINVAL;
+
+	if (params->num_different_channels > c->num_different_channels)
+		return -EINVAL;
+
+	limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
+			 GFP_KERNEL);
+	if (!limits)
+		return -ENOMEM;
+
+	ret = cfg80211_validate_iface_limits(wiphy,
+					     params->iftype_num,
+					     limits,
+					     c->n_limits,
+					     all_iftypes);
+
+	kfree(limits);
+
+	return ret;
+}
+
+static u16 cfg80211_get_iftype_info(struct wiphy *wiphy,
+				    const int iftype_num[NUM_NL80211_IFTYPES],
+				    u32 *used_iftypes)
+{
+	enum nl80211_iftype iftype;
+	u16 num_interfaces = 0;
+
+	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
+		num_interfaces += iftype_num[iftype];
+		if (iftype_num[iftype] > 0 &&
+		    !cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
+			*used_iftypes |= BIT(iftype);
+	}
+
+	return num_interfaces;
+}
+
 int cfg80211_iter_combinations(struct wiphy *wiphy,
 			       struct iface_combination_params *params,
 			       void (*iter)(const struct ieee80211_iface_combination *c,
@@ -2368,11 +2446,13 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 {
 	const struct ieee80211_regdomain *regdom;
 	enum nl80211_dfs_regions region = 0;
-	int i, j, iftype;
-	int num_interfaces = 0, hw_chan_idx = -1;
+	int i;
+	int hw_chan_idx = -1;
 	u32 used_iftypes = 0;
 	u32 beacon_int_gcd;
+	u16 num_interfaces = 0;
 	bool beacon_int_different;
+	int ret = 0;
 
 	/*
 	 * This is a bit strange, since the iteration used to rely only on
@@ -2395,50 +2475,33 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		rcu_read_unlock();
 	}
 
-	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
-		num_interfaces += params->iftype_num[iftype];
-		if (params->iftype_num[iftype] > 0 &&
-		    !cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
-			used_iftypes |= BIT(iftype);
-	}
+	num_interfaces = cfg80211_get_iftype_info(wiphy,
+						  params->iftype_num,
+						  &used_iftypes);
 
 	for (i = 0; i < wiphy->n_iface_combinations; i++) {
 		const struct ieee80211_iface_combination *c;
-		struct ieee80211_iface_limit *limits;
 		u32 all_iftypes = 0;
 
 		c = &wiphy->iface_combinations[i];
 
-		if (num_interfaces > c->max_interfaces)
-			continue;
-		if (params->num_different_channels > c->num_different_channels)
+		ret = cfg80211_validate_iface_comb_limits(wiphy, params,
+							  c, num_interfaces,
+							  &all_iftypes);
+		if (ret == -ENOMEM) {
+			break;
+		} else if (ret) {
+			ret = 0;
 			continue;
-
-		limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
-				 GFP_KERNEL);
-		if (!limits)
-			return -ENOMEM;
-
-		for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
-			if (cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
-				continue;
-			for (j = 0; j < c->n_limits; j++) {
-				all_iftypes |= limits[j].types;
-				if (!(limits[j].types & BIT(iftype)))
-					continue;
-				if (limits[j].max < params->iftype_num[iftype])
-					goto cont;
-				limits[j].max -= params->iftype_num[iftype];
-			}
 		}
 
 		if (params->radar_detect !=
 			(c->radar_detect_widths & params->radar_detect))
-			goto cont;
+			continue;
 
 		if (params->radar_detect && c->radar_detect_regions &&
 		    !(c->radar_detect_regions & BIT(region)))
-			goto cont;
+			continue;
 
 		/* Finally check that all iftypes that we're currently
 		 * using are actually part of this combination. If they
@@ -2446,14 +2509,14 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		 * to continue to the next.
 		 */
 		if ((all_iftypes & used_iftypes) != used_iftypes)
-			goto cont;
+			continue;
 
 		if (beacon_int_gcd) {
 			if (c->beacon_int_min_gcd &&
 			    beacon_int_gcd < c->beacon_int_min_gcd)
-				goto cont;
+				continue;
 			if (!c->beacon_int_min_gcd && beacon_int_different)
-				goto cont;
+				continue;
 		}
 
 		/* This combination covered all interface types and
@@ -2461,11 +2524,9 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		 */
 
 		(*iter)(c, hw_chan_idx, data);
- cont:
-		kfree(limits);
 	}
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(cfg80211_iter_combinations);
 
-- 
2.34.1

Re: [PATCH 08/13] wifi: cfg80211: Refactor the iface combination iteration helper function

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
> +static int
> +cfg80211_validate_iface_comb_limits(struct wiphy *wiphy,
> +				    struct iface_combination_params *params,
> +				    const struct ieee80211_iface_combination *c,
> +				    u16 num_interfaces, u32 *all_iftypes)
> +{
> +	struct ieee80211_iface_limit *limits;
> +	int ret = 0;

That initialization is useless.

> +
> +	if (num_interfaces > c->max_interfaces)
> +		return -EINVAL;
> +
> +	if (params->num_different_channels > c->num_different_channels)
> +		return -EINVAL;
> +
> +	limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
> +			 GFP_KERNEL);
> +	if (!limits)
> +		return -ENOMEM;
> +
> +	ret = cfg80211_validate_iface_limits(wiphy,
> +					     params->iftype_num,
> +					     limits,
> +					     c->n_limits,
> +					     all_iftypes);
> +
> +	kfree(limits);
> +
> +	return ret;
> +}
> +
> +static u16 cfg80211_get_iftype_info(struct wiphy *wiphy,
> +				    const int iftype_num[NUM_NL80211_IFTYPES],
> +				    u32 *used_iftypes)
> +{
> +	enum nl80211_iftype iftype;
> +	u16 num_interfaces = 0;
> +

This should probably set *used_iftypes = 0.

> +	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
> +		num_interfaces += iftype_num[iftype];
> +		if (iftype_num[iftype] > 0 &&
> +		    !cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
> +			*used_iftypes |= BIT(iftype);

and that could really use a rewrite like

		if (!iftype_num[iftype])
			continue;

		num_interfaces += ...

		if (!allowed...)
			*used_iftypes |= ...;

I'd say.

>  	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>  		const struct ieee80211_iface_combination *c;
> -		struct ieee80211_iface_limit *limits;
>  		u32 all_iftypes = 0;
>  
>  		c = &wiphy->iface_combinations[i];
>  
> -		if (num_interfaces > c->max_interfaces)
> -			continue;
> -		if (params->num_different_channels > c->num_different_channels)
> +		ret = cfg80211_validate_iface_comb_limits(wiphy, params,
> +							  c, num_interfaces,
> +							  &all_iftypes);
> +		if (ret == -ENOMEM) {
> +			break;
> +		} else if (ret) {
> +			ret = 0;
>  			continue;

Seems that 'break' is equivalent to just 'return ret'? And that setting
ret = 0 seems ... strange.

> -	return 0;
> +	return ret;
>  }

And then you don't need that either which is much nicer anyway...

johannes

Re: [PATCH 08/13] wifi: cfg80211: Refactor the iface combination iteration helper function

[Karthikeyan Periyasamy]

On 3/28/2024 7:13 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>
>> +static int
>> +cfg80211_validate_iface_comb_limits(struct wiphy *wiphy,
>> +				    struct iface_combination_params *params,
>> +				    const struct ieee80211_iface_combination *c,
>> +				    u16 num_interfaces, u32 *all_iftypes)
>> +{
>> +	struct ieee80211_iface_limit *limits;
>> +	int ret = 0;
> 
> That initialization is useless.

sure

> 
>> +
>> +	if (num_interfaces > c->max_interfaces)
>> +		return -EINVAL;
>> +
>> +	if (params->num_different_channels > c->num_different_channels)
>> +		return -EINVAL;
>> +
>> +	limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
>> +			 GFP_KERNEL);
>> +	if (!limits)
>> +		return -ENOMEM;
>> +
>> +	ret = cfg80211_validate_iface_limits(wiphy,
>> +					     params->iftype_num,
>> +					     limits,
>> +					     c->n_limits,
>> +					     all_iftypes);
>> +
>> +	kfree(limits);
>> +
>> +	return ret;
>> +}
>> +
>> +static u16 cfg80211_get_iftype_info(struct wiphy *wiphy,
>> +				    const int iftype_num[NUM_NL80211_IFTYPES],
>> +				    u32 *used_iftypes)
>> +{
>> +	enum nl80211_iftype iftype;
>> +	u16 num_interfaces = 0;
>> +
> 
> This should probably set *used_iftypes = 0.

sure

> 
>> +	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
>> +		num_interfaces += iftype_num[iftype];
>> +		if (iftype_num[iftype] > 0 &&
>> +		    !cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
>> +			*used_iftypes |= BIT(iftype);
> 
> and that could really use a rewrite like
> 
> 		if (!iftype_num[iftype])
> 			continue;
> 
> 		num_interfaces += ...
> 
> 		if (!allowed...)
> 			*used_iftypes |= ...;
> 
> I'd say.
> 
>>   	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>>   		const struct ieee80211_iface_combination *c;
>> -		struct ieee80211_iface_limit *limits;
>>   		u32 all_iftypes = 0;
>>   
>>   		c = &wiphy->iface_combinations[i];
>>   
>> -		if (num_interfaces > c->max_interfaces)
>> -			continue;
>> -		if (params->num_different_channels > c->num_different_channels)
>> +		ret = cfg80211_validate_iface_comb_limits(wiphy, params,
>> +							  c, num_interfaces,
>> +							  &all_iftypes);
>> +		if (ret == -ENOMEM) {
>> +			break;
>> +		} else if (ret) {
>> +			ret = 0;
>>   			continue;
> 
> Seems that 'break' is equivalent to just 'return ret'? And that setting
> ret = 0 seems ... strange.
> 

sure, will address above comments in the next version of the patch

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 09/13] wifi: cfg80211: Add multi-hardware iface combination support

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Currently, the interface combination parameter supports a single physical
hardware configuration. To support multiple physical hardware interface
combination, add per hardware configuration parameters
(like num_different_channels, iftype_num[NUM_NL80211_IFTYPES]) and channel
definition for which the interface combination will be checked. Modify the
iface combination iterate helper function to retrieve the per hardware
parameters and validate the iface combination limits. For the per hardware
parameters, retrieve the respective hardware index from the channel
definition and pass them to the iterator callback.

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 |  53 +++++++++++
 net/wireless/util.c    | 196 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 241 insertions(+), 8 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 246a8c07becf..8668b877fc3a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1585,6 +1585,23 @@ struct cfg80211_color_change_settings {
 	u8 color;
 };
 
+/**
+ * struct iface_comb_per_hw_params - HW specific interface combinations input
+ *
+ * Used to pass per-hw interface combination parameters
+ *
+ * @num_different_channels: the number of different channels we want to use
+ *	with in the per-hw supported channels.
+ * @iftype_num: array with the number of interfaces of each interface
+ *	type. The index is the interface type as specified in &enum
+ *	nl80211_iftype.
+ */
+
+struct iface_comb_per_hw_params {
+	int num_different_channels;
+	int iftype_num[NUM_NL80211_IFTYPES];
+};
+
 /**
  * struct iface_combination_params - input parameters for interface combinations
  *
@@ -1601,12 +1618,27 @@ struct cfg80211_color_change_settings {
  * @new_beacon_int: set this to the beacon interval of a new interface
  *	that's not operating yet, if such is to be checked as part of
  *	the verification
+ * @chandef: Channel definition for which the interface combination is to be
+ *	checked, when checking during interface preparation on a new channel,
+ *	for example. This will be used when the driver advertises underlying
+ *	hw specific interface combination in a multi physical hardware device.
+ *	This will be NULL when the interface combination check is not due to
+ *	channel or the interface combination does not include per-hw
+ *	advertisement.
+ * @n_per_hw: number of Per-HW interface combinations.
+ * @per_hw: @n_per_hw of hw specific interface combinations. Per-hw channel
+ *	list index as advertised in wiphy @hw_chans is used as index
+ *	in @per_hw to maintain the interface combination of the corresponding
+ *	hw.
  */
 struct iface_combination_params {
 	int num_different_channels;
 	u8 radar_detect;
 	int iftype_num[NUM_NL80211_IFTYPES];
 	u32 new_beacon_int;
+	const struct cfg80211_chan_def *chandef;
+	u8 n_per_hw;
+	const struct iface_comb_per_hw_params *per_hw;
 };
 
 /**
@@ -9236,6 +9268,27 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 					    int hw_chan_idx, void *data),
 			       void *data);
 
+/**
+ * cfg80211_per_hw_iface_comb_advertised - if per-hw iface combination supported
+ *
+ * @wiphy: the wiphy
+ *
+ * This function is used to check underlying per-hw interface combination is
+ * advertised by the driver.
+ */
+bool cfg80211_per_hw_iface_comb_advertised(struct wiphy *wiphy);
+
+/**
+ * cfg80211_get_hw_idx_by_chan - get the hw index by the channel
+ *
+ * @wiphy: the wiphy
+ * @chan: channel for which the supported hw index is required
+ *
+ * returns -1 in case the channel is not supported by any of the constituent hw
+ */
+int cfg80211_get_hw_idx_by_chan(struct wiphy *wiphy,
+				const struct ieee80211_channel *chan);
+
 /*
  * cfg80211_stop_iface - trigger interface disconnection
  *
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 39358b69dd36..635fd2637b73 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2390,6 +2390,78 @@ cfg80211_validate_iface_limits(struct wiphy *wiphy,
 	return 0;
 }
 
+static const struct ieee80211_iface_per_hw *
+cfg80211_get_hw_iface_comb_by_idx(struct wiphy *wiphy,
+				  const struct ieee80211_iface_combination *c,
+				  int idx)
+{
+	int i;
+
+	for (i = 0; i < c->n_hw_list; i++)
+		if (c->iface_hw_list[i].hw_chans_idx == idx)
+			break;
+
+	if (i == c->n_hw_list)
+		return NULL;
+
+	return &c->iface_hw_list[i];
+}
+
+static int
+cfg80211_validate_iface_comb_per_hw_limits(struct wiphy *wiphy,
+					   struct iface_combination_params *params,
+					   const struct ieee80211_iface_combination *c,
+					   u16 *num_ifaces, u32 *all_iftypes)
+{
+	struct ieee80211_iface_limit *limits;
+	const struct iface_comb_per_hw_params *per_hw;
+	const struct ieee80211_iface_per_hw *per_hw_comb;
+	int i, ret = 0;
+
+	for (i = 0; i < params->n_per_hw; i++) {
+		per_hw = &params->per_hw[i];
+
+		per_hw_comb = cfg80211_get_hw_iface_comb_by_idx(wiphy, c, i);
+		if (!per_hw_comb) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (num_ifaces[i] > per_hw_comb->max_interfaces) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (per_hw->num_different_channels >
+		    per_hw_comb->num_different_channels) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		limits = kmemdup(per_hw_comb->limits,
+				 sizeof(limits[0]) * per_hw_comb->n_limits,
+				 GFP_KERNEL);
+		if (!limits) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = cfg80211_validate_iface_limits(wiphy,
+						     per_hw->iftype_num,
+						     limits,
+						     per_hw_comb->n_limits,
+						     all_iftypes);
+
+		kfree(limits);
+
+		if (ret)
+			goto out;
+	}
+
+out:
+	return ret;
+}
+
 static int
 cfg80211_validate_iface_comb_limits(struct wiphy *wiphy,
 				    struct iface_combination_params *params,
@@ -2421,6 +2493,23 @@ cfg80211_validate_iface_comb_limits(struct wiphy *wiphy,
 	return ret;
 }
 
+bool cfg80211_per_hw_iface_comb_advertised(struct wiphy *wiphy)
+{
+	int i;
+
+	for (i = 0; i < wiphy->n_iface_combinations; i++)
+		if (wiphy->iface_combinations[i].n_hw_list)
+			return true;
+
+	return false;
+}
+EXPORT_SYMBOL(cfg80211_per_hw_iface_comb_advertised);
+
+static void cfg80211_put_iface_comb_iftypes(u16 *num_ifaces)
+{
+	kfree(num_ifaces);
+}
+
 static u16 cfg80211_get_iftype_info(struct wiphy *wiphy,
 				    const int iftype_num[NUM_NL80211_IFTYPES],
 				    u32 *used_iftypes)
@@ -2438,6 +2527,69 @@ static u16 cfg80211_get_iftype_info(struct wiphy *wiphy,
 	return num_interfaces;
 }
 
+static u16*
+cfg80211_get_iface_comb_iftypes(struct wiphy *wiphy,
+				struct iface_combination_params *params,
+				u32 *used_iftypes)
+{
+	const struct iface_comb_per_hw_params *per_hw;
+	u16 *num_ifaces;
+	int i;
+	u8 num_hw;
+
+	num_hw = params->n_per_hw ? params->n_per_hw : 1;
+
+	num_ifaces = kcalloc(num_hw, sizeof(*num_ifaces), GFP_KERNEL);
+	if (!num_ifaces)
+		return NULL;
+
+	if (!params->n_per_hw) {
+		num_ifaces[0] = cfg80211_get_iftype_info(wiphy,
+							 params->iftype_num,
+							 used_iftypes);
+	} else {
+		/* account per_hw interfaces, if advertised */
+		for (i = 0; i < params->n_per_hw; i++) {
+			per_hw = &params->per_hw[i];
+			num_ifaces[i] = cfg80211_get_iftype_info(wiphy,
+								 per_hw->iftype_num,
+								 used_iftypes);
+		}
+	}
+
+	return num_ifaces;
+}
+
+static bool
+cfg80211_chan_supported_by_sub_hw(const struct ieee80211_chans_per_hw *hw_chans,
+				  const struct ieee80211_channel *chan)
+{
+	int i;
+
+	for (i = 0; i < hw_chans->n_chans; i++)
+		if (chan->center_freq == hw_chans->chans[i].center_freq)
+			return true;
+
+	return false;
+}
+
+int
+cfg80211_get_hw_idx_by_chan(struct wiphy *wiphy,
+			    const struct ieee80211_channel *chan)
+{
+	int i;
+
+	if (!chan)
+		return -1;
+
+	for (i = 0; i < wiphy->num_hw; i++)
+		if (cfg80211_chan_supported_by_sub_hw(wiphy->hw_chans[i], chan))
+			return i;
+
+	return -1;
+}
+EXPORT_SYMBOL(cfg80211_get_hw_idx_by_chan);
+
 int cfg80211_iter_combinations(struct wiphy *wiphy,
 			       struct iface_combination_params *params,
 			       void (*iter)(const struct ieee80211_iface_combination *c,
@@ -2450,8 +2602,8 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 	int hw_chan_idx = -1;
 	u32 used_iftypes = 0;
 	u32 beacon_int_gcd;
-	u16 num_interfaces = 0;
-	bool beacon_int_different;
+	u16 *num_ifaces = NULL;
+	bool beacon_int_different, is_per_hw;
 	int ret = 0;
 
 	/*
@@ -2475,9 +2627,26 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		rcu_read_unlock();
 	}
 
-	num_interfaces = cfg80211_get_iftype_info(wiphy,
-						  params->iftype_num,
-						  &used_iftypes);
+	is_per_hw = cfg80211_per_hw_iface_comb_advertised(wiphy);
+	/* check per HW validation */
+	if (params->n_per_hw) {
+		if (!is_per_hw)
+			return -EINVAL;
+
+		if (params->n_per_hw > wiphy->num_hw)
+			return -EINVAL;
+	}
+
+	if (is_per_hw && params->chandef &&
+	    cfg80211_chandef_valid(params->chandef))
+		hw_chan_idx = cfg80211_get_hw_idx_by_chan(wiphy,
+							  params->chandef->chan);
+
+	num_ifaces = cfg80211_get_iface_comb_iftypes(wiphy,
+						     params,
+						     &used_iftypes);
+	if (!num_ifaces)
+		return -ENOMEM;
 
 	for (i = 0; i < wiphy->n_iface_combinations; i++) {
 		const struct ieee80211_iface_combination *c;
@@ -2485,9 +2654,18 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 
 		c = &wiphy->iface_combinations[i];
 
-		ret = cfg80211_validate_iface_comb_limits(wiphy, params,
-							  c, num_interfaces,
-							  &all_iftypes);
+		if (params->n_per_hw)
+			ret = cfg80211_validate_iface_comb_per_hw_limits(wiphy,
+									 params,
+									 c,
+									 num_ifaces,
+									 &all_iftypes);
+		else
+			ret = cfg80211_validate_iface_comb_limits(wiphy,
+								  params,
+								  c,
+								  num_ifaces[0],
+								  &all_iftypes);
 		if (ret == -ENOMEM) {
 			break;
 		} else if (ret) {
@@ -2526,6 +2704,8 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 		(*iter)(c, hw_chan_idx, data);
 	}
 
+	cfg80211_put_iface_comb_iftypes(num_ifaces);
+
 	return ret;
 }
 EXPORT_SYMBOL(cfg80211_iter_combinations);
-- 
2.34.1

Re: [PATCH 09/13] wifi: cfg80211: Add multi-hardware iface combination support

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
>   * @new_beacon_int: set this to the beacon interval of a new interface
>   *	that's not operating yet, if such is to be checked as part of
>   *	the verification
> + * @chandef: Channel definition for which the interface combination is to be
> + *	checked, when checking during interface preparation on a new channel,
> + *	for example. This will be used when the driver advertises underlying
> + *	hw specific interface combination in a multi physical hardware device.
> + *	This will be NULL when the interface combination check is not due to
> + *	channel or the interface combination does not include per-hw
> + *	advertisement.

This is input, so "will be" doesn't make much sense, more like "must
be"?

But I'm confused as to how that works with num_different_channels being
here too?

This function was, as far as I can tell, always checking the _full_
state. Now you're changing that, and I'm neither sure why, nor does it
seem well documented.

> + * @n_per_hw: number of Per-HW interface combinations.
> + * @per_hw: @n_per_hw of hw specific interface combinations. Per-hw channel
> + *	list index as advertised in wiphy @hw_chans is used as index
> + *	in @per_hw to maintain the interface combination of the corresponding
> + *	hw.

What?

If I'm reading that correctly, which is all but guaranteed, doesn't that
actually mean you don't need n_per_hw at all, since it necessarily equal
to n_hw_chans?

> +/**
> + * cfg80211_per_hw_iface_comb_advertised - if per-hw iface combination supported
> + *
> + * @wiphy: the wiphy
> + *
> + * This function is used to check underlying per-hw interface combination is
> + * advertised by the driver.
> + */
> +bool cfg80211_per_hw_iface_comb_advertised(struct wiphy *wiphy);

Is that even worth an export rather than being inline? Is it even needed
outside of cfg80211 itself?

Also for cfg80211_get_hw_idx_by_chan(), is it really needed?

I'm also wondering if we really should use the "hw_idx" everywhere.
Maybe that'd be more useful as a pointer to struct
ieee80211_chans_per_hw in most places (other than nl80211, obviously)?

The index always feels pretty fragile, a pointer at least gives us type-
checking?

Even in the interface combination advertising, perhaps, though not sure
how that'd work for the drivers.

> +static const struct ieee80211_iface_per_hw *
> +cfg80211_get_hw_iface_comb_by_idx(struct wiphy *wiphy,
> +				  const struct ieee80211_iface_combination *c,
> +				  int idx)
> +{
> +	int i;
> +
> +	for (i = 0; i < c->n_hw_list; i++)
> +		if (c->iface_hw_list[i].hw_chans_idx == idx)
> +			break;
> +
> +	if (i == c->n_hw_list)
> +		return NULL;
> +
> +	return &c->iface_hw_list[i];
> +}

???

Hint: it's perfectly legal to return directly from a loop.

> +static int
> +cfg80211_validate_iface_comb_per_hw_limits(struct wiphy *wiphy,
> +					   struct iface_combination_params *params,
> +					   const struct ieee80211_iface_combination *c,
> +					   u16 *num_ifaces, u32 *all_iftypes)
> +{
> +	struct ieee80211_iface_limit *limits;
> +	const struct iface_comb_per_hw_params *per_hw;
> +	const struct ieee80211_iface_per_hw *per_hw_comb;
> +	int i, ret = 0;

The = 0 doesn't seem needed.

> +		ret = cfg80211_validate_iface_limits(wiphy,
> +						     per_hw->iftype_num,
> +						     limits,
> +						     per_hw_comb->n_limits,
> +						     all_iftypes);
> +
> +		kfree(limits);
> +
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	return ret;

That 'out' label is pointless.

> +static void cfg80211_put_iface_comb_iftypes(u16 *num_ifaces)
> +{
> +	kfree(num_ifaces);
> +}

Not sure I see value in that indirection?

> +static u16*

missing space

> +cfg80211_get_iface_comb_iftypes(struct wiphy *wiphy,
> +				struct iface_combination_params *params,
> +				u32 *used_iftypes)
> +{
> +	const struct iface_comb_per_hw_params *per_hw;
> +	u16 *num_ifaces;
> +	int i;
> +	u8 num_hw;
> +
> +	num_hw = params->n_per_hw ? params->n_per_hw : 1;

I think we're allowed to use the "?:" shortcut.

> +	num_ifaces = kcalloc(num_hw, sizeof(*num_ifaces), GFP_KERNEL);
> +	if (!num_ifaces)
> +		return NULL;

But ... maybe we should just cap num_hw to a reasonable limit (4? 5?)
and use a static array in the caller instead of allocating here.

> +	is_per_hw = cfg80211_per_hw_iface_comb_advertised(wiphy);

Maybe call that "have_per_hw_combinations" or so? Or "check_per_hw"
even, "is" seems not clear - "what is?"

> +	/* check per HW validation */
> +	if (params->n_per_hw) {
> +		if (!is_per_hw)
> +			return -EINVAL;
> +
> +		if (params->n_per_hw > wiphy->num_hw)
> +			return -EINVAL;
> +	}
> +
> +	if (is_per_hw && params->chandef &&
> +	    cfg80211_chandef_valid(params->chandef))
> +		hw_chan_idx = cfg80211_get_hw_idx_by_chan(wiphy,
> +							  params->chandef->chan);
> +
> +	num_ifaces = cfg80211_get_iface_comb_iftypes(wiphy,
> +						     params,
> +						     &used_iftypes);
> +	if (!num_ifaces)
> +		return -ENOMEM;

But still like I said above, all this code seems really odd to me now,
it's checking *either* the per-hw and then only for a single HW, *or*
the global, but ... seems it should do full checks for both, if needed?

johannes

Re: [PATCH 09/13] wifi: cfg80211: Add multi-hardware iface combination support

[Karthikeyan Periyasamy]

On 3/28/2024 7:46 PM, Johannes Berg wrote:

> 
>> +static const struct ieee80211_iface_per_hw *
>> +cfg80211_get_hw_iface_comb_by_idx(struct wiphy *wiphy,
>> +				  const struct ieee80211_iface_combination *c,
>> +				  int idx)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < c->n_hw_list; i++)
>> +		if (c->iface_hw_list[i].hw_chans_idx == idx)
>> +			break;
>> +
>> +	if (i == c->n_hw_list)
>> +		return NULL;
>> +
>> +	return &c->iface_hw_list[i];
>> +}
> 
> ???
> 
> Hint: it's perfectly legal to return directly from a loop.

sure

> 
>> +static int
>> +cfg80211_validate_iface_comb_per_hw_limits(struct wiphy *wiphy,
>> +					   struct iface_combination_params *params,
>> +					   const struct ieee80211_iface_combination *c,
>> +					   u16 *num_ifaces, u32 *all_iftypes)
>> +{
>> +	struct ieee80211_iface_limit *limits;
>> +	const struct iface_comb_per_hw_params *per_hw;
>> +	const struct ieee80211_iface_per_hw *per_hw_comb;
>> +	int i, ret = 0;
> 
> The = 0 doesn't seem needed.

sure

> 
>> +		ret = cfg80211_validate_iface_limits(wiphy,
>> +						     per_hw->iftype_num,
>> +						     limits,
>> +						     per_hw_comb->n_limits,
>> +						     all_iftypes);
>> +
>> +		kfree(limits);
>> +
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +out:
>> +	return ret;
> 
> That 'out' label is pointless.

sure

> 
>> +static void cfg80211_put_iface_comb_iftypes(u16 *num_ifaces)
>> +{
>> +	kfree(num_ifaces);
>> +}
> 
> Not sure I see value in that indirection?

sure

> 
>> +static u16*
> 
> missing space

sure

> 
>> +cfg80211_get_iface_comb_iftypes(struct wiphy *wiphy,
>> +				struct iface_combination_params *params,
>> +				u32 *used_iftypes)
>> +{
>> +	const struct iface_comb_per_hw_params *per_hw;
>> +	u16 *num_ifaces;
>> +	int i;
>> +	u8 num_hw;
>> +
>> +	num_hw = params->n_per_hw ? params->n_per_hw : 1;
> 
> I think we're allowed to use the "?:" shortcut.

sure

> 
>> +	num_ifaces = kcalloc(num_hw, sizeof(*num_ifaces), GFP_KERNEL);
>> +	if (!num_ifaces)
>> +		return NULL;
> 
> But ... maybe we should just cap num_hw to a reasonable limit (4? 5?)
> and use a static array in the caller instead of allocating here.
> 
>> +	is_per_hw = cfg80211_per_hw_iface_comb_advertised(wiphy);
> 
> Maybe call that "have_per_hw_combinations" or so? Or "check_per_hw"
> even, "is" seems not clear - "what is?"

sure, will address all the above comments in the next version of the patch.


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 10/13] wifi: mac80211: expose channel context helper function

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Currently, link specific channel context helper function scope is static.
For multi hardware iface combination support, mac80211 need this helper
function, so expose it internally.

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>
---
 net/mac80211/chan.c        | 2 +-
 net/mac80211/ieee80211_i.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 5a7fb0e4f89d..08a362892d9a 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -67,7 +67,7 @@ static bool ieee80211_can_create_new_chanctx(struct ieee80211_local *local)
 	return ieee80211_num_chanctx(local) < ieee80211_max_num_channels(local);
 }
 
-static struct ieee80211_chanctx *
+struct ieee80211_chanctx *
 ieee80211_link_get_chanctx(struct ieee80211_link_data *link)
 {
 	struct ieee80211_local *local __maybe_unused = link->sdata->local;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index def611e4e55f..202bbffec746 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2577,6 +2577,8 @@ void ieee80211_link_copy_chanctx_to_vlans(struct ieee80211_link_data *link,
 					  bool clear);
 int ieee80211_chanctx_refcount(struct ieee80211_local *local,
 			       struct ieee80211_chanctx *ctx);
+struct ieee80211_chanctx *
+ieee80211_link_get_chanctx(struct ieee80211_link_data *link);
 
 void ieee80211_recalc_smps_chanctx(struct ieee80211_local *local,
 				   struct ieee80211_chanctx *chanctx);
-- 
2.34.1

[PATCH 11/13] wifi: mac80211: Add multi-hardware support in the iface comb helper

[Karthikeyan Periyasamy]

From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

Currently, iface combination param supports single physical hardware.
To support multi physical hardware, add hardware specific checks on the
channel context creation and populate hardware specific parameters from
the channel definition for which the interface combination need to check.

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>
---
 net/mac80211/chan.c        |  33 +++++--
 net/mac80211/ieee80211_i.h |   3 +-
 net/mac80211/util.c        | 194 ++++++++++++++++++++++++++++++++++---
 3 files changed, 207 insertions(+), 23 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 08a362892d9a..0558eafa45aa 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -47,24 +47,43 @@ int ieee80211_chanctx_refcount(struct ieee80211_local *local,
 	       ieee80211_chanctx_num_reserved(local, ctx);
 }
 
-static int ieee80211_num_chanctx(struct ieee80211_local *local)
+static int ieee80211_num_chanctx(struct ieee80211_local *local,
+				 const struct cfg80211_chan_def *chandef)
 {
 	struct ieee80211_chanctx *ctx;
-	int num = 0;
+	int num = 0, hw_idx = -1, ctx_idx;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	list_for_each_entry(ctx, &local->chanctx_list, list)
-		num++;
+	if (chandef && cfg80211_chandef_valid(chandef))
+		hw_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+						     chandef->chan);
+
+	if (hw_idx < 0) {
+		list_for_each_entry(ctx, &local->chanctx_list, list)
+			num++;
+	} else {
+		list_for_each_entry(ctx, &local->chanctx_list, list) {
+			ctx_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+							      ctx->conf.def.chan);
+			if (ctx_idx != hw_idx)
+				continue;
+
+			num++;
+		}
+	}
 
 	return num;
 }
 
-static bool ieee80211_can_create_new_chanctx(struct ieee80211_local *local)
+static
+bool ieee80211_can_create_new_chanctx(struct ieee80211_local *local,
+				      const struct cfg80211_chan_def *chandef)
 {
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	return ieee80211_num_chanctx(local) < ieee80211_max_num_channels(local);
+	return ieee80211_num_chanctx(local, chandef) <
+	       ieee80211_max_num_channels(local, chandef);
 }
 
 struct ieee80211_chanctx *
@@ -1029,7 +1048,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 
 	new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode);
 	if (!new_ctx) {
-		if (ieee80211_can_create_new_chanctx(local)) {
+		if (ieee80211_can_create_new_chanctx(local, &chanreq->oper)) {
 			new_ctx = ieee80211_new_chanctx(local, chanreq, mode);
 			if (IS_ERR(new_ctx))
 				return PTR_ERR(new_ctx);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 202bbffec746..42f4211c622b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2600,7 +2600,8 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 				 const struct cfg80211_chan_def *chandef,
 				 enum ieee80211_chanctx_mode chanmode,
 				 u8 radar_detect);
-int ieee80211_max_num_channels(struct ieee80211_local *local);
+int ieee80211_max_num_channels(struct ieee80211_local *local,
+			       const struct cfg80211_chan_def *chandef);
 void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
 				       struct ieee80211_chanctx *ctx);
 
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index b1f3b1eb053d..f8ffc8ad0cd8 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -3926,6 +3926,44 @@ static u8 ieee80211_chanctx_radar_detect(struct ieee80211_local *local,
 	return radar_detect;
 }
 
+static void
+ieee80211_get_per_hw_sdata_active_iface(struct ieee80211_sub_if_data *sdata,
+					struct iface_comb_per_hw_params *per_hw,
+					int iftype_num[NUM_NL80211_IFTYPES],
+					int *total)
+{
+	struct ieee80211_local *local = sdata->local;
+	unsigned int link_id;
+	int hw_idx;
+
+	for (link_id = 0; link_id < ARRAY_SIZE(sdata->link); link_id++) {
+		struct ieee80211_link_data *link;
+		struct ieee80211_chanctx *ctx;
+
+		link = sdata_dereference(sdata->link[link_id], sdata);
+		if (!link)
+			continue;
+
+		ctx = ieee80211_link_get_chanctx(link);
+		if (ctx &&
+		    ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
+			ctx = ctx->replace_ctx;
+
+		hw_idx = -1;
+		if (ctx && cfg80211_chandef_valid(&ctx->conf.def))
+			hw_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+							     ctx->conf.def.chan);
+
+		if (hw_idx >= 0)
+			per_hw[hw_idx].iftype_num[sdata->wdev.iftype]++;
+		else
+			iftype_num[sdata->wdev.iftype]++;
+
+		if (total)
+			(*total)++;
+	}
+}
+
 int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 				 const struct cfg80211_chan_def *chandef,
 				 enum ieee80211_chanctx_mode chanmode,
@@ -3936,9 +3974,12 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 	enum nl80211_iftype iftype = sdata->wdev.iftype;
 	struct ieee80211_chanctx *ctx;
 	int total = 1;
+	struct iface_comb_per_hw_params *per_hw __free(kfree) = NULL;
 	struct iface_combination_params params = {
 		.radar_detect = radar_detect,
 	};
+	bool is_per_hw;
+	int hchan_idx = -1;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
@@ -3969,26 +4010,68 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 		return 0;
 	}
 
-	if (chandef)
-		params.num_different_channels = 1;
+	is_per_hw = cfg80211_per_hw_iface_comb_advertised(local->hw.wiphy);
+	if (is_per_hw) {
+		per_hw = kcalloc(local->hw.wiphy->num_hw, sizeof(*per_hw),
+				 GFP_KERNEL);
+		if (!per_hw)
+			return -ENOMEM;
+
+		if (chandef && cfg80211_chandef_valid(chandef)) {
+			hchan_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+								chandef->chan);
+			if (hchan_idx < 0)
+				goto skip;
 
-	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
-		params.iftype_num[iftype] = 1;
+			per_hw[hchan_idx].num_different_channels = 1;
 
+			if (iftype != NL80211_IFTYPE_UNSPECIFIED)
+				per_hw[hchan_idx].iftype_num[iftype] = 1;
+		}
+	} else {
+		if (chandef)
+			params.num_different_channels = 1;
+
+		if (iftype != NL80211_IFTYPE_UNSPECIFIED)
+			params.iftype_num[iftype] = 1;
+	}
+
+skip:
 	list_for_each_entry(ctx, &local->chanctx_list, list) {
 		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
 			continue;
+
+		if (is_per_hw) {
+			if (WARN_ON(!cfg80211_chandef_valid(&ctx->conf.def)))
+				continue;
+
+			hchan_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+								ctx->conf.def.chan);
+			if (WARN_ON(hchan_idx < 0))
+				continue;
+		}
+
 		params.radar_detect |=
 			ieee80211_chanctx_radar_detect(local, ctx);
+
 		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) {
-			params.num_different_channels++;
+			if (is_per_hw)
+				per_hw[hchan_idx].num_different_channels++;
+			else
+				params.num_different_channels++;
+
 			continue;
 		}
+
 		if (chandef && chanmode == IEEE80211_CHANCTX_SHARED &&
 		    cfg80211_chandef_compatible(chandef,
 						&ctx->conf.def))
 			continue;
-		params.num_different_channels++;
+
+		if (is_per_hw)
+			per_hw[hchan_idx].num_different_channels++;
+		else
+			params.num_different_channels++;
 	}
 
 	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
@@ -4002,13 +4085,25 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 					    wdev_iter->iftype, 0, 1))
 			continue;
 
-		params.iftype_num[wdev_iter->iftype]++;
-		total++;
+		if (is_per_hw) {
+			ieee80211_get_per_hw_sdata_active_iface(sdata_iter,
+								per_hw,
+								params.iftype_num,
+								&total);
+		} else {
+			params.iftype_num[wdev_iter->iftype]++;
+			total++;
+		}
 	}
 
 	if (total == 1 && !params.radar_detect)
 		return 0;
 
+	if (is_per_hw) {
+		params.n_per_hw = local->hw.wiphy->num_hw;
+		params.per_hw = per_hw;
+	}
+
 	return cfg80211_check_combinations(local->hw.wiphy, &params);
 }
 
@@ -4022,31 +4117,100 @@ ieee80211_iter_max_chans(const struct ieee80211_iface_combination *c,
 					  c->num_different_channels);
 }
 
-int ieee80211_max_num_channels(struct ieee80211_local *local)
+static void
+ieee80211_iter_per_hw_max_chans(const struct ieee80211_iface_combination *c,
+				int hw_chan_idx, void *data)
+{
+	u32 *max_num_diff_chans = data;
+	u32 max_supp_diff_chans = 0;
+	int i;
+
+	for (i = 0; i < c->n_hw_list; i++) {
+		const struct ieee80211_iface_per_hw *h;
+
+		h = &c->iface_hw_list[i];
+		if (hw_chan_idx != -1) {
+			if (h->hw_chans_idx == hw_chan_idx) {
+				max_supp_diff_chans = h->num_different_channels;
+				break;
+			}
+			continue;
+		}
+		max_supp_diff_chans += h->num_different_channels;
+	}
+
+	*max_num_diff_chans = max(*max_num_diff_chans, max_supp_diff_chans);
+}
+
+int ieee80211_max_num_channels(struct ieee80211_local *local,
+			       const struct cfg80211_chan_def *chandef)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_chanctx *ctx;
+	struct iface_comb_per_hw_params *per_hw __free(kfree) = NULL;
+	struct iface_combination_params params = {0};
+	void (*iter)(const struct ieee80211_iface_combination *c,
+		     int hw_chan_idx, void *data) = ieee80211_iter_max_chans;
 	u32 max_num_different_channels = 1;
+	bool is_per_hw;
 	int err;
-	struct iface_combination_params params = {0};
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
+	is_per_hw = cfg80211_per_hw_iface_comb_advertised(local->hw.wiphy);
+	if (is_per_hw) {
+		per_hw = kcalloc(local->hw.wiphy->num_hw,
+				 sizeof(*params.per_hw),
+				 GFP_KERNEL);
+		if (!per_hw)
+			return -ENOMEM;
+
+		params.chandef = chandef;
+		iter = ieee80211_iter_per_hw_max_chans;
+	}
+
 	list_for_each_entry(ctx, &local->chanctx_list, list) {
-		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
+		if (ctx->replace_state ==
+		    IEEE80211_CHANCTX_WILL_BE_REPLACED)
 			continue;
 
-		params.num_different_channels++;
+		if (is_per_hw) {
+			int hw_idx;
+
+			if (WARN_ON(!cfg80211_chandef_valid(&ctx->conf.def)))
+				continue;
+
+			hw_idx = cfg80211_get_hw_idx_by_chan(local->hw.wiphy,
+							     ctx->conf.def.chan);
+			if (WARN_ON(hw_idx < 0))
+				continue;
+
+			per_hw[hw_idx].num_different_channels++;
+		} else {
+			params.num_different_channels++;
+		}
 
 		params.radar_detect |=
 			ieee80211_chanctx_radar_detect(local, ctx);
 	}
 
-	list_for_each_entry_rcu(sdata, &local->interfaces, list)
-		params.iftype_num[sdata->wdev.iftype]++;
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (is_per_hw && ieee80211_sdata_running(sdata))
+			ieee80211_get_per_hw_sdata_active_iface(sdata,
+								per_hw,
+								params.iftype_num,
+								NULL);
+		else
+			params.iftype_num[sdata->wdev.iftype]++;
+	}
+
+	if (is_per_hw) {
+		params.n_per_hw = local->hw.wiphy->num_hw;
+		params.per_hw = per_hw;
+	}
 
 	err = cfg80211_iter_combinations(local->hw.wiphy, &params,
-					 ieee80211_iter_max_chans,
+					 iter,
 					 &max_num_different_channels);
 	if (err < 0)
 		return err;
-- 
2.34.1

Re: [PATCH 11/13] wifi: mac80211: Add multi-hardware support in the iface comb helper

[Johannes Berg]

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
> 
> Currently, iface combination param supports single physical hardware.
> To support multi physical hardware, add hardware specific checks on the
> channel context creation and populate hardware specific parameters from
> the channel definition for which the interface combination need to check.

I haven't gone through this patch in detail, but again like on patch 9,
I'm not so sure about the implementation of the XOR behaviour of
checking global and per-HW limitations.

I probably wrote on the other patch that it seems it should check both,
but I realize now that doesn't make sense: After all, the top-level
combinations data must encode something that's supported _regardless_ of
channels, so likely only a subset of the combinations supported across
the different HW.

But that doesn't mean that I'm yet convinced that the design in patch 9
is actually good, with how it checks that depending on the 'chandef'
parameter. I'd like to explore other possibilities such as a different
function for that, for example. It could use the same underlying helpers
and mechanics, but it seems that might be better overall.

Or probably better yet: explore an approach where mac80211 doesn't even
have to _know_ about the cfg80211_get_hw_idx_by_chan() and all that, but
just prepares the information in a way that encodes enough data to
handle that, which really means going from

 int num_different_channels;
 int iftype_num[...];

to

 struct {
   enum nl80211_iftype iftype;
   u32 freq;
 } interfaces[];

or something like that.


I was almost going to write "links" instead of "interfaces" there, which
reminds me that none of this really even works well for MLO yet. Not
sure if that's better addressed as a separate first step?

johannes

Re: [PATCH 11/13] wifi: mac80211: Add multi-hardware support in the iface comb helper

[Jeff Johnson]

On 3/28/2024 7:41 AM, Johannes Berg wrote:
> I was almost going to write "links" instead of "interfaces" there, which
> reminds me that none of this really even works well for MLO yet. Not
> sure if that's better addressed as a separate first step?

we are following the sequence:
1) go from per-hw wiphy to wiphy that supports multi-hw (multiple patchsets
under review)
2) add hw-to-wiphy grouping infrastructure (hopefully coming soon)
3) add MLO on top of the grouping infrastructure

So we consider this to be part of the 1st phase

Re: [PATCH 11/13] wifi: mac80211: Add multi-hardware support in the iface comb helper

[Karthikeyan Periyasamy]

On 3/28/2024 8:11 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>> From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
>>
>> Currently, iface combination param supports single physical hardware.
>> To support multi physical hardware, add hardware specific checks on the
>> channel context creation and populate hardware specific parameters from
>> the channel definition for which the interface combination need to check.
> 
> I haven't gone through this patch in detail, but again like on patch 9,
> I'm not so sure about the implementation of the XOR behaviour of
> checking global and per-HW limitations.
> 
> I probably wrote on the other patch that it seems it should check both,
> but I realize now that doesn't make sense: After all, the top-level
> combinations data must encode something that's supported _regardless_ of
> channels, so likely only a subset of the combinations supported across
> the different HW.
> 
> But that doesn't mean that I'm yet convinced that the design in patch 9
> is actually good, with how it checks that depending on the 'chandef'
> parameter. I'd like to explore other possibilities such as a different
> function for that, for example. It could use the same underlying helpers
> and mechanics, but it seems that might be better overall.
> 
> Or probably better yet: explore an approach where mac80211 doesn't even
> have to _know_ about the cfg80211_get_hw_idx_by_chan() and all that, but
> just prepares the information in a way that encodes enough data to
> handle that, which really means going from
> 
>   int num_different_channels;
>   int iftype_num[...];
> 
> to
> 
>   struct {
>     enum nl80211_iftype iftype;
>     u32 freq;
>   } interfaces[];
> 
> or something like that.
> 
> 
> I was almost going to write "links" instead of "interfaces" there, which
> reminds me that none of this really even works well for MLO yet. Not
> sure if that's better addressed as a separate first step?
> 

Sure, will post this change as a separate RFC patch.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

[PATCH 12/13] wifi: ath12k: Introduce iface combination cleanup helper

[Karthikeyan Periyasamy]

Introduce a cleanup helper function to avoid redundant code for iface
combination cleanup. Remove the cleanup code from
ath12k_mac_hw_unregister() and ath12k_mac_hw_register() and replace it
with new cleanup helper function.

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

Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 52a5fb8b03e9..44c8bf6eb6ae 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7781,6 +7781,14 @@ static bool ath12k_mac_is_iface_mode_enable(struct ath12k_hw *ah,
 	return is_enable;
 }
 
+static void ath12k_mac_cleanup_iface_combinations(struct ath12k_hw *ah)
+{
+	struct wiphy *wiphy = ah->hw->wiphy;
+
+	kfree(wiphy->iface_combinations[0].limits);
+	kfree(wiphy->iface_combinations);
+}
+
 static int ath12k_mac_setup_iface_combinations(struct ath12k_hw *ah)
 {
 	struct wiphy *wiphy = ah->hw->wiphy;
@@ -7905,7 +7913,6 @@ static void ath12k_mac_cleanup_unregister(struct ath12k *ar)
 static void ath12k_mac_hw_unregister(struct ath12k_hw *ah)
 {
 	struct ieee80211_hw *hw = ah->hw;
-	struct wiphy *wiphy = hw->wiphy;
 	struct ath12k *ar = ath12k_ah_to_ar(ah);
 
 	cancel_work_sync(&ar->regd_update_work);
@@ -7914,8 +7921,7 @@ static void ath12k_mac_hw_unregister(struct ath12k_hw *ah)
 
 	ath12k_mac_cleanup_unregister(ar);
 
-	kfree(wiphy->iface_combinations[0].limits);
-	kfree(wiphy->iface_combinations);
+	ath12k_mac_cleanup_iface_combinations(ah);
 
 	SET_IEEE80211_DEV(hw, NULL);
 }
@@ -8087,7 +8093,7 @@ static int ath12k_mac_hw_register(struct ath12k_hw *ah)
 	ret = ieee80211_register_hw(hw);
 	if (ret) {
 		ath12k_err(ab, "ieee80211 registration failed: %d\n", ret);
-		goto err_free_if_combs;
+		goto err_cleanup_if_combs;
 	}
 
 	if (!ab->hw_params->supports_monitor)
@@ -8110,9 +8116,8 @@ static int ath12k_mac_hw_register(struct ath12k_hw *ah)
 err_unregister_hw:
 	ieee80211_unregister_hw(hw);
 
-err_free_if_combs:
-	kfree(wiphy->iface_combinations[0].limits);
-	kfree(wiphy->iface_combinations);
+err_cleanup_if_combs:
+	ath12k_mac_cleanup_iface_combinations(ah);
 
 err_cleanup_unregister:
 	ath12k_mac_cleanup_unregister(ar);
-- 
2.34.1

[PATCH 13/13] wifi: ath12k: Advertise multi hardware iface combination

[Karthikeyan Periyasamy]

From: Harshitha Prem <quic_hprem@quicinc.com>

The prerequisite for MLO support in cfg80211/mac80211 requires that all
the links participating in MLO belong to the same wiphy/ieee80211_hw.
The driver needs to group multiple discrete hardware each acting as a
link in MLO, under one wiphy. Consequently, the driver advertises multi
hardware interface combination capabilities with a list of supported
frequencies.

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

Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com>
Co-developed-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 128 +++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 44c8bf6eb6ae..3d0cae8d883a 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7781,10 +7781,126 @@ static bool ath12k_mac_is_iface_mode_enable(struct ath12k_hw *ah,
 	return is_enable;
 }
 
+static
+struct ieee80211_chans_per_hw *ath12k_setup_per_hw_chan(struct ath12k *ar)
+{
+	struct ieee80211_chans_per_hw *chans;
+	struct ieee80211_supported_band *band;
+
+	if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_2G_CAP)
+		band = &ar->mac.sbands[NL80211_BAND_2GHZ];
+	else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
+		 !ar->supports_6ghz)
+		band = &ar->mac.sbands[NL80211_BAND_5GHZ];
+	else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
+		 ar->supports_6ghz)
+		band = &ar->mac.sbands[NL80211_BAND_6GHZ];
+
+	chans = kzalloc(struct_size(chans, chans, band->n_channels),
+			GFP_KERNEL);
+	if (!chans)
+		return NULL;
+
+	memcpy(chans->chans, band->channels,
+	       sizeof(*band->channels) * band->n_channels);
+	chans->n_chans = band->n_channels;
+
+	return chans;
+}
+
+static void ath12k_mac_cleanup_hw_channels(struct ath12k_hw *ah)
+{
+	struct wiphy *wiphy = ah->hw->wiphy;
+	int i;
+
+	for (i = 0; i < ah->num_radio; i++)
+		kfree(wiphy->hw_chans[i]);
+
+	kfree(wiphy->hw_chans);
+}
+
+static int
+ath12k_mac_setup_hw_channels(struct ath12k_hw *ah)
+{
+	struct wiphy *wiphy = ah->hw->wiphy;
+	int i, ret;
+
+	wiphy->hw_chans = kcalloc(ah->num_radio, sizeof(*wiphy->hw_chans),
+				  GFP_KERNEL);
+	if (!wiphy->hw_chans)
+		return -ENOMEM;
+
+	for (i = 0; i < ah->num_radio; i++) {
+		wiphy->hw_chans[i] = ath12k_setup_per_hw_chan(&ah->radio[i]);
+		if (!wiphy->hw_chans[i]) {
+			ret = -ENOMEM;
+			goto cleanup_hw_chan;
+		}
+	}
+
+	wiphy->num_hw = ah->num_radio;
+
+	return 0;
+
+cleanup_hw_chan:
+	for (i = i - 1; i >= 0; i--)
+		kfree(wiphy->hw_chans[i]);
+
+	kfree(wiphy->hw_chans);
+
+	return ret;
+}
+
+static void
+ath12k_mac_cleanup_per_hw_iface_comb(struct ath12k_hw *ah)
+{
+	struct wiphy *wiphy = ah->hw->wiphy;
+
+	ath12k_mac_cleanup_hw_channels(ah);
+
+	kfree(wiphy->iface_combinations[0].iface_hw_list);
+}
+
+static int
+ath12k_mac_setup_per_hw_iface_comb(struct ath12k_hw *ah,
+				   struct ieee80211_iface_combination *comb)
+{
+	struct ieee80211_iface_per_hw *iface_hw;
+	struct ieee80211_hw *hw = ah->hw;
+	int i, ret;
+
+	ret = ath12k_mac_setup_hw_channels(ah);
+	if (ret)
+		return ret;
+
+	iface_hw  = kcalloc(ah->num_radio, sizeof(*iface_hw), GFP_KERNEL);
+	if (!iface_hw) {
+		ath12k_mac_cleanup_hw_channels(ah);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < ah->num_radio; i++) {
+		iface_hw[i].hw_chans_idx = i;
+		iface_hw[i].num_different_channels =
+					comb->num_different_channels;
+		iface_hw[i].max_interfaces = comb->max_interfaces;
+		iface_hw[i].limits = comb->limits;
+		iface_hw[i].n_limits = comb->n_limits;
+	}
+
+	comb->iface_hw_list = iface_hw;
+	comb->n_hw_list = hw->wiphy->num_hw;
+
+	return 0;
+}
+
 static void ath12k_mac_cleanup_iface_combinations(struct ath12k_hw *ah)
 {
 	struct wiphy *wiphy = ah->hw->wiphy;
 
+	if (ah->num_radio > 1)
+		ath12k_mac_cleanup_per_hw_iface_comb(ah);
+
 	kfree(wiphy->iface_combinations[0].limits);
 	kfree(wiphy->iface_combinations);
 }
@@ -7794,7 +7910,7 @@ static int ath12k_mac_setup_iface_combinations(struct ath12k_hw *ah)
 	struct wiphy *wiphy = ah->hw->wiphy;
 	struct ieee80211_iface_combination *combinations;
 	struct ieee80211_iface_limit *limits;
-	int n_limits, max_interfaces;
+	int n_limits, max_interfaces, ret;
 	bool ap, mesh, p2p;
 
 	ap = ath12k_mac_is_iface_mode_enable(ah, NL80211_IFTYPE_AP);
@@ -7857,6 +7973,16 @@ static int ath12k_mac_setup_iface_combinations(struct ath12k_hw *ah)
 						BIT(NL80211_CHAN_WIDTH_40) |
 						BIT(NL80211_CHAN_WIDTH_80);
 
+	if (ah->num_radio > 1) {
+		ret = ath12k_mac_setup_per_hw_iface_comb(ah, combinations);
+		if (ret) {
+			kfree(limits);
+			kfree(combinations);
+
+			return ret;
+		}
+	}
+
 	wiphy->iface_combinations = combinations;
 	wiphy->n_iface_combinations = 1;
 
-- 
2.34.1

Re: [PATCH 13/13] wifi: ath12k: Advertise multi hardware iface combination

[kernel test robot]

Hi Karthikeyan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d69aef8084cc72df7b0f2583096d9b037c647ec8]

url:    https://github.com/intel-lab-lkp/linux/commits/Karthikeyan-Periyasamy/wifi-cfg80211-Add-provision-to-advertise-multiple-radio-in-one-wiphy/20240328-153303
base:   d69aef8084cc72df7b0f2583096d9b037c647ec8
patch link:    https://lore.kernel.org/r/20240328072916.1164195-14-quic_periyasa%40quicinc.com
patch subject: [PATCH 13/13] wifi: ath12k: Advertise multi hardware iface combination
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20240329/202403290724.p8vSV474-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 79ba323bdd0843275019e16b6e9b35133677c514)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240329/202403290724.p8vSV474-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403290724.p8vSV474-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/wireless/ath/ath12k/mac.c:7:
   In file included from include/net/mac80211.h:18:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/arm64/include/asm/cacheflush.h:11:
   In file included from include/linux/kgdb.h:19:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:13:
   In file included from include/linux/kallsyms.h:13:
   In file included from include/linux/mm.h:2188:
   include/linux/vmstat.h:508:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     508 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     509 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:515:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     515 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     516 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     522 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:527:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     527 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     528 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:536:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     536 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     537 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath12k/mac.c:7795:11: warning: variable 'band' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    7795 |         else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    7796 |                  ar->supports_6ghz)
         |                  ~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath12k/mac.c:7799:44: note: uninitialized use occurs here
    7799 |         chans = kzalloc(struct_size(chans, chans, band->n_channels),
         |                                                   ^~~~
   include/linux/overflow.h:294:53: note: expanded from macro 'struct_size'
     294 |                 size_add(sizeof(*(p)), flex_array_size(p, member, count)))
         |                                                                   ^~~~~
   include/linux/overflow.h:278:12: note: expanded from macro 'flex_array_size'
     278 |                 size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
         |                          ^~~~~
   drivers/net/wireless/ath/ath12k/mac.c:7795:7: note: remove the 'if' if its condition is always true
    7795 |         else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    7796 |                  ar->supports_6ghz)
         |                  ~~~~~~~~~~~~~~~~~~
    7797 |                 band = &ar->mac.sbands[NL80211_BAND_6GHZ];
>> drivers/net/wireless/ath/ath12k/mac.c:7795:11: warning: variable 'band' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
    7795 |         else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath12k/mac.c:7799:44: note: uninitialized use occurs here
    7799 |         chans = kzalloc(struct_size(chans, chans, band->n_channels),
         |                                                   ^~~~
   include/linux/overflow.h:294:53: note: expanded from macro 'struct_size'
     294 |                 size_add(sizeof(*(p)), flex_array_size(p, member, count)))
         |                                                                   ^~~~~
   include/linux/overflow.h:278:12: note: expanded from macro 'flex_array_size'
     278 |                 size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
         |                          ^~~~~
   drivers/net/wireless/ath/ath12k/mac.c:7795:11: note: remove the '&&' if its condition is always true
    7795 |         else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath12k/mac.c:7788:39: note: initialize the variable 'band' to silence this warning
    7788 |         struct ieee80211_supported_band *band;
         |                                              ^
         |                                               = NULL
   7 warnings generated.


vim +7795 drivers/net/wireless/ath/ath12k/mac.c

  7783	
  7784	static
  7785	struct ieee80211_chans_per_hw *ath12k_setup_per_hw_chan(struct ath12k *ar)
  7786	{
  7787		struct ieee80211_chans_per_hw *chans;
  7788		struct ieee80211_supported_band *band;
  7789	
  7790		if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_2G_CAP)
  7791			band = &ar->mac.sbands[NL80211_BAND_2GHZ];
  7792		else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
  7793			 !ar->supports_6ghz)
  7794			band = &ar->mac.sbands[NL80211_BAND_5GHZ];
> 7795		else if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP &&
  7796			 ar->supports_6ghz)
  7797			band = &ar->mac.sbands[NL80211_BAND_6GHZ];
  7798	
  7799		chans = kzalloc(struct_size(chans, chans, band->n_channels),
  7800				GFP_KERNEL);
  7801		if (!chans)
  7802			return NULL;
  7803	
  7804		memcpy(chans->chans, band->channels,
  7805		       sizeof(*band->channels) * band->n_channels);
  7806		chans->n_chans = band->n_channels;
  7807	
  7808		return chans;
  7809	}
  7810	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki