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

[Karthikeyan Periyasamy]

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
--
கார்த்திகேயன் பெரியசாமி