Re: [PATCH v7] 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: controller left in reset when exynos_pcie_suspend_noirq() fails
- [Medium] PCI: exynos: exynos_pcie_phy_exit() calls phy_exit() when phy_power_off() fails
--

commit fdc1ec02af91afa2f7f426dbdec623fdb1841665
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 tracks PHY and regulator state across probe,
remove, suspend, and resume to prevent unbalanced cleanup calls.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..3e76a818a45c5 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/device.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> @@ -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;
> +}

[Severity: Medium]
As reported by Sashiko on v6 [1], if phy_power_off() fails in
exynos_pcie_phy_exit() in drivers/pci/controller/dwc/pci-exynos.c when
ep->phy_powered is true, should this function avoid calling phy_exit() while
the PHY is still powered on?

In drivers/phy/phy-core.c:phy_power_off(), a failure returns before
decrementing phy->power_count:

	if (phy->power_count == 1 && phy->ops->power_off) {
		ret = phy->ops->power_off(phy);
		if (ret < 0) {
			dev_err(&phy->dev, "phy poweroff failed --> %d\n", ret);
			mutex_unlock(&phy->mutex);
			return ret;
		}
	}
	--phy->power_count;

Because exynos_pcie_phy_exit() continues into the if (ep->phy_initialized)
block even when phy_power_off() failed, it calls phy_exit() on a still-powered
PHY. If phy_exit() succeeds, phy->init_count drops to 0 while
phy->power_count remains 1 (and ep->phy_initialized becomes false while
ep->phy_powered stays true), which then triggers a warning in
drivers/phy/phy-core.c:phy_init() on a subsequent re-initialization:

	if (phy->power_count > phy->init_count)
		dev_warn(&phy->dev, "phy_power_on was called before phy_init\n");

[ ... ]

> @@ -332,22 +389,46 @@ 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);
> +		return 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);
> +			return ret;
> +		}
> +
> +		ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
When exynos_pcie_phy_exit() or regulator_bulk_disable() fails in
exynos_pcie_suspend_noirq() in drivers/pci/controller/dwc/pci-exynos.c, can
returning a non-zero error code leave the PCIe Root Complex held in reset
with its PHY powered down after suspend aborts?

exynos_pcie_suspend_noirq() unconditionally calls
exynos_pcie_assert_core_reset(ep) and then exynos_pcie_phy_exit(ep). If
exynos_pcie_phy_exit() or regulator_bulk_disable() returns an error,
drivers/base/power/main.c:device_suspend_noirq() jumps to End before setting
dev->power.is_noirq_suspended:

Run:
	error = dpm_run_callback(callback, dev, state, info);
	if (error) {
		WRITE_ONCE(async_error, error);
		dpm_save_failed_dev(dev_name(dev));
		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
		goto End;
	}

Skip:
	dev->power.is_noirq_suspended = true;

During the resulting suspend-abort recovery in
dpm_resume_noirq() -> device_resume_noirq() in drivers/base/power/main.c, the
PM core checks dev->power.is_noirq_suspended and skips calling
exynos_pcie_resume_noirq():

	if (!dev->power.is_noirq_suspended) {
		if (dev_pm_skip_suspend(dev))
			dev->power.must_resume = false;

		goto Out;
	}

Wouldn't this leave the controller in core reset with its PHY exited while
downstream PCI devices (which suspended before the host bridge) are resumed?

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

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