Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The local variable `pcie` is used uninitialized on the `firmware_managed` code path, leading to undefined behavior and a likely kernel panic when evaluated and dereferenced.
- [High] Calling `dw_pcie_free_msi()` during `.shutdown` destroys the active MSI IRQ domain while child endpoints still have IRQs mapped, triggering a spurious kernel warning and leaving dangling state.
--

commit 27c88a203aab0fef576a235de5067454639a8757
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: qcom: Implement shutdown() callback

This commit implements a shutdown callback for PCIe host controllers to
bring the link down cleanly before system shutdown or reboot proceeds.
It reuses dw_pcie_suspend_noirq() in the shutdown path to force the link
into L2 and masks chained MSI IRQs to prevent late spurious interrupts.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2693a98d0b279..3133e2b2eda69 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -2408,6 +2411,37 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> +	const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
> +	struct qcom_pcie *pcie;
> +	struct dw_pcie *pci;
> +
> +	if (pcie_cfg && pcie_cfg->firmware_managed) {
> +		/*
> +		 * Firmware owns the link teardown and clock/PHY shutdown in
> +		 * this mode; Linux only owns the chained MSI IRQ(s), which
> +		 * still need to be masked off before shutdown proceeds.
> +		 */
> +		pci = platform_get_drvdata(pdev);
> +	} else {
> +		pcie = platform_get_drvdata(pdev);
> +		pci = pcie->pci;
> +
> +		if (pcie->global_irq)
> +			disable_irq(pcie->global_irq);
> +	}
> +
> +	if (pci->pp.use_imsi_rx)
> +		dw_pcie_free_msi(&pci->pp);

[Severity: High]
Does calling dw_pcie_free_msi() here trigger a core IRQ subsystem warning?

During system shutdown, child endpoints have their .shutdown callbacks
executed rather than .remove, so they do not naturally release their
allocated MSI vectors.

dw_pcie_free_msi() unconditionally destroys the IRQ domain:

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_free_msi() {
    ...
    irq_domain_remove(pp->irq_domain);
}

Will destroying this domain while child endpoints still hold IRQ mappings
leave dangling IRQ descriptors and trigger a warning when checking if the
domain tree is empty?

> +
> +	if (pcie)
> +		dw_pcie_suspend_noirq(pcie->pci, true);

[Severity: Critical]
Is the pcie variable left uninitialized in the firmware_managed path?

When pcie_cfg->firmware_managed is true, pci is initialized but pcie is
not. It looks like this branch will then evaluate stack garbage for the
condition check.

If it evaluates to true, could this cause a kernel panic by dereferencing
stack garbage when accessing pcie->pci in dw_pcie_suspend_noirq()?

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-shutdown-v6-0-62b7bd123236@oss.qualcomm.com?part=3