Re: [PATCH v2 net-next 08/14] net: enetc: add multiple command BD rings support

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: enetc: add multiple command BD rings support

This patch adds support for multiple command BD rings to the NTMP driver.
The switch has two command BD rings, and netc_select_cbdr() is added to
select an appropriate ring when the current one is busy.

> diff --git a/drivers/net/ethernet/freescale/enetc/ntmp.c b/drivers/net/ethernet/freescale/enetc/ntmp.c
> --- a/drivers/net/ethernet/freescale/enetc/ntmp.c
> +++ b/drivers/net/ethernet/freescale/enetc/ntmp.c
> @@ -117,6 +117,25 @@ static void ntmp_clean_cbdr(struct netc_cbdr *cbdr)
>  	cbdr->next_to_clean = i;
>  }
>
> +static struct netc_cbdr *netc_select_cbdr(struct ntmp_user *user)
> +{
> +	int cpu, i;
> +
> +	for (i = 0; i < user->cbdr_num; i++) {
> +		if (spin_is_locked(&user->ring[i].ring_lock))
> +			continue;
> +
> +		return &user->ring[i];
> +	}

Frank Li raised a concern about the use of spin_is_locked() here. Is this
lockless check reliable across all architectures? spin_is_locked() behavior
varies across configurations - it can return false negatives on UP builds and
may not provide the ordering guarantees needed for lockless checks.

Would spin_trylock() be more appropriate here? Or if this racy check is
intentional and acceptable, should it be documented why the potential race is
safe in this context?

> +
> +	/* If all the command BDRs are busy now, we select
> +	 * one of them, but need to wait for a while to use.
> +	 */
> +	cpu = raw_smp_processor_id();
> +
> +	return &user->ring[cpu % user->cbdr_num];
> +}
> +
>  static int netc_xmit_ntmp_cmd(struct ntmp_user *user, union netc_cbd *cbd)
>  {
>  	union netc_cbd *cur_cbd;
> @@ -125,10 +144,10 @@ static int netc_xmit_ntmp_cmd(struct ntmp_user *user, union netc_cbd *cbd)
>  	u16 status;
>  	u32 val;
>
> -	/* Currently only i.MX95 ENETC is supported, and it only has one
> -	 * command BD ring
> -	 */
> -	cbdr = &user->ring[0];
> +	if (user->cbdr_num == 1)
> +		cbdr = &user->ring[0];
> +	else
> +		cbdr = netc_select_cbdr(user);

This isn't a bug, but Frank Li suggested that the special case for
cbdr_num == 1 might be redundant. Since netc_select_cbdr() would handle the
single ring case correctly (the loop would find the only ring on the first
iteration), would it be simpler to just unconditionally call:

    cbdr = netc_select_cbdr(user);

>
>  	spin_lock_bh(&cbdr->ring_lock);
>