[PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Marek Vasut]

Initialize PCIEDMAINTSTSEN in rcar_gen4_pcie_ep_pre_init() to 0,
and in case rcar_gen4_pcie_common_init() fails and the result
propagates to dw_pcie_ep_init(), the invoke common deinit instead
of endpoint deinit, because we are certain the PCIEDMAINTSTSEN
register is already 0 and the common deinit does not do any register
accesses.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Koichiro Den <den@valinux.co.jp>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index fbe465a29068f..b47281b30db96 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -487,6 +487,8 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
 	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
 	int ret;
 
+	writel(0, rcar->base + PCIEDMAINTSTSEN);
+
 	ret = rcar_gen4_pcie_common_init(rcar);
 	if (ret)
 		return ret;
@@ -571,7 +573,7 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 
 	ret = dw_pcie_ep_init(ep);
 	if (ret) {
-		rcar_gen4_pcie_ep_deinit(rcar);
+		rcar_gen4_pcie_common_deinit(rcar);
 		return ret;
 	}
 
-- 
2.53.0

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Koichiro Den]

On Thu, Aug 13, 2026 at 11:18:22AM +0200, Marek Vasut wrote:
> Initialize PCIEDMAINTSTSEN in rcar_gen4_pcie_ep_pre_init() to 0,
> and in case rcar_gen4_pcie_common_init() fails and the result
> propagates to dw_pcie_ep_init(), the invoke common deinit instead
> of endpoint deinit, because we are certain the PCIEDMAINTSTSEN
> register is already 0 and the common deinit does not do any register
> accesses.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Koichiro Den <den@valinux.co.jp>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index fbe465a29068f..b47281b30db96 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -487,6 +487,8 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>  	int ret;
>  
> +	writel(0, rcar->base + PCIEDMAINTSTSEN);
> +

Hi Marek,

Thanks for adding me to Cc here. I tested this patch on S4 Spider and saw no
regression. I have two questions though.

1. The new writel(0, PCIEDMAINTSTSEN) runs before clk_bulk_prepare_enable().
   Is MSTP024 expected to be already released at this point? I added the quick
   check like below on Spider, and confirmed that the clock was already enabled.

   --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
   +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
   @@ -9,6 +9,7 @@
     */
    
    #include <linux/delay.h>
   +#include <linux/clk-provider.h>
    #include <linux/firmware.h>
    #include <linux/interrupt.h>
    #include <linux/io.h>
   @@ -487,6 +488,10 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
           struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
           int ret;
    
   +       if (__clk_is_enabled(dw->core_clks[DW_PCIE_CORE_CLK].clk))
   +               dev_info(dw->dev,
   +                        "PCIEC module clock was already enabled\n");
   +
           writel(0, rcar->base + PCIEDMAINTSTSEN);
    
           ret = rcar_gen4_pcie_common_init(rcar);

   If that is not guaranteed, wouldn't this write rely on firmware or some
   earlier user leaving the module clock enabled?

>  	ret = rcar_gen4_pcie_common_init(rcar);
>  	if (ret)
>  		return ret;
> @@ -571,7 +573,7 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
>  
>  	ret = dw_pcie_ep_init(ep);
>  	if (ret) {
> -		rcar_gen4_pcie_ep_deinit(rcar);
> +		rcar_gen4_pcie_common_deinit(rcar);

2. It seems to me dw_pcie_ep_init() can fail:
   - before pre_init() has run,
   - from inside common_init(), or
   - after pre_init() has succeeded and written PCIEDMAINTSTSEN_INIT (e.g.
     pci_epc_mem_init() fails)

   The caller cannot tell which case returned the error. In the first case,
   common_init() has not run, while in the last case PCIEDMAINTSTSEN is not 0.
   So this seems to me different from what the commit message says. Is
   common_deinit() intended to be safe in all three cases?

I may be missing some detail here. Could you clarify this for me?

Best regards,
Koichiro

>  		return ret;
>  	}
>  
> -- 
> 2.53.0
> 

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Geert Uytterhoeven]

Hi Den-san,

On Tue, 18 Aug 2026 at 07:55, Koichiro Den <den@valinux.co.jp> wrote:
> On Thu, Aug 13, 2026 at 11:18:22AM +0200, Marek Vasut wrote:
> > Initialize PCIEDMAINTSTSEN in rcar_gen4_pcie_ep_pre_init() to 0,
> > and in case rcar_gen4_pcie_common_init() fails and the result
> > propagates to dw_pcie_ep_init(), the invoke common deinit instead
> > of endpoint deinit, because we are certain the PCIEDMAINTSTSEN
> > register is already 0 and the common deinit does not do any register
> > accesses.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -487,6 +487,8 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> >       struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> >       int ret;
> >
> > +     writel(0, rcar->base + PCIEDMAINTSTSEN);
> > +
>
> 1. The new writel(0, PCIEDMAINTSTSEN) runs before clk_bulk_prepare_enable().
>    Is MSTP024 expected to be already released at this point? I added the quick
>    check like below on Spider, and confirmed that the clock was already enabled.
>
>    --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>    +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>    @@ -9,6 +9,7 @@
>      */
>
>     #include <linux/delay.h>
>    +#include <linux/clk-provider.h>
>     #include <linux/firmware.h>
>     #include <linux/interrupt.h>
>     #include <linux/io.h>
>    @@ -487,6 +488,10 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
>            struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>            int ret;
>
>    +       if (__clk_is_enabled(dw->core_clks[DW_PCIE_CORE_CLK].clk))
>    +               dev_info(dw->dev,
>    +                        "PCIEC module clock was already enabled\n");
>    +
>            writel(0, rcar->base + PCIEDMAINTSTSEN);
>
>            ret = rcar_gen4_pcie_common_init(rcar);
>
>    If that is not guaranteed, wouldn't this write rely on firmware or some
>    earlier user leaving the module clock enabled?

The module clock is enabled through Runtime PM:

    rcar_gen4_pcie_probe
    -> rcar_gen4_pcie_prepare
            -> pm_runtime_resume_and_get

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Koichiro Den]

On Tue, Aug 18, 2026 at 08:57:55AM +0200, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> On Tue, 18 Aug 2026 at 07:55, Koichiro Den <den@valinux.co.jp> wrote:
> > On Thu, Aug 13, 2026 at 11:18:22AM +0200, Marek Vasut wrote:
> > > Initialize PCIEDMAINTSTSEN in rcar_gen4_pcie_ep_pre_init() to 0,
> > > and in case rcar_gen4_pcie_common_init() fails and the result
> > > propagates to dw_pcie_ep_init(), the invoke common deinit instead
> > > of endpoint deinit, because we are certain the PCIEDMAINTSTSEN
> > > register is already 0 and the common deinit does not do any register
> > > accesses.
> > >
> > > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> 
> > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > @@ -487,6 +487,8 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> > >       struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > >       int ret;
> > >
> > > +     writel(0, rcar->base + PCIEDMAINTSTSEN);
> > > +
> >
> > 1. The new writel(0, PCIEDMAINTSTSEN) runs before clk_bulk_prepare_enable().
> >    Is MSTP024 expected to be already released at this point? I added the quick
> >    check like below on Spider, and confirmed that the clock was already enabled.
> >
> >    --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> >    +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> >    @@ -9,6 +9,7 @@
> >      */
> >
> >     #include <linux/delay.h>
> >    +#include <linux/clk-provider.h>
> >     #include <linux/firmware.h>
> >     #include <linux/interrupt.h>
> >     #include <linux/io.h>
> >    @@ -487,6 +488,10 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> >            struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> >            int ret;
> >
> >    +       if (__clk_is_enabled(dw->core_clks[DW_PCIE_CORE_CLK].clk))
> >    +               dev_info(dw->dev,
> >    +                        "PCIEC module clock was already enabled\n");
> >    +
> >            writel(0, rcar->base + PCIEDMAINTSTSEN);
> >
> >            ret = rcar_gen4_pcie_common_init(rcar);
> >
> >    If that is not guaranteed, wouldn't this write rely on firmware or some
> >    earlier user leaving the module clock enabled?
> 
> The module clock is enabled through Runtime PM:
> 
>     rcar_gen4_pcie_probe
>     -> rcar_gen4_pcie_prepare
>             -> pm_runtime_resume_and_get

Hi Geert,

I see, CPG_MOD 624. Thanks! Then I am not sure the original writel() is actually
unclocked. It runs before pm_runtime_put(), so runtime PM still keeps that clock
enabled. If "unclocked" means ref clock instead, the new writel() also runs
before clk_bulk_prepare_enable(). Probably I'm missing something again. I would
appreciate it if you could shed light on this.


P.S. I also ran two error injection experiments on an S4 Spider. This is also
related to question #2 in my previous comment.

- When I injected a failure inside common_init(), its error path unwound the
  bulk clocks itself. Calling common_deinit() afterwards triggered "already
  disabled/unprepared" warnings.
- When I injected a failure after pre_init() had succeeded, PCIEDMAINTSTSEN read
  back as 0xffff both before and after common_deinit().

So I think clean-up needs to know whether pre_init() completed, rather than
always calling common_deinit(). Something like this (field declaration omitted):

--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -487,17 +487,24 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
        struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
        int ret;

        ret = rcar_gen4_pcie_common_init(rcar);
        if (ret)
                return ret;

        writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
+       rcar->ep_pre_init_done = true;

        return 0;
 }

 static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
 {
+       if (!rcar->ep_pre_init_done)
+               return;
+
+       rcar->ep_pre_init_done = false;
        writel(0, rcar->base + PCIEDMAINTSTSEN);
        rcar_gen4_pcie_common_deinit(rcar);
 }

I may be missing something again, so please just take it with a pinch of salt.

Best regards,
Koichiro

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Marek Vasut]

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

On 8/18/26 5:09 PM, Koichiro Den wrote:

Hello Den-san,

> I see, CPG_MOD 624. Thanks! Then I am not sure the original writel() is actually
> unclocked. It runs before pm_runtime_put(), so runtime PM still keeps that clock
> enabled. If "unclocked" means ref clock instead, the new writel() also runs
> before clk_bulk_prepare_enable(). Probably I'm missing something again. I would
> appreciate it if you could shed light on this.

This is clearly my error, the commit message is poorly written.

The rcar_gen4_add_dw_pcie_ep() contains two calls which can fail, 
dw_pcie_ep_init() and dw_pcie_ep_init_registers().

The first one, dw_pcie_ep_init(), internally calls .pre_init callback, 
which is implemented in rcar_gen4_pcie_ep_pre_init() on R-Car Gen4 .

The rcar_gen4_pcie_ep_pre_init() internally calls 
rcar_gen4_pcie_common_init(), which may succeed or fail. At this point, 
two possibilities can happen, the fail one is of interest:
- If rcar_gen4_pcie_common_init() succeeds , then 
clk_bulk_prepare_enable() in rcar_gen4_pcie_common_init() did enable all 
dw->core_clks and those clock are kept enabled on exit from 
rcar_gen4_pcie_common_init() . The return value from 
rcar_gen4_pcie_common_init() is 0. OK.
- If rcar_gen4_pcie_common_init() failed, then 
clk_bulk_disable_unprepare() is called in rcar_gen4_pcie_common_init() 
fail path and dw->core_clks clock are disabled. Return value from 
rcar_gen4_pcie_common_init() is non-zero. NG.

Back in rcar_gen4_pcie_ep_pre_init():
- If rcar_gen4_pcie_ep_pre_init() succeeded , then write PCIEDMAINTSTSEN 
and enable edma_int , and exit with return value 0, OK.
- If rcar_gen4_pcie_common_init() failed, then immediately exit with 
return value non-zero. Do not write PCIEDMAINTSTSEN and do not enable 
the edma_interrupts, do depend on previously set content of 
PCIEDMAINTSTSEN register, which was zeroed out at the beginning of this 
function instead. NG.

Finally, back in rcar_gen4_add_dw_pcie_ep():
- If dw_pcie_ep_init() failed, then dw->core_clks have to be disabled at 
this point (*), and PCIEDMAINTSTSEN register is 0, therefore, do not 
call rcar_gen4_pcie_ep_deinit() which writes PCIEDMAINTSTSEN register to 
0 (again) and calls rcar_gen4_pcie_common_deinit(), directly call 
rcar_gen4_pcie_common_deinit() (**).

But as I wrote this part, I realized the (*) is not true, and 
rcar_gen4_pcie_common_deinit() is not the correct function to call in 
case dw_pcie_ep_init() fails.

I think we might need this kind of a patch:

"
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
b/drivers/pci/controller/dwc/pcie-designware-ep.c
index de8ee3db43601..a5801a74dec67 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -1553,7 +1553,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
                                ep->page_size);
         if (ret < 0) {
                 dev_err(dev, "Failed to initialize address space\n");
-               return ret;
+               goto err_deinit;
         }

         ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
@@ -1568,6 +1568,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)

  err_exit_epc_mem:
         pci_epc_mem_exit(epc);
+err_deinit:
+       if (ep->ops->pre_deinit)
+               ep->ops->pre_deinit(ep);

         return ret;
  }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h 
b/drivers/pci/controller/dwc/pcie-designware.h
index 0735ae9409240..733d8b6ad6d2b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -475,6 +475,7 @@ struct dw_pcie_rp {

  struct dw_pcie_ep_ops {
         int     (*pre_init)(struct dw_pcie_ep *ep);
+       void    (*pre_deinit)(struct dw_pcie_ep *ep);
         int     (*init)(struct dw_pcie_ep *ep);
         int     (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
                              unsigned int type, u16 interrupt_num);
"

Then implement .pre_deinit callback for R-Car Gen4 such, that it would 
assert reset and stop the dw->core_clks by calling 
rcar_gen4_pcie_common_deinit(). Then we are sure (*) is true.

Finally, if dw_pcie_ep_init() call in rcar_gen4_add_dw_pcie_ep() fails, 
then with the aforementioned deinit implementation, I think it will be 
possible to simply do the following to cover (**).

ret = dw_pcie_ep_init(ep);
if (ret)
   return ret;

(I am also attaching the entire example as a diff, compile tested only 
thus far)

What do you think ?

> P.S. I also ran two error injection experiments on an S4 Spider. This is also
> related to question #2 in my previous comment.
> 
> - When I injected a failure inside common_init(), its error path unwound the
>    bulk clocks itself. Calling common_deinit() afterwards triggered "already
>    disabled/unprepared" warnings.

I think this ought to be addressed by the pre_deinit above.

> - When I injected a failure after pre_init() had succeeded, PCIEDMAINTSTSEN read
>    back as 0xffff both before and after common_deinit().

I think that with the aforementioned pre_deinit implementation, in case 
pre_init succeeded and a failure occurred in dw_pcie_ep_init_registers() 
instead, the PCIEDMAINTSTSEN will be cleared in 
rcar_gen4_pcie_ep_deinit() correctly.

Does that cover this concern or is there another fail path which I missed ?

> So I think clean-up needs to know whether pre_init() completed, rather than
> always calling common_deinit(). Something like this (field declaration omitted):
> 
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -487,17 +487,24 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
>          struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>          int ret;
> 
>          ret = rcar_gen4_pcie_common_init(rcar);
>          if (ret)
>                  return ret;
> 
>          writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
> +       rcar->ep_pre_init_done = true;
> 
>          return 0;
>   }
> 
>   static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
>   {
> +       if (!rcar->ep_pre_init_done)
> +               return;
> +
> +       rcar->ep_pre_init_done = false;
>          writel(0, rcar->base + PCIEDMAINTSTSEN);
>          rcar_gen4_pcie_common_deinit(rcar);
>   }
> 
> I may be missing something again, so please just take it with a pinch of salt.
Thank you for your feedback, it allowed me to find an error in my train 
of thoughts, it is very much appreciated. I hope we can also find a good 
solution.

Thank you for your help !

-- 
Best regards,
Marek Vasut

[-- Attachment #2: 0001-FIXME-pre_deinit.patch --]
[-- Type: text/x-patch, Size: 3120 bytes --]

From 6a747687dca4467a9c3eea2c2ac577e017fa1ba3 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marek.vasut+renesas@mailbox.org>
Date: Wed, 19 Aug 2026 08:10:15 +0200
Subject: [PATCH] FIXME: pre_deinit

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c |  5 ++++-
 drivers/pci/controller/dwc/pcie-designware.h    |  1 +
 drivers/pci/controller/dwc/pcie-rcar-gen4.c     | 13 ++++++++++---
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index de8ee3db43601..a5801a74dec67 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -1553,7 +1553,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 			       ep->page_size);
 	if (ret < 0) {
 		dev_err(dev, "Failed to initialize address space\n");
-		return ret;
+		goto err_deinit;
 	}
 
 	ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
@@ -1568,6 +1568,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 err_exit_epc_mem:
 	pci_epc_mem_exit(epc);
+err_deinit:
+	if (ep->ops->pre_deinit)
+		ep->ops->pre_deinit(ep);
 
 	return ret;
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 0735ae9409240..733d8b6ad6d2b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -475,6 +475,7 @@ struct dw_pcie_rp {
 
 struct dw_pcie_ep_ops {
 	int	(*pre_init)(struct dw_pcie_ep *ep);
+	void	(*pre_deinit)(struct dw_pcie_ep *ep);
 	int	(*init)(struct dw_pcie_ep *ep);
 	int	(*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
 			     unsigned int type, u16 interrupt_num);
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index b47281b30db96..5fee2e3e2c91e 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -498,6 +498,14 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
 	return 0;
 }
 
+static void rcar_gen4_pcie_ep_pre_deinit(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *dw = to_dw_pcie_from_ep(ep);
+	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
+
+	rcar_gen4_pcie_common_deinit(rcar);
+}
+
 static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
 {
 	writel(0, rcar->base + PCIEDMAINTSTSEN);
@@ -554,6 +562,7 @@ static unsigned int rcar_gen4_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep,
 
 static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.pre_init = rcar_gen4_pcie_ep_pre_init,
+	.pre_deinit = rcar_gen4_pcie_ep_pre_deinit,
 	.raise_irq = rcar_gen4_pcie_ep_raise_irq,
 	.get_features = rcar_gen4_pcie_ep_get_features,
 	.get_dbi_offset = rcar_gen4_pcie_ep_get_dbi_offset,
@@ -572,10 +581,8 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 	ep->ops = &pcie_ep_ops;
 
 	ret = dw_pcie_ep_init(ep);
-	if (ret) {
-		rcar_gen4_pcie_common_deinit(rcar);
+	if (ret)
 		return ret;
-	}
 
 	ret = dw_pcie_ep_init_registers(ep);
 	if (ret) {
-- 
2.53.0

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Koichiro Den]

On Wed, Aug 19, 2026 at 08:28:39AM +0200, Marek Vasut wrote:
> On 8/18/26 5:09 PM, Koichiro Den wrote:
> 
> Hello Den-san,
> 
> > I see, CPG_MOD 624. Thanks! Then I am not sure the original writel() is actually
> > unclocked. It runs before pm_runtime_put(), so runtime PM still keeps that clock
> > enabled. If "unclocked" means ref clock instead, the new writel() also runs
> > before clk_bulk_prepare_enable(). Probably I'm missing something again. I would
> > appreciate it if you could shed light on this.
> 
> This is clearly my error, the commit message is poorly written.
> 
> The rcar_gen4_add_dw_pcie_ep() contains two calls which can fail,
> dw_pcie_ep_init() and dw_pcie_ep_init_registers().
> 
> The first one, dw_pcie_ep_init(), internally calls .pre_init callback, which
> is implemented in rcar_gen4_pcie_ep_pre_init() on R-Car Gen4 .
> 
> The rcar_gen4_pcie_ep_pre_init() internally calls
> rcar_gen4_pcie_common_init(), which may succeed or fail. At this point, two
> possibilities can happen, the fail one is of interest:
> - If rcar_gen4_pcie_common_init() succeeds , then clk_bulk_prepare_enable()
> in rcar_gen4_pcie_common_init() did enable all dw->core_clks and those clock
> are kept enabled on exit from rcar_gen4_pcie_common_init() . The return
> value from rcar_gen4_pcie_common_init() is 0. OK.
> - If rcar_gen4_pcie_common_init() failed, then clk_bulk_disable_unprepare()
> is called in rcar_gen4_pcie_common_init() fail path and dw->core_clks clock
> are disabled. Return value from rcar_gen4_pcie_common_init() is non-zero.
> NG.
> 
> Back in rcar_gen4_pcie_ep_pre_init():
> - If rcar_gen4_pcie_ep_pre_init() succeeded , then write PCIEDMAINTSTSEN and
> enable edma_int , and exit with return value 0, OK.
> - If rcar_gen4_pcie_common_init() failed, then immediately exit with return
> value non-zero. Do not write PCIEDMAINTSTSEN and do not enable the
> edma_interrupts, do depend on previously set content of PCIEDMAINTSTSEN
> register, which was zeroed out at the beginning of this function instead.
> NG.
> 
> Finally, back in rcar_gen4_add_dw_pcie_ep():
> - If dw_pcie_ep_init() failed, then dw->core_clks have to be disabled at
> this point (*), and PCIEDMAINTSTSEN register is 0, therefore, do not call
> rcar_gen4_pcie_ep_deinit() which writes PCIEDMAINTSTSEN register to 0
> (again) and calls rcar_gen4_pcie_common_deinit(), directly call
> rcar_gen4_pcie_common_deinit() (**).
> 
> But as I wrote this part, I realized the (*) is not true, and
> rcar_gen4_pcie_common_deinit() is not the correct function to call in case
> dw_pcie_ep_init() fails.
> 
> I think we might need this kind of a patch:
> 
> "
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index de8ee3db43601..a5801a74dec67 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1553,7 +1553,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>                                ep->page_size);
>         if (ret < 0) {
>                 dev_err(dev, "Failed to initialize address space\n");
> -               return ret;
> +               goto err_deinit;
>         }
> 
>         ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
> @@ -1568,6 +1568,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> 
>  err_exit_epc_mem:
>         pci_epc_mem_exit(epc);
> +err_deinit:
> +       if (ep->ops->pre_deinit)
> +               ep->ops->pre_deinit(ep);
> 
>         return ret;
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 0735ae9409240..733d8b6ad6d2b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -475,6 +475,7 @@ struct dw_pcie_rp {
> 
>  struct dw_pcie_ep_ops {
>         int     (*pre_init)(struct dw_pcie_ep *ep);
> +       void    (*pre_deinit)(struct dw_pcie_ep *ep);
>         int     (*init)(struct dw_pcie_ep *ep);
>         int     (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
>                              unsigned int type, u16 interrupt_num);
> "
> 
> Then implement .pre_deinit callback for R-Car Gen4 such, that it would
> assert reset and stop the dw->core_clks by calling
> rcar_gen4_pcie_common_deinit(). Then we are sure (*) is true.
> 
> Finally, if dw_pcie_ep_init() call in rcar_gen4_add_dw_pcie_ep() fails, then
> with the aforementioned deinit implementation, I think it will be possible
> to simply do the following to cover (**).
> 
> ret = dw_pcie_ep_init(ep);
> if (ret)
>   return ret;
> 
> (I am also attaching the entire example as a diff, compile tested only thus
> far)
> 
> What do you think ?

Hi Marek,

Thanks for sharing your thoughts.

I think the callback idea is much cleaner. I wonder if .post_deinit might
describe its role more clearly.


P.S. I took a quick look through the related call paths, and it made me wonder
if we could take this a little further. Perhaps dw_pcie_ep_deinit() could call
the same hook at the end (I use .post_deinit as a tentative name below):

  dw_pcie_ep_init()
    -> .pre_init()
    -> generic intialization
    error:
      -> generic unwind
      -> .post_deinit()

  dw_pcie_ep_deinit()
    -> generic teardown
    -> .post_deinit()

Then would let the generic init/deinit pair own both hooks, and the
pcie-rcar-gen4 driver would no longer need to call rcar_gen4_pcie_ep_deinit()
separately. After dropping those explicit calls, the function could naturally be
renamed rcar_gen4_pcie_ep_post_deinit() and be used only as the hook.

This might be too much for this small fix. Even without this wider change,
however, I think rcar_gen4_pcie_ep_pre_deinit() should do the same operations as
rcar_gen4_pcie_ep_deinit(), rather than only calling common_deinit(). For
example, if pci_epc_mem_init() fails after .pre_init() succeeds, PCIEDMAINTSTSEN
is already 0xffff, and common_deinit() does not clear it.

Best regards,
Koichiro

> 
> > P.S. I also ran two error injection experiments on an S4 Spider. This is also
> > related to question #2 in my previous comment.
> > 
> > - When I injected a failure inside common_init(), its error path unwound the
> >    bulk clocks itself. Calling common_deinit() afterwards triggered "already
> >    disabled/unprepared" warnings.
> 
> I think this ought to be addressed by the pre_deinit above.
> 
> > - When I injected a failure after pre_init() had succeeded, PCIEDMAINTSTSEN read
> >    back as 0xffff both before and after common_deinit().
> 
> I think that with the aforementioned pre_deinit implementation, in case
> pre_init succeeded and a failure occurred in dw_pcie_ep_init_registers()
> instead, the PCIEDMAINTSTSEN will be cleared in rcar_gen4_pcie_ep_deinit()
> correctly.
> 
> Does that cover this concern or is there another fail path which I missed ?
> 
> > So I think clean-up needs to know whether pre_init() completed, rather than
> > always calling common_deinit(). Something like this (field declaration omitted):
> > 
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -487,17 +487,24 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> >          struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> >          int ret;
> > 
> >          ret = rcar_gen4_pcie_common_init(rcar);
> >          if (ret)
> >                  return ret;
> > 
> >          writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
> > +       rcar->ep_pre_init_done = true;
> > 
> >          return 0;
> >   }
> > 
> >   static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
> >   {
> > +       if (!rcar->ep_pre_init_done)
> > +               return;
> > +
> > +       rcar->ep_pre_init_done = false;
> >          writel(0, rcar->base + PCIEDMAINTSTSEN);
> >          rcar_gen4_pcie_common_deinit(rcar);
> >   }
> > 
> > I may be missing something again, so please just take it with a pinch of salt.
> Thank you for your feedback, it allowed me to find an error in my train of
> thoughts, it is very much appreciated. I hope we can also find a good
> solution.
> 
> Thank you for your help !
> 
> -- 
> Best regards,
> Marek Vasut

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Marek Vasut]

On 8/19/26 10:48 AM, Koichiro Den wrote:

Hello Den-san,

>> Finally, if dw_pcie_ep_init() call in rcar_gen4_add_dw_pcie_ep() fails, then
>> with the aforementioned deinit implementation, I think it will be possible
>> to simply do the following to cover (**).
>>
>> ret = dw_pcie_ep_init(ep);
>> if (ret)
>>    return ret;
>>
>> (I am also attaching the entire example as a diff, compile tested only thus
>> far)
>>
>> What do you think ?
> 
> Hi Marek,
> 
> Thanks for sharing your thoughts.
> 
> I think the callback idea is much cleaner. I wonder if .post_deinit might
> describe its role more clearly.

I think it does, and I will include that in V2.

> P.S. I took a quick look through the related call paths, and it made me wonder
> if we could take this a little further. Perhaps dw_pcie_ep_deinit() could call
> the same hook at the end (I use .post_deinit as a tentative name below):
> 
>    dw_pcie_ep_init()
>      -> .pre_init()
>      -> generic intialization
>      error:
>        -> generic unwind
>        -> .post_deinit()
> 
>    dw_pcie_ep_deinit()
>      -> generic teardown
>      -> .post_deinit()
> 
> Then would let the generic init/deinit pair own both hooks, and the
> pcie-rcar-gen4 driver would no longer need to call rcar_gen4_pcie_ep_deinit()
> separately. After dropping those explicit calls, the function could naturally be
> renamed rcar_gen4_pcie_ep_post_deinit() and be used only as the hook.

That is really good.

> This might be too much for this small fix. Even without this wider change,
> however, I think rcar_gen4_pcie_ep_pre_deinit() should do the same operations as
> rcar_gen4_pcie_ep_deinit(), rather than only calling common_deinit(). For
> example, if pci_epc_mem_init() fails after .pre_init() succeeds, PCIEDMAINTSTSEN
> is already 0xffff, and common_deinit() does not clear it.

I will send a proper V2 shortly to cater for this. Thank you !

-- 
Best regards,
Marek Vasut

Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

[Marek Vasut]

On 8/18/26 7:55 AM, Koichiro Den wrote:

Hello Den-san,

> 2. It seems to me dw_pcie_ep_init() can fail:
>     - before pre_init() has run,
>     - from inside common_init(), or
>     - after pre_init() has succeeded and written PCIEDMAINTSTSEN_INIT (e.g.
>       pci_epc_mem_init() fails)
> 
>     The caller cannot tell which case returned the error. In the first case,
>     common_init() has not run, while in the last case PCIEDMAINTSTSEN is not 0.
>     So this seems to me different from what the commit message says. Is
>     common_deinit() intended to be safe in all three cases?

I hope this is addressed by the introduction of .post_deinit() callback, 
which makes the endpoint deinit safe in all cases.

The commit message was poorly written, and is rewritten since.

-- 
Best regards,
Marek Vasut