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

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

[Biju]

From: Tu Nguyen <tu.nguyen.xg@renesas.com>

Testing CANFD on RZ/G3E shows that many registers do not reset to their
initial values with the current flow of deasserting resets first and then
enabling clocks.

Based on the HW manual, clocks should be supplied first and the
resets deasserted afterward.

 section 7.4.3 Procedure for Activating Modules: RZ/G2L
 section 4.4.9.3 Procedure for Starting up Units: RZ/G3E

So, update the order of the initializing flow for resets and clocks
to match the hardware manual, resetting all CANFD registers to their
initial values. Also update rcar_canfd_global_deinit() to assert
resets before disabling clocks, so the teardown path mirrors the new
init ordering.

Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family")
Signed-off-by: Tu Nguyen <tu.nguyen.xg@renesas.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 32 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index eaf8cac78038..fcc37b73ed43 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -2003,20 +2003,12 @@ static int rcar_canfd_global_init(struct rcar_canfd_global *gpriv)
 	u32 ch, sts;
 	int err;
 
-	err = reset_control_reset(gpriv->rstc1);
-	if (err)
-		return err;
-
-	err = reset_control_reset(gpriv->rstc2);
-	if (err)
-		goto fail_reset1;
-
 	/* Enable peripheral clock for register access */
 	err = clk_prepare_enable(gpriv->clkp);
 	if (err) {
 		dev_err(dev, "failed to enable peripheral clock: %pe\n",
 			ERR_PTR(err));
-		goto fail_reset2;
+		return err;
 	}
 
 	/* Enable RAM clock */
@@ -2027,10 +2019,18 @@ static int rcar_canfd_global_init(struct rcar_canfd_global *gpriv)
 		goto fail_clk;
 	}
 
+	err = reset_control_reset(gpriv->rstc1);
+	if (err)
+		goto fail_ram_clk;
+
+	err = reset_control_reset(gpriv->rstc2);
+	if (err)
+		goto fail_reset1;
+
 	err = rcar_canfd_reset_controller(gpriv);
 	if (err) {
 		dev_err(dev, "reset controller failed: %pe\n", ERR_PTR(err));
-		goto fail_ram_clk;
+		goto fail_reset2;
 	}
 
 	/* Controller in Global reset & Channel reset mode */
@@ -2068,14 +2068,14 @@ static int rcar_canfd_global_init(struct rcar_canfd_global *gpriv)
 
 fail_mode:
 	rcar_canfd_disable_global_interrupts(gpriv);
-fail_ram_clk:
-	clk_disable_unprepare(gpriv->clk_ram);
-fail_clk:
-	clk_disable_unprepare(gpriv->clkp);
 fail_reset2:
 	reset_control_assert(gpriv->rstc2);
 fail_reset1:
 	reset_control_assert(gpriv->rstc1);
+fail_ram_clk:
+	clk_disable_unprepare(gpriv->clk_ram);
+fail_clk:
+	clk_disable_unprepare(gpriv->clkp);
 	return err;
 }
 
@@ -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);
 }
 
 static int rcar_canfd_probe(struct platform_device *pdev)
-- 
2.43.0

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

[sashiko-bot]

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@renesas.com>

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 is a pre-existing 
potential double-cleanup and unclocked register access on initialization 
failure.

In rcar_canfd_probe(), if rcar_canfd_global_init() encounters an error, 
it disables the clocks and asserts the resets before returning. However, 
the probe function then jumps to the fail_mode label and unconditionally 
calls rcar_canfd_global_deinit(). 

The deinit function calls rcar_canfd_disable_global_interrupts(), which 
writes to the hardware registers. Since the peripheral clock has already 
been disabled by the failed init, does this write trigger a synchronous 
external abort or bus fault?

[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. 

If the interrupt is shared and fires immediately, the handler 
rcar_canfd_global_err_interrupt() will invoke rcar_canfd_handle_global_err().
This function dereferences priv = gpriv->ch[ch], which is currently NULL.
Could this lead to a NULL pointer dereference, as well as an unclocked 
hardware read?

[Severity: High]
This is a pre-existing issue, but there appears to be a dangling pointer 
risk on channel probe failure.

In rcar_canfd_channel_probe(), the channel pointer is saved globally via 
gpriv->ch[priv->channel] = priv. If register_candev() fails shortly after, 
the error path uses free_candev(ndev) to release the memory but leaves the 
pointer in the global array. 

During teardown, rcar_canfd_probe() calls rcar_canfd_channel_remove() for 
all channels in the mask. Since the dangling pointer still evaluates to true, 
will this result in a use-after-free and double free of the memory?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625135216.130450-1-biju.das.jz@bp.renesas.com?part=1

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

[Vincent Mailhol]

On 25/06/2026 at 15:51, Biju wrote:
> From: Tu Nguyen <tu.nguyen.xg@renesas.com>
> 
> Testing CANFD on RZ/G3E shows that many registers do not reset to their
> initial values with the current flow of deasserting resets first and then
> enabling clocks.
> 
> Based on the HW manual, clocks should be supplied first and the
> resets deasserted afterward.
> 
>  section 7.4.3 Procedure for Activating Modules: RZ/G2L
>  section 4.4.9.3 Procedure for Starting up Units: RZ/G3E
> 
> So, update the order of the initializing flow for resets and clocks
> to match the hardware manual, resetting all CANFD registers to their
> initial values. Also update rcar_canfd_global_deinit() to assert
> resets before disabling clocks, so the teardown path mirrors the new
> init ordering.
> 
> Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family")
> Signed-off-by: Tu Nguyen <tu.nguyen.xg@renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>


Yours sincerely,
Vincent Mailhol

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4774 bytes --]

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@renesas.com>
> 
> 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 is a pre-existing 
> potential double-cleanup and unclocked register access on initialization 
> failure.
> 
> In rcar_canfd_probe(), if rcar_canfd_global_init() encounters an error, 
> it disables the clocks and asserts the resets before returning. However, 
> the probe function then jumps to the fail_mode label and unconditionally 
> calls rcar_canfd_global_deinit(). 
> 
> The deinit function calls rcar_canfd_disable_global_interrupts(), which 
> writes to the hardware registers. Since the peripheral clock has already 
> been disabled by the failed init, does this write trigger a synchronous 
> external abort or bus fault?
> 
> [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. 
> 
> If the interrupt is shared and fires immediately, the handler 
> rcar_canfd_global_err_interrupt() will invoke rcar_canfd_handle_global_err().
> This function dereferences priv = gpriv->ch[ch], which is currently NULL.
> Could this lead to a NULL pointer dereference, as well as an unclocked 
> hardware read?
> 
> [Severity: High]
> This is a pre-existing issue, but there appears to be a dangling pointer 
> risk on channel probe failure.
> 
> In rcar_canfd_channel_probe(), the channel pointer is saved globally via 
> gpriv->ch[priv->channel] = priv. If register_candev() fails shortly after, 
> the error path uses free_candev(ndev) to release the memory but leaves the 
> pointer in the global array. 
> 
> During teardown, rcar_canfd_probe() calls rcar_canfd_channel_remove() for 
> all channels in the mask. Since the dangling pointer still evaluates to true, 
> will this result in a use-after-free and double free of the memory?
> 
> [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?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260625135216.130450-1-biju.das.jz@bp.renesas.com?part=1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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@sang-engineering.com>
> 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?

Thanks, Will send patches for fixing the pre-existing issues reported below.

Cheers,
Biju

> 
> > 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@renesas.com>
> >
> > 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 is a pre-existing
> > potential double-cleanup and unclocked register access on
> > initialization failure.
> >
> > In rcar_canfd_probe(), if rcar_canfd_global_init() encounters an
> > error, it disables the clocks and asserts the resets before returning.
> > However, the probe function then jumps to the fail_mode label and
> > unconditionally calls rcar_canfd_global_deinit().
> >
> > The deinit function calls rcar_canfd_disable_global_interrupts(),
> > which writes to the hardware registers. Since the peripheral clock has
> > already been disabled by the failed init, does this write trigger a
> > synchronous external abort or bus fault?
> >
> > [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.
> >
> > If the interrupt is shared and fires immediately, the handler
> > rcar_canfd_global_err_interrupt() will invoke rcar_canfd_handle_global_err().
> > This function dereferences priv = gpriv->ch[ch], which is currently NULL.
> > Could this lead to a NULL pointer dereference, as well as an unclocked
> > hardware read?
> >
> > [Severity: High]
> > This is a pre-existing issue, but there appears to be a dangling
> > pointer risk on channel probe failure.
> >
> > In rcar_canfd_channel_probe(), the channel pointer is saved globally
> > via
> > gpriv->ch[priv->channel] = priv. If register_candev() fails shortly
> > gpriv->after,
> > the error path uses free_candev(ndev) to release the memory but leaves
> > the pointer in the global array.
> >
> > During teardown, rcar_canfd_probe() calls rcar_canfd_channel_remove()
> > for all channels in the mask. Since the dangling pointer still
> > evaluates to true, will this result in a use-after-free and double free of the memory?
> >
> > [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?
> >
> > --
> > Sashiko AI review ·
> > https://sashiko.dev/#/patchset/20260625135216.130450-1-biju.das.jz@bp.
> > renesas.com?part=1

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

[Biju Das]

Hi Wolfram,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 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@renesas.com>
> >
> > 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

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@sang-engineering.com>
> 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@renesas.com>
> >
> > 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