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

[Koichiro Den]

On Tue, Oct 20, 2026 at 11:54:18AM +0800, Shawn Lin wrote:
> Hi Koichiro
> 
> 在 2026/10/20 星期二 11:10, Koichiro Den 写道:
> > 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.
> > A glue driver that owns the parent interrupt and demultiplexes the MSIs
> > itself through dw_handle_msi_irq() can only stop its handler in
> > .deinit(), after the domain is gone, so a late interrupt can still
> > dispatch into the freed domain.
> > 
> 
> I think you need a rebase anyway since there are two patches touching
> MSI irq naming stuff in pci-tree's next branch and will probably
> show up in tommorow's linux-next.

Hi Shawn, thanks for the heads-up! And thanks for the review.

> 
> ...
> 
> > +static void dw_pcie_free_msi_domain(struct dw_pcie_rp *pp)
> > +{
> >   	irq_domain_remove(pp->irq_domain);
> > +	pp->irq_domain = NULL;
> > +}
> > +
> > +void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> > +{
> > +	dw_pcie_remove_msi_handlers(pp);
> > +	dw_pcie_free_msi_domain(pp);
> >   }
> >   EXPORT_SYMBOL_GPL(dw_pcie_free_msi);
> 
> 
> dw_pcie_free_msi() now calls freeing irq_domain unconditionally:
> 
> The problem is that pcie-qcom.c uses the ECAM init path, which does not
> go through dw_pcie_host_init(), and registers its own devres action,
> see:
> drivers/pci/controller/dwc/pcie-qcom.c:2034
>      return devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
> 
> drivers/pci/controller/dwc/pcie-qcom.c:1995
>      static void qcom_pci_free_msi(void *ptr)
>      {
>             struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
> 
>             if (pp && pp->use_imsi_rx)
>                     dw_pcie_free_msi(pp);
>      }
> 
> This leads to the following sequence:
> 
>   1. dw_pcie_host_init() succeeds and pp->irq_domain is created;
>   2. the driver is removed or probe fails, dw_pcie_host_deinit() calls
>      dw_pcie_free_msi_domain() and sets pp->irq_domain to NULL;
>   3. the devres action qcom_pci_free_msi() still holds the same pp and
>      calls dw_pcie_free_msi(), which enters dw_pcie_free_msi_domain()
>      again;

I'm a bit confused. Steps 1 and 2 here seem to describe the regular DWC path,
but the devres action in step 3 looks like it's only registered in the
firmware-managed ECAM path. I don't see where that path would call
dw_pcie_host_deinit(). Am I missing something?

Best regards,
Koichiro Den

>   4. irq_domain_remove(NULL) dereferences a NULL pointer and the kernel
>      crashes.
> 
> 
> So I suggest you to add a safe check if (!pp->irq_domain) before doing
> that.
> 
> 
> > @@ -633,7 +643,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> >   	if (ret)
> > -		goto err_free_msi;
> > +		goto err_deinit_host;
> >   	/*
> >   	 * Allocate the resource for MSG TLP before programming the iATU
> > @@ -649,7 +659,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   	ret = dw_pcie_edma_detect(pci);
> >   	if (ret)
> > -		goto err_free_msi;
> > +		goto err_deinit_host;
> >   	ret = dw_pcie_setup_rc(pp);
> >   	if (ret)
> > @@ -686,14 +696,16 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   err_remove_edma:
> >   	dw_pcie_edma_remove(pci);
> > -err_free_msi:
> > -	if (pp->use_imsi_rx)
> > -		dw_pcie_free_msi(pp);
> > -
> >   err_deinit_host:
> > +	if (pp->use_imsi_rx && pp->irq_domain)
> > +		dw_pcie_remove_msi_handlers(pp);
> > +
> >   	if (pp->ops->deinit)
> >   		pp->ops->deinit(pp);
> > +	if (pp->use_imsi_rx && pp->irq_domain)
> > +		dw_pcie_free_msi_domain(pp);
> > +
> >   err_free_ecam:
> >   	if (pp->cfg)
> >   		pci_ecam_free(pp->cfg);
> > @@ -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);
> > +	if (pp->use_imsi_rx)
> > +		dw_pcie_free_msi_domain(pp);
> > +
> >   	if (pp->cfg)
> >   		pci_ecam_free(pp->cfg);
> >   }
>