Re: [PATCH v9 4/4] PCI: qcom: Implement shutdown() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] PCI: qcom: chained MSI IRQs left unmasked by
  dw_pcie_mask_msi() in shutdown
--

commit 4909e5bc39d1622646337d65d38a92d3c39b97f5
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: qcom: Implement shutdown() callback

This commit implements the platform driver shutdown() callback for the
Qualcomm PCIe host controller to mask interrupts, transition the link to
L2, and disable runtime PM during system shutdown.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5327792957340..0f548c6ce6a88 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -2467,6 +2478,24 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> +	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->global_irq)
> +		disable_irq(pcie->global_irq);
> +
> +	if (pci->pp.use_imsi_rx)
> +		dw_pcie_mask_msi(&pci->pp);
> +
> +	if (!pcie->cfg->firmware_managed && !pci->suspended)
> +		dw_pcie_suspend_noirq(pci, true);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}

[Severity: Medium]
Does dw_pcie_mask_msi() actually mask or detach the chained MSI handler
before DBI clocks are turned off?

dw_pcie_msi_host_init() registers each pp->msi_irq[ctrl] as a chained
handler via irq_set_chained_handler_and_data() with dw_chained_msi_isr(),
and dw_pcie_mask_msi() only calls disable_irq() on those interrupts.  On
interrupt controllers whose irq_chip does not implement .irq_disable (such
as GICv3 on Qualcomm SoCs) and when unlazy disable is not set,
__irq_disable() only marks the descriptor disabled in software without
calling mask_irq().

Because generic_handle_irq_desc() invokes desc->handle_irq directly for
chained interrupts, dw_chained_msi_isr() does not check
irqd_irq_disabled().  If a late or spurious MSI arrives after
dw_pcie_suspend_noirq() or pm_runtime_put_sync() cuts DBI clocks and
power, dw_chained_msi_isr() will still call dw_handle_msi_irq() and read
unclocked DBI registers, causing an SError or NoC fault.

Clearing the chained handler via irq_set_chained_handler_and_data() (as
dw_pcie_free_msi() does) would shut down and mask the parent IRQ line at
the interrupt controller while leaving the MSI domain and mappings intact.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-shutdown-v9-0-5430c9f242ed@oss.qualcomm.com?part=4