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

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 <[email protected]>

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/[email protected]?part=2