[PATCH v3 1/2] PCI: dwc: Add .post_deinit counterpart to endpoint .pre_init callback

[PATCH v3 1/2] PCI: dwc: Add .post_deinit counterpart to endpoint .pre_init callback

[Marek Vasut]

If .pre_init() succeeds but subsequent initialization fails, it is
mandatory to correctly undo the hardware configuration which the
.pre_init() callback did. Intrododuce .post_deinit() callback to do
exactly that, undo what the .pre_init() callback did. Usually, that
means stop the clock, assert reset, and possibly program some registers
to quiescense the hardware. Invoke the .post_deinit() callback in
dw_pcie_ep_deinit() to retain functional symmetry.

Reviewed-by: Koichiro Den <den@valinux.co.jp>
Tested-by: Koichiro Den <den@valinux.co.jp>
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
---
V2: New patch
V3: - Update commit message slightly
    - Add RB/TB from Den-san
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++++-
 drivers/pci/controller/dwc/pcie-designware.h    | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index de8ee3db43601..1a3491b5003ec 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -1194,6 +1194,9 @@ void dw_pcie_ep_deinit(struct dw_pcie_ep *ep)
 			      epc->mem->window.page_size);
 
 	pci_epc_mem_exit(epc);
+
+	if (ep->ops->post_deinit)
+		ep->ops->post_deinit(ep);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit);
 
@@ -1553,7 +1556,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 +1571,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->post_deinit)
+		ep->ops->post_deinit(ep);
 
 	return ret;
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 0735ae9409240..a53ac27cd2447 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	(*post_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);
-- 
2.53.0

[PATCH v3 2/2] PCI: dwc: rcar-gen4: Use .post_deinit to handle dw_pcie_ep_init() failures

[Marek Vasut]

Implement .post_deinit() callback in R-Car Gen4 struct dw_pcie_ep_ops {}
which asserts reset and stops the clock. This undoes start of clock and
deassert of reset performed in .pre_init() in case dw_pcie_ep_init() fails
after successful call of .pre_init() callback.

The use of .post_deinit() callback correctly handles the clock and reset
stop, unlike the call of rcar_gen4_pcie_ep_deinit() in dw_pcie_ep_init()
which could not discern at which point the dw_pcie_ep_init() failed and
might have attempted to stop clock and assert reset twice, remove it.

Since dw_pcie_ep_deinit() also invokes the .post_deinit() callback, drop
calls to rcar_gen4_pcie_ep_deinit() in both rcar_gen4_add_dw_pcie_ep()
dw_pcie_ep_init_registers() fail path and rcar_gen4_remove_dw_pcie_ep()
to avoid duplicate stop of clock and assert of reset, and drop no longer
used rcar_gen4_pcie_ep_deinit() entirely.

Initialize PCIEDMAINTSTSEN early in rcar_gen4_pcie_ep_pre_init() to 0,
to make sure that edma_int bits will never be left set in case of
failure of dw_pcie_ep_init(), and will only be set in case
dw_pcie_ep_init() succeeds.

Reviewed-by: Koichiro Den <den@valinux.co.jp>
Tested-by: Koichiro Den <den@valinux.co.jp>
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
---
V2: This is reworked version of
    PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()
V3: - Update commit message slightly
    - Add RB/TB from Den-san
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index fbe465a29068f..157e33c4089b9 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;
@@ -496,8 +498,11 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
 	return 0;
 }
 
-static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
+static void rcar_gen4_pcie_ep_post_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);
+
 	writel(0, rcar->base + PCIEDMAINTSTSEN);
 	rcar_gen4_pcie_common_deinit(rcar);
 }
@@ -552,6 +557,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,
+	.post_deinit = rcar_gen4_pcie_ep_post_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,
@@ -570,16 +576,13 @@ 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_ep_deinit(rcar);
+	if (ret)
 		return ret;
-	}
 
 	ret = dw_pcie_ep_init_registers(ep);
 	if (ret) {
 		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
 		dw_pcie_ep_deinit(ep);
-		rcar_gen4_pcie_ep_deinit(rcar);
 	}
 
 	pci_epc_init_notify(ep->epc);
@@ -590,7 +593,6 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 {
 	dw_pcie_ep_deinit(&rcar->dw.ep);
-	rcar_gen4_pcie_ep_deinit(rcar);
 }
 
 /* Common */
-- 
2.53.0

Re: [PATCH v3 1/2] PCI: dwc: Add .post_deinit counterpart to endpoint .pre_init callback

[sashiko-bot]

> If .pre_init() succeeds but subsequent initialization fails, it is
> mandatory to correctly undo the hardware configuration which the
> .pre_init() callback did. Intrododuce .post_deinit() callback to do
> exactly that, undo what the .pre_init() callback did. Usually, that
> means stop the clock, assert reset, and possibly program some registers
> to quiescense the hardware. Invoke the .post_deinit() callback in
> dw_pcie_ep_deinit() to retain functional symmetry.
> 
> Reviewed-by: Koichiro Den <den@valinux.co.jp>
> Tested-by: Koichiro Den <den@valinux.co.jp>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821122808.238248-1-marek.vasut+renesas@mailbox.org?part=1

Re: [PATCH v3 2/2] PCI: dwc: rcar-gen4: Use .post_deinit to handle dw_pcie_ep_init() failures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing early return on error path leads to invalid EPC initialization notification and potential unclocked access panic.
--

--- Patch [2]: [PATCH v3 2/2] PCI: dwc: rcar-gen4: Use .post_deinit to handle dw_pcie_ep_init() failures ---
commit 983f248d176d3915fd934d946c124b82fe3530d1
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

PCI: dwc: rcar-gen4: Use .post_deinit to handle dw_pcie_ep_init() failures

This commit implements the .post_deinit() callback in the R-Car Gen4
endpoint ops to handle reset assertion and clock stopping upon failure.
It drops duplicate deinitialization calls in the failure paths to prevent
stopping the clock or asserting resets twice. It also initializes
PCIEDMAINTSTSEN early to ensure EDMA bits are clear on failure.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[ ... ]

> @@ -576,13 +576,10 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
>  
>  	ret = dw_pcie_ep_init_registers(ep);
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
>  		dw_pcie_ep_deinit(ep);
> -		rcar_gen4_pcie_ep_deinit(rcar);
>  	}
>  
>  	pci_epc_init_notify(ep->epc);

[Severity: High]
This isn't a bug introduced by this patch, but should there be an explicit
return statement here?

If dw_pcie_ep_init_registers() fails, execution falls through and
unconditionally calls pci_epc_init_notify(ep->epc).

Could this incorrectly set epc->init_complete = true and invoke the
initialization callbacks of any bound EPF drivers on a controller where
the memory is freed, resets are asserted, and clocks are stopped?

If an EPF driver binds and accesses the controller in this deinitialized
state, could it trigger a synchronous external abort (unclocked access) or a
use-after-free?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821122808.238248-1-marek.vasut+renesas@mailbox.org?part=2