Re: [PATCH v4 10/18] PCI: dwc: Free the MSI domain after the host .deinit() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: dwc: missing synchronize_irq() after removing chained MSI handlers
--

commit 5f29144f527d9dfd0c8c7686f0d2791d38c14098
Author: Koichiro Den <den@valinux.co.jp>

PCI: dwc: Free the MSI domain after the host .deinit() callback

dw_pcie_host_deinit() frees the iMSI-RX domain before calling the host
.deinit() callback. For a core-owned parent interrupt,
dw_pcie_free_msi() removes the chained handler right before the domain.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 63b38532d069a..774e1936d9991 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -228,7 +228,7 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_allocate_domains);
>  
> -void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> +static void dw_pcie_remove_msi_handlers(struct dw_pcie_rp *pp)
>  {
>  	u32 ctrl;
>  
> @@ -236,8 +236,18 @@ void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>  		if (pp->msi_irq[ctrl] > 0)
>  			irq_set_chained_handler_and_data(pp->msi_irq[ctrl], NULL, NULL);
>  	}
> +}

[Severity: High]
Does this code need a call to synchronize_irq() here?

irq_set_chained_handler_and_data() uninstalls the handler but does not wait
for concurrent executions to finish. This means an interrupt handler like
dw_chained_msi_isr() could still be executing on another CPU.

[ ... ]

> @@ -718,11 +730,14 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
>  	dw_pcie_edma_remove(pci);
>  
>  	if (pp->use_imsi_rx)
> -		dw_pcie_free_msi(pp);
> +		dw_pcie_remove_msi_handlers(pp);
>  
>  	if (pp->ops->deinit)
>  		pp->ops->deinit(pp);

[Severity: High]
Could the lack of interrupt synchronization cause a regression here?

If the unsynchronized interrupt handler is still in flight, pp->ops->deinit(pp)
might power down the controller out from under it.

If the in-flight handler proceeds to call dw_handle_msi_irq(), it will read
PCIE_MSI_INTR0_STATUS from the powered-down hardware, potentially resulting in
an asynchronous SError (bus crash) or use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015033103.3076869-1-den@valinux.co.jp?part=10