RE: [PATCH net-next v5 08/14] net: renesas: rswitch: add locking for agent clock control

[Michael Dege]

Hello Jakub,

Thank you for your comment.

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Monday, May 25, 2026 10:41 PM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; niklas.soderlund
> <niklas.soderlund@xxxxxxxxxxxx>; paul@xxxxxxxxxxx; richardcochran@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> linux-renesas-soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v5 08/14] net: renesas: rswitch: add locking for agent clock control
> 
> 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: add locking for agent clock control
> 
> This commit introduces an agent_lock to serialize access to the shared RCEC and RCDC agent clock
> registers. This aims to prevent potential race conditions between ETHA and GWCA ports when enabling or
> disabling clocks.
> 
> > diff --git a/drivers/net/ethernet/renesas/rswitch.h
> > b/drivers/net/ethernet/renesas/rswitch.h
> > --- a/drivers/net/ethernet/renesas/rswitch.h
> > +++ b/drivers/net/ethernet/renesas/rswitch.h
> [ ... ]
> > @@ -1207,6 +1208,8 @@ struct rswitch_private {
> >  	struct list_head port_list;
> >
> >  	spinlock_t lock;	/* lock interrupt registers' control */
> > +	spinlock_t agent_lock;	/* rswitch agent clock control */
> > +
> 
> [Severity: High]
> Is this lock initialized anywhere before it is used?
> 
> It looks like agent_lock is added to the structure, but spin_lock_init() is missing from the driver
> probe path. Depending on kzalloc for zero-initialization is not valid for spinlocks and can cause
> issues on architectures or kernel configurations that require explicit initialization.
> 

Good point fixed this.

> > diff --git a/drivers/net/ethernet/renesas/rswitch_main.c
> > b/drivers/net/ethernet/renesas/rswitch_main.c
> > --- a/drivers/net/ethernet/renesas/rswitch_main.c
> > +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> > @@ -173,8 +173,12 @@ int rswitch_gwca_change_mode(struct
> > rswitch_private *priv,  {
> >  	int ret;
> >
> > -	if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index))
> > +	if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index)) {
> > +		spin_lock(&priv->agent_lock);
> >  		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 1);
> > +		spin_lock(&priv->agent_lock);
> > +	}
> 
> [Severity: High]
> Does this code cause an immediate deadlock?
> 
> Because spin_lock() is called twice consecutively without a spin_unlock(), it appears the CPU will
> spin indefinitely waiting for the lock it already holds.

This is a bug. I also fixed it.

Best regards,

Michael