Received-SPF: pass (google.com: domain of linux-wireless+bounces-5458-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: <29fc2719d6814a8e84f5db9ad81c76116a52828b.camel@sipsolutions.net>
Subject: Re: [PATCH 06/13] wifi: nl80211: send iface combination to user
 space in multi-hardware 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 14:33:49 +0100
In-Reply-To: <20240328072916.1164195-7-quic_periyasa@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
	 <20240328072916.1164195-7-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

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>=20
> + *	When describing per-hardware combinations in multi-hardware in
> + *	one wiphy model, the first possibility can further include the finer
> + *	capabilities like below

Not sure I'd say "below" rather than e.g. "like this:"

> + *	hw_chan_idx =3D 0, numbers =3D [ #{STA} <=3D 1, #{AP} <=3D 1 ],
> + *	channels =3D 1, max =3D 2
> + *	=3D> allows a STA plus an AP interface on the underlying hardware mac
> + *	   advertised at index 0 in wiphy @hw_chans array.
> + *	hw_chan_idx =3D 1, numbers =3D [ #{STA} <=3D 1, #{AP} <=3D 2 ],
> + *	channels =3D 1, max =3D 3
> + *	=3D> allows a STA plus two AP interfaces on the underlying hardware m=
ac
> + *	   advertised at index 1 in wiphy @hw_chans array.

Have you checked the rst output for this? Seems likely that's not going
to be great with that formatting, but I haven't checked.

> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the i=
ndex
> + *	to the wiphy @hw_chans list for which the iface combination is being
> + *	described.

"@hw_chans" doesn't make sense here, this is nl80211, it should refer to
some attribute

but why didn't you just _say_ in the patch 2 discussion that it's used
here ...

> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing t=
he
> + *	limits for the given interface types, see
> + *	&enum nl80211_iface_limit_attrs.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the max=
imum
> + *	number of interfaces that can be created in this group. This number
> + *	does not apply to the interfaces purely managed in software.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifyin=
g the
> + *	number of different channels that can be used in this group.
> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
> + */
> +enum nl80211_if_comb_per_hw_comb_attrs {
> +	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> +	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
> +	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
> +	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
> +	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,

Almost all these attributes duplicate - including their docs -
attributes from enum nl80211_if_combination_attrs. Is it really worth
doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
the different uses of the attribute set?

> +	hw_combis =3D nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
> +	if (!hw_combis)
> +		return -ENOBUFS;
> +
> +	for (i =3D 0; i < c->n_hw_list; i++) {
> +		struct nlattr *hw_combi, *limits;
> +
> +		hw_combi =3D nla_nest_start(msg, i + 1);

And of course the array and splitting discussions apply here too.

johannes