Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0af1c2b6e7b964b559989e860e600a5c27372e83.camel@sipsolutions.net>
Subject: Re: [RFC 1/4] wifi: cfg80211: Add provision to advertise multiple
 radio in one wiphy
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
Cc:     linux-wireless@vger.kernel.org
Date:   Fri, 21 Oct 2022 14:04:17 +0200
In-Reply-To: <20220920100518.19705-2-quic_vthiagar@quicinc.com>
References: <20220920100518.19705-1-quic_vthiagar@quicinc.com>
         <20220920100518.19705-2-quic_vthiagar@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> The prerequisite for MLO support in cfg80211/mac80211 is that all
> the links participating in MLO must be from the same wiphy/ieee80211_hw.
> To meet this expectation, some drivers may need to group multiple discret=
e
> hardware each acting as a link in MLO under one wiphy. Though most of the
> hardware abstractions must be handled within the driver itself, it may be
> required to have some sort of mapping while describing interface
> combination capabilities for each of the underlying hardware. This commit
> adds an advertisement provision for drivers supporting such MLO mode of
> operation.
>=20
> Capability advertisement such as the number of supported channels for eac=
h
> physical hardware has been identified to support some of the common use
> cases.
>=20
> Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
> ---
>  include/net/cfg80211.h |  24 +++++++++
>  net/wireless/core.c    | 109 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
>=20
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index e09ff87146c1..4662231ad068 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -5088,6 +5088,18 @@ struct wiphy_iftype_akm_suites {
>  	int n_akm_suites;
>  };
> =20
> +/**
> + * struct ieee80211_supported_chans_per_hw - supported channels as per t=
he
> + * underlying physical hardware configuration
> + *
> + * @n_chans: number of channels in @chans
> + * @chans: list of channels supported by the constituent hardware
> + */
> +struct ieee80211_chans_per_hw {
> +	int n_chans;

nit: unsigned?

> + * @hw_chans: list of the channels supported by every constituent underl=
ying

"every" here refers to the fact that you have all the channels, right? I
mean, hw_chans is per hardware, basically.

> + *	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.
> + * @num_hw: number of underlying hardware for which the channels list ar=
e
> + *	advertised in @hw_chans.
> + *
>   */
>  struct wiphy {
>  	struct mutex mtx;
> @@ -5445,6 +5466,9 @@ struct wiphy {
>  	u8 ema_max_profile_periodicity;
>  	u16 max_num_akm_suites;
> =20
> +	struct ieee80211_chans_per_hw **hw_chans;
> +	int num_hw;

also here, maybe unsigned.

> +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;
> +	bool found;
> +	int i, j;
> +
> +	for (i =3D 0; i < chans->n_chans; i++) {
> +		found =3D false;

nit: you can move the variable declaration here

	bool found =3D false;
	\n

since you don't need it outside the loop.

> +	for (i =3D 0; i < wiphy->num_hw; i++) {
> +		for (j =3D 0; j < wiphy->num_hw; j++) {
> +			const struct ieee80211_chans_per_hw *hw_chans1;
> +			const struct ieee80211_chans_per_hw *hw_chans2;
> +
> +			if (i =3D=3D j)
> +				continue;

It's symmetric, if I read the code above right, so you can do

	for (j =3D i; j < ...; j++)

in the second loop and avoid this?


> +		hw_chans =3D wiphy->hw_chans[i];
> +		if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans)) {
> +			WARN_ON(1);

I can kind of see the point in not using WARN_ON(!cfg80211_hw_chans...)
since it gets really long, but maybe just remove the warning?
=20
> +	if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) {
> +		WARN_ON(1);
> +		return -EINVAL;
>=20

Anyway you'll have one here, and it's not directly visible which
condition failed anyway.

And you could use WARN_ON(validate(...)) here :)

johannes