Re: [PATCH 2/2] PCI: dwc: Handle return value from endpoint .pre_init callback

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional cleanup in the rcar_gen4 endpoint driver leads to a double-disable of clocks and an unsafe unclocked register write when `pre_init` fails.
--

--- Patch [2]: [PATCH 2/2] PCI: dwc: Handle return value from endpoint .pre_init callback ---
commit 275f56803dca69c843553d94d9e95ba4e8dc68ae
Author: Marek Vasut <[email protected]>

PCI: dwc: Handle return value from endpoint .pre_init callback

This commit updates the DWC PCIe endpoint code to check and propagate the
return value of the .pre_init() callback. Drivers implementing this callback
are updated to return integer status codes rather than void.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index d78fb845582b2..b00239097d893 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
[ ... ]
> @@ -1524,8 +1524,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ret)
>  		return ret;
>  
> -	if (ep->ops->pre_init)
> -		ep->ops->pre_init(ep);
> +	if (ep->ops->pre_init) {
> +		ret = ep->ops->pre_init(ep);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 8b03c42f8c84c..55ef0da9146c2 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -373,7 +373,7 @@ static void rcar_gen4_remove_dw_pcie_rp(struct rcar_gen4_pcie *rcar)
>  }
>  
>  /* Endpoint mode */
> -static void rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> +static int rcar_gen4_pcie_ep_pre_init(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);
> @@ -381,9 +381,11 @@ static void rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
>  
>  	ret = rcar_gen4_pcie_common_init(rcar);
>  	if (ret)
> -		return;
> +		return ret;

[Severity: High]
If rcar_gen4_pcie_common_init() fails, it will disable the PCIe clocks before
returning the error. With this patch, the error is now propagated back up
through dw_pcie_ep_init().

The caller of dw_pcie_ep_init() will then unconditionally perform cleanup:

drivers/pci/controller/dwc/pcie-rcar-gen4.c:rcar_gen4_add_dw_pcie_ep() {
	...
	ret = dw_pcie_ep_init(ep);
	if (ret) {
		rcar_gen4_pcie_ep_deinit(rcar);
		return ret;
	}
	...
}

Because rcar_gen4_pcie_ep_deinit() accesses the unclocked PCIEDMAINTSTSEN
register and then calls rcar_gen4_pcie_common_deinit(), could this result in
writing to an unclocked register and triggering a synchronous external abort?

Could it also cause a clock framework reference count underflow when the clocks
are disabled a second time during the cleanup?

>  
>  	writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
> +
> +	return 0;
>  }
>  
>  static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2