Re: [PATCH v8 3/4] PCI: dwc: Add dw_pcie_mask_msi() to mask chained MSI without freeing domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] PCI: dwc: dw_pcie_mask_msi() fails to mask chained IRQs under
  lazy disable
--

commit ab8d07addca7ae6a44fdcf9867d820b3c416593e
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI: dwc: Add dw_pcie_mask_msi() to mask chained MSI without freeing domain

This commit adds dw_pcie_mask_msi() to disable the DesignWare PCIe
controller's chained MSI parent interrupt lines during shutdown without
removing the MSI IRQ domain.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 0837bb9e67e02..7f0126c5336bc 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -240,6 +240,28 @@ void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_free_msi);
>  
> +/**
> + * dw_pcie_mask_msi - Mask the chained MSI parent interrupt(s)
> + * @pp: DWC PCIe RP private data
> + *
> + * Disable the controller's chained MSI interrupt line(s) to stop further
> + * MSI delivery, while leaving the MSI IRQ domain and the MSI mappings held
> + * by downstream devices intact. Unlike dw_pcie_free_msi(), this does not
> + * remove the IRQ domain, so it is safe to call from the shutdown()/reboot
> + * path where PCI devices are not removed and their MSI descriptors must not
> + * be orphaned.
> + */
> +void dw_pcie_mask_msi(struct dw_pcie_rp *pp)
> +{
> +	u32 ctrl;
> +
> +	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) {
> +		if (pp->msi_irq[ctrl] > 0)
> +			disable_irq(pp->msi_irq[ctrl]);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_mask_msi);

[Severity: High]
Does calling disable_irq() on pp->msi_irq[ctrl] actually mask or stop a
chained interrupt on controllers with lazy disable (such as ARM GICv2/v3)?

dw_pcie_msi_host_init() registers pp->msi_irq[ctrl] via
irq_set_chained_handler_and_data() without setting IRQ_DISABLE_UNLAZY.
When the parent irq_chip does not implement .irq_disable, disable_irq()
only sets IRQD_IRQ_DISABLED and leaves the hardware interrupt unmasked.
Standard flow handlers mask the line on the next interrupt when checking
irqd_irq_disabled(), and set IRQD_IRQ_INPROGRESS so synchronize_irq() can
wait for them, but chained handlers replace desc->handle_irq with
dw_chained_msi_isr(), which does neither.

If an MSI arrives during shutdown after controller clocks and power are
cut, dw_chained_msi_isr() -> dw_handle_msi_irq() will still run and read
unclocked DBI registers, causing an external abort or bus hang.

Detaching the chained handler with
irq_set_chained_handler_and_data(pp->msi_irq[ctrl], NULL, NULL), as
dw_pcie_free_msi() does before irq_domain_remove(), masks the hardware
interrupt via mask_ack_irq() and resets desc->handle_irq while leaving
pp->msi_irq_domain intact.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-shutdown-v8-0-bf607c6429b0@oss.qualcomm.com?part=3