[RFC 0/4] wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy

[RFC 0/4] wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy

[Vasanthakumar Thiagarajan]

There may be hardware design (ath12k as of now) supporting MLO across multiple
discrete hardware each acting as a link in the MLO operation. Since the prerequisite
for MLO support in cfg80211/mac80211 is that all the links participating in MLO
must be from the same wiphy/ieee80211_hw, driver needs to handle the discreate hardware
abstraction under single wiphy.  Though most of the hw specific abstractions
can be handled with in the driver, there are some capabilities like interface
combination which can be specific to each constituent 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

- Make runtime iface combination validation logic be aware of this extension
- 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. 
- Should we make the capability advertisement changes to mac80211_hwsim?
- Should we enable some of concurrent operations like allow scan on each physical
  hardware concurrently?


Vasanthakumar Thiagarajan (4):
  wifi: cfg80211: Add provision to advertise multiple radio in one wiphy
  wifi: nl80211: send underlying multi-hardware channel capabilities to
    user space
  wifi: cfg80211/mac80211: extend iface comb advertisement for
    multi-hardware dev
  wifi: nl80211: send iface combination to user space in multi-hardware
    wiphy

 include/net/cfg80211.h       | 130 +++++++++++++++++
 include/uapi/linux/nl80211.h |  78 +++++++++-
 net/mac80211/main.c          |  54 +++++++
 net/wireless/core.c          | 275 +++++++++++++++++++++++++++++------
 net/wireless/nl80211.c       | 118 +++++++++++++++
 net/wireless/util.c          |  18 +++
 6 files changed, 629 insertions(+), 44 deletions(-)

-- 
2.17.1

[RFC 1/4] wifi: cfg80211: Add provision to advertise multiple radio in one wiphy

[Vasanthakumar Thiagarajan]

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 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. This commit
adds 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.

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
---
 include/net/cfg80211.h |  24 +++++++++
 net/wireless/core.c    | 109 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index e09ff87146c1..4662231ad068 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5088,6 +5088,18 @@ struct wiphy_iftype_akm_suites {
 	int n_akm_suites;
 };
 
+/**
+ * struct ieee80211_supported_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 {
+	int n_chans;
+	struct ieee80211_channel chans[];
+};
+
 /**
  * struct wiphy - wireless hardware description
  * @mtx: mutex for the data (structures) of this device
@@ -5297,6 +5309,15 @@ struct wiphy_iftype_akm_suites {
  *	NL80211_MAX_NR_AKM_SUITES in order to avoid compatibility issues with
  *	legacy userspace and maximum allowed value is
  *	CFG80211_MAX_NUM_AKM_SUITES.
+ *
+ * @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.
+ *
  */
 struct wiphy {
 	struct mutex mtx;
@@ -5445,6 +5466,9 @@ struct wiphy {
 	u8 ema_max_profile_periodicity;
 	u16 max_num_akm_suites;
 
+	struct ieee80211_chans_per_hw **hw_chans;
+	int num_hw;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 5b0c4d5b80cf..4163602a1b9a 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -653,6 +653,110 @@ 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[i].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;
+	bool found;
+	int i, j;
+
+	for (i = 0; i < chans->n_chans; i++) {
+		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 = 0; j < wiphy->num_hw; j++) {
+			const struct ieee80211_chans_per_hw *hw_chans1;
+			const struct ieee80211_chans_per_hw *hw_chans2;
+
+			if (i == j)
+				continue;
+
+			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)) {
+			WARN_ON(1);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 int wiphy_register(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
@@ -908,6 +1012,11 @@ int wiphy_register(struct wiphy *wiphy)
 		return -EINVAL;
 	}
 
+	if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < rdev->wiphy.n_vendor_commands; i++) {
 		/*
 		 * Validate we have a policy (can be explicitly set to
-- 
2.17.1

Re: [RFC 1/4] wifi: cfg80211: Add provision to advertise multiple radio in one wiphy

[Johannes Berg]

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> 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 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. This commit
> adds 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.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
> ---
>  include/net/cfg80211.h |  24 +++++++++
>  net/wireless/core.c    | 109 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index e09ff87146c1..4662231ad068 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -5088,6 +5088,18 @@ struct wiphy_iftype_akm_suites {
>  	int n_akm_suites;
>  };
>  
> +/**
> + * struct ieee80211_supported_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 {
> +	int n_chans;

nit: unsigned?

> + * @hw_chans: list of the channels supported by every constituent underlying

"every" here refers to the fact that you have all the channels, right? I
mean, hw_chans is per hardware, basically.

> + *	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.
> + *
>   */
>  struct wiphy {
>  	struct mutex mtx;
> @@ -5445,6 +5466,9 @@ struct wiphy {
>  	u8 ema_max_profile_periodicity;
>  	u16 max_num_akm_suites;
>  
> +	struct ieee80211_chans_per_hw **hw_chans;
> +	int num_hw;

also here, maybe unsigned.

> +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;
> +	bool found;
> +	int i, j;
> +
> +	for (i = 0; i < chans->n_chans; i++) {
> +		found = false;

nit: you can move the variable declaration here

	bool found = false;
	\n

since you don't need it outside the loop.

> +	for (i = 0; i < wiphy->num_hw; i++) {
> +		for (j = 0; j < wiphy->num_hw; j++) {
> +			const struct ieee80211_chans_per_hw *hw_chans1;
> +			const struct ieee80211_chans_per_hw *hw_chans2;
> +
> +			if (i == j)
> +				continue;

It's symmetric, if I read the code above right, so you can do

	for (j = i; j < ...; j++)

in the second loop and avoid this?


> +		hw_chans = wiphy->hw_chans[i];
> +		if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans)) {
> +			WARN_ON(1);

I can kind of see the point in not using WARN_ON(!cfg80211_hw_chans...)
since it gets really long, but maybe just remove the warning?
 
> +	if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) {
> +		WARN_ON(1);
> +		return -EINVAL;
> 

Anyway you'll have one here, and it's not directly visible which
condition failed anyway.

And you could use WARN_ON(validate(...)) here :)

johannes

Re: [RFC 1/4] wifi: cfg80211: Add provision to advertise multiple radio in one wiphy

[Vasanthakumar Thiagarajan]

On 10/21/2022 5:34 PM, Johannes Berg wrote:
> On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
>> 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 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. This commit
>> adds 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.
>>
>> Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
>> ---
>>   include/net/cfg80211.h |  24 +++++++++
>>   net/wireless/core.c    | 109 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 133 insertions(+)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index e09ff87146c1..4662231ad068 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -5088,6 +5088,18 @@ struct wiphy_iftype_akm_suites {
>>   	int n_akm_suites;
>>   };
>>   
>> +/**
>> + * struct ieee80211_supported_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 {
>> +	int n_chans;
> 
> nit: unsigned?
> 
>> + * @hw_chans: list of the channels supported by every constituent underlying
> 
> "every" here refers to the fact that you have all the channels, right? I
> mean, hw_chans is per hardware, basically.

Correct, it refers all the channels supported per hardware registered 
something like below

hw_chans[0] =	{
  			// 2 GHz radio
			<num_2ghz_chans>,
			{
				<ieee80211_channel_2ghz_1>,
				<ieee80211_channel_2ghz_2>, ...
			}
		}

hw_chans[1] = {
		{
  			// 5 GHz radio
			<num_5ghz_chans>,
			{
				<ieee80211_channel_5ghz_1>,
				<ieee80211_channel_5ghz_2>, ...
			}
		}
		
...


> 
>> + *	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.
>> + *
>>    */
>>   struct wiphy {
>>   	struct mutex mtx;
>> @@ -5445,6 +5466,9 @@ struct wiphy {
>>   	u8 ema_max_profile_periodicity;
>>   	u16 max_num_akm_suites;
>>   
>> +	struct ieee80211_chans_per_hw **hw_chans;
>> +	int num_hw;
> 
> also here, maybe unsigned.
> 
>> +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;
>> +	bool found;
>> +	int i, j;
>> +
>> +	for (i = 0; i < chans->n_chans; i++) {
>> +		found = false;
> 
> nit: you can move the variable declaration here
> 
> 	bool found = false;
> 	\n
> 
> since you don't need it outside the loop.
> 
>> +	for (i = 0; i < wiphy->num_hw; i++) {
>> +		for (j = 0; j < wiphy->num_hw; j++) {
>> +			const struct ieee80211_chans_per_hw *hw_chans1;
>> +			const struct ieee80211_chans_per_hw *hw_chans2;
>> +
>> +			if (i == j)
>> +				continue;
> 
> It's symmetric, if I read the code above right, so you can do
> 
> 	for (j = i; j < ...; j++)
> 
> in the second loop and avoid this?

Sure

> 
> 
>> +		hw_chans = wiphy->hw_chans[i];
>> +		if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans)) {
>> +			WARN_ON(1);
> 
> I can kind of see the point in not using WARN_ON(!cfg80211_hw_chans...)
> since it gets really long, but maybe just remove the warning?
>   
>> +	if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) {
>> +		WARN_ON(1);
>> +		return -EINVAL;
>>
> 
> Anyway you'll have one here, and it's not directly visible which
> condition failed anyway.
> 
> And you could use WARN_ON(validate(...)) here :)

Sure, thanks!

Vasanth

[RFC 2/4] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Vasanthakumar Thiagarajan]

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.

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@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 c32e7616a366..070b31277402 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2749,6 +2749,12 @@ enum nl80211_commands {
  *	When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
  *	timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
  *	the incoming frame RX timestamp.
+ *
+ * @NL80211_ATTR_MULTI_HW_MACS: nested attribute to send the hardware mac
+ *	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
@@ -3277,6 +3283,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_TX_HW_TIMESTAMP,
 	NL80211_ATTR_RX_HW_TIMESTAMP,
 
+	NL80211_ATTR_MULTI_HW_MACS,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7720,4 +7728,24 @@ enum nl80211_ap_settings_flags {
 	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
 };
 
+/**
+ * nl80211_multi_hw_mac_attrs - multi-hw mac attributes
+ *
+ * @NL80211_MULTI_HW_MAC_ATTR_INVALID: invalid
+ * @NL80211_MULTI_HW_MAC_ATTR_IDX: (u8) array index in wiphy @hw_chans to refer an
+ *	underlying hw mac for which the supported channel list is advertised.
+ * @NL80211_MULTI_HW_MAC_ATTR_FREQS: array of supported center frequencies
+ * @__NL80211_MULTI_HW_MAC_ATTR_LAST: internal use
+ * @NL80211_MULTI_HW_MAC_ATTR_MAX: maximum multi-hw mac attribute
+ */
+enum nl80211_multi_hw_mac_attrs {
+	__NL80211_MULTI_HW_MAC_ATTR_INVALID,
+
+	NL80211_MULTI_HW_MAC_ATTR_IDX,
+	NL80211_MULTI_HW_MAC_ATTR_FREQS,
+
+	/* keep last */
+	__NL80211_MULTI_HW_MAC_ATTR_LAST,
+	NL80211_MULTI_HW_MAC_ATTR_MAX = __NL80211_MULTI_HW_MAC_ATTR_LAST - 1
+};
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8ff8b1c040f0..b7d466010e81 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2355,6 +2355,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_MACS);
+	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_MAC_ATTR_IDX, i))
+			return -ENOBUFS;
+
+		freqs = nla_nest_start(msg,
+				       NL80211_MULTI_HW_MAC_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;
@@ -2959,6 +3000,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_MLO)
 			nla_put_flag(msg, NL80211_ATTR_MLO_SUPPORT);
 
+		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.17.1

Re: [RFC 2/4] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> 
> +++ b/include/uapi/linux/nl80211.h
> @@ -2749,6 +2749,12 @@ enum nl80211_commands {
>   *	When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
>   *	timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
>   *	the incoming frame RX timestamp.
> + *
> + * @NL80211_ATTR_MULTI_HW_MACS: nested attribute to send the hardware mac

Not sure I'd call this multiple MACs? It's multiple devices in some
sense, but from a spec POV at least, I'd think our NIC also has multiple
MACs when it doesn't use this infrastructure. Might get a bit confusing?

Maybe just stick to "multi_hw" or so?

> +/**
> + * nl80211_multi_hw_mac_attrs - multi-hw mac attributes
> + *
> + * @NL80211_MULTI_HW_MAC_ATTR_INVALID: invalid
> + * @NL80211_MULTI_HW_MAC_ATTR_IDX: (u8) array index in wiphy @hw_chans to refer an
> + *	underlying hw mac for which the supported channel list is advertised.

I'd prefer this to be primarily written from a userspace POV, so the
whole "@hw_chans" etc isn't really right. Maybe say something like

"(u8) multi-HW index used to refer to an underlying HW ...; internally
the index of the wiphy's @hw_chans array."

or so?

> + * @NL80211_MULTI_HW_MAC_ATTR_FREQS: array of supported center frequencies

FWIW, Jakub has started advertising for using the same attribute
multiple times to have arrays, so you'd just have

 {NL80211_MULTI_HW_ATTR_FREQ: 2412},
 {NL80211_MULTI_HW_ATTR_FREQ: 2417},
 {NL80211_MULTI_HW_ATTR_FREQ: 2422},

etc. in the message. Not sure we want to try that here, but it'd also
simplify splitting messages for dumps.


> +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_MACS);
> +	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_MAC_ATTR_IDX, i))
> +			return -ENOBUFS;
> +
> +		freqs = nla_nest_start(msg,
> +				       NL80211_MULTI_HW_MAC_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;

Ah you used a nested array even.

So the argument for using a real array would've been that it's smaller,
but I guess with nested that argument goes way.

Would you mind trying Jakub's preferred approach here and see how that
works out?

For the generator basically you'd just have

hw_mac = nla_nest_start();
nla_put_u8(IDX, i)
for (c = 0; c < ...; c++)
	nla_put_u32(MULTI_HW_ATTR_FREQ, ...->chans[c].center_freq);


johannes

Re: [RFC 2/4] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Vasanthakumar Thiagarajan]

On 10/21/2022 5:43 PM, Johannes Berg wrote:
> On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
>>
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -2749,6 +2749,12 @@ enum nl80211_commands {
>>    *	When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
>>    *	timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
>>    *	the incoming frame RX timestamp.
>> + *
>> + * @NL80211_ATTR_MULTI_HW_MACS: nested attribute to send the hardware mac
> 
> Not sure I'd call this multiple MACs? It's multiple devices in some
> sense, but from a spec POV at least, I'd think our NIC also has multiple
> MACs when it doesn't use this infrastructure. Might get a bit confusing?
> 
> Maybe just stick to "multi_hw" or so?

Yeah, I was not very comfortable calling it multiple MACs either. Sure, 
let me just stick to multi_hw.

> 
>> +/**
>> + * nl80211_multi_hw_mac_attrs - multi-hw mac attributes
>> + *
>> + * @NL80211_MULTI_HW_MAC_ATTR_INVALID: invalid
>> + * @NL80211_MULTI_HW_MAC_ATTR_IDX: (u8) array index in wiphy @hw_chans to refer an
>> + *	underlying hw mac for which the supported channel list is advertised.
> 
> I'd prefer this to be primarily written from a userspace POV, so the
> whole "@hw_chans" etc isn't really right. Maybe say something like
> 
> "(u8) multi-HW index used to refer to an underlying HW ...; internally
> the index of the wiphy's @hw_chans array."
> 
> or so?

Sure, thanks.

> 
>> + * @NL80211_MULTI_HW_MAC_ATTR_FREQS: array of supported center frequencies
> 
> FWIW, Jakub has started advertising for using the same attribute
> multiple times to have arrays, so you'd just have
> 
>   {NL80211_MULTI_HW_ATTR_FREQ: 2412},
>   {NL80211_MULTI_HW_ATTR_FREQ: 2417},
>   {NL80211_MULTI_HW_ATTR_FREQ: 2422},
> 
> etc. in the message. Not sure we want to try that here, but it'd also
> simplify splitting messages for dumps.
>

we have to do that for every channel? let me check.

> 
>> +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_MACS);
>> +	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_MAC_ATTR_IDX, i))
>> +			return -ENOBUFS;
>> +
>> +		freqs = nla_nest_start(msg,
>> +				       NL80211_MULTI_HW_MAC_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;
> 
> Ah you used a nested array even.
> 
> So the argument for using a real array would've been that it's smaller,
> but I guess with nested that argument goes way.
> 
> Would you mind trying Jakub's preferred approach here and see how that
> works out?
> 
> For the generator basically you'd just have
> 
> hw_mac = nla_nest_start();
> nla_put_u8(IDX, i)
> for (c = 0; c < ...; c++)
> 	nla_put_u32(MULTI_HW_ATTR_FREQ, ...->chans[c].center_freq);
> 

I'll try this, thanks!

Vasanth

[RFC 3/4] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Vasanthakumar Thiagarajan]

When 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. Iface combination for each
underlying hardware is described with an identifier, the same index which
is used in wiphy->hw_chans[] to learn the channel capabilities of the
respective hardware. It should be noted that the supporting drivers also
need to signal the iface comb capabilities that are common for all the
hardware through the existing interface to maintain the backward
compatibility with the user space. Provision to advertise per physical
hardware specific iface comb capabilities and the sanity checks on the
advertised capabilities are implemented in this commit.

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),
			},
		      },
};

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
---
 include/net/cfg80211.h | 106 ++++++++++++++++++++++++
 net/mac80211/main.c    |  54 +++++++++++++
 net/wireless/core.c    | 178 +++++++++++++++++++++++++++++------------
 net/wireless/util.c    |  18 +++++
 4 files changed, 307 insertions(+), 49 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4662231ad068..175c2ad4a3e8 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4726,6 +4726,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.
+ *
+ * @hw_chans_idx: index of hardware specific channel list as per wiphy @hw_chans
+ * @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
+ */
+struct ieee80211_iface_per_hw {
+	u8 hw_chans_idx;
+	const struct ieee80211_iface_limit *limits;
+	u32 num_different_channels;
+	u16 max_interfaces;
+	u8 n_limits;
+};
+
 /**
  * struct ieee80211_iface_combination - possible interface combination
  *
@@ -4784,6 +4810,62 @@ struct ieee80211_iface_limit {
  *		.num_different_channels = 2,
  *	};
  *
+ *
+ * 4. Hardware specific interface combination with driver supporting two hw
+ *    (MAC), one underlying MAC 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 limits4[] = {
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
+ *	};
+ *	struct ieee80211_iface_limit limits5_2ghz[] = {
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
+ *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
+ *	};
+ *	struct ieee80211_iface_limit limits5_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 = limits5_2ghz,
+ *			.num_different_channels = 1,
+ *			.max_interfaces = 2,
+ *			.n_limits = ARRAY_SIZE(limits5_2ghz),
+ *		 },
+ *		{
+ *			.hw_chans_idx = 1,
+ *			.limits = limits5_5ghz,
+ *			.num_different_channels = 1,
+ *			.max_interfaces = 3,
+ *			.n_limits = ARRAY_SIZE(limits5_5ghz),
+ *		 },
+ *	};
+ *	struct ieee80211_iface_combination combination4 = {
+ *		.limits = limits4,
+ *		.n_limits = ARRAY_SIZE(limits4),
+ *		.max_interfaces = 2,
+ *		.num_different_channels = 1,
+ *		.iface_hw_list = hw_combinations,
+ *		.n_hw_list = ARRAY_SIZE(hw_combinations),
+ *	};
+ *
  */
 struct ieee80211_iface_combination {
 	/**
@@ -4841,6 +4923,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 {
@@ -8791,6 +8887,16 @@ bool cfg80211_iftype_allowed(struct wiphy *wiphy, enum nl80211_iftype iftype,
 void cfg80211_assoc_comeback(struct net_device *netdev,
 			     const u8 *ap_addr, u32 timeout);
 
+/**
+ * cfg80211_hw_chans_includes_dfs - check if per-hardware channel includes DFS
+ *	@chans: hardware channel list
+ *
+ * Check if the given per-hardware list includes channels in DFS range.
+ * Please note the channel is checked against the entire range of DFS
+ * freq in 5 GHz irrespective of regulatory configurations.
+ */
+bool cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 46f3eddc2388..2e008fa976e9 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -933,6 +933,45 @@ 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 h, l;
+	u32 hw_idx_bm = 0;
+
+	if (!local->use_chanctx)
+		return -EINVAL;
+
+	for (h = 0; h < c->n_hw_list; h++) {
+		const struct ieee80211_iface_per_hw *hl;
+		const struct ieee80211_chans_per_hw *chans;
+
+		hl = &c->iface_hw_list[h];
+
+		if (hl->hw_chans_idx >= local->hw.wiphy->num_hw)
+			return -EINVAL;
+
+		chans = local->hw.wiphy->hw_chans[hl->hw_chans_idx];
+		if (c->radar_detect_widths &&
+		    cfg80211_hw_chans_includes_dfs(chans) &&
+		    hl->num_different_channels > 1)
+			return -EINVAL;
+
+		for (l = 0; l < hl->n_limits; l++)
+			if ((hl->limits[l].types & BIT(NL80211_IFTYPE_ADHOC)) &&
+			    hl->limits[l].max > 1)
+				return -EINVAL;
+
+		if (hw_idx_bm & BIT(h))
+			return -EINVAL;
+
+		hw_idx_bm |= BIT(h);
+	}
+
+	return 0;
+}
+
 int ieee80211_register_hw(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -1035,6 +1074,21 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 		}
 	}
 
+	for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
+		const struct ieee80211_iface_combination *comb;
+
+		comb = &local->hw.wiphy->iface_combinations[i];
+
+		if (comb->n_hw_list && !local->hw.wiphy->num_hw)
+			return -EINVAL;
+
+		if (!comb->n_hw_list)
+			continue;
+
+		if (ieee80211_check_per_hw_iface_comb(local, comb))
+			return -EINVAL;
+	}
+
 	/* Only HW csum features are currently compatible with mac80211 */
 	if (WARN_ON(hw->netdev_features & ~MAC80211_SUPPORTED_FEATURES))
 		return -EINVAL;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4163602a1b9a..50fd7f131736 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -563,10 +563,126 @@ 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 l;
+
+	for (l = 0; l < n_limits; l++) {
+		u16 types = limits[l].types;
+
+		/*
+		 * Don't advertise an unsupported type
+		 * in a combination.
+		 */
+		if (WARN_ON((wiphy->interface_modes & types) != types))
+			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[l].max > 1))
+			return -EINVAL;
+
+		/* Only a single NAN can be allowed */
+		if (WARN_ON(types & BIT(NL80211_IFTYPE_NAN) &&
+			    limits[l].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[l].max;
+	}
+
+	return 0;
+}
+
+static int
+wiphy_verify_comb_per_hw(struct wiphy *wiphy,
+			 const struct ieee80211_iface_combination *comb)
+{
+	int h;
+	u32 hw_idx_bitmap = 0;
+	int ret;
+
+	for (h = 0; h < comb->n_hw_list; h++) {
+		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[h];
+
+		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;
-	int i, j;
+	int i;
+	int ret;
 
 	for (i = 0; i < wiphy->n_iface_combinations; i++) {
 		u32 cnt = 0;
@@ -593,54 +709,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;
@@ -648,6 +721,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;
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 0b28d00ba8f5..dee9bf309345 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2504,3 +2504,21 @@ cfg80211_get_iftype_ext_capa(struct wiphy *wiphy, enum nl80211_iftype type)
 	return NULL;
 }
 EXPORT_SYMBOL(cfg80211_get_iftype_ext_capa);
+
+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;
+}
+EXPORT_SYMBOL(cfg80211_hw_chans_includes_dfs);
-- 
2.17.1

Re: [RFC 3/4] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Johannes Berg]

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> 
> +struct ieee80211_iface_per_hw {
> +	u8 hw_chans_idx;
> +	const struct ieee80211_iface_limit *limits;
> +	u32 num_different_channels;
> +	u16 max_interfaces;
> +	u8 n_limits;
> +};

nit: moving hw_chans_idx last would get rid of all the padding :)


> + *    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 limits4[] = {
> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
> + *	};
> + *	struct ieee80211_iface_limit limits5_2ghz[] = {
> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
> + *	};
> + *	struct ieee80211_iface_limit limits5_5ghz[] = {
> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
> + *		{ .max = 2, .types = BIT(NL80211_IFTYPE_AP), },
> + *	};

Where does the limits4/limits5 naming come from? The number of
interfaces I guess? To me that wasn't so clear, maybe it makes more
sense to name them

	limits_overall,
	limits_2ghz, and
	limits_5ghz

respectively?

(yeah, obviously I know this is just an example)

> +/**
> + * cfg80211_hw_chans_includes_dfs - check if per-hardware channel includes DFS
> + *	@chans: hardware channel list

prefer space instead of tab I think?

> + * Please note the channel is checked against the entire range of DFS
> + * freq in 5 GHz irrespective of regulatory configurations.

Not sure what you mean by this? Is that different somehow from what we
did before?

> +++ b/net/mac80211/main.c
> @@ -933,6 +933,45 @@ 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)
> +{

Why is this in mac80211? Wouldn't such a check apply equally to any non-
mac80211 driver?

> +	int h, l;
> +	u32 hw_idx_bm = 0;
> +
> +	if (!local->use_chanctx)
> +		return -EINVAL;

Maybe mac80211 has this extra check, and can keep it, but

> +
> +	for (h = 0; h < c->n_hw_list; h++) {
> +		const struct ieee80211_iface_per_hw *hl;
> +		const struct ieee80211_chans_per_hw *chans;
> +
> +		hl = &c->iface_hw_list[h];
> +
> +		if (hl->hw_chans_idx >= local->hw.wiphy->num_hw)
> +			return -EINVAL;
> +
> +		chans = local->hw.wiphy->hw_chans[hl->hw_chans_idx];
> +		if (c->radar_detect_widths &&
> +		    cfg80211_hw_chans_includes_dfs(chans) &&
> +		    hl->num_different_channels > 1)
> +			return -EINVAL;
> +
> +		for (l = 0; l < hl->n_limits; l++)
> +			if ((hl->limits[l].types & BIT(NL80211_IFTYPE_ADHOC)) &&
> +			    hl->limits[l].max > 1)
> +				return -EINVAL;
> +
> +		if (hw_idx_bm & BIT(h))
> +			return -EINVAL;
> +
> +		hw_idx_bm |= BIT(h);

this pretty much seems applicable to do in cfg80211?

> @@ -1035,6 +1074,21 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>  		}
>  	}
>  
> +	for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
> +		const struct ieee80211_iface_combination *comb;
> +
> +		comb = &local->hw.wiphy->iface_combinations[i];
> +
> +		if (comb->n_hw_list && !local->hw.wiphy->num_hw)
> +			return -EINVAL;
> +
> +		if (!comb->n_hw_list)
> +			continue;
> +
> +		if (ieee80211_check_per_hw_iface_comb(local, comb))
> +			return -EINVAL;
> +	}

and this then, of course.

> +++ b/net/wireless/core.c
> @@ -563,10 +563,126 @@ 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)

oh wait, you did it twice?

is there anything that mac80211 adds extra?

>  static int wiphy_verify_combinations(struct wiphy *wiphy)
>  {
>  	const struct ieee80211_iface_combination *c;
> -	int i, j;
> +	int i;
> +	int ret;
>  
>  	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>  		u32 cnt = 0;
> @@ -593,54 +709,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;
> 
[...]
> +		ret = wiphy_verify_comb_limit(wiphy, c->limits, c->n_limits,
> +					      c->beacon_int_min_gcd,
> +					      &cnt, &all_iftypes);


Might be nice to break out this refactoring to a separate patch (and
feel free to send it right away as PATCH, it's kind of worthwhile
anyway), I think? Unless I missed something that changed here, but then
it'd be even more worthwhile so I see it ;-)

> +bool
> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
> +{

[...]
> +EXPORT_SYMBOL(cfg80211_hw_chans_includes_dfs);

Since it's exported - who would use it and for what?

johannes

Re: [RFC 3/4] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

[Vasanthakumar Thiagarajan]

On 10/21/2022 5:52 PM, Johannes Berg wrote:
> On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
>>
>> +struct ieee80211_iface_per_hw {
>> +	u8 hw_chans_idx;
>> +	const struct ieee80211_iface_limit *limits;
>> +	u32 num_different_channels;
>> +	u16 max_interfaces;
>> +	u8 n_limits;
>> +};
> 
> nit: moving hw_chans_idx last would get rid of all the padding :)

Oops, I missed to check this, thanks.

> 
> 
>> + *    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 limits4[] = {
>> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
>> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
>> + *	};
>> + *	struct ieee80211_iface_limit limits5_2ghz[] = {
>> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
>> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_AP), },
>> + *	};
>> + *	struct ieee80211_iface_limit limits5_5ghz[] = {
>> + *		{ .max = 1, .types = BIT(NL80211_IFTYPE_STATION), },
>> + *		{ .max = 2, .types = BIT(NL80211_IFTYPE_AP), },
>> + *	};
> 
> Where does the limits4/limits5 naming come from? The number of
> interfaces I guess? To me that wasn't so clear, maybe it makes more
> sense to name them
> 
> 	limits_overall,
> 	limits_2ghz, and
> 	limits_5ghz
> 
> respectively?

Yes, this is more clear.

> 
> (yeah, obviously I know this is just an example)
> 

This is very critical reference for the interface combination

>> +/**
>> + * cfg80211_hw_chans_includes_dfs - check if per-hardware channel includes DFS
>> + *	@chans: hardware channel list
> 
> prefer space instead of tab I think?
> 
>> + * Please note the channel is checked against the entire range of DFS
>> + * freq in 5 GHz irrespective of regulatory configurations.
> 
> Not sure what you mean by this? Is that different somehow from what we
> did before?
> 

Normally the DFS marking in the channel is done based on the regulatory
settings for the allowed channels. But here it is a bit different in a 
sense that the channel is to validated against the complete set of DFS 
channels irrespective of the regulatory domain because it is done in
the registration time. Added that note so that the helper is not used
for the regular channel operation.


>> +++ b/net/mac80211/main.c
>> @@ -933,6 +933,45 @@ 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)
>> +{
> 
> Why is this in mac80211? Wouldn't such a check apply equally to any non-
> mac80211 driver?

I had this confusion. I see few sanity checks duplicated in mac80211 in 
the existing code. But you are right, most of this should belong to 
cfg80211.

> 
>> +	int h, l;
>> +	u32 hw_idx_bm = 0;
>> +
>> +	if (!local->use_chanctx)
>> +		return -EINVAL;
> 
> Maybe mac80211 has this extra check, and can keep it, but
> 
>> +
>> +	for (h = 0; h < c->n_hw_list; h++) {
>> +		const struct ieee80211_iface_per_hw *hl;
>> +		const struct ieee80211_chans_per_hw *chans;
>> +
>> +		hl = &c->iface_hw_list[h];
>> +
>> +		if (hl->hw_chans_idx >= local->hw.wiphy->num_hw)
>> +			return -EINVAL;
>> +
>> +		chans = local->hw.wiphy->hw_chans[hl->hw_chans_idx];
>> +		if (c->radar_detect_widths &&
>> +		    cfg80211_hw_chans_includes_dfs(chans) &&
>> +		    hl->num_different_channels > 1)
>> +			return -EINVAL;
>> +
>> +		for (l = 0; l < hl->n_limits; l++)
>> +			if ((hl->limits[l].types & BIT(NL80211_IFTYPE_ADHOC)) &&
>> +			    hl->limits[l].max > 1)
>> +				return -EINVAL;
>> +
>> +		if (hw_idx_bm & BIT(h))
>> +			return -EINVAL;
>> +
>> +		hw_idx_bm |= BIT(h);
> 
> this pretty much seems applicable to do in cfg80211?

Sure.

> 
>> @@ -1035,6 +1074,21 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>>   		}
>>   	}
>>   
>> +	for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>> +		const struct ieee80211_iface_combination *comb;
>> +
>> +		comb = &local->hw.wiphy->iface_combinations[i];
>> +
>> +		if (comb->n_hw_list && !local->hw.wiphy->num_hw)
>> +			return -EINVAL;
>> +
>> +		if (!comb->n_hw_list)
>> +			continue;
>> +
>> +		if (ieee80211_check_per_hw_iface_comb(local, comb))
>> +			return -EINVAL;
>> +	}
> 
> and this then, of course.
> 
>> +++ b/net/wireless/core.c
>> @@ -563,10 +563,126 @@ 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)
> 
> oh wait, you did it twice?
> 
> is there anything that mac80211 adds extra?
> 
>>   static int wiphy_verify_combinations(struct wiphy *wiphy)
>>   {
>>   	const struct ieee80211_iface_combination *c;
>> -	int i, j;
>> +	int i;
>> +	int ret;
>>   
>>   	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>>   		u32 cnt = 0;
>> @@ -593,54 +709,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;
>>
> [...]
>> +		ret = wiphy_verify_comb_limit(wiphy, c->limits, c->n_limits,
>> +					      c->beacon_int_min_gcd,
>> +					      &cnt, &all_iftypes);
> 
> 
> Might be nice to break out this refactoring to a separate patch (and
> feel free to send it right away as PATCH, it's kind of worthwhile
> anyway), I think? Unless I missed something that changed here, but then
> it'd be even more worthwhile so I see it ;-)
> 
>> +bool
>> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
>> +{
> 
> [...]
>> +EXPORT_SYMBOL(cfg80211_hw_chans_includes_dfs);
> 
> Since it's exported - who would use it and for what?

This was used in mac80211 sanity checker :) This will go away finally.

Thanks!

Vasanth

[RFC 4/4] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Vasanthakumar Thiagarajan]

A new nested nl attribute is added to the same existing NL command to
advertise the iface combination capability for each underlying hardware
when driver groups more than one physical hardware under one wiphy to
enable MLO among them.

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
---
 include/uapi/linux/nl80211.h | 50 ++++++++++++++++++++++++-
 net/wireless/nl80211.c       | 71 ++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 070b31277402..678da076b122 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -5779,6 +5779,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
  *
@@ -5795,7 +5799,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.
  *
@@ -5814,12 +5830,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 b7d466010e81..1f3b79e10697 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1595,6 +1595,74 @@ static int nl80211_put_iftypes(struct sk_buff *msg, u32 attr, u16 ifmodes)
 	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;
+		int l;
+
+		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;
+
+		for (l = 0; l < c->iface_hw_list->n_limits; l++) {
+			struct nlattr *limit;
+
+			limit = nla_nest_start(msg, l + 1);
+			if (!limit)
+				return -ENOBUFS;
+
+			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_MAX,
+					c->iface_hw_list[i].limits[l].max))
+				return -ENOBUFS;
+
+			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_TYPES,
+					c->iface_hw_list[i].limits[l].types))
+				return -ENOBUFS;
+
+			nla_nest_end(msg, limit);
+		}
+		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)
@@ -1658,6 +1726,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.17.1

Re: [RFC 4/4] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Johannes Berg]

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> A new nested nl attribute is added to the same existing NL command to
> advertise the iface combination capability for each underlying hardware
> when driver groups more than one physical hardware under one wiphy to
> enable MLO among them.

That's a good point - are we assuming this implies you can do MLO across
the groups? Maybe somebody would want to use this advertisement without
allowing MLO? (Not sure that's useful vs. multiple wiphys though, but
maybe to simplify driver if there are some devices w/o MLO?)

> +		for (l = 0; l < c->iface_hw_list->n_limits; l++) {
> +			struct nlattr *limit;
> +
> +			limit = nla_nest_start(msg, l + 1);
> +			if (!limit)
> +				return -ENOBUFS;
> +
> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_MAX,
> +					c->iface_hw_list[i].limits[l].max))
> +				return -ENOBUFS;
> +
> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_TYPES,
> +					c->iface_hw_list[i].limits[l].types))
> +				return -ENOBUFS;
> +
> +			nla_nest_end(msg, limit);
> +		}
> +		nla_nest_end(msg, limits);


Feels like this part is kind of pre-existing code, or should be, could
it be refactored?

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

And even this feels like it must already exist in some way? Wouldn't it
even be easier to parse for userspace if it wasn't a separate set of
attributes?

johannes

Re: [RFC 4/4] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

[Vasanthakumar Thiagarajan]

On 10/21/2022 5:55 PM, Johannes Berg wrote:
> On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
>> A new nested nl attribute is added to the same existing NL command to
>> advertise the iface combination capability for each underlying hardware
>> when driver groups more than one physical hardware under one wiphy to
>> enable MLO among them.
> 
> That's a good point - are we assuming this implies you can do MLO across
> the groups? Maybe somebody would want to use this advertisement without
> allowing MLO? (Not sure that's useful vs. multiple wiphys though, but
> maybe to simplify driver if there are some devices w/o MLO?)

Good point. Yes, this can be used even without MLO (i.e. not quite tied 
to MLO feature). Ill update the doc accordingly. Thanks.

> 
>> +		for (l = 0; l < c->iface_hw_list->n_limits; l++) {
>> +			struct nlattr *limit;
>> +
>> +			limit = nla_nest_start(msg, l + 1);
>> +			if (!limit)
>> +				return -ENOBUFS;
>> +
>> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_MAX,
>> +					c->iface_hw_list[i].limits[l].max))
>> +				return -ENOBUFS;
>> +
>> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_TYPES,
>> +					c->iface_hw_list[i].limits[l].types))
>> +				return -ENOBUFS;
>> +
>> +			nla_nest_end(msg, limit);
>> +		}
>> +		nla_nest_end(msg, limits);
> 
> 
> Feels like this part is kind of pre-existing code, or should be, could
> it be refactored?
> 

let me check, thanks.

>> +		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);
> 
> And even this feels like it must already exist in some way? Wouldn't it
> even be easier to parse for userspace if it wasn't a separate set of
> attributes?
>

Yes, reusing existing attribute will be simpler. let me check that.

Thanks for the review!


Vasanth

Re: [RFC 0/4] wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy

[Johannes Berg]

Hi,

Sorry for the delay!

> - Should we make the capability advertisement changes to mac80211_hwsim?

I don't think we can, unless we teach hwsim and even mac80211 about
this? IMHO doesn't make much sense.

> - Should we enable some of concurrent operations like allow scan on each physical
>   hardware concurrently?

Isn't that up to the driver? If userspace requests scanning all bands in
a single scan request you can parallelize it using multiple HW
resources?

johannes

Re: [RFC 0/4] wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy

[Vasanthakumar Thiagarajan]

On 10/21/2022 5:27 PM, Johannes Berg wrote:
> Hi,
> 
> Sorry for the delay!

no worries.

> 
>> - Should we make the capability advertisement changes to mac80211_hwsim?
> 
> I don't think we can, unless we teach hwsim and even mac80211 about
> this? IMHO doesn't make much sense.
> 

Sure.

>> - Should we enable some of concurrent operations like allow scan on each physical
>>    hardware concurrently?
> 
> Isn't that up to the driver? If userspace requests scanning all bands in
> a single scan request you can parallelize it using multiple HW
> resources?

Correct, driver should be able to handle the scanning on multiple HW in 
parallel. We saw resource busy error code when tried to bring up 
multiple AP interfaces on different band at the same time due to
an existing scan request still in progress. I agree, it makes sense
to give a single scan request with all the bands and let the ACS in
user space share the results for AP bring up on any band.

Vasanth