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

[Koichiro Den]

On Thu, Aug 20, 2026 at 05:57:56AM +0200, Marek Vasut wrote:
> 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

I think s/will never be set/will never be left set/, because .pre_init() writes
0xffff before later initialization can fail, and .post_deinit() then sets it
back to 0. Otherwise LGTM.

(Again, I did some quick tests on an S4 Spider, covering the normal path and
several injected failure paths. I don't have V4H hardware, though.)
Please feel free to pick either or both tags, if that helps.

Reviewed-by: Koichiro Den <den@xxxxxxxxxxxxx>
Tested-by: Koichiro Den <den@xxxxxxxxxxxxx>

Best regards,
Koichiro

> 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@xxxxxxxxxxx>
> ---
> Cc: "Krzysztof Wilczyński" <kwilczynski@xxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Cc: Koichiro Den <den@xxxxxxxxxxxxx>
> Cc: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
> Cc: Magnus Damm <magnus.damm@xxxxxxxxx>
> Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> ---
> 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(-)

LGTM, thank you!

Reviewed-by: Koichiro Den <den@xxxxxxxxxxxxx>
Tested-by: Koichiro Den <den@xxxxxxxxxxxxx>

Best regards,

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