Received-SPF: pass (google.com: domain of linux-wireless+bounces-5464-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <a24b7d967dbe31ef613fae8c331f4ff718482c93.camel@sipsolutions.net>
Subject: Re: [PATCH 09/13] wifi: cfg80211: Add multi-hardware iface
 combination support
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 15:16:26 +0100
In-Reply-To: <20240328072916.1164195-10-quic_periyasa@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
	 <20240328072916.1164195-10-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
>   * @new_beacon_int: set this to the beacon interval of a new interface
>   *	that's not operating yet, if such is to be checked as part of
>   *	the verification
> + * @chandef: Channel definition for which the interface combination is t=
o be
> + *	checked, when checking during interface preparation on a new channel,
> + *	for example. This will be used when the driver advertises underlying
> + *	hw specific interface combination in a multi physical hardware device=
.
> + *	This will be NULL when the interface combination check is not due to
> + *	channel or the interface combination does not include per-hw
> + *	advertisement.

This is input, so "will be" doesn't make much sense, more like "must
be"?

But I'm confused as to how that works with num_different_channels being
here too?

This function was, as far as I can tell, always checking the _full_
state. Now you're changing that, and I'm neither sure why, nor does it
seem well documented.

> + * @n_per_hw: number of Per-HW interface combinations.
> + * @per_hw: @n_per_hw of hw specific interface combinations. Per-hw chan=
nel
> + *	list index as advertised in wiphy @hw_chans is used as index
> + *	in @per_hw to maintain the interface combination of the corresponding
> + *	hw.

What?

If I'm reading that correctly, which is all but guaranteed, doesn't that
actually mean you don't need n_per_hw at all, since it necessarily equal
to n_hw_chans?

> +/**
> + * cfg80211_per_hw_iface_comb_advertised - if per-hw iface combination s=
upported
> + *
> + * @wiphy: the wiphy
> + *
> + * This function is used to check underlying per-hw interface combinatio=
n is
> + * advertised by the driver.
> + */
> +bool cfg80211_per_hw_iface_comb_advertised(struct wiphy *wiphy);

Is that even worth an export rather than being inline? Is it even needed
outside of cfg80211 itself?

Also for cfg80211_get_hw_idx_by_chan(), is it really needed?

I'm also wondering if we really should use the "hw_idx" everywhere.
Maybe that'd be more useful as a pointer to struct
ieee80211_chans_per_hw in most places (other than nl80211, obviously)?

The index always feels pretty fragile, a pointer at least gives us type-
checking?

Even in the interface combination advertising, perhaps, though not sure
how that'd work for the drivers.

> +static const struct ieee80211_iface_per_hw *
> +cfg80211_get_hw_iface_comb_by_idx(struct wiphy *wiphy,
> +				  const struct ieee80211_iface_combination *c,
> +				  int idx)
> +{
> +	int i;
> +
> +	for (i =3D 0; i < c->n_hw_list; i++)
> +		if (c->iface_hw_list[i].hw_chans_idx =3D=3D idx)
> +			break;
> +
> +	if (i =3D=3D c->n_hw_list)
> +		return NULL;
> +
> +	return &c->iface_hw_list[i];
> +}

???

Hint: it's perfectly legal to return directly from a loop.

> +static int
> +cfg80211_validate_iface_comb_per_hw_limits(struct wiphy *wiphy,
> +					   struct iface_combination_params *params,
> +					   const struct ieee80211_iface_combination *c,
> +					   u16 *num_ifaces, u32 *all_iftypes)
> +{
> +	struct ieee80211_iface_limit *limits;
> +	const struct iface_comb_per_hw_params *per_hw;
> +	const struct ieee80211_iface_per_hw *per_hw_comb;
> +	int i, ret =3D 0;

The =3D 0 doesn't seem needed.

> +		ret =3D cfg80211_validate_iface_limits(wiphy,
> +						     per_hw->iftype_num,
> +						     limits,
> +						     per_hw_comb->n_limits,
> +						     all_iftypes);
> +
> +		kfree(limits);
> +
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	return ret;

That 'out' label is pointless.

> +static void cfg80211_put_iface_comb_iftypes(u16 *num_ifaces)
> +{
> +	kfree(num_ifaces);
> +}

Not sure I see value in that indirection?

> +static u16*

missing space

> +cfg80211_get_iface_comb_iftypes(struct wiphy *wiphy,
> +				struct iface_combination_params *params,
> +				u32 *used_iftypes)
> +{
> +	const struct iface_comb_per_hw_params *per_hw;
> +	u16 *num_ifaces;
> +	int i;
> +	u8 num_hw;
> +
> +	num_hw =3D params->n_per_hw ? params->n_per_hw : 1;

I think we're allowed to use the "?:" shortcut.

> +	num_ifaces =3D kcalloc(num_hw, sizeof(*num_ifaces), GFP_KERNEL);
> +	if (!num_ifaces)
> +		return NULL;

But ... maybe we should just cap num_hw to a reasonable limit (4? 5?)
and use a static array in the caller instead of allocating here.

> +	is_per_hw =3D cfg80211_per_hw_iface_comb_advertised(wiphy);

Maybe call that "have_per_hw_combinations" or so? Or "check_per_hw"
even, "is" seems not clear - "what is?"

> +	/* check per HW validation */
> +	if (params->n_per_hw) {
> +		if (!is_per_hw)
> +			return -EINVAL;
> +
> +		if (params->n_per_hw > wiphy->num_hw)
> +			return -EINVAL;
> +	}
> +
> +	if (is_per_hw && params->chandef &&
> +	    cfg80211_chandef_valid(params->chandef))
> +		hw_chan_idx =3D cfg80211_get_hw_idx_by_chan(wiphy,
> +							  params->chandef->chan);
> +
> +	num_ifaces =3D cfg80211_get_iface_comb_iftypes(wiphy,
> +						     params,
> +						     &used_iftypes);
> +	if (!num_ifaces)
> +		return -ENOMEM;

But still like I said above, all this code seems really odd to me now,
it's checking *either* the per-hw and then only for a single HW, *or*
the global, but ... seems it should do full checks for both, if needed?

johannes