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

[Ajay Agarwal <ajayagarwal@google.com>]

On Thu, Jan 11, 2024 at 09:38:03AM -0800, William McVicker wrote:
> Hi Ajay,
> 
> Thanks for sending the patch!
> 
> On 01/11/2024, Ajay Agarwal wrote:
> > There can be platforms that do not use/have 32-bit DMA addresses
> > but want to enumerate endpoints which support only 32-bit MSI
> > address. The current implementation of 32-bit IOVA allocation can
> > fail for such platforms, eventually leading to the probe failure.
> > 
> > If there is a memory region reserved for the pci->dev, pick up
> > the MSI data from this region. This can be used by the platforms
> > described above.
> > 
> > Else, if the memory region is not reserved, try to allocate a
> > 32-bit IOVA. Additionally, if this allocation also fails, attempt
> > a 64-bit allocation for probe to be successful. If the 64-bit MSI
> > address is allocated, then the EPs supporting 32-bit MSI address
> > will not work.
> > 
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> > 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 | 50 ++++++++++++++-----
> >  drivers/pci/controller/dwc/pcie-designware.h  |  1 +
> >  2 files changed, 39 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 7991f0e179b2..8c7c77b49ca8 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -331,6 +331,8 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> >  	u64 *msi_vaddr;
> >  	int ret;
> >  	u32 ctrl, num_ctrls;
> > +	struct device_node *np;
> > +	struct resource r;
> >  
> >  	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
> >  		pp->irq_mask[ctrl] = ~0;
> > @@ -374,20 +376,44 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> >  	 * order not to miss MSI TLPs from those devices the MSI target
> >  	 * address has to be within the lowest 4GB.
> >  	 *
> > -	 * Note until there is a better alternative found the reservation is
> > -	 * done by allocating from the artificially limited DMA-coherent
> > -	 * memory.
> > +	 * Check if there is memory region reserved for this device. If yes,
> > +	 * pick up the msi_data from this region. This will be helpful for
> > +	 * platforms that do not use/have 32-bit DMA addresses but want to use
> > +	 * endpoints which support only 32-bit MSI address.
> > +	 * Else, if the memory region is not reserved, try to allocate a 32-bit
> > +	 * IOVA. Additionally, if this allocation also fails, attempt a 64-bit
> > +	 * allocation. If the 64-bit MSI address is allocated, then the EPs
> > +	 * supporting 32-bit MSI address will not work.
> >  	 */
> > -	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");
> > +	np = of_parse_phandle(dev->of_node, "memory-region", 0);
> > +	if (np) {
> > +		ret = of_address_to_resource(np, 0, &r);
> > +		if (ret) {
> > +			dev_err(dev, "No memory address assigned to the region\n");
> > +			return ret;
> > +		}
> >  
> > -	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;
> > +		pp->msi_data = r.start;
> > +	} else {
> > +		dev_dbg(dev, "No %s specified\n", "memory-region");
> > +		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");
> > +
> > +		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > +						GFP_KERNEL);
> > +		if (!msi_vaddr) {
> > +			dev_warn(dev, "Failed to alloc 32-bit MSI data. Attempting 64-bit now\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 alloc and map MSI data\n");
> > +			dw_pcie_free_msi(pp);
> > +			return -ENOMEM;
> > +		}
> 
> Should we just put this second if-check inside the above fallback?
>
Yeah, we can do that. Will fix it in the next version after waiting for
comments from others.
> >  	}
> >  
> >  	return 0;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 55ff76e3d384..c85cf4d56e98 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -317,6 +317,7 @@ struct dw_pcie_rp {
> >  	phys_addr_t		io_bus_addr;
> >  	u32			io_size;
> >  	int			irq;
> > +	u8			coherent_dma_bits;
> >  	const struct dw_pcie_host_ops *ops;
> >  	int			msi_irq[MAX_MSI_CTRLS];
> >  	struct irq_domain	*irq_domain;
> 
> Looks like this is a lingering change? Please drop.
> 
Sorry about that. Will remove in the next version.
> Thanks,
> Will
> 
> > -- 
> > 2.43.0.275.g3460e3d667-goog
> >