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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: exynos: PHY and regulator ref leak in exynos_pcie_resume_noirq()
- [Medium] PCI: exynos: missing core reset assertion on post-init probe failure
--

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

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks return values from phy_init(), phy_power_on(), and
exynos_pcie_host_init(), and introduces boolean flags on struct exynos_pcie
to track PHY and regulator states across probe, remove, suspend, and resume.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..d82bdfea1f17b 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -55,8 +56,34 @@ struct exynos_pcie {
>  	struct clk_bulk_data		*clks;
>  	struct phy			*phy;
>  	struct regulator_bulk_data	supplies[2];
> +	bool phy_initialized;
> +	bool phy_powered;
> +	bool supplies_enabled;
>  };
>  
> +static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
> +{
> +	int ret;
> +
> +	if (ep->phy_powered) {
> +		ret = phy_power_off(ep->phy);
> +		if (ret)
> +			return ret;
> +
> +		ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		ret = phy_exit(ep->phy);
> +		if (ret)
> +			return ret;
> +
> +		ep->phy_initialized = false;
> +	}
> +
> +	return 0;
> +}
> +
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
>  {
>  	writel(val, base + reg);
> @@ -225,13 +252,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct exynos_pcie *ep = to_exynos_pcie(pci);
> +	int ret;
>  
>  	pp->bridge->ops = &exynos_pci_ops;
>  
>  	exynos_pcie_assert_core_reset(ep);
>  
> -	phy_init(ep->phy);
> -	phy_power_on(ep->phy);
> +	ret = phy_init(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_initialized = true;
> +
> +	ret = phy_power_on(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_powered = true;
>  
>  	exynos_pcie_deassert_core_reset(ep);
>  	exynos_pcie_enable_irq_pulse(ep);
> @@ -314,6 +351,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
>  	platform_set_drvdata(pdev, ep);
>  
>  	ret = exynos_add_pcie_port(ep, pdev);
> @@ -323,8 +361,19 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_probe:
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +	if (exynos_pcie_phy_exit(ep))
> +		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
> +
> +	if (ep->supplies_enabled) {
> +		int err;
> +
> +		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (err)
> +			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
> +		else
> +			ep->supplies_enabled = false;
> +
> +	}

[Severity: Medium]
When dw_pcie_host_init() fails after exynos_pcie_host_init() has already
succeeded (for example, if dw_pcie_wait_for_link() returns -ETIMEDOUT or
pci_host_probe() fails), exynos_pcie_host_init() has already deasserted core
reset via exynos_pcie_deassert_core_reset(ep) and enabled pulse interrupts.

Because exynos_pcie_host_ops does not provide a .deinit callback for the
err_deinit_host path in dw_pcie_host_init(), control returns to fail_probe in
exynos_pcie_probe(), which powers off and exits ep->phy and disables
ep->supplies without calling exynos_pcie_assert_core_reset(ep) first (unlike
exynos_pcie_remove() and exynos_pcie_suspend_noirq()). Note that calling
exynos_pcie_assert_core_reset(ep) unconditionally in fail_probe would not be
safe either, since fail_probe is also reached when exynos_add_pcie_port()
fails before dw_pcie_get_resources() maps pci->elbi_base.

Would it be cleaner and safer to unwind phy_init(ep->phy) locally inside
exynos_pcie_host_init() when phy_power_on(ep->phy) fails, and implement
dw_pcie_host_ops.deinit to call exynos_pcie_assert_core_reset(ep),
phy_power_off(ep->phy), and phy_exit(ep->phy), thereby ensuring core reset is
asserted on post-init probe failures and avoiding manual boolean state
tracking across probe, remove, suspend, and resume?

>  	return ret;
>  }

[ ... ]

> @@ -332,22 +381,42 @@ static void exynos_pcie_remove(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);
> +		else
> +			ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }
>  
>  static int exynos_pcie_resume_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(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);

[Severity: High]
As reported by Sashiko on v4 [1], in exynos_pcie_suspend_noirq(), if
exynos_pcie_phy_exit(ep) or regulator_bulk_disable() fails, the error is
logged and 0 is returned so system suspend proceeds, while ep->phy_powered,
ep->phy_initialized, or ep->supplies_enabled remains true. Furthermore, if
phy_power_off(ep->phy) fails in exynos_pcie_phy_exit(), it returns early
without calling phy_exit(ep->phy) (which also leaks phy->init_count in
fail_probe and exynos_pcie_remove()), and exynos_pcie_suspend_noirq() still
proceeds to call regulator_bulk_disable() on the vdd18 and vdd10 supplies
while the PHY remains powered on.

When exynos_pcie_resume_noirq() subsequently runs, it unconditionally calls
regulator_bulk_enable() and exynos_pcie_host_init(pp) -> phy_init(ep->phy) /
phy_power_on(ep->phy) without checking whether ep->supplies_enabled,
ep->phy_initialized, or ep->phy_powered are already true.

Because regulator_bulk_disable(), phy_power_off(), and phy_exit() leave their
internal enable_count, power_count, and init_count at 1 on failure, calling
regulator_bulk_enable(), phy_init(), or phy_power_on() again increments those
counts to 2 while struct exynos_pcie only tracks state with single booleans.
In addition, phy_init() and phy_power_on() only invoke phy->ops->init and
phy->ops->power_on when init_count == 0 and power_count == 0, so hardware PHY
re-initialization is skipped on resume. Subsequent teardown in
exynos_pcie_remove() or the next suspend then decrements the counts only once
(from 2 to 1) before clearing the booleans to false.

Doesn't this permanently leak the PHY and regulator reference counts and skip
hardware PHY re-initialization on resume?

[1] https://lore.kernel.org/r/20261030163654.2733-1-sreekuttan2156239@gmail.com

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