Re: [PATCH] PCI: exynos: Handle 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: double phy_exit() on probe error after exynos_pcie_host_init()
- [High] PCI: exynos: unchecked exynos_pcie_host_init() in exynos_pcie_resume_noirq()
--

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

PCI: exynos: Handle PHY initialization errors

This commit checks the return values of phy_init() and phy_power_on() in
exynos_pcie_host_init() and calls phy_exit() if phy_power_on() fails.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..5ad07c0be8e51 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -225,13 +225,21 @@ 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;
> +
> +	ret = phy_power_on(ep->phy);
> +	if (ret) {
> +		phy_exit(ep->phy);
> +		return ret;
> +	}

[Severity: High]
Does this cause an unbalanced phy_exit(ep->phy) call when probe fails in
exynos_pcie_probe() -> exynos_add_pcie_port() -> dw_pcie_host_init() ->
exynos_pcie_host_init()?

When phy_init(ep->phy) fails, ep->phy->init_count remains 0, and when
phy_power_on(ep->phy) fails, phy_exit(ep->phy) in exynos_pcie_host_init()
decrements ep->phy->init_count back to 0. Both dw_pcie_host_init() and
exynos_add_pcie_port() return that negative error code to
exynos_pcie_probe():

drivers/pci/controller/dwc/pci-exynos.c:exynos_pcie_probe() {
	...
	ret = exynos_add_pcie_port(ep, pdev);
	if (ret < 0)
		goto fail_probe;

	return 0;

fail_probe:
	phy_exit(ep->phy);
	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);

	return ret;
}

Because fail_probe unconditionally calls phy_exit(ep->phy), phy_exit() in
drivers/phy/phy-core.c decrements ep->phy->init_count from 0 to -1, which
prevents phy_init() from invoking phy->ops->init() on subsequent probe
attempts.

[Severity: High]
What happens during system resume in exynos_pcie_resume_noirq() if
exynos_pcie_host_init() returns early with an error here?

In exynos_pcie_host_init(), exynos_pcie_assert_core_reset(ep) is called
before phy_init(ep->phy) and phy_power_on(ep->phy). Returning early skips
exynos_pcie_deassert_core_reset(ep) and exynos_pcie_enable_irq_pulse(ep),
leaving the PCIe core held in reset and the PHY off, while
exynos_pcie_resume_noirq() does not check the return value of
exynos_pcie_host_init(pp):

drivers/pci/controller/dwc/pci-exynos.c:exynos_pcie_resume_noirq() {
	...
	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
	if (ret)
		return ret;

	/* exynos_pcie_host_init controls ep->phy */
	exynos_pcie_host_init(pp);
	dw_pcie_setup_rc(pp);
	exynos_pcie_start_link(pci);
	return dw_pcie_wait_for_link(pci);
}

Should exynos_pcie_resume_noirq() check the return value of
exynos_pcie_host_init(pp) and disable ep->supplies before returning the
error instead of calling dw_pcie_setup_rc(pp) and exynos_pcie_start_link(pci)
while the controller core is in reset?

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