Re: [PATCH v7 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 7 potential issue(s) to consider:
- [High] Using read-modify-write operations on a mixed RW/W1C register unintentionally clears pending hardware interrupts.
- [High] The ISR unconditionally clears the link down status bit even if the interrupt is disabled, dropping shared link recovery events.
- [High] Unsafe, lockless iteration over the PCI bus devices list without holding pci_bus_sem.
- [High] The threaded interrupt handler is not synchronized during system shutdown, leading to a race condition with controller power-off.
- [High] Accessing shared LUT GPR registers without acquiring imx_pcie->lock causes a data race.
- [Medium] The `imx_pcie_reset_root_port` function silently ignores failures to recover the PCIe link.
- [Low] The patch references undeclared functions and undefined struct members, causing build failures.
--

commit 1a657928e9eb6aa6da8f1cc80545e5be5d066898
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 98e1db751132a..b38eb61983e15 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1442,6 +1450,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]
Is reset_root_port defined in struct pci_host_bridge? Looking at
include/linux/pci.h, this member does not appear to exist in the standard
kernel. Does this patch depend on a prerequisite patch that adds it?

[ ... ]

> @@ -1774,6 +1795,86 @@ 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);
> +	if (val & IMX95_LINK_DOWN_INT_STS) {
> +		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);
> +		if (!(val & IMX95_LINK_DOWN_INT_EN))
> +			return IRQ_NONE;

[Severity: High]
If this interrupt is shared and masked during thread execution, could this
unconditionally clear a new link down event?

Since the status is cleared before checking IMX95_LINK_DOWN_INT_EN, if a new
event triggers concurrently with the threaded handler, it appears the event
will be cleared and then discarded when the ISR returns IRQ_NONE.

> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);
> +
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	regmap_read(imx_pcie->iomuxc_gpr, IMX95_PE0_INT_STS, &val);
> +	if (unlikely(val))
> +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_INT_STS, val);
> +
> +	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)
> +		if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);

[Severity: High]
Is it safe to iterate over the PCI bus devices list asynchronously here without
holding the pci_bus_sem semaphore?

If concurrent PCI enumeration, hotplug device removal, or SR-IOV VF
modifications occur, could this result in list pointer corruption or a
use-after-free?

[Severity: Low]
Also, is pci_host_handle_link_down() declared in the standard kernel headers?
I am unable to find it in include/linux/pci.h, which might cause a build
failure without a prerequisite patch.

> +
> +	regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +			IMX95_LINK_DOWN_INT_EN);

[Severity: High]
Can regmap_set_bits() unintentionally clear pending hardware interrupts here?

Since regmap_set_bits() performs a read-modify-write, and the
IMX95_LINK_DOWN_INT_STS (BIT 11) is a write-1-to-clear bit, could it
accidentally write back a 1 for a newly arrived link down event and
permanently lose that event?

> +
> +	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]
Does imx_pcie_lut_save() need to acquire the imx_pcie->lock?

Since imx_pcie_reset_root_port() runs asynchronously in the recovery thread,
could this cause a data race on the shared IMX95_PE0_LUT_ACSCTRL index
register if another device probe concurrently calls imx_pcie_add_lut()?

> +	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)
> +		return ret;
> +
> +	imx_pcie_start_link(pci);
> +	dw_pcie_wait_for_link(pci);

[Severity: Medium]
Should the return value of dw_pcie_wait_for_link() be checked and propagated?

If the link fails to come up (for example, returning -ETIMEDOUT because the
endpoint was unplugged), dropping the error allows the function to
unconditionally return 0. Could this leave the system in an inconsistent
state?

> +
> +	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT))
> +		imx_pcie_lut_restore(imx_pcie);

[Severity: High]
Similar to the save function above, does imx_pcie_lut_restore() need to
acquire imx_pcie->lock to prevent data races on the shared LUT index register
during restoration?

[ ... ]

> @@ -1973,15 +2074,46 @@ static int imx_pcie_probe(struct platform_device *pdev)

[ ... ]

>  static void imx_pcie_shutdown(struct platform_device *pdev)
>  {
>  	struct imx_pcie *imx_pcie = platform_get_drvdata(pdev);
>  
> +	if (imx_pcie->lnk_intr > 0)
> +		regmap_clear_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> +				  IMX95_LINK_DOWN_INT_EN);
>  	/* bring down link, so bootloader gets clean state in case of reboot */
>  	imx_pcie_assert_core_reset(imx_pcie);

[Severity: High]
Can a concurrent threaded interrupt handler run during this shutdown process?

Since disable_irq() is not called, if a link down event triggers just before
shutdown, could the thread access PCIe configuration space right as
pci_pwrctrl_power_off_devices() powers off the hardware, potentially causing a
synchronous external abort?

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