Re: [PATCH v4] PCI: qcom-ep: Fix runtime PM reference leak in probe error paths

[Manivannan Sadhasivam]

On Wed, Jun 24, 2026 at 09:49:32PM +0800, Wentao Liang wrote:
> The qcom_pcie_ep_probe() function obtains a runtime PM reference via
> pm_runtime_get_noresume() at the beginning of the probe, but this
> reference is only released on the successful completion path using
> pm_runtime_put_sync().
> 
> However, the error paths fail to release this reference. The devres
> cleanup registered by devm_pm_runtime_enable() only calls
> pm_runtime_disable() and does not decrement the usage_count. As a
> result, if any error occurs during probe (e.g., resource acquisition
> failure or endpoint initialization failure), the reference is leaked,
> permanently elevating the device's usage_count. This prevents proper
> runtime suspend and clean device removal.
> 
> Fix this by properly distinguishing error paths that occur before and
> after devm_pm_runtime_enable() succeeds:
> 
> - For errors before devm_pm_runtime_enable() succeeds: call
>   pm_runtime_put_noidle() and pm_runtime_disable() manually to clean
>   up the initial reference and disable runtime PM.
> 
> - For errors after devm_pm_runtime_enable() succeeds: only call
>   pm_runtime_put_sync() to drop the initial reference. The
>   pm_runtime_disable() is left to the devres cleanup handler,
>   avoiding a double-disable.
> 
> - On the successful probe path: call pm_runtime_put_sync() to drop
>   the initial reference. The return value is not checked because
>   -EAGAIN/-EBUSY only indicate that the device cannot be suspended
>   at this moment and do not represent a probe failure. Checking
>   the return value would cause a double-put if it fails.
> 
> This ensures the runtime PM reference is correctly released on all
> error paths without introducing double-disables or double-puts.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 5b026a9e714d ("PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint")
> Signed-off-by: Wentao Liang <vulab@xxxxxxxxxxx>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 257c2bcb5f76..4fb21ef50cd0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -892,16 +892,16 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(dev);
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
> -		return ret;
> +		goto err_manual_cleanup;
>  
>  	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
>  	if (ret)
> -		return ret;
> +		goto err_put_ref;
>  
>  	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
> -		return ret;
> +		goto err_put_ref;
>  	}
>  
>  	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> @@ -914,11 +914,7 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  		goto err_disable_irqs;
>  	}
>  
> -	ret = pm_runtime_put_sync(dev);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to suspend device: %d\n", ret);
> -		goto err_disable_irqs;
> -	}
> +	pm_runtime_put_sync(dev);
>  
>  	pcie_ep->debugfs = debugfs_create_dir(name, NULL);
>  	qcom_pcie_ep_init_debugfs(pcie_ep);
> @@ -932,6 +928,13 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  err_ep_deinit:
>  	dw_pcie_ep_deinit(&pcie_ep->pci.ep);
>  
> +err_put_ref:
> +	pm_runtime_put_sync(dev);
> +	return ret;

Returning from 2 different error labels is not a standard pattern in kernel.

> +
> +err_manual_cleanup:

Weird error label name.

> +	pm_runtime_put_noidle(dev);

Why pm_runtime_put_noidle() and not pm_runtime_put_sync()?

> +	pm_runtime_disable(dev);

If devm_pm_runtime_enable() fails, pm_runtime_disable() will be called in the
error path as well. So this pm_runtime_disable() will decrement the disable
counter twice.

- Mani

-- 
மணிவண்ணன் சதாசிவம்