Re: [PATCH v2] 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: unbalanced regulator disable and unpowered access on resume error
- [High] PCI: exynos: exynos_pcie_resume_noirq() powers down controller on -ENODEV
--

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

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks return values from phy_init() and phy_power_on() in
exynos_pcie_host_init(), tracks PHY initialization and power state in struct
exynos_pcie, and adds error handling in exynos_pcie_resume_noirq() to clean up
the PHY and regulators on failure.

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

[ ... ]

> @@ -364,10 +403,30 @@ static int exynos_pcie_resume_noirq(struct device *dev)
>  		return ret;
>  
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);
> +	if (ret)
> +		goto err_cleanup;
> +
>  	dw_pcie_setup_rc(pp);
>  	exynos_pcie_start_link(pci);
> -	return dw_pcie_wait_for_link(pci);
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_cleanup;

[Severity: High]
Should exynos_pcie_resume_noirq() power down the PHY and disable regulators
when dw_pcie_wait_for_link() returns a non-zero value such as -ENODEV?

In drivers/pci/controller/dwc/pcie-designware.c, dw_pcie_wait_for_link()
returns -ENODEV when no endpoint device is detected (LTSSM in
DW_PCIE_LTSSM_DETECT_QUIET or DW_PCIE_LTSSM_DETECT_ACT) and -EIO when a device
is in Poll.Active or Poll.Compliance. During initial probe,
dw_pcie_host_init() only aborts on -ETIMEDOUT, so probe succeeds and registers
the host bridge with the PHY and regulators active even when the slot is
empty.

With this change, resuming a system with no connected or active PCIe endpoint
causes dw_pcie_wait_for_link() to return -ENODEV or -EIO and jump to
err_cleanup, shutting down ep->phy and ep->supplies while the PCI root bus
remains registered (and without asserting core reset after
exynos_pcie_deassert_core_reset() and exynos_pcie_start_link()).

> +
> +	return 0;
> +
> +err_cleanup:
> +	if (ep->phy_powered) {
> +		phy_power_off(ep->phy);
> +		ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		phy_exit(ep->phy);
> +		ep->phy_initialized = false;
> +	}
> +	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +	return ret;

[Severity: High]
What happens on subsequent PCI bus operations, the next system suspend, or
driver unbind if exynos_pcie_resume_noirq() fails and disables the PHY and
ep->supplies here?

During exynos_pcie_suspend_noirq(), the vdd18 and vdd10 supplies in
ep->supplies are disabled and the PHY is powered off and exited. If
exynos_pcie_resume_noirq() fails in regulator_bulk_enable(), or if
exynos_pcie_host_init() or dw_pcie_wait_for_link() fails and jumps to
err_cleanup, the PHY is powered off/exited and regulator_bulk_disable() brings
the regulator enable count back to zero.

However, returning an error from a resume_noirq callback in
device_resume_noirq() does not unbind the driver or unregister the PCI host
bridge, and the PM core continues resuming subordinate PCI devices on the root
bus.

Because the controller and PHY reference clock are now powered down while the
host bridge remains active, won't this lead to two issues:

1. Subsequent config space accesses via exynos_pcie_rd_own_conf() and
exynos_pcie_wr_own_conf(), shared IRQ handling, dw_pcie_host_deinit() in
exynos_pcie_remove(), and exynos_pcie_assert_core_reset() in
exynos_pcie_suspend_noirq() and exynos_pcie_remove() will perform MMIO
accesses to pci->dbi_base and pci->elbi_base while unpowered and unclocked.

2. Unlike ep->phy_powered and ep->phy_initialized, the regulator state of
ep->supplies is not tracked. On the next exynos_pcie_suspend_noirq() or
exynos_pcie_remove(), regulator_bulk_disable(ARRAY_SIZE(ep->supplies),
ep->supplies) will be called unconditionally on already-disabled regulators,
triggering an unbalanced regulator disable warning in _regulator_disable().

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