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

[Ajay Agarwal <ajayagarwal@google.com>]

On Wed, Feb 21, 2024 at 03:52:43PM +0300, Serge Semin wrote:
> On Wed, Feb 21, 2024 at 12:18:46PM +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.
> > 
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> > 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, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index d5fc31f8345f..9c905e5c4904 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -379,15 +379,22 @@ 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 will be left uninitialized if the mask setting fails. Robin
> had it initialized in v2 discussion:
> https://lore.kernel.org/linux-pci/7cd42851-37cc-49d6-b9ad-74a256a73904@arm.com/
> 
ACK. Will initialize to NULL in the next version.

> 
> >  
> > -	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 configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\n");
> 
> Can we short it up already? I suggested something like "Failed to
> allocate 32-bit MSI target address" in v2. It means the problem with
> 32-bit MSIs which can be perceived as the possible reason of the
> respective peripheral devices not working correctly.
>
ACK. Will shorten the message in the next version.

> > +		ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> > +		if (!ret)
> 
> Redundant check. It was discussed some time ago. See the comment from
> Robin:
> https://lore.kernel.org/linux-pci/335d730d-529e-7ce0-8bac-008a4daa6e3c@arm.com/
> 
ACK. Will remove the check in the next version.

> -Serge(y)
> 
> > +			msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > +							GFP_KERNEL);
> > +
> > +		if (!msi_vaddr) {
> > +			dev_err(dev, "Failed to configure MSI address\n");
> > +			dw_pcie_free_msi(pp);
> > +			return -ENOMEM;
> > +		}
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.44.0.rc0.258.g7320e95886-goog
> >