Received-SPF: pass (google.com: domain of linux-wireless+bounces-5751-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <25b78e30-4f70-7836-cb73-a2ff2584700b@quicinc.com>
Date: Tue, 2 Apr 2024 22:05:11 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 08/13] wifi: cfg80211: Refactor the iface combination
 iteration helper function
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Vasanthakumar Thiagarajan
	<quic_vthiagar@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
 <20240328072916.1164195-9-quic_periyasa@quicinc.com>
 <e2c5f81882f86b26be4d9bcf3c9d4b1fd6001b22.camel@sipsolutions.net>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <e2c5f81882f86b26be4d9bcf3c9d4b1fd6001b22.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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

sure

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

sure

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

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

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