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

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

[Marek Vasut]

In case the .pre_init() callback 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.

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
---
 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 v2 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 set in case of failure
of dw_pcie_ep_init(), and will only be set in case dw_pcie_ep_init()
succeeds.

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()
---
 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 v2 1/2] PCI: dwc: Add .post_deinit counterpart to endpoint .pre_init callback

[sashiko-bot]

> In case the .pre_init() callback 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.
> 
> 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/20260820035944.163472-1-marek.vasut+renesas@mailbox.org?part=1

Re: [PATCH v2 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 in dw_pcie_ep_init_registers() error path causes EPC to be incorrectly notified as ready.
--

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

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

Implement the .post_deinit() callback in R-Car Gen4 to cleanly stop clocks
and assert resets if endpoint initialization fails. This prevents duplicate
cleanup attempts by removing explicit deinit calls from the error paths.

> 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
[ ... ]
> @@ -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);
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path fall through and
incorrectly notify the EPC framework?

If dw_pcie_ep_init_registers() fails in rcar_gen4_add_dw_pcie_ep(), the
code de-initializes the endpoint via dw_pcie_ep_deinit(), but seems to miss
a return statement. It will unconditionally fall through and call
pci_epc_init_notify(ep->epc).

Could this cause bound Endpoint Function (EPF) drivers to receive an
initialization complete notification on a disabled controller, potentially
leading to an asynchronous external abort (SError) or kernel panic when they
attempt memory mapping and register accesses?

>  
>  	pci_epc_init_notify(ep->epc);

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