Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic

[Robin Murphy <robin.murphy@arm.com>]

On 11/03/2024 4:58 pm, Bjorn Helgaas wrote:
> On Wed, Feb 21, 2024 at 09:08:40PM +0530, Ajay Agarwal wrote:
>> There can be platforms that do not use/have 32-bit DMA addresses.
>> The current implementation of 32-bit IOVA allocation can fail for
>> such platforms, eventually leading to the probe failure.
>>
>> Try to allocate a 32-bit msi_data. If this allocation fails,
>> attempt a 64-bit address allocation. Please note that if the
>> 64-bit MSI address is allocated, then the EPs supporting 32-bit
>> MSI address only will not work.
> 
> What happens when we fail to allocate a 32-bit address, we allocate a
> 64-bit address, we have an endpoint that only supports 32-bit
> addresses, and the driver tries to enable MSI?  Does it fall back to
> INTx?  Fail the MSI enable?  Emit a warning?

In general __pci_write_msi_msg() will write the address, which will then 
be truncated by the device, and when it subsequently tries to signal the 
MSI it will go off into the weeds and not raise an interrupt. We'll have 
already warned about failing to get a 32-bit MSI address when initially 
probing the controller - although apparently now no longer explicitly 
calling out the implication for 32-bit devices :( - which seems to be 
about as much as we can reasonably do.

I suppose pci_write_msg_msi() could additionally scream if it sees 
address_hi being nonzero when the given capability is only 32-bit, but 
it still can't fail. Within the current design, I'm not sure we can 
reasonably tell that a device is incompatible with the MSI domain until 
irq_chip_compose_msi_msg(), by which point it's already way too late...

Thanks,
Robin.

> 
>> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
>> ---
>> Changelog since v5:
>>   - Initialize temp variable 'msi_vaddr' to NULL
>>   - Remove redundant print and check
>>
>> Changelog since v4:
>>   - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
>>   - Refactor the comments and msi_data allocation logic
>>
>> Changelog since v3:
>>   - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
>>   - Refactor the comments and print statements
>>
>> Changelog since v2:
>>   - If the vendor driver has setup the msi_data, use the same
>>
>> Changelog since v1:
>>   - Use reserved memory, if it exists, to setup the MSI data
>>   - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>>
>>   .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index d5fc31f8345f..d15a5c2d5b48 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>   	struct device *dev = pci->dev;
>>   	struct platform_device *pdev = to_platform_device(dev);
>> -	u64 *msi_vaddr;
>> +	u64 *msi_vaddr = NULL;
>>   	int ret;
>>   	u32 ctrl, num_ctrls;
>>   
>> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   	 * memory.
>>   	 */
>>   	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>> -	if (ret)
>> -		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
>> +	if (!ret)
>> +		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> +						GFP_KERNEL);
>>   
>> -	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> -					GFP_KERNEL);
>>   	if (!msi_vaddr) {
>> -		dev_err(dev, "Failed to alloc and map MSI data\n");
>> -		dw_pcie_free_msi(pp);
>> -		return -ENOMEM;
>> +		dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
>> +		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>> +		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> +						GFP_KERNEL);
>> +		if (!msi_vaddr) {
>> +			dev_err(dev, "Failed to allocate MSI address\n");
>> +			dw_pcie_free_msi(pp);
>> +			return -ENOMEM;
>> +		}
>>   	}
>>   
>>   	return 0;
>> -- 
>> 2.44.0.rc0.258.g7320e95886-goog
>>