Received-SPF: pass (google.com: domain of linux-wireless+bounces-8671-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: <0ae7869140c8c2537fc638dba14223b25383f3d9.camel@sipsolutions.net>
Subject: Re: [RFC v3 2/8] wifi: cfg80211: add support for advertising
 multiple radios belonging to a wiphy
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:24:56 +0200
In-Reply-To: <db7d83ea6d97e118a14029727e9e18d6e47b753d.1717696995.git-series.nbd@nbd.name>
References: 
	<cover.386a66ec6a89750d4890f63f0d28582a52b838b5.1717696995.git-series.nbd@nbd.name>
	 <db7d83ea6d97e118a14029727e9e18d6e47b753d.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:

> The prerequisite for MLO support in cfg80211/mac80211 is that all the lin=
ks
> participating in MLO must be from the same wiphy/ieee80211_hw. To meet th=
is
> expectation, some drivers may need to group multiple discrete hardware ea=
ch
> acting as a link in MLO under single wiphy.

This is of course the motivation now, but I do wonder if this wouldn't
potentially also apply to a single device that's full dual-band capable
in some way? But doesn't really matter now.

But the thing is that it would let userspace differentiate between what
we mostly have today in a single device (multiple channels can be used,
but you have to go to powersave etc.), vs. a fully concurrent device.

IOW, it feels like this could be used to advertise fully concurrent
capabilities?

> + * struct wiphy_radio - This structure describes a physical radio belong=
ing
> + * to a wiphy. It is used to describe concurrent-channel capabilities of=
 the
> + * phy. Only one channel can be active on the radio described by struct
> + * wiphy_radio.

that's a bit long for the 'short description' :P

maybe just say "struct wiphy_radio - physical radio of a wiphy" and move
the full description down.

> + *
> + * @radio: radios belonging to this wiphy
> + * @n_radio: number of radios

Somewhere - either here or above - we should probably say that it's
assumed you only have a single radio (with the properties covered by the
interface combinations in the wiphy itself) if this isn't given at all.

(Which is what we assume today, more or less.)

> +++ b/include/uapi/linux/nl80211.h
> @@ -3401,6 +3401,8 @@ enum nl80211_attrs {
> =20
>  	NL80211_ATTR_ASSOC_SPP_AMSDU,
> =20
> +	NL80211_ATTR_RADIOS,

missing docs

> +/**
> + * enum nl80211_wiphy_radio_attrs - wiphy radio attributes
> + *
> + * @__NL80211_WIPHY_RADIO_ATTR_INVALID: Invalid

maybe if this is WIPHY_RADIO also call it NL80211_ATTR_WIPHY_RADIOS
above?

> + * @NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES: Nested array of frequency rang=
es
> + *	supported by this radio.

Do we really want this complexity? We only have a single range now, do
we expect that to change? Non-contiguous ranges, where a hole in the
middle is supported by another radio?

Not sure I see the value vs. just having min/max freq directly here?

> +	freqs =3D nla_nest_start_noflag(msg, NL80211_WIPHY_RADIO_ATTR_FREQ_RANG=
ES);

Please don't add new _noflag code.

> +	nl_combis =3D nla_nest_start_noflag(msg,
> +					  NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS);

same here

(and yes maybe userspace wants to unify the parsing of this with the
existing interface combinations attribute and pass the attribute ID or
something, but then it can fix the nested flag too.)

johannes