Re: [PATCH v6 3/3] PCI: imx6: Add root port reset to support link recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Read-modify-write operations on mixed RW/W1C register silently drop interrupts.
- [High] Shared interrupt handler checks status but fails to verify if the interrupt is enabled.
- [High] Ignored return value of `dw_pcie_wait_for_link()` hides downstream link recovery failures.
- [High] Lockless traversal of PCI bus devices in an asynchronous IRQ thread exposes the kernel to Use-After-Free and list corruption.
- [High] Link down interrupt is not disabled during system shutdown, causing an AXI bus fault and system hang.
- [High] Failed root port reset leaves the controller unclocked while still active, causing a crash on subsequent PM suspend.
- [High] Unprotected hardware indirect register access during root port reset.
- [Medium] Ignored negative error codes from `platform_get_irq_byname_optional()` break probe deferral.
- [Low] Build failure due to undefined functions and struct members.
--

commit 5bee25b25210b0bc28349bc8474d10204a25cd72
Author: Richard Zhu <hongxing.zhu@nxp.com>

PCI: imx6: Add root port reset to support link recovery

The PCIe link can go down due to various unexpected circumstances. Add
root port reset support to enable link recovery for the i.MX PCIe
controller when the optional "intr" interrupt is present.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 773ab65b2afac..aa5c90be23df8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1394,6 +1401,13 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	imx_setup_phy_mpll(imx_pcie);
>  
> +	/*
> +	 * Callback invoked by PCI core when link down is detected and
> +	 * recovery is needed.
> +	 */
> +	if (pp->bridge)
> +		pp->bridge->reset_root_port = imx_pcie_reset_root_port;

[Severity: Low]
Does the reset_root_port member actually exist in struct pci_host_bridge? 
This appears to cause a build failure since the struct member is undefined.

[ ... ]

> @@ -1661,6 +1675,9 @@ static int imx_pcie_suspend_noirq(struct device *dev)
>  	if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND))
>  		return 0;
>  
> +	if (imx_pcie->lnk_intr > 0)
> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);

[Severity: High]
Since IMX95_LINK_INT_CTRL_STS mixes an RW enable bit and a write-1-to-clear 
status bit, will using regmap_clear_bits perform a read-modify-write cycle 
that inadvertently clears pending status bits if they are latched during the 
read?

[ ... ]

> +static irqreturn_t imx_pcie_lnk_irq_isr(int irq, void *priv)
> +{
> +	struct imx_pcie *imx_pcie = priv;
> +	struct dw_pcie *pci = imx_pcie->pci;
> +	struct device *dev = pci->dev;
> +	u32 val;
> +
> +	regmap_read(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS, &val);
> +	if (val & IMX95_LINK_DOWN_INT_STS) {

[Severity: High]
Is it possible for this shared interrupt handler to be invoked while the 
interrupt is disabled, see the latched status bit, and erroneously return 
IRQ_WAKE_THREAD? 

If a PME interrupt fires while the link down interrupt is intentionally 
disabled, the ISR will still claim it. Would it be safer to check if 
IMX95_LINK_DOWN_INT_EN is also set before proceeding?

> +		dev_dbg(dev, "PCIe link down detected, initiating recovery\n");
> +		/* Clear link down interrupt status by writing 1b'1 to it */
> +		regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				IMX95_LINK_DOWN_INT_STS);

[Severity: High]
Similar to the regmap_clear_bits call above, could using regmap_set_bits here 
perform a read-modify-write cycle that inadvertently clears other pending 
write-1-to-clear bits in the same register?

> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);
> +
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t imx_pcie_lnk_irq_thread(int irq, void *priv)
> +{
> +	struct imx_pcie *imx_pcie = priv;
> +	struct dw_pcie *pci = imx_pcie->pci;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	struct pci_dev *port;
> +
> +	for_each_pci_bridge(port, pp->bridge->bus)

[Severity: High]
Is it safe to iterate the bus list locklessly in this IRQ thread? 
Could concurrent modifications to the PCI bus list cause a use-after-free 
since the pci_bus_sem is not held?

> +		if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);

[Severity: Low]
Does the pci_host_handle_link_down function exist in the tree? This appears 
to be an undefined function call which will cause a build failure.

> +
> +	regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +			IMX95_LINK_DOWN_INT_EN);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int imx_pcie_reset_root_port(struct pci_host_bridge *bridge,
> +				    struct pci_dev *pdev)
> +{
> +	struct pci_bus *bus = bridge->bus;
> +	struct dw_pcie_rp *pp = bus->sysdata;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> +	int ret;
> +
> +	imx_pcie_msi_save_restore(imx_pcie, true);
> +	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT))
> +		imx_pcie_lut_save(imx_pcie);

[Severity: High]
The i.MX95 PCIe hardware uses an indirect access mechanism for LUTs. Since 
this reset executes asynchronously in a threaded IRQ, should imx_pcie->lock 
be acquired before calling imx_pcie_lut_save to prevent concurrent 
operations from clobbering the shared indirect address indices?

> +	imx_pcie_stop_link(pci);
> +	imx_pcie_host_exit(pp);
> +
> +	ret = imx_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(pci->dev, "Failed to re-init PCIe\n");
> +		return ret;
> +	}
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto err_host_deinit;
> +
> +	imx_pcie_start_link(pci);
> +	dw_pcie_wait_for_link(pci);

[Severity: High]
Should the return value of dw_pcie_wait_for_link be checked here? 
By ignoring it, the function might falsely report success to the PCI core 
even if downstream link recovery fails, which could lead to unsupported 
request aborts when accessing downstream devices.

> +
> +	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT))
> +		imx_pcie_lut_restore(imx_pcie);
> +	imx_pcie_msi_save_restore(imx_pcie, false);
> +
> +	dev_dbg(pci->dev, "Root port reset completed\n");
> +	return 0;
> +
> +err_host_deinit:
> +	imx_pcie_host_exit(pp);

[Severity: High]
If the re-initialization fails and imx_pcie_host_exit disables the clocks, 
the device still remains bound. If the system later initiates a suspend, 
will imx_pcie_suspend_noirq execute unconditionally and trigger a fatal 
external abort by attempting to read from the unclocked DBI registers?

> +
> +	return ret;
> +}
> +

[ ... ]

> @@ -1919,9 +2018,33 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  			val |= PCI_MSI_FLAGS_ENABLE;
>  			dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
>  		}
> +
> +		/* Get link event irq if it is present */
> +		imx_pcie->lnk_intr = platform_get_irq_byname_optional(pdev, "intr");
> +		if (imx_pcie->lnk_intr > 0) {

[Severity: Medium]
If the interrupt controller is not yet ready, the platform irq function 
returns -EPROBE_DEFER. Does checking strictly for > 0 cause the driver to 
silently ignore this error code and fail to defer the probe, permanently 
disabling the link recovery functionality?

[ ... ]

> +err_host_deinit:
> +	dw_pcie_host_deinit(&pci->pp);
> +
> +	return ret;
>  }
>  
>  static void imx_pcie_shutdown(struct platform_device *pdev)

[Severity: High]
The patch enables the link down interrupt during probe, but looking at 
imx_pcie_shutdown(), it does not disable it before asserting core reset. 
Does asserting the core reset drop the link and trigger the interrupt? 
If so, this could wake the threaded handler to access DBI registers while 
the core is in reset, causing a synchronous external abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603062510.3767610-1-hongxing.zhu@oss.nxp.com?part=3