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

[Marek Vasut <marek.vasut@mailbox.org>]

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