Re: [PATCH v8] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: exynos: regulator refcount leak in exynos_pcie_suspend_noirq()
--

commit b886839653b4a8156d97c162a00a5d1ee22e14d1
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks return values from PHY initialization and power-on calls,
and tracks PHY and regulator state across probe, remove, suspend, and resume
to ensure proper error handling and cleanup.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..45163b1dad81d 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
[ ... ]
> @@ -332,22 +388,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
[ ... ]
>  static int exynos_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
> +	int ret;
>  
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		dev_err(dev, "failed to exit PCIe PHY during suspend: %d\n", ret);
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			dev_err(dev, "failed to disable regulators during suspend: %d\n", ret);
> +
> +		ep->supplies_enabled = false;
> +	}

[Severity: Medium]
In exynos_pcie_suspend_noirq(), if regulator_bulk_disable() fails, its error
recovery path re-enables any supplies that were already disabled during the
call and leaves ep->supplies enabled.

Unlike the error paths in exynos_pcie_probe(), exynos_pcie_remove(), and
exynos_pcie_resume_noirq(), ep->supplies_enabled is unconditionally set to
false here even when regulator_bulk_disable() returns an error.

When exynos_pcie_resume_noirq() later runs:

	if (!ep->supplies_enabled) {
		ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
		if (ret)
			return ret;

		ep->supplies_enabled = true;
	}

won't this call regulator_bulk_enable() a second time on already-enabled
supplies and leak a regulator enable reference count (or skip
regulator_bulk_disable() if the driver is removed first)? Should setting
ep->supplies_enabled = false be placed in an else branch as in the other
functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031073548.3916-1-sreekuttan2156239@gmail.com?part=1