Received-SPF: pass (google.com: domain of linux-wireless+bounces-5796-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <7bbd676a-748b-a1d6-aa5c-2e8868017159@quicinc.com>
Date: Wed, 3 Apr 2024 15:28:49 +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 09/13] wifi: cfg80211: Add multi-hardware iface
 combination support
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-10-quic_periyasa@quicinc.com>
 <a24b7d967dbe31ef613fae8c331f4ff718482c93.camel@sipsolutions.net>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <a24b7d967dbe31ef613fae8c331f4ff718482c93.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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

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

sure

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

sure

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

sure

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

sure

> 
>> +static u16*
> 
> missing space

sure

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

sure

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

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


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