[PATCH v5 0/3] Add root port reset to support link recovery

[PATCH v5 0/3] Add root port reset to support link recovery

[Richard Zhu]

Based on the following patch-set[1] issued by Mani.
Add support for resetting the Root Port for i.MX PCIe to enable link recovery.

[1] [PATCH v8 0/5] PCI: Add support for resetting the Root Ports in a platform specific way

PCIe links can go down due to various unexpected circumstances. This patch series
adds root port reset support for link recovery on i.MX PCIe controllers when the
optional "intr" interrupt is present.

When a link down event is detected, the root port reset uninitializes and
reinitializes the PCIe controller, then restarts the PCIe link.

On i.MX95 platforms, link events and PME share the same interrupt line.
Link event interrupts cannot use only an IRQ thread handler because the PME
driver uses request_irq() to bind the PME interrupt directly with only the
IRQF_SHARED flag set.

To address this, we register one handler with IRQF_SHARED for link event
interrupts and manipulate the enable bits of link events to ensure the same
interrupt source is triggered only once at a time.

Additionally, this series adds 'intr', 'aer', and 'pme' interrupt entries to
the i.MX6Q PCIe binding to support PCIe event-based interrupts for general
controller events, Advanced Error Reporting, and Power Management Events
respectively.

Changes in v5:
- Update the commit message of the first dt-binding patch for clarity.
- Add explicit comment explaining that writing 1 to IMX95_LINK_DOWN_INT_STS
clears the bit

Changes in v4:
- Set these new added three interrupts as optional interrupt.

Changes in v3:
- Don't add a new if:block; Drop the maxItems constraint of the interrupts
  property for i.MX95 PCIe.
- Add constraints for the interrupts property for other variants.
- Regarding the ABI break: add descriptions explaining why these new
  interrupts are mandatory and required by i.MX95 PCIe.

Changes in v2:
- Constrain the new added three interrupt entries to be valid only for the
  i.MX95 variant using conditional schemas

[PATCH v5 1/3] dt-bindings: imx6q-pcie: Add optional interrupt
[PATCH v5 2/3] arm64: dts: imx95: Add dma, intr, aer and pme
[PATCH v5 3/3] PCI: imx6: Add root port reset to support link

Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml |   6 +++++
arch/arm64/boot/dts/freescale/imx95.dtsi                  |  16 +++++++++---
drivers/pci/controller/dwc/pci-imx6.c                     | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 142 insertions(+), 4 deletions(-)

[PATCH v5 1/3] dt-bindings: imx6q-pcie: Add optional interrupt entries for intr, aer and pme

[Richard Zhu]

The i.MX95 PCIe controller introduces three dedicated hardware interrupt
lines:
- intr: general controller events
- aer: Advanced Error Reporting
- pme: Power Management Events

Earlier i.MX PCIe variants (imx6q, imx6sx, imx6qp, imx7d, imx8mm, imx8mp,
imx8mq, imx8q) do not have these dedicated interrupt lines.

PCIe basic functionality (enumeration, configuration, and data transfer)
works correctly regardless of whether these interrupts are present. Mark
these interrupts as optional to maintain backward compatibility with SoCs
that lack these hardware interrupt lines.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index 9d1349855b42..cf709132ff1e 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -58,12 +58,18 @@ properties:
     items:
       - description: builtin MSI controller.
       - description: builtin DMA controller.
+      - description: PCIe event interrupt.
+      - description: builtin AER SPI standalone interrupt line.
+      - description: builtin PME SPI standalone interrupt line.
 
   interrupt-names:
     minItems: 1
     items:
       - const: msi
       - const: dma
+      - const: intr
+      - const: aer
+      - const: pme
 
   reset-gpio:
     description: Should specify the GPIO for controlling the PCI bus device
-- 
2.37.1

Re: [PATCH v5 1/3] dt-bindings: imx6q-pcie: Add optional interrupt entries for intr, aer and pme

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Expanding the global `interrupts` and `interrupt-names` properties without adding conditional blocks allows legacy compatibles to incorrectly pass validation with unsupported interrupts.
- [Medium] Using an ordered list for `items` fails validation if a device omits the optional intermediate `dma` interrupt but includes subsequent interrupts.
--

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

dt-bindings: imx6q-pcie: Add optional interrupt entries for intr, aer and pme

This commit adds three new dedicated hardware interrupt lines (intr, aer, pme)
introduced in the i.MX95 PCIe controller. These interrupts are added to the
global interrupts property as optional entries to maintain compatibility with
older SoC variants.

> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index 9d1349855b422..cf709132ff1e2 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -58,12 +58,18 @@ properties:
>      items:
>        - description: builtin MSI controller.
>        - description: builtin DMA controller.
> +      - description: PCIe event interrupt.
> +      - description: builtin AER SPI standalone interrupt line.
> +      - description: builtin PME SPI standalone interrupt line.

[Severity: Medium]
Will this expansion allow legacy compatibles to incorrectly pass validation
with unsupported interrupts?

The commit message notes that earlier i.MX PCIe variants lack these
dedicated hardware interrupt lines. Without adding an if-then conditional
block to restrict interrupts and interrupt-names to maxItems: 2 for older
variants, invalid device trees specifying these unsupported interrupts will
now silently pass validation.

>  
>    interrupt-names:
>      minItems: 1
>      items:
>        - const: msi
>        - const: dma
> +      - const: intr
> +      - const: aer
> +      - const: pme

[Severity: Medium]
Does using an ordered list for items cause validation to fail if a device
omits the optional dma interrupt?

In YAML schema validation, an items list enforces strict positional matching.
If a device tree defines msi, intr, aer, and pme but omits the optional dma
interrupt, the intr string will appear at index 1 and fail the strict
evaluation against "- const: dma".

Could the schema use an unordered list (such as oneOf or items: enum: [...])
to allow independently optional interrupts without strict sequencing errors?

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

[PATCH v5 2/3] arm64: dts: imx95: Add dma, intr, aer and pme interrupts for PCIe

[Richard Zhu]

The current PCIe device tree configuration only defines the MSI
interrupt, which is sufficient for basic PCIe operation but limits
advanced functionality.

Add the following interrupt lines to pcie0 and pcie1 nodes:
- dma: DMA interrupt for PCIe DMA operations
- intr: General controller events and link state changes
- aer: Advanced Error Reporting interrupt
- pme: Power Management Event interrupt

This enables enhanced PCIe features and capabilities that were
previously unavailable due to missing interrupt definitions.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx95.dtsi | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
index adcc0e1d3696..f40388958717 100644
--- a/arch/arm64/boot/dts/freescale/imx95.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
@@ -1948,8 +1948,12 @@ pcie0: pcie@4c300000 {
 			bus-range = <0x00 0xff>;
 			num-lanes = <1>;
 			num-viewport = <8>;
-			interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "msi";
+			interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi", "dma", "intr", "aer", "pme";
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0x7>;
 			interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
@@ -2023,8 +2027,12 @@ pcie1: pcie@4c380000 {
 			bus-range = <0x00 0xff>;
 			num-lanes = <1>;
 			num-viewport = <8>;
-			interrupts = <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "msi";
+			interrupts = <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi", "dma", "intr", "aer", "pme";
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0x7>;
 			interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>,
-- 
2.37.1

Re: [PATCH v5 2/3] arm64: dts: imx95: Add dma, intr, aer and pme interrupts for PCIe

[sashiko-bot]

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

Pre-existing issues:
- [High] The pcie0 node is missing the msi-map-mask property, which can cause MSI mapping failures for downstream devices on higher bus numbers.
--

commit 0e2b46336a5b22c94438abb01ead59266994b86b
Author: Richard Zhu <hongxing.zhu@nxp.com>

arm64: dts: imx95: Add dma, intr, aer and pme interrupts for PCIe

This commit updates the device tree configurations for pcie0 and pcie1
on imx95 to include additional interrupts for dma, intr, aer, and pme.
These new interrupt definitions enable advanced PCIe functionality beyond
basic MSI support.

> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
> index 71394871d8dd0..6896d9c15bf53 100644
> --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
> @@ -1861,8 +1861,12 @@ pcie0: pcie@4c300000 {
>  			bus-range = <0x00 0xff>;
>  			num-lanes = <1>;
>  			num-viewport = <8>;
> -			interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
> -			interrupt-names = "msi";
> +			interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "msi", "dma", "intr", "aer", "pme";

[Severity: High]
This is a pre-existing issue, but I noticed the pcie0 node is missing an
msi-map-mask property. The node defines an iommu-map-mask but lacks a
corresponding msi-map-mask.

Without msi-map-mask, of_map_id() applies a default mask of 0xffffffff to
the PCI requestor ID during MSI allocation. If a PCIe switch is connected
and downstream devices are enumerated on a bus number >= 2, their requestor
IDs (e.g., 0x200, 0x300) might fail to match the defined msi-map entries
which only cover 0x0 and 0x100 ranges.

Could this cause of_map_id to bypass translation, leaving the msi domain
unassigned and breaking MSI functionality for downstream topologies beyond
the first bus?

[ ... ]

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

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

[Richard Zhu]

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.

Reset root port to uninitialize, initialize the PCIe controller, and
restart the PCIe link at end when a link down event happens.

On i.MX95 platforms, link events and PME share the same interrupt line.
The link event interrupt cannot use a threaded-only IRQ handler because
the PME driver uses request_irq() with only the IRQF_SHARED flag set,
which requires a primary handler.

To handle this shared interrupt scenario, register a primary interrupt
handler with IRQF_SHARED for link events and manipulate the link event
enable bits to ensure the shared interrupt source triggers only one
handler at a time.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 124 ++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 1034ac5c5f5c..c286581bede8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -34,6 +34,7 @@
 #include <linux/pm_runtime.h>
 
 #include "../../pci.h"
+#include "../pci-host-common.h"
 #include "pcie-designware.h"
 
 #define IMX8MQ_GPR_PCIE_REF_USE_PAD		BIT(9)
@@ -78,6 +79,10 @@
 #define IMX95_SID_MASK				GENMASK(5, 0)
 #define IMX95_MAX_LUT				32
 
+#define IMX95_LINK_INT_CTRL_STS			0x1040
+#define IMX95_LINK_DOWN_INT_STS			BIT(11)
+#define IMX95_LINK_DOWN_INT_EN			BIT(10)
+
 #define IMX95_PCIE_RST_CTRL			0x3010
 #define IMX95_PCIE_COLD_RST			BIT(0)
 
@@ -125,6 +130,8 @@ enum imx_pcie_variants {
 #define IMX_PCIE_MAX_INSTANCES	2
 
 struct imx_pcie;
+static int imx_pcie_reset_root_port(struct pci_host_bridge *bridge,
+				    struct pci_dev *pdev);
 
 struct imx_pcie_drvdata {
 	enum imx_pcie_variants variant;
@@ -158,6 +165,7 @@ struct imx_pcie {
 	bool			supports_clkreq;
 	bool			enable_ext_refclk;
 	struct regmap		*iomuxc_gpr;
+	u32			lnk_intr;
 	u16			msi_ctrl;
 	u32			controller_id;
 	struct reset_control	*pciephy_reset;
@@ -1301,6 +1309,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;
+
 	return 0;
 
 err_phy_off:
@@ -1568,6 +1583,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);
 	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);
@@ -1618,6 +1636,9 @@ static int imx_pcie_resume_noirq(struct device *dev)
 	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);
+	if (imx_pcie->lnk_intr)
+		regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
+				IMX95_LINK_DOWN_INT_EN);
 
 	return 0;
 }
@@ -1627,6 +1648,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) {
+		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);
+		/* 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);
+
+		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)
+		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);
+	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);
+
+	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);
+
+	return ret;
+}
+
 static int imx_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1829,9 +1929,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) {
+			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);
+		}
 	}
 
 	return 0;
+
+err_host_deinit:
+	dw_pcie_host_deinit(&pci->pp);
+
+	return ret;
 }
 
 static void imx_pcie_shutdown(struct platform_device *pdev)
-- 
2.37.1

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

[sashiko-bot]

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

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

[Hongxing Zhu]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Thursday, May 21, 2026 7:21 PM
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: devicetree@vger.kernel.org; robh@kernel.org; Frank.Li@kernel.org; linux-
> pci@vger.kernel.org; conor+dt@kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v5 3/3] PCI: imx6: Add root port reset to support link recovery
> 
> 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?
> 
Good catch! will update it later.
> [ ... ]
> 
> > @@ -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.
No worries, the dependency had been specified.
> 
> [ ... ]
> 
> > @@ -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.
> 
Okay, would clear the W1C bit firstly, then do the regmap_clear_bits().
> [ ... ]
> 
> > @@ -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.
IRQ_WAKE_THREAD only be returned when IMX95_LINK_DOWN_INT_STS is asserted.
> 
> [ ... ]
> 
> > +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.
> 
No worries, the dependency had been specified.
> [ ... ]
> 
> > +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.
No worries, there are no race when do the lut save/restore.
> 
> [ ... ]
> 
> > @@ -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?
Okay, would update it later.

Best Regards
Richard Zhu

> 
> --
> Sashiko AI
> review * https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsa
> shiko.dev%2F%23%2Fpatchset%2F20260521093255.61585-1-
> hongxing.zhu%40nxp.com%3Fpart%3D3&data=05%7C02%7Chongxing.zhu%40nx
> p.com%7Ce831884f29d14575cb8408deb72b1830%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C639149592897765731%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Ui0aRHNXO9FFLrgBoH
> ClFZFn6eYvAj1gwXiWD45uNlc%3D&reserved=0