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

[Shawn Lin <shawn.lin@rock-chips.com>]

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