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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: exynos: return of int in void exynos_pcie_remove() and regulator leak
- [Medium] PCI: exynos: regulator leak in exynos_pcie_resume_noirq() on PHY exit error
- [Medium] PCI: exynos: unrecovered hardware state on exynos_pcie_suspend_noirq() error
--

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

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks the return values of phy_init() and phy_power_on() in
exynos_pcie_host_init() and tracks PHY and regulator state across probe,
remove, suspend, and resume paths.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..d7504b0e5d96c 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c

[ ... ]

> @@ -332,22 +381,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  static void exynos_pcie_remove(struct platform_device *pdev)
>  {
>  	struct exynos_pcie *ep = platform_get_drvdata(pdev);
> +	int ret;
>  
>  	dw_pcie_host_deinit(&ep->pci.pp);
>  	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)
> +		return ret;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
> +		else
> +			ep->supplies_enabled = false;
> +	}
>  }

[Severity: High]
In drivers/pci/controller/dwc/pci-exynos.c, exynos_pcie_remove() has a void
return type, so returning ret when exynos_pcie_phy_exit(ep) fails causes a
build error.

Also, if exynos_pcie_phy_exit(ep) returns a non-zero error during driver
removal, doesn't returning early here skip regulator_bulk_disable() and leave
the vdd18 and vdd10 supplies in ep->supplies enabled? Should this log an
error and continue to disable ep->supplies, similar to the fail_probe path in
exynos_pcie_probe()?

>  
>  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)
> +		return ret;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			return ret;
> +
> +		ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }

[Severity: Medium]
In exynos_pcie_suspend_noirq(), exynos_pcie_assert_core_reset(ep) is called
before exynos_pcie_phy_exit(ep) and regulator_bulk_disable().

If exynos_pcie_phy_exit(ep) or regulator_bulk_disable() returns an error,
device_suspend_noirq() in the PM core aborts before setting
dev->power.is_noirq_suspended, which means device_resume_noirq() will not
call exynos_pcie_resume_noirq() during suspend-abort recovery.

Would that leave the Exynos PCIe controller held in core reset (and with its
PHY powered off or regulators still enabled) when the system aborts suspend?

> @@ -358,13 +427,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
>  	struct dw_pcie *pci = &ep->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	int ret;
> +	int err;
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
> +
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);
> +	if (ret) {
> +		err = exynos_pcie_phy_exit(ep);
> +		if (err)
> +			return err;
> +
> +		if (ep->supplies_enabled) {
> +			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +			if (err)
> +				return err;
> +
> +			ep->supplies_enabled = false;
> +		}
> +
> +		return ret;
> +
> +	}

[Severity: Medium]
In exynos_pcie_resume_noirq(), regulator_bulk_enable() enables ep->supplies
before calling exynos_pcie_host_init(pp).

If exynos_pcie_host_init(pp) fails (for instance, when phy_init() succeeds
and phy_power_on() fails) and the subsequent cleanup call to
exynos_pcie_phy_exit(ep) also returns an error, won't returning err early
bypass regulator_bulk_disable() and leak the regulator enable reference on
ep->supplies?

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