Re: [net-next,v3,01/13] net: renesas: rswitch: improve port change mode functions

[Paolo Abeni]

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

For ETHA and GWCA ports every mode change from operational to
configuration and vice-versa requires to change the mode to "disable"
first. This was done by calling the function twice. Let the change_mode
function take care of this required step in a single call.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index 6fe964816322..f6d1e610e7fa 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> @@ -176,14 +176,19 @@ 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 (mode == GWMC_OPC_DISABLE)
> +	if (mode == GWMC_OPC_DISABLE) {
>  		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 0);
>
> -	return ret;
> +		return ret;
> +	}
> +
> +	iowrite32(mode, priv->addr + GWMC);
> +
> +	return rswitch_reg_wait(priv->addr, GWMS, GWMS_OPS_MASK, mode);

Should the error from the initial DISABLE transition be checked before
proceeding? When mode != GWMC_OPC_DISABLE, this code stores the result of
rswitch_reg_wait() for the DISABLE transition in 'ret' but never checks it.
If the hardware fails to reach DISABLE state, the function silently proceeds
to write the target mode.

Before this patch, callers explicitly checked the error after calling
rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE):

rswitch_gwca_hw_init() and rswitch_gwca_hw_deinit():
    err = rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE);
    if (err < 0)
        return err;

This patch removes those caller-side checks but doesn't replicate the error
checking inside rswitch_gwca_change_mode(). If the first rswitch_reg_wait()
returns -ETIMEDOUT, should the function return that error before attempting
to write the target mode?

>  }
>
>  static int rswitch_gwca_mcast_table_reset(struct rswitch_private *priv)
> @@ -682,9 +687,6 @@ static int rswitch_gwca_hw_init(struct rswitch_private *priv)
>  	unsigned int i;
>  	int err;
>
> -	err = rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE);
> -	if (err < 0)
> -		return err;
>  	err = rswitch_gwca_change_mode(priv, GWMC_OPC_CONFIG);
>  	if (err < 0)
>  		return err;

[ ... ]

> @@ -717,9 +719,6 @@ static int rswitch_gwca_hw_init(struct rswitch_private *priv)
>  			return err;
>  	}
>
> -	err = rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE);
> -	if (err < 0)
> -		return err;
>  	return rswitch_gwca_change_mode(priv, GWMC_OPC_OPERATION);
>  }
>
> @@ -727,9 +726,6 @@ static int rswitch_gwca_hw_deinit(struct rswitch_private *priv)
>  {
>  	int err;
>
> -	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 +1112,18 @@ 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 (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);

Same pattern here in rswitch_etha_change_mode(). When mode !=
EAMC_OPC_DISABLE, the return value from rswitch_reg_wait() for the
intermediate DISABLE transition is stored in 'ret' but never checked.

Before this patch, rswitch_etha_hw_init() explicitly called
rswitch_etha_change_mode(etha, EAMC_OPC_DISABLE) with error checking before
calling it with EAMC_OPC_CONFIG. This patch removes that caller-side check
but doesn't replicate it inside the function. Should there be a check like
'if (ret < 0) return ret;' after the first rswitch_reg_wait()?

>  }
>
>  static void rswitch_etha_read_mac_address(struct rswitch_etha *etha)

[ ... ]