Received-SPF: pass (google.com: domain of linux-wireless+bounces-5460-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e2c5f81882f86b26be4d9bcf3c9d4b1fd6001b22.camel@sipsolutions.net>
Subject: Re: [PATCH 08/13] wifi: cfg80211: Refactor the iface combination
 iteration helper function
From: Johannes Berg <johannes@sipsolutions.net>
To: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>, 
	ath12k@lists.infradead.org
Cc: linux-wireless@vger.kernel.org, Vasanthakumar Thiagarajan
	 <quic_vthiagar@quicinc.com>
Date: Thu, 28 Mar 2024 14:43:21 +0100
In-Reply-To: <20240328072916.1164195-9-quic_periyasa@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
	 <20240328072916.1164195-9-quic_periyasa@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>=20
> +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 =3D 0;

That initialization is useless.

> +
> +	if (num_interfaces > c->max_interfaces)
> +		return -EINVAL;
> +
> +	if (params->num_different_channels > c->num_different_channels)
> +		return -EINVAL;
> +
> +	limits =3D kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
> +			 GFP_KERNEL);
> +	if (!limits)
> +		return -ENOMEM;
> +
> +	ret =3D 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 =3D 0;
> +

This should probably set *used_iftypes =3D 0.

> +	for (iftype =3D 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
> +		num_interfaces +=3D iftype_num[iftype];
> +		if (iftype_num[iftype] > 0 &&
> +		    !cfg80211_iftype_allowed(wiphy, iftype, 0, 1))
> +			*used_iftypes |=3D BIT(iftype);

and that could really use a rewrite like

		if (!iftype_num[iftype])
			continue;

		num_interfaces +=3D ...

		if (!allowed...)
			*used_iftypes |=3D ...;

I'd say.

>  	for (i =3D 0; i < wiphy->n_iface_combinations; i++) {
>  		const struct ieee80211_iface_combination *c;
> -		struct ieee80211_iface_limit *limits;
>  		u32 all_iftypes =3D 0;
> =20
>  		c =3D &wiphy->iface_combinations[i];
> =20
> -		if (num_interfaces > c->max_interfaces)
> -			continue;
> -		if (params->num_different_channels > c->num_different_channels)
> +		ret =3D cfg80211_validate_iface_comb_limits(wiphy, params,
> +							  c, num_interfaces,
> +							  &all_iftypes);
> +		if (ret =3D=3D -ENOMEM) {
> +			break;
> +		} else if (ret) {
> +			ret =3D 0;
>  			continue;

Seems that 'break' is equivalent to just 'return ret'? And that setting
ret =3D 0 seems ... strange.

> -	return 0;
> +	return ret;
>  }

And then you don't need that either which is much nicer anyway...

johannes