Received-SPF: pass (google.com: domain of linux-wireless+bounces-5456-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <8e1d403251a8c8e5c9fe34d9dea51b8ea60b6b6b.camel@sipsolutions.net>
Subject: Re: [PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb
 advertisement for multi-hardware dev
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:22:43 +0100
In-Reply-To: <20240328072916.1164195-5-quic_periyasa@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
	 <20240328072916.1164195-5-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

Not sure why this is a combined cfg/mac patch, there doesn't seem to be
all that much need for that? I'd probably rather see this and patch 6
squashed, but cfg/mac kept separate.


On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:

>=20
> +static int
> +ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local,
> +				  const struct ieee80211_iface_combination *c)
> +{
> +	int hw_idx, lmt_idx;
> +	u32 hw_idx_bm =3D 0;

lmt_idx? (also that's only needed in the loop)
bm?

> +	if (!local->hw.wiphy->num_hw)
> +		return -EINVAL;
> +
> +	if (local->emulate_chanctx)
> +		return -EINVAL;
> +
> +	for (hw_idx =3D 0; hw_idx < c->n_hw_list; hw_idx++) {
> +		const struct ieee80211_iface_per_hw *hl;

hl?

Could have a bit more evocative names :)

I'd rather see 'i' for some iteration thingie  than "lmt_idx" which
means nothing on first reading either, but you start thinking it should
mean something ...

> +static bool
> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chan=
s)
> +{
> +	int i;
> +
> +	for (i =3D 0; i < chans->n_chans; i++) {
> +		if (chans->chans[i].band =3D=3D NL80211_BAND_5GHZ &&
> +		    ((chans->chans[i].center_freq >=3D 5250 &&
> +		     chans->chans[i].center_freq <=3D 5340) ||
> +		    (chans->chans[i].center_freq >=3D 5480 &&
> +		     chans->chans[i].center_freq <=3D 5720)))

???

That's not how this works upstream.

> +		if (WARN_ON(hl->max_interfaces < 2 &&
> +			    (!comb->radar_detect_widths ||
> +			     !(cfg80211_hw_chans_includes_dfs(chans)))))

No need for extra parentheses.

> @@ -701,6 +786,13 @@ static int wiphy_verify_combinations(struct wiphy *w=
iphy)
>  		/* You can't even choose that many! */
>  		if (WARN_ON(cnt < c->max_interfaces))
>  			return -EINVAL;
> +
> +		/* Do similar validations on the freq range specific interface
> +		 * combinations when advertised.
> +		 */
> +		if (WARN_ON(c->n_hw_list &&
> +			    wiphy_verify_comb_per_hw(wiphy, c)))

Don't need the n_hw_list check here, the function just does nothing if
it's 0 anyway.

johannes