Re: [PATCH v4 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 8 potential issue(s) to consider:
- [High] Optional IRQ negative error code implicitly cast to `u32` causes probe failure on older platforms.
- [High] Iterating over `bus->devices` without acquiring `pci_bus_sem` creates a Use-After-Free race condition.
- [High] Read-Modify-Write regmap operations on W1C status bits silently clear unhandled interrupts.
- [High] Disabling hardware clocks while a shared interrupt remains active enables a synchronous external abort vector.
- [High] Using a standard mmio regmap from a primary hardirq handler causes sleep-in-atomic deadlocks on PREEMPT_RT.
- [Medium] System shutdown callback inadvertently triggers a link recovery thread, leaving hardware out of reset.
- [Medium] Non-atomic LUT save/restore operations in the recovery thread lack mutex protection.
- [Medium] Failure to mask the hardware interrupt on driver unbind causes an unhandled interrupt storm.
--

commit c32252b83882f7c8756d22c0a5aa880135207cb1
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 e35044cc52185..38e0a16655092 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
[ ... ]
> @@ -1573,6 +1588,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)
> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);

Does using regmap_clear_bits() and regmap_set_bits() here unintentionally
clear pending Write-1-to-Clear (W1C) status bits?

Since these functions perform a read-modify-write operation, if a W1C status
bit like IMX95_LINK_DOWN_INT_STS is active during the read phase, writing it
back will clear the status bit. This could potentially result in losing
pending link-down or PME events before their handlers can process them.

[ ... ]
> @@ -1632,6 +1653,84 @@ static const struct dev_pm_ops imx_pcie_pm_ops = {
>  				  imx_pcie_resume_noirq)
>  };
>  
> +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);

Could this regmap_read() call cause a sleep-in-atomic deadlock on PREEMPT_RT?

Since this handler is registered with IRQF_SHARED, it runs in primary hardirq
context. The iomuxc_gpr regmap is initialized via devm_regmap_init_mmio()
without enabling the use_raw_spinlock flag, meaning it relies on a standard
spinlock. On PREEMPT_RT kernels, this maps to a sleepable rt_mutex, which
cannot be acquired safely from hardirq context.

> +	if (val & IMX95_LINK_DOWN_INT_STS) {
> +		dev_dbg(dev, "PCIe link down detected, initiating recovery\n");
> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);
> +		regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				IMX95_LINK_DOWN_INT_STS);
> +
> +		return IRQ_WAKE_THREAD;
> +	} else {
> +		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)

Is it safe to iterate over the bus devices list here without acquiring the
pci_bus_sem read lock?

Since this threaded handler executes asynchronously, concurrent PCI subsystem
modifications like hotplug events or driver unbinding could mutate the list.
Traversing bus->devices without down_read(&pci_bus_sem) might expose the
system to list corruption or use-after-free dereferences.

> +		if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);
> +
> +	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);

Should these LUT save and restore operations be protected by the
imx_pcie->lock mutex?

Other parts of the driver appear to protect concurrent LUT accesses using
this mutex. Since the recovery thread runs asynchronously, racing with
concurrent endpoint configuration routines could lead to interleaved I/O
operations and corrupt the LUT index.

> +	imx_pcie_stop_link(pci);
> +	imx_pcie_host_exit(pp);

Does fully disabling the clocks here risk a synchronous external abort?

The imx_pcie_host_exit() function completely disables the PCIe controller
clocks. If another device sharing this interrupt line triggers an interrupt
while the clocks are off, the kernel will invoke all registered primary
handlers. If the PME handler attempts to read PCIe Root Port configuration
registers, accessing unclocked memory-mapped registers could trigger a
synchronous external abort.

[ ... ]
> @@ -1834,9 +1933,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(pdev, "intr");
> +		if (imx_pcie->lnk_intr > 0) {

Could this check fail to handle missing optional interrupts on older
platforms?

The platform_get_irq_byname() function returns a negative error code like
-ENXIO when the interrupt is missing. Because lnk_intr is declared as a u32,
the negative error code is implicitly cast to a large positive integer.
The condition > 0 will evaluate to true, and the driver will attempt to
request an invalid massive IRQ number, unconditionally failing the probe.

> +			ret = devm_request_threaded_irq(dev, imx_pcie->lnk_intr,
> +							imx_pcie_lnk_irq_isr,
> +							imx_pcie_lnk_irq_thread,
> +							IRQF_SHARED,
> +							"lnk", imx_pcie);
> +			if (ret) {
> +				dev_err_probe(dev, ret,
> +					      "unable to request LNK IRQ\n");
> +				goto err_host_deinit;
> +			}
> +
> +			regmap_set_bits(imx_pcie->iomuxc_gpr,
> +					IMX95_LINK_INT_CTRL_STS,
> +					IMX95_LINK_DOWN_INT_EN);

Is there a mechanism to disable the hardware interrupt on driver unbind?

The interrupt is enabled in the IOMUXC GPR register during probe. If the
driver is unbound and its IRQ handler is freed without a removal callback
clearing the IMX95_LINK_DOWN_INT_EN bit, the hardware could assert the
shared interrupt continuously if the link goes down. Since no handler
would be registered to acknowledge it, this might lead to an unhandled
interrupt storm.

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

Will asserting the core reset during shutdown unintentionally trigger the
link recovery thread?

During system reboot, imx_pcie_shutdown() calls imx_pcie_assert_core_reset()
to quiesce the hardware, which physically drops the PCIe link. Because the
interrupt enable bit is not cleared during shutdown, this intentional link
drop will trigger the hardware interrupt.

The resulting interrupt will queue imx_pcie_lnk_irq_thread(), which calls
imx_pcie_reset_root_port(). This will explicitly deassert the core reset,
unintentionally bringing the hardware back out of reset and defeating the
purpose of the shutdown routine.

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