Re: [PATCH RFC] wifi: cfg80211: Refactor interface combination input parameter

[Karthikeyan Periyasamy <quic_periyasa@quicinc.com>]

On 5/7/2024 3:17 PM, Johannes Berg wrote:
> On Sat, 2024-04-27 at 08:45 +0530, Karthikeyan Periyasamy wrote:
>> Currently, the interface combination input parameter num_different_channels
>> and iftype_num are directly filled in by the caller under the assumption
>> that all channels and interfaces belong to a single hardware device. This
>> assumption is incorrect for multi-device interface combinations because
>> each device supports a different set of channels and interfaces. As
>> discussed in [1], need to refactor the input parameters to encode enough
>> data to handle both single and multiple device interface combinations.
>> This can be achieved by encoding the frequency and interface type under
>> the interface entity itself. With this new input parameter structure, the
>> cfg80211 can classify and construct the device parameters, then verify them
>> against the device specific interface combinations.
> 
> ^^ This should probably mention _something_ about links too :)
> 

sure

> 
>> [1]: https://lore.kernel.org/linux-wireless/ca70eeb3cdee1e8c3caee69db595bc8160eb4115.camel@sipsolutions.net/
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/wil6210/cfg80211.c   |  44 +++++--
>>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  60 +++++++--
>>   .../net/wireless/quantenna/qtnfmac/cfg80211.c |  32 +++--
>>   include/net/cfg80211.h                        |  37 +++++-
>>   net/mac80211/util.c                           | 124 +++++++++++++++---
>>   net/wireless/util.c                           |  56 ++++++--
>>   6 files changed, 276 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
>> index 8993028709ec..3f9f5f56bd19 100644
>> --- a/drivers/net/wireless/ath/wil6210/cfg80211.c
>> +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
>> @@ -625,17 +625,25 @@ static int wil_cfg80211_validate_add_iface(struct wil6210_priv *wil,
>>   {
>>   	int i;
>>   	struct wireless_dev *wdev;
>> -	struct iface_combination_params params = {
>> -		.num_different_channels = 1,
>> -	};
>> +	struct iface_combination_params params = { 0 };
> 
> nit: just use "= {}".
> 

sure, will address in the next version.


>> +	ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL);
>> +	if (!ifaces)
>> +		return -ENOMEM;
>> +
>> +	list_for_each_entry(pos, &cfg->vif_list, list) {
>> +		if (params.num_iface >= total_iface)
>> +			continue;
> 
> ??
> Seems like that should be a WARN_ON or something?

WARN_ON() is good.

> 
>> +	struct iface_combination_interface *ifaces = NULL;
>> +	u16 total_iface = 0;
>> +	int ret;
>>   
>>   	list_for_each_entry(pos, &cfg->vif_list, list)
>> -		params.iftype_num[pos->wdev.iftype]++;
>> +		total_iface++;
>>   
>> -	params.iftype_num[new_type]++;
>> -	return cfg80211_check_combinations(cfg->wiphy, &params);
>> +	ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL);
> 
> No point in "= NULL" if you overwrite it immediately.
> 

sure, will address in the next version.

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