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

Karthikeyan Periyasamy
2024-04-03T09:59:25Z

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