diff mbox series

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

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

Commit Message

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

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

Comments

Johannes Berg March 28, 2024, 1:43 p.m. UTC | #1 
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
Karthikeyan Periyasamy April 2, 2024, 4:35 p.m. UTC | #2 
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
diff mbox series

Patch

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