Re: [PATCH v5 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] Setting platform drvdata to `struct dw_pcie *` for `firmware_managed` devices introduces a fatal type confusion in system suspend/resume callbacks.
- [High] Calling dw_pcie_free_msi() during shutdown removes chained handlers without synchronization and destroys the non-empty MSI IRQ domain, causing WARN_ON and potential system crashes.
--

commit 8fb4302517aeceeb9b7232585ce3188b6458e92d
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: qcom: Implement shutdown() callback

PCIe host controllers should bring the link down cleanly before system
shutdown/reboot proceeds to remove power/clocks from the controller.
This commit reuses dw_pcie_suspend_noirq() in the shutdown path and
masks chained MSI and global IRQs before teardown.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b58a607b713f5..38801830fb70f 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);
> +
>  	return devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>  }

[Severity: Critical]
Does setting the platform device's drvdata to a struct dw_pcie pointer here
introduce a fatal type confusion in the system suspend/resume callbacks?

The PM suspend callback qcom_pcie_suspend_noirq() unconditionally retrieves
drvdata expecting a struct qcom_pcie pointer:

qcom_pcie_suspend_noirq() {
	pcie = dev_get_drvdata(dev);
	if (!pcie)
		return 0;

	ret = dw_pcie_suspend_noirq(pcie->pci);
	...
}

For firmware_managed devices, accessing pcie->pci will evaluate to the
first member of struct dw_pcie (which is struct device *dev), passing a
corrupted memory pointer to dw_pcie_suspend_noirq() and causing a guaranteed
kernel panic on sleep.

[ ... ]

> +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;
> +
> +	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.
> +		 */
> +		struct dw_pcie *pci = platform_get_drvdata(pdev);
> +
> +		if (pci->pp.use_imsi_rx)
> +			dw_pcie_free_msi(&pci->pp);
> +		return;
> +	}
> +
> +	pcie = platform_get_drvdata(pdev);
> +	if (pcie) {
> +		if (pcie->pci->pp.use_imsi_rx)
> +			dw_pcie_free_msi(&pcie->pci->pp);

[Severity: High]
Is it safe to call dw_pcie_free_msi() during the shutdown path while child
endpoint mappings still exist and interrupts might be in flight?

In dw_pcie_free_msi(), the code unhooks the chained handler and destroys the
IRQ domain:

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_free_msi() {
	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) {
		if (pp->msi_irq[ctrl] > 0)
			irq_set_chained_handler_and_data(pp->msi_irq[ctrl], NULL, NULL);
	}

	irq_domain_remove(pp->irq_domain);
}

Removing the active IRQ domain triggers a WARN_ON because the domain is
not empty, as child PCI devices haven't freed their IRQs during shutdown.

Furthermore, removing the chained handler does not synchronize against
in-flight IRQs. If an interrupt executes on another CPU while the
controller's clocks and PHY are powered off later in this shutdown
sequence, it risks an SError (synchronous external abort) crashing the
system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-shutdown-v5-0-f0c6bafa1d4f@oss.qualcomm.com?part=3