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

[Biju Das]

Hi Wolfram,

> -----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?

Since you have gone through the list, I would like to discuss some of the issues here

> >
> > 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 isn't a bug introduced by this patch, but there might be a
> > use-after-free vulnerability with the device-managed IRQs.
> >
> > In rcar_canfd_channel_probe(), the per-channel interrupt is requested
> > using devm_request_irq() with priv as the callback data. However,
> > rcar_canfd_channel_remove() manually frees the channel structure using
> > free_candev(priv->ndev).
> >
> > Because the devm-managed IRQ is not unregistered until the driver
> > probe or remove function completely returns, could an interrupt firing
> > in this teardown window access the freed priv structure?

Source level IRQ for CANFD are enabled by the rcar_canfd_enable_channel_interrupts()
in rcar_canfd_start(). I agree, in the probe we requested IRQ, CLK on, reset desserted
and per-channel irq handler is ready for triggering IRQ.

But without enabling source level IRQ, Do you think interrupt can fire on the
teardown window as mentioned by sashiko?

Even if bootloader programs the value, reset assert/deassert in global_init
will reset the values. So there is no chance of firing interrupt in this case.

Please share your view on this.

Cheers,
Biju