Re: [PATCH v5 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] The `lnk_intr` member is defined as `u32`, which wraps negative error codes from `platform_get_irq_byname` to large positive integers, breaking driver probe.
- [High] Read-modify-write (RMW) operations via `regmap_clear_bits` and `regmap_set_bits` unintentionally clear Write-1-to-Clear (W1C) status bits.
- [High] Iterating over the PCI bus device list without holding the `pci_bus_sem` read lock causes a race condition.
- [High] The patch references `reset_root_port` and calls `pci_host_handle_link_down()`, neither of which exist in the mainline kernel tree.
- [High] Hardware LUT state is accessed in `imx_pcie_reset_root_port` without acquiring the protective `imx_pcie->lock` mutex.
- [Medium] The shared interrupt handler claims the interrupt without verifying that its specific interrupt source is actually enabled, leading to spurious wakeups.
- [Low] Using `platform_get_irq_byname` instead of `platform_get_irq_byname_optional` for an optional interrupt will cause unnecessary kernel error logs.
--

commit c3df91626a3cc2df53cc4e1da0e0b1aeb4d5d8cd
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
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -158,6 +165,7 @@ struct imx_pcie {
>  	bool			supports_clkreq;
>  	bool			enable_ext_refclk;
>  	struct regmap		*iomuxc_gpr;
> +	u32			lnk_intr;

[Severity: High]
Will platform_get_irq_byname returning a negative error code (like -ENXIO)
wrap to a large positive integer here, causing the later
if (imx_pcie->lnk_intr > 0) check to pass and break driver probe on systems
without the optional interrupt? Should this be an int instead?

[ ... ]

> @@ -1306,6 +1314,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: High]
Does struct pci_host_bridge have a reset_root_port member? It appears this
patch relies on out-of-tree or unmerged prerequisites and will cause build
failures.

[ ... ]

> @@ -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);

[Severity: High]
Does this read-modify-write operation accidentally clear Write-1-to-Clear
status bits?

IMX95_LINK_INT_CTRL_STS contains W1C bits like IMX95_LINK_DOWN_INT_STS.
When regmap_clear_bits reads the register, any pending W1C bits will be
read as 1. Writing them back during the modify phase will inadvertently
clear the status bits without handling the interrupt.

[ ... ]

> @@ -1632,6 +1653,85 @@ 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) {

[Severity: Medium]
Should this check verify that the interrupt is actually enabled?

Since this is an IRQF_SHARED handler sharing a line with the PME driver, if
the hardware status bit is asserted but IMX95_LINK_DOWN_INT_EN is masked, a
subsequent interrupt triggered by the PME driver could cause this ISR to
incorrectly claim the interrupt and return IRQ_WAKE_THREAD.

[ ... ]

> +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 over the PCI bus device list without holding the
pci_bus_sem read lock? Traversing the bus->devices list asynchronously
risks list corruption and use-after-free vulnerabilities if PCI devices
are concurrently hot-added or removed. Should pci_walk_bus() or
down_read(&pci_bus_sem) be used here?

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

[Severity: High]
Is pci_host_handle_link_down() available in the mainline kernel tree?
This function appears to be undefined, which will cause a build failure.

[ ... ]

> +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 this access the hardware LUT state without acquiring the protective
imx_pcie->lock mutex?

Since imx_pcie_reset_root_port can be invoked asynchronously from the
link down interrupt handler, concurrent accesses to the LUT configuration
from other execution paths (which do use the lock) could lead to a race
condition and corrupt the hardware LUT configuration.

[ ... ]

> @@ -1834,9 +1934,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");

[Severity: Low]
Since this interrupt is optional, should this use
platform_get_irq_byname_optional() to avoid printing unnecessary error
messages to the kernel log when it is absent?

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