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

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

[Ajay Agarwal]

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 the vendor driver has already setup the MSI address using
some mechanism, use the same. This method can be used by the
platforms described above to support EPs they wish to. Such
drivers should set the DW_PCIE_CAP_MSI_DATA_SET flag.

Else, 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 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  | 18 +++++++++++++++---
 drivers/pci/controller/dwc/pcie-designware.h   |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d5fc31f8345f..06ee2e5deebc 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -373,11 +373,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	 * peripheral PCIe devices may lack 64-bit message support. In
 	 * order not to miss MSI TLPs from those devices the MSI target
 	 * address has to be within the lowest 4GB.
+	 * Permit the platforms to override the MSI target address if they
+	 * have a free PCIe-bus memory specifically reserved for that. Such
+	 * platforms should set the 'DW_PCIE_CAP_MSI_DATA_SET' cap flag.
 	 *
 	 * Note until there is a better alternative found the reservation is
 	 * done by allocating from the artificially limited DMA-coherent
 	 * memory.
 	 */
+	if (dw_pcie_cap_is(pci, MSI_DATA_SET))
+		return 0;
+
 	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");
@@ -385,9 +391,15 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	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");
+		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 configure MSI address\n");
+			dw_pcie_free_msi(pp);
+			return -ENOMEM;
+		}
 	}
 
 	return 0;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 26dae4837462..feb19c5edd4a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -54,6 +54,7 @@
 #define DW_PCIE_CAP_EDMA_UNROLL		1
 #define DW_PCIE_CAP_IATU_UNROLL		2
 #define DW_PCIE_CAP_CDM_CHECK		3
+#define DW_PCIE_CAP_MSI_DATA_SET	4
 
 #define dw_pcie_cap_is(_pci, _cap) \
 	test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
-- 
2.43.0.687.g38aa6559b0-goog

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

[Manivannan Sadhasivam]

On Wed, Feb 14, 2024 at 11:04:15AM +0530, 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 the vendor driver has already setup the MSI address using
> some mechanism, use the same. This method can be used by the
> platforms described above to support EPs they wish to. Such
> drivers should set the DW_PCIE_CAP_MSI_DATA_SET flag.
> 
> Else, 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 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  | 18 +++++++++++++++---
>  drivers/pci/controller/dwc/pcie-designware.h   |  1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..06ee2e5deebc 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -373,11 +373,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  	 * peripheral PCIe devices may lack 64-bit message support. In
>  	 * order not to miss MSI TLPs from those devices the MSI target
>  	 * address has to be within the lowest 4GB.
> +	 * Permit the platforms to override the MSI target address if they
> +	 * have a free PCIe-bus memory specifically reserved for that. Such
> +	 * platforms should set the 'DW_PCIE_CAP_MSI_DATA_SET' cap flag.
>  	 *
>  	 * Note until there is a better alternative found the reservation is
>  	 * done by allocating from the artificially limited DMA-coherent
>  	 * memory.
>  	 */

Now the above comments are misplaced. Please move the comments related to
setting coherent mask just above the dma_set_coherent_mask() API and keep the
flag related comments here.

> +	if (dw_pcie_cap_is(pci, MSI_DATA_SET))

Who is setting this flag? You should not add code when there are no in-kernel
consumers.

> +		return 0;
> +
>  	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");
> @@ -385,9 +391,15 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  	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");

This is a duplicated error log.

> +		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));

Is there a guarantee that this will never fail?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Ajay Agarwal]

On Wed, Feb 14, 2024 at 12:38:42PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Feb 14, 2024 at 11:04:15AM +0530, 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 the vendor driver has already setup the MSI address using
> > some mechanism, use the same. This method can be used by the
> > platforms described above to support EPs they wish to. Such
> > drivers should set the DW_PCIE_CAP_MSI_DATA_SET flag.
> > 
> > Else, 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 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  | 18 +++++++++++++++---
> >  drivers/pci/controller/dwc/pcie-designware.h   |  1 +
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index d5fc31f8345f..06ee2e5deebc 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -373,11 +373,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> >  	 * peripheral PCIe devices may lack 64-bit message support. In
> >  	 * order not to miss MSI TLPs from those devices the MSI target
> >  	 * address has to be within the lowest 4GB.
> > +	 * Permit the platforms to override the MSI target address if they
> > +	 * have a free PCIe-bus memory specifically reserved for that. Such
> > +	 * platforms should set the 'DW_PCIE_CAP_MSI_DATA_SET' cap flag.
> >  	 *
> >  	 * Note until there is a better alternative found the reservation is
> >  	 * done by allocating from the artificially limited DMA-coherent
> >  	 * memory.
> >  	 */
> 
> Now the above comments are misplaced. Please move the comments related to
> setting coherent mask just above the dma_set_coherent_mask() API and keep the
> flag related comments here.
>
ACK. Will take care of this.

> > +	if (dw_pcie_cap_is(pci, MSI_DATA_SET))
> 
> Who is setting this flag? You should not add code when there are no in-kernel
> consumers.
>
ACK. I will remove this flag and only add the 64-bit address support via
this patch.

> > +		return 0;
> > +
> >  	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");
> > @@ -385,9 +391,15 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> >  	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");
> 
> This is a duplicated error log.
> 
ACK. Will remove.

> > +		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> 
> Is there a guarantee that this will never fail?
> 
> - Mani
> 
ACK. No guarantee that this will succeed. Will fix in the next version.
> -- 
> மணிவண்ணன் சதாசிவம்