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

[Shawn Lin]

在 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.

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