Re: [PATCH v6] 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()
- [Low] PCI: exynos: commit message omits regulator tracking and reset changes
--

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

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks the return values of phy_init(), phy_power_on(), and
exynos_pcie_host_init(), tracks PHY and regulator state across probe, remove,
suspend, and resume, and updates teardown and error recovery paths in the
Exynos PCIe driver.

>     PCI: exynos: Handle PCIe PHY initialization errors
>     
>     Check the return values of "phy_init()" and "phy_power_on()" to
>     avoid continuing PCIe host initialization when PHY setup fails.
>     
>     Track the PHY initialization and power state to ensure cleanup is
>     performed only when appropriate, avoiding double calls to
>     "phy_exit()" on probe failure.
>     
>     Also check the return value of "exynos_pcie_host_init()" during
>     resume and abort the resume sequence if PHY initialization fails.
>     
>     Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
This isn't a bug, but could the commit message also describe the regulator
state tracking and reset handling changes introduced by this patch?

In addition to PHY state tracking, the patch introduces ep->supplies_enabled
across exynos_pcie_probe(), exynos_pcie_remove(), exynos_pcie_suspend_noirq(),
and exynos_pcie_resume_noirq(), disables regulators when
exynos_pcie_host_init() fails during resume, asserts core reset via
exynos_pcie_assert_core_reset() in the probe failure path when PHY setup was
reached, and logs teardown errors in remove and suspend. Since the revision
notes below the tear line are stripped when the patch is applied, these
changes would otherwise be missing from the commit log.

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

[ ... ]

> @@ -55,8 +56,35 @@ 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 = 0;
> +	int err;
> +
> +	if (ep->phy_powered) {
> +		err = phy_power_off(ep->phy);
> +		if (err)
> +			ret = err;
> +		else
> +			ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		err = phy_exit(ep->phy);
> +		if (err)
> +			ret = err;
> +		else
> +			ep->phy_initialized = false;
> +	}
> +
> +	return ret;
> +}
> +
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
>  {
>  	writel(val, base + reg);
> @@ -225,13 +253,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;

[ ... ]

> @@ -332,22 +385,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 v5 [1], can this sequence leak ep->supplies
regulator enable counts and ep->phy init_count/power_count on resume if
teardown fails during suspend?

In exynos_pcie_suspend_noirq(), if exynos_pcie_phy_exit() or
regulator_bulk_disable() fails, the error is logged, the corresponding state
flag (ep->phy_powered, ep->phy_initialized, or ep->supplies_enabled) remains
true, and the function returns 0 so suspend completes. At that point, the
underlying phy->power_count, phy->init_count, or regulator->enable_count is
still 1 (and in exynos_pcie_phy_exit(), phy_exit() is also called while the
PHY is still powered if phy_power_off() fails).

When exynos_pcie_resume_noirq() later runs, it unconditionally calls
regulator_bulk_enable() without checking !ep->supplies_enabled, and calls
exynos_pcie_host_init(), which unconditionally calls phy_init() and
phy_power_on() without checking !ep->phy_initialized and !ep->phy_powered.

Because the boolean flags only track a single reference, calling enable/init
again when a flag is already true increments regulator->enable_count,
phy->init_count, or phy->power_count to 2. In addition, phy_init() and
phy_power_on() only invoke phy->ops->init and phy->ops->power_on when the
counts transition from 0 to 1, so hardware PHY re-initialization is skipped
on resume even though core reset was asserted during suspend.

Later, when exynos_pcie_suspend_noirq(), the resume error path, or
exynos_pcie_remove() runs, regulator_bulk_disable(), phy_power_off(), and
phy_exit() are called at most once before clearing the booleans to false,
leaving the reference counts at 1 and triggering WARN_ON(regulator->enable_count)
on driver unbind.

Should exynos_pcie_resume_noirq() and exynos_pcie_host_init() guard
regulator_bulk_enable(), phy_init(), and phy_power_on() with checks on
ep->supplies_enabled, ep->phy_initialized, and ep->phy_powered, or otherwise
keep the state tracking in sync with the underlying reference counts?

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

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