Re: [PATCH v9 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 d4b2b9fb18b8b61f597edf1b19b5aa67a2433fc6
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 a dw_pcie_mask_msi() helper to disable the controller's
chained MSI parent interrupts 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 disable_irq() actually mask pp->msi_irq[ctrl] when it is registered as
a chained handler via irq_set_chained_handler_and_data()?

On interrupt controllers that do not implement .irq_disable (such as ARM
GICv2 and GICv3), disable_irq() performs a lazy disable unless
_IRQ_DISABLE_UNLAZY is set: __irq_disable() only sets IRQD_IRQ_DISABLED and
leaves the hardware line unmasked, relying on standard flow handlers to
check irqd_irq_disabled() and mask the interrupt on the next delivery.

Because dw_pcie_msi_host_init() installs dw_chained_msi_isr() as a chained
flow handler, generic_handle_irq_desc() invokes dw_chained_msi_isr()
directly without checking irqd_irq_disabled() or setting
IRQD_IRQ_INPROGRESS.  As a result, disable_irq() neither masks the hardware
interrupt nor synchronizes with an in-flight handler, and any MSI arriving
during or after controller clock/power teardown in the shutdown path will
still run dw_handle_msi_irq() and read unclocked DBI registers.

Calling irq_set_chained_handler_and_data(pp->msi_irq[ctrl], NULL, NULL), as
dw_pcie_free_msi() does before removing the domain, detaches the handler
and masks the interrupt line under desc->lock while leaving
pp->msi_irq_domain intact.

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