Received-SPF: pass (google.com: domain of linux-wireless+bounces-5444-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e36599d3269496de59505b36be46f873976f8706.camel@sipsolutions.net>
Subject: Re: [PATCH 01/13] wifi: cfg80211: Add provision to advertise
 multiple radio in one wiphy
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 08:46:50 +0100
In-Reply-To: <20240328072916.1164195-2-quic_periyasa@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
	 <20240328072916.1164195-2-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

That's a big set, not sure I can review it all at once :)


> +struct ieee80211_chans_per_hw {
> +	u32 n_chans;
> +	struct ieee80211_channel chans[];

That should probably use __counted_by() these days.

> + * @hw_chans: list of the channels supported by every constituent underl=
ying
> + *	hardware. Drivers abstracting multiple discrete hardware (radio) unde=
r
> + *	one wiphy can advertise the list of channels supported by each physic=
al
> + *	hardware in this list. Underlying hardware specific channel list can =
be
> + *	used while describing interface combination for each of them.

I'd expect there to be a limit on channels being within a single band on
a single "hardware"?

> + * @num_hw: number of underlying hardware for which the channels list ar=
e

"number of [...] hardware" sounds odd to me, perhaps 'devices'?

> + *	advertised in @hw_chans, 0 if multi hardware is not support. Expect >=
2
> + *	if multi hardware is support.

>1, I guess, not >2 - and =3D=3D1 makes no sense really, maybe it should sa=
y
that?

> +static bool
> +cfg80211_hw_chans_in_supported_list(struct wiphy *wiphy,
> +				    const struct ieee80211_chans_per_hw *chans)
> +{
> +	enum nl80211_band band;
> +	struct ieee80211_supported_band *sband;
> +	int i, j;
> +
> +	for (i =3D 0; i < chans->n_chans; i++) {
> +		bool found =3D false;

So if there should be a limit on single band per HW then probably this
check could be written more efficiently: finding the band first based on
the first channel, and then checking all the channels exist within the
band.

johannes