Received-SPF: pass (google.com: domain of linux-wireless+bounces-8675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <a1cff51f789c21b2b307c58ee4d743a62874cec6.camel@sipsolutions.net>
Subject: Re: [RFC v3 8/8] wifi: mac80211: add wiphy radio assignment and
 validation
From: Johannes Berg <johannes@sipsolutions.net>
To: Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
Cc: quic_adisi@quicinc.com, quic_periyasa@quicinc.com, 
	ath12k@lists.infradead.org
Date: Fri, 07 Jun 2024 11:44:00 +0200
In-Reply-To: <9b331a61b8d53284b044bc594cf9952c60164e5f.1717696995.git-series.nbd@nbd.name>
References: 
	<cover.386a66ec6a89750d4890f63f0d28582a52b838b5.1717696995.git-series.nbd@nbd.name>
	 <9b331a61b8d53284b044bc594cf9952c60164e5f.1717696995.git-series.nbd@nbd.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.2 (3.52.2-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
>=20
> +static bool
> +ieee80211_radio_freq_match(const struct wiphy_radio *radio,
> +			   const struct ieee80211_chan_req *chanreq)
> +{
> +	const struct wiphy_radio_freq_range *r;
> +	u32 freq;
> +	int i;
> +
> +	freq =3D ieee80211_channel_to_khz(chanreq->oper.chan);
> +	for (i =3D 0; i < radio->n_freq_range; i++) {
> +		r =3D &radio->freq_range[i];
> +		if (freq >=3D r->start_freq && freq <=3D r->end_freq)
> +			return true;

IMHO should be inclusive only on one side of the range. Can always make
it work since channels are at least a few MHz apart, but the purist in
me says it's easier to grok if the end is not inclusive :)

Maybe this should be a cfg80211 helper like

struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef);

which could also check that the _whole_ chandef fits (though that should
probably also be handled elsewhere, like chandef_valid), and you can use
it to get the radio pointer and then check for =3D=3D below.

The point would be to have a single place with this kind of range logic.
This is only going to get done by mac80211 now, but it'd really be
awkward if some other driver had some other logic later.

>  	if (!curr_ctx || (curr_ctx->replace_state =3D=3D
>  			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
> @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *=
local,
>  			if (!list_empty(&ctx->reserved_links))
>  				continue;
> =20
> +			if (ctx->conf.radio_idx >=3D 0) {
> +					radio =3D &wiphy->radio[ctx->conf.radio_idx];
> +					if (!ieee80211_radio_freq_match(radio, chanreq))
> +							continue;
> +			}

something happened to indentation here :)

> +static bool
> +ieee80211_find_available_radio(struct ieee80211_local *local,
> +			       const struct ieee80211_chan_req *chanreq,
> +			       int *radio_idx)
> +{
> +	struct wiphy *wiphy =3D local->hw.wiphy;
> +	const struct wiphy_radio *radio;
> +	int i;
> +
> +	*radio_idx =3D -1;
> +	if (!wiphy->n_radio)
> +		return true;
> +
> +	for (i =3D 0; i < wiphy->n_radio; i++) {
> +		radio =3D &wiphy->radio[i];
> +		if (!ieee80211_radio_freq_match(radio, chanreq))
> +			continue;
> +
> +		if (!ieee80211_can_create_new_chanctx(local, i))
> +			continue;
> +
> +		*radio_idx =3D i;
> +		return true;
> +	}
> +
> +	return false;
> +}

which would also get used here to find the radio first, though would
have to differentiate n_radio still, of course.

johannes