diff mbox series

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

Message ID 20240328072916.1164195-10-quic_periyasa@quicinc.com
State New
Headers show
Series wifi: Add multi physical hardware iface combination support | expand

Commit Message

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

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

Comments

Johannes Berg March 28, 2024, 2:16 p.m. UTC | #1 
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
Karthikeyan Periyasamy April 3, 2024, 9:58 a.m. UTC | #2 
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.
diff mbox series

Patch

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