Re: [PATCH 08/13] wifi: cfg80211: Refactor the iface combination iteration helper function

Karthikeyan Periyasamy
2024-04-02T16:35:25Z

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