Re: [PATCH net-next v5 01/14] net: renesas: rswitch: improve port change mode functions

[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: improve port change mode functions

This commit consolidates the mode change logic for ETHA and GWCA ports
in the Renesas switch driver. It ensures the required transition to
the "disable" mode is handled automatically within a single call to
the mode change functions, removing redundant manual calls.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index 6fe964816322a..9b739594cc028 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> @@ -176,14 +176,21 @@ static int rswitch_gwca_change_mode(struct rswitch_private *priv,
>  	if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index))
>  		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 1);
>  
> -	iowrite32(mode, priv->addr + GWMC);
> +	iowrite32(GWMC_OPC_DISABLE, priv->addr + GWMC);
>  
> -	ret = rswitch_reg_wait(priv->addr, GWMS, GWMS_OPS_MASK, mode);
> +	ret = rswitch_reg_wait(priv->addr, GWMS, GWMS_OPS_MASK, GWMC_OPC_DISABLE);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Does this error path leak the agent clock resource?

In the pre-patch code, the clock was disabled unconditionally if the target
mode was DISABLE, even if rswitch_reg_wait() timed out. By returning early
here when ret < 0, it appears the rswitch_agent_clock_ctrl(..., 0) cleanup
is skipped if the hardware times out during the transition.

>  
> -	if (mode == GWMC_OPC_DISABLE)
> +	if (mode == GWMC_OPC_DISABLE) {
>  		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 0);
>  
> -	return ret;
> +		return ret;
> +	}

[ ... ]

> @@ -727,9 +728,6 @@ static int rswitch_gwca_hw_deinit(struct rswitch_private *priv)
>  {
>  	int err;
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the hardware teardown
sequence leave transmitted SKBs queued indefinitely?

When shutting down the driver, rswitch_deinit() invokes this function
to disable the hardware data paths before the unregister_netdev() loop
executes. While the netdevs are still registered and up, the network stack
can continue to invoke ndo_start_xmit.

Because the hardware is disabled early, are these transmitted SKBs and their
DMA mappings queued without ever completing, causing them to be leaked later
when rswitch_txdmac_free() frees the rings? Should unregister_netdev() be
called before shutting down the hardware?

> -	err = rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE);
> -	if (err < 0)
> -		return err;
>  	err = rswitch_gwca_change_mode(priv, GWMC_OPC_RESET);
>  	if (err < 0)
>  		return err;

[ ... ]

> @@ -1116,14 +1114,20 @@ static int rswitch_etha_change_mode(struct rswitch_etha *etha,
>  	if (!rswitch_agent_clock_is_enabled(etha->coma_addr, etha->index))
>  		rswitch_agent_clock_ctrl(etha->coma_addr, etha->index, 1);
>  
> -	iowrite32(mode, etha->addr + EAMC);
> -
> -	ret = rswitch_reg_wait(etha->addr, EAMS, EAMS_OPS_MASK, mode);
> +	iowrite32(EAMC_OPC_DISABLE, etha->addr + EAMC);
> +	ret = rswitch_reg_wait(etha->addr, EAMS, EAMS_OPS_MASK, EAMC_OPC_DISABLE);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Will this return cause a similar clock resource leak if the hardware wait times
out, skipping the clock cleanup for EAMC_OPC_DISABLE?

>  
> -	if (mode == EAMC_OPC_DISABLE)
> +	if (mode == EAMC_OPC_DISABLE) {
>  		rswitch_agent_clock_ctrl(etha->coma_addr, etha->index, 0);
>  
> -	return ret;
> +		return ret;
> +	}
> +
> +	iowrite32(mode, etha->addr + EAMC);
> +
> +	return rswitch_reg_wait(etha->addr, EAMS, EAMS_OPS_MASK, mode);
>  }

[ ... ]

> @@ -1203,9 +1207,6 @@ static int rswitch_etha_hw_init(struct rswitch_etha *etha, const u8 *mac)
>  {
>  	int err;
>  
> -	err = rswitch_etha_change_mode(etha, EAMC_OPC_DISABLE);
> -	if (err < 0)
> -		return err;

[Severity: Low]
Is this explicit DISABLE mode transition still necessary?

Looking at the end of this function after the patch is applied, there is
another transition to DISABLE before switching to OPERATION:

    err = rswitch_etha_change_mode(etha, EAMC_OPC_DISABLE);
    if (err < 0)
        return err;

    return rswitch_etha_change_mode(etha, EAMC_OPC_OPERATION);

Since rswitch_etha_change_mode() now handles the DISABLE transition
internally, does leaving this explicit call result in redundant transitions
and unnecessary clock toggling?

>  	err = rswitch_etha_change_mode(etha, EAMC_OPC_CONFIG);
>  	if (err < 0)
>  		return err;