Re: [PATCH v4 2/2] PCI: qcom: Implement shutdown() callback

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The newly added platform_set_drvdata() in qcom_pcie_ecam_host_init() overwrites drvdata with a struct dw_pcie* for firmware-managed platforms, causing catastrophic type confusion and invalid memory dereferences in PM callbacks that expect a struct qcom_pcie*.
- [High] Using disable_irq() on chained MSI interrupts in qcom_pcie_shutdown() fails to synchronize with currently executing handlers, leaving a race condition that can crash the system.

Pre-existing issues:
- [High] Unconditional dereference of optional elbi_base in qcom_pcie_host_pme_turn_off() triggers a crash on platforms where ELBI is not present.
--

commit cb528be41a06ef2211c8efca75a786c37e95604e
Author: Manivannan Sadhasivam <mani@kernel.org>
Date:   Wed Aug 26 07:16:31 2026 +0000

    PCI: qcom: Implement shutdown() callback

This commit implements the shutdown() callback for the Qualcomm PCIe host
controller driver. It tears down the link by reusing the suspend_noirq logic,
forcing the link into L2. It also masks chained MSI IRQs to prevent
late or spurious interrupts after hardware clocks and power are removed.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b58a607b713f5..fad2f4e2d6be5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1924,6 +1924,9 @@ static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>  	pp->use_imsi_rx = true;
>  	dw_pcie_msi_init(pp);
>  
> +	/* Stash pci so qcom_pcie_shutdown() can mask the MSI IRQ(s) later */
> +	platform_set_drvdata(to_platform_device(dev), pci);

[Severity: Critical]
Will this overwrite the platform driver data with a divergent structure type?

PM callbacks like qcom_pcie_suspend_noirq() unconditionally cast the fetched
drvdata to a struct qcom_pcie pointer. Since a struct dw_pcie pointer is stashed
here instead for firmware-managed platforms, won't accessing pcie->pci in the
suspend path resolve to the first field of struct dw_pcie (which is a struct
device pointer)? 

Could this cause an invalid pointer dereference when that corrupted pointer
is passed into dw_pcie_suspend_noirq()?

>  
>  	return devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>  }
>  
[ ... ]
> +static void qcom_pcie_mask_msi_irqs(struct dw_pcie_rp *pp)
> +{
> +	u32 ctrl;
> +
> +	/*
> +	 * Mask the chained MSI IRQ(s) before tearing down the link and
> +	 * clocks/PHY. Unlike suspend_noirq(), device_shutdown() runs with
> +	 * interrupts enabled, so a late/spurious MSI could otherwise hit
> +	 * dw_chained_msi_isr() and touch DBI registers after the controller
> +	 * is powered off.
> +	 */
> +	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) {
> +		if (pp->msi_irq[ctrl] > 0)
> +			disable_irq(pp->msi_irq[ctrl]);

[Severity: High]
Does disable_irq() successfully block for chained handlers here?

Chained IRQ handlers (like dw_chained_msi_isr) act as flow handlers and do not
have the IRQD_IRQ_INPROGRESS flag set on the parent IRQ descriptor. Because of
this, won't the synchronize_irq() call hidden inside disable_irq() return
immediately without waiting for the chained handler to finish?

If an MSI interrupt fires concurrently on another CPU exactly as the system
initiates shutdown, couldn't this leave a race condition where the shutdown
sequence cuts the DBI clocks and power while the chained handler is still
actively reading registers?

> +	}
> +}
> +
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
[ ... ]
> +	pcie = platform_get_drvdata(pdev);
> +	if (pcie) {
> +		qcom_pcie_mask_msi_irqs(&pcie->pci->pp);
> +
> +		if (pcie->global_irq)
> +			disable_irq(pcie->global_irq);
> +
> +		dw_pcie_suspend_noirq(pcie->pci);

[Severity: High]
This is a pre-existing issue, but does calling the core suspend logic here
unconditionally trigger a crash on platforms without an ELBI region?

In qcom_pcie_host_pme_turn_off(), which is called during the suspend sequence,
there is an unconditional write to elbi_base:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_host_pme_turn_off() {
    ...
    writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, pci->elbi_base + ELBI_SYS_CTRL);
    ...
}

Because the new shutdown path unconditionally executes this suspend callback
on all platform resets, won't this guarantee a NULL pointer dereference on
platforms where ELBI is not present (such as Qcom IP rev 2.1.0 APQ8064/IPQ8064)
when they attempt to write to offset 0x04 of a NULL pointer?

> +	}
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-shutdown-v4-0-eb5fe9d454ae@oss.qualcomm.com?part=2