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

[Michael Dege]

Hello Paolo,

Thank you for your review.

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Thursday, April 2, 2026 2:59 PM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: paul@xxxxxxxxxxx; niklas.soderlund <niklas.soderlund@xxxxxxxxxxxx>; kuba@xxxxxxxxxx; Yoshihiro
> Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx
> Subject: Re: [net-next,v3,01/13] net: renesas: rswitch: improve port change mode functions
>
> 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&data=05%7C02%7Cmichael.dege%40renesas.com%7C4f84d222a25841bd46eb08de90b7abdd%7C53d82571da19
> 47e49cb4625a166a4a2a%7C0%7C0%7C639107315837152138%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=DbF%2FVyoOLfJSYrnI
> ceiszOHcbvGrEtenm%2Fk%2Fah%2FIrys%3D&reserved=0
> ---
> 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?

I have fixed this and will provide a new version of the patches.

Best regards,

Michael
>
> >  }
> >
> >  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)
>
> [ ... ]