Re: [PATCH net-next v5 07/14] net: renesas: rswitch: make helper functions available to whole driver

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: renesas: rswitch: make helper functions available to whole driver

These functions will be needed in L2 and vlan handling. Remove static
declaration and add function prototype to rswitch.h.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index 1f4ec65f6f9bd..20ac8f3b9357f 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
[ ... ]
> @@ -168,14 +168,13 @@ static int rswitch_fwd_init(struct rswitch_private *priv)
>  }
>  
>  /* Gateway CPU agent block (GWCA) */
> -static int rswitch_gwca_change_mode(struct rswitch_private *priv,
> -				    enum rswitch_gwca_mode mode)
> +int rswitch_gwca_change_mode(struct rswitch_private *priv,
> +			     enum rswitch_gwca_mode mode)
>  {
>  	int ret;
>  
>  	if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index))
>  		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 1);

[Severity: High]
This isn't a bug introduced by this patch, but does making these mode change
functions globally available expose an unsynchronized read-modify-write data
race?

Looking at rswitch_agent_clock_ctrl() being called here, it performs an
unprotected read-modify-write on the shared RCEC and RCDC hardware registers:

static void rswitch_agent_clock_ctrl(...)
{
	...
	if (enable) {
		val = ioread32(coma_addr + RCEC);
		iowrite32(val | RCEC_RCE | BIT(port), coma_addr + RCEC);
	}
	...
}

Could concurrent calls from L2 or VLAN paths cause lost clock enable/disable
commands, leading to silent initialization failures for network ports?

I noticed a subsequent commit in this series ("net: renesas: rswitch: add
locking for agent clock control") attempts to fix this using agent_lock.
However, doesn't that fix introduce a hard deadlock by calling
spin_lock(&priv->agent_lock) twice consecutively in
rswitch_gwca_change_mode(), while also failing to initialize the spinlock?

Would it be better to address the missing synchronization correctly before
exporting these functions for broader concurrent use?

> -
>  	iowrite32(GWMC_OPC_DISABLE, priv->addr + GWMC);
>  
>  	ret = rswitch_reg_wait(priv->addr, GWMS, GWMS_OPS_MASK, GWMC_OPC_DISABLE);