Re: [PATCH v4] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Ignored return value and missing cleanup in endpoint initialization (`imx_add_pcie_ep`).
- [High] Calling a sleeping function (`usleep_range`) in a `noirq` system suspend/resume phase where local interrupts are disabled.
--

--- Patch [1]: [PATCH v4] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing ---
commit cdeb44e5cad8e8dc7dee3f092df4048d6cca6e75
Author: Richard Zhu <[email protected]>

PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

This commit fixes a boot hang on i.MX6Q/DL variants by reordering the PHY
initialization sequence. It moves TEST_PD control into the reset functions
and adds an explicit delay for the PHY power-up sequence.

Closes: https://lore.kernel.org/lkml/[email protected]/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..da9d4d6d7d46a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -835,11 +832,15 @@ static int imx6qp_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
>  
>  static int imx6q_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
>  {
> -	if (!assert)
> -		return 0;
> +	if (assert)
> +		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +				IMX6Q_GPR1_PCIE_TEST_PD);
> +	else
> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +				  IMX6Q_GPR1_PCIE_TEST_PD);
>  
> -	regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_TEST_PD);
> -	regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_REF_CLK_EN);
> +	if (!assert)
> +		usleep_range(200, 500);

[Severity: High]
This is a pre-existing issue, but does calling usleep_range() here cause
a scheduling while atomic bug during system resume?

During system resume, the noirq phase calls imx_pcie_resume_noirq() with
local interrupts disabled across all CPUs. This eventually calls down to
this function:

imx_pcie_resume_noirq()
   imx_pcie_deassert_core_reset()
      imx6q_pcie_core_reset(assert=false)
         usleep_range(200, 500);

Since usleep_range() sets the task state to TASK_UNINTERRUPTIBLE and calls
schedule(), will this hard-lock or trigger a bug when timer interrupts
cannot fire to wake the task?

[ ... ]

> @@ -1445,6 +1446,7 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>  	return 0;
>  
>  err_phy_off:
> +	imx_pcie_assert_core_reset(imx_pcie);
>  	phy_power_off(imx_pcie->phy);
>  err_phy_exit:
>  	phy_exit(imx_pcie->phy);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe for
imx_add_pcie_ep() to ignore the return value of imx_pcie_host_init()?

If imx_pcie_host_init() fails, for example when enabling clocks or
regulators, it seems imx_add_pcie_ep() will continue execution:

imx_add_pcie_ep() {
    ...
    imx_pcie_host_init(pp);
    ...
    dw_pcie_ep_init(&imx_pcie->pci->ep);
}

Could this cause an external abort when dw_pcie_ep_init_registers()
attempts to access unclocked or unpowered PCIe registers?

Also, if dw_pcie_ep_init() subsequently fails, it looks like
imx_pcie_host_exit() is never called to clean up resources.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1