[PATCH] PCI: qcom: Mask PARF_INT_ALL_3 events on Nord

[PATCH] PCI: qcom: Mask PARF_INT_ALL_3 events on Nord

[Shawn Guo]

On Nord, the PARF_INT_ALL_3 interrupt group is unmasked out of reset
(PARF_INT_ALL_3_MASK reads 0x43fff), and its events are routed to the
same 'global' IRQ as PARF_INT_ALL. The driver neither handles nor clears
any of them, and the global IRQ handler only looks at
PARF_INT_ALL_STATUS.

Some endpoints send a Device Readiness Status (DRS) message after link
up, e.g. the SK hynix HFS001TFM9X186N NVMe drive (1c5c:1069). This
latches DRS_MSG_RECEIVED (bit 12) in PARF_INT_ALL_3_STATUS and keeps
the level triggered global IRQ asserted. The threaded handler then runs
in a tight loop with PARF_INT_ALL_STATUS reading 0, starving other work
and stalling the boot (RCU/SRCU stall warnings).

Mask all PARF_INT_ALL_3 events and clear any latched status on Nord,
before the global IRQ is requested in probe and again in
qcom_pcie_reset_root_port(), since the register is reset together with
PARF_INT_ALL_MASK. To keep the two writes together, move the
PARF_INT_ALL_MASK programming in probe into the same helper, which now
runs right before devm_request_threaded_irq() instead of after it.

Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 30 +++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index eba1b2cb30f0..03b2e61fc20f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -73,6 +73,8 @@
 #define PARF_DEVICE_TYPE			0x1000
 #define PARF_BDF_TO_SID_TABLE_N			0x2000
 #define PARF_BDF_TO_SID_CFG			0x2c00
+#define PARF_INT_ALL_3_MASK			0x2d8c
+#define PARF_INT_ALL_3_CLEAR			0x2d90
 
 /*
  * Outbound blocker range registers. The blocked range has a write pair
@@ -294,6 +296,8 @@ struct qcom_pcie_ops {
   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
   * snooping
   * @firmware_managed: Set if the Root Complex is firmware managed
+  * @mask_int_all_3: Mask PARF_INT_ALL_3 events, which are unmasked by default
+  * and would otherwise keep the 'global' IRQ asserted
   */
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
@@ -301,6 +305,7 @@ struct qcom_pcie_cfg {
 	bool firmware_managed;
 	bool no_l0s;
 	bool disable_cgc;
+	bool mask_int_all_3;
 };
 
 struct qcom_pcie_perst {
@@ -1738,6 +1743,7 @@ static const struct qcom_pcie_cfg cfg_nord = {
 	.override_no_snoop = true,
 	.no_l0s = true,
 	.disable_cgc = true,
+	.mask_int_all_3 = true,
 };
 
 static const struct qcom_pcie_cfg cfg_sc8280xp = {
@@ -1871,6 +1877,22 @@ static int qcom_pcie_set_max_opp(struct device *dev)
 	return ret;
 }
 
+static void qcom_pcie_enable_global_irq_events(struct qcom_pcie *pcie)
+{
+	/*
+	 * The PARF_INT_ALL_3 events (e.g. DRS_MSG_RECEIVED) are unmasked by
+	 * default, but not handled by the driver. Once latched, they keep the
+	 * level triggered 'global' IRQ asserted, so mask and clear them.
+	 */
+	if (pcie->cfg->mask_int_all_3) {
+		writel_relaxed(0, pcie->parf + PARF_INT_ALL_3_MASK);
+		writel_relaxed(~0U, pcie->parf + PARF_INT_ALL_3_CLEAR);
+	}
+
+	writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
+		       pcie->parf + PARF_INT_ALL_MASK);
+}
+
 /*
  * Qcom PCIe controllers only support one Root Port per controller instance. So
  * this function ignores the 'pci_dev' associated with the Root Port and just
@@ -1928,8 +1950,7 @@ static int qcom_pcie_reset_root_port(struct pci_host_bridge *bridge,
 	 * non-sticky.
 	 */
 	if (pcie->global_irq)
-		writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
-				pcie->parf + PARF_INT_ALL_MASK);
+		qcom_pcie_enable_global_irq_events(pcie);
 
 	qcom_pcie_start_link(pci);
 
@@ -2429,6 +2450,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 			goto err_host_deinit;
 		}
 
+		qcom_pcie_enable_global_irq_events(pcie);
+
 		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 						qcom_pcie_global_irq_thread,
 						IRQF_ONESHOT, name, pcie);
@@ -2438,9 +2461,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 			goto err_host_deinit;
 		}
 
-		writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
-				pcie->parf + PARF_INT_ALL_MASK);
-
 		pcie->global_irq = irq;
 	}
 
-- 
2.43.0

Re: [PATCH] PCI: qcom: Mask PARF_INT_ALL_3 events on Nord

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] PCI: qcom: missing qcom_pcie_enable_global_irq_events() call in
  qcom_pcie_resume_noirq()
- [Medium] PCI: qcom: late pcie->global_irq assignment in
  qcom_pcie_probe()
- [Low] PCI: qcom: missing Fixes: trailer for Nord PCIe controller
  support commit
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] PCI: qcom: asynchronous pm_runtime_put() in qcom_pcie_probe()
  [1]
--

commit 38b51880eb10af5087eac2fa97392397368f3684
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

> PCI: qcom: Mask PARF_INT_ALL_3 events on Nord
>
> On Nord, the PARF_INT_ALL_3 interrupt group is unmasked out of reset
> (PARF_INT_ALL_3_MASK reads 0x43fff), and its events are routed to the
> same 'global' IRQ as PARF_INT_ALL. The driver neither handles nor clears
> any of them, and the global IRQ handler only looks at
> PARF_INT_ALL_STATUS.
>
> Some endpoints send a Device Readiness Status (DRS) message after link
> up, e.g. the SK hynix HFS001TFM9X186N NVMe drive (1c5c:1069). This
> latches DRS_MSG_RECEIVED (bit 12) in PARF_INT_ALL_3_STATUS and keeps
> the level triggered global IRQ asserted. The threaded handler then runs
> in a tight loop with PARF_INT_ALL_STATUS reading 0, starving other work
> and stalling the boot (RCU/SRCU stall warnings).
>
> Mask all PARF_INT_ALL_3 events and clear any latched status on Nord,
> before the global IRQ is requested in probe and again in
> qcom_pcie_reset_root_port(), since the register is reset together with
> PARF_INT_ALL_MASK. To keep the two writes together, move the
> PARF_INT_ALL_MASK programming in probe into the same helper, which now
> runs right before devm_request_threaded_irq() instead of after it.
>
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>

[Severity: Low]
This fixes a boot stall on the Nord PCIe controller.  Should it include:

  Fixes: f75d2d64fff1 ("PCI: qcom: Add Nord PCIe controller support")

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index eba1b2cb30f0a..03b2e61fc20f6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -1871,6 +1877,22 @@ static int qcom_pcie_set_max_opp(struct device *dev)
>  	return ret;
>  }
>  
> +static void qcom_pcie_enable_global_irq_events(struct qcom_pcie *pcie)
> +{
> +	/*
> +	 * The PARF_INT_ALL_3 events (e.g. DRS_MSG_RECEIVED) are unmasked by
> +	 * default, but not handled by the driver. Once latched, they keep the
> +	 * level triggered 'global' IRQ asserted, so mask and clear them.
> +	 */
> +	if (pcie->cfg->mask_int_all_3) {
> +		writel_relaxed(0, pcie->parf + PARF_INT_ALL_3_MASK);
> +		writel_relaxed(~0U, pcie->parf + PARF_INT_ALL_3_CLEAR);
> +	}
> +
> +	writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
> +		       pcie->parf + PARF_INT_ALL_MASK);
> +}
> +
>  /*
>   * Qcom PCIe controllers only support one Root Port per controller instance. So
>   * this function ignores the 'pci_dev' associated with the Root Port and just
> @@ -1928,8 +1950,7 @@ static int qcom_pcie_reset_root_port(struct pci_host_bridge *bridge,
>  	 * non-sticky.
>  	 */
>  	if (pcie->global_irq)
> -		writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
> -				pcie->parf + PARF_INT_ALL_MASK);
> +		qcom_pcie_enable_global_irq_events(pcie);
>  
>  	qcom_pcie_start_link(pci);
>  

[Severity: High]
Does qcom_pcie_resume_noirq() also need to call
qcom_pcie_enable_global_irq_events() when pcie->global_irq is set?

When resuming from system suspend where pcie->pci->suspended is true,
qcom_pcie_resume_noirq() -> dw_pcie_resume_noirq() ->
qcom_pcie_host_init() -> qcom_pcie_init_2_7_0() pulses the controller core
reset (res->rst), which resets the non-sticky PARF_INT_ALL_3_MASK and
PARF_INT_ALL_MASK registers back to their hardware defaults before
restarting the link.  On Nord, an endpoint that sends a DRS message on
link up will then latch DRS_MSG_RECEIVED in PARF_INT_ALL_3_STATUS and wedge
the system in qcom_pcie_global_irq_thread() once interrupts are re-enabled
on resume, and PARF_INT_ALL_MASK remains cleared to 0.

> @@ -2429,6 +2450,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  			goto err_host_deinit;
>  		}
>  
> +		qcom_pcie_enable_global_irq_events(pcie);
> +
>  		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>  						qcom_pcie_global_irq_thread,
>  						IRQF_ONESHOT, name, pcie);
> @@ -2438,9 +2461,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  			goto err_host_deinit;
>  		}
>  
> -		writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
> -				pcie->parf + PARF_INT_ALL_MASK);
> -
>  		pcie->global_irq = irq;
>  	}

[Severity: Medium]
Can qcom_pcie_reset_root_port() run before pcie->global_irq is assigned if
a link-down interrupt is already pending or fires as soon as
devm_request_threaded_irq() enables the IRQ?

CPU0                                  CPU1
qcom_pcie_probe()
  qcom_pcie_enable_global_irq_events()
  devm_request_threaded_irq()
                                      qcom_pcie_global_irq_thread()
                                        pci_host_handle_link_down()
                                          ...
                                            qcom_pcie_reset_root_port()
                                              qcom_pcie_host_init()
                                              if (pcie->global_irq) <-- 0, skipped
    return 0;
  pcie->global_irq = irq;

Because qcom_pcie_enable_global_irq_events() now unmasks
PARF_INT_ALL_LINK_DOWN before devm_request_threaded_irq(), the threaded
handler can run and reset the controller while pcie->global_irq is still
0.  qcom_pcie_reset_root_port() then skips
qcom_pcie_enable_global_irq_events(), leaving PARF_INT_ALL_MASK cleared
and PARF_INT_ALL_3_MASK unmasked after the reset.  Setting
pcie->global_irq = irq before devm_request_threaded_irq() avoids this race.

[Severity: Medium]
This problem wasn't introduced by this patch, but the err_pm_runtime_put
error path in qcom_pcie_probe() [1] calls asynchronous
pm_runtime_put(dev) immediately followed by pm_runtime_disable(dev):

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_probe() {
    ...
err_pm_runtime_put:
	pm_runtime_put(dev);
	pm_runtime_disable(dev);
}

Because pm_runtime_put() only queues an asynchronous suspend request and
pm_runtime_disable() cancels pending runtime PM work, the device is never
suspended when probe fails after pm_runtime_get_sync(), leaving it powered
on.

[1] https://sashiko.dev/#/bug/linux-26465cc9-b0b2-43e4-ac7d-ce66bfbb2db5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114084700.907031-1-shengchao.guo@oss.qualcomm.com?part=1