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

[sashiko-bot@kernel.org]

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