RE: [PATCH] can: rcar_canfd: Change the initializing flow for clocks and resets

[Biju Das]

Hi Wolfram Sang,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 25 June 2026 21:27
> Subject: Re: [PATCH] can: rcar_canfd: Change the initializing flow for clocks and resets
> 
> Hi Biju,
> 
> Sashiko found issues with your patch. I curated the list and left those which I see as reasonable. I know
> that being pointed to pre-existing issues is annoying, but maybe you have interest to look at these
> issues?
> 
> Thank you and happy hacking,
> 
>    Wolfram
> 
> > Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] Double-cleanup and unclocked register access on initialization failure in
> `rcar_canfd_probe()`.
> > - [High] Premature registration of global interrupt handlers causes NULL pointer dereference and
> unclocked register access.
> > - [High] Dangling pointer in the global state array leads to a double free on channel probe failure.
> > - [High] Mismatch between `devm_request_irq` and manual memory freeing causes use-after-free.
> > --
> >
> > commit 6b6a83169ac0a7297a4dc2c78e16dd98f4f2ed0f
> > Author: Tu Nguyen <tu.nguyen.xg@xxxxxxxxxxx>
> >
> > can: rcar_canfd: Change the initializing flow for clocks and resets
> >
> > This patch updates the initialization order for clocks and resets in
> > the rcar_canfd driver to match the hardware manual for RZ/G2L and RZ/G3E.
> > It ensures clocks are enabled before deasserting resets, and updates
> > the teardown path to mirror the new sequence.
> >
> > > diff --git a/drivers/net/can/rcar/rcar_canfd.c
> > > b/drivers/net/can/rcar/rcar_canfd.c
> > > index eaf8cac780387..fcc37b73ed436 100644
> > > --- a/drivers/net/can/rcar/rcar_canfd.c
> > > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > [ ... ]
> > > @@ -2090,10 +2090,10 @@ static void rcar_canfd_global_deinit(struct rcar_canfd_global *gpriv, bool
> full)
> > >  		rcar_canfd_set_bit(gpriv->base, RCANFD_GCTR, RCANFD_GCTR_GSLPR);
> > >  	}
> > >
> > > -	clk_disable_unprepare(gpriv->clk_ram);
> > > -	clk_disable_unprepare(gpriv->clkp);
> > >  	reset_control_assert(gpriv->rstc2);
> > >  	reset_control_assert(gpriv->rstc1);
> > > +	clk_disable_unprepare(gpriv->clk_ram);
> > > +	clk_disable_unprepare(gpriv->clkp);
> > >  }
> > >
> > [Severity: High]
> > This is a pre-existing issue, but I noticed a potential problem with
> > interrupt registration order.
> >
> > In rcar_canfd_probe(), global interrupts are requested via
> > devm_request_irq() before rcar_canfd_global_init() enables the clocks
> > and before the per-channel structures in gpriv->ch[] are allocated.

OK. Ordering needs to be changed on safer side. After Channel probe
Register global interrupts.

> >
> > If the interrupt is shared and fires immediately, the handler
> > rcar_canfd_global_err_interrupt() will invoke rcar_canfd_handle_global_err().

Since rcar_canfd_global_init() is not called before, bootloader enabling the
interrupt enable can fire the interrupt. This is a real issue needs to be fixed.

> > This function dereferences priv = gpriv->ch[ch], which is currently NULL.
> > Could this lead to a NULL pointer dereference, 

OK.

> > as well as an unclocked hardware read?

Without a clock running on the IP, Do you agree with sashiko that
the IP will trigger an interrupt and does unclocked hardware read?

Cheers,
Biju