Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()
From: Marek Vasut
Date: Wed Aug 19 2026 - 02:29:12 EST
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 thanThank 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.
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 help !
--
Best regards,
Marek VasutFrom 6a747687dca4467a9c3eea2c2ac577e017fa1ba3 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
Date: Wed, 19 Aug 2026 08:10:15 +0200
Subject: [PATCH] FIXME: pre_deinit
Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
---
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