Re: [PATCH v4] 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: unrecovered PHY and RC state in exynos_pcie_suspend_noirq()
--

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

PCI: exynos: Handle PCIe PHY initialization errors

This commit adds error checking for phy_init() and phy_power_on() during host
initialization, tracks PHY and regulator state for cleanup, and handles host
initialization errors during resume.

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

[ ... ]

> @@ -332,24 +381,48 @@ 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)
> +		goto err_deassert_reset;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			goto err_deassert_reset;
> +
> +		ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
> +
> +err_deassert_reset:
> +	exynos_pcie_deassert_core_reset(ep);
> +	return ret;
>  }

[Severity: Medium]
As reported by Sashiko on v3 [1], if exynos_pcie_phy_exit() or
regulator_bulk_disable() fails in exynos_pcie_suspend_noirq(), jumping to
err_deassert_reset only calls exynos_pcie_deassert_core_reset(ep) before
returning a negative error code.

By that point, exynos_pcie_assert_core_reset(ep) has already cleared
PCIE_CORE_RESET_ENABLE, PCIE_STICKY_RESET, and PCIE_NONSTICKY_RESET, and
exynos_pcie_phy_exit(ep) may have already powered off and exited the PHY
(which in exynos5433_pcie_phy_exit() gates the reference clock via
PCIE_REFCLK_GATING_EN and isolates the PHY in the PMU). Deasserting core
reset while the PHY is powered down and uninitialized violates the ordering in
exynos_pcie_host_init(), where phy_init() and phy_power_on() precede
exynos_pcie_deassert_core_reset().

Furthermore, in exynos_pcie_phy_exit():

	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;
	}

if phy_power_off() succeeds and phy_exit() fails, ep->phy_powered is false
while ep->phy_initialized remains true with phy->init_count == 1. Any
subsequent call to exynos_pcie_host_init() will call phy_init(ep->phy)
unconditionally, incrementing phy->init_count to 2 while ep->phy_initialized
only tracks a single boolean reference.

Finally, because exynos_pcie_suspend_noirq() returns a non-zero error to
device_suspend_noirq(), dev->power.is_noirq_suspended is not set and
exynos_pcie_resume_noirq() will not be called during suspend-abort recovery.

Would this leave the Root Complex active from the PM core's perspective while
its PHY remains powered down and uninitialized, and without restoring IRQ
pulse configuration, Root Complex setup, or the PCIe link?

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

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