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

[Karthikeyan Periyasamy <quic_periyasa@xxxxxxxxxxx>]

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