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

[Koichiro Den]

On Tue, Oct 20, 2026 at 03:00:36PM +0800, Shawn Lin wrote:
> 在 2026/10/20 星期二 14:41, 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?
> > 
> 
> My apologies for any confusion my earlier response may have caused. I
> brought up qcom merely as an example of a driver managing the MSI IRQ
> lifecycle with its own devres action, with a remove path that can go
> through dw_pcie_host_deinit(). For such combinations the safe check I
> suggested would matter, I intended this as a defensive improvement, not
> necessarily implying a bug exists now.

Oh I see. I personally agree, and it's quite a cheap check, but I'm not sure
what the maintainers of this file think about defensive programming here.

+Cc: Mani

Mani, I'd appreciate your thoughts on this.

Thanks for the review!

Best regards,
Koichiro Den

> 
> > 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);
> > > >    }
> > > 
> > 
>