diff mbox series

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

Message ID 20240221064846.3798047-1-ajayagarwal@google.com (mailing list archive)
State Superseded
Headers show
Series [v5] PCI: dwc: Strengthen the MSI address allocation logic | expand

Commit Message

Ajay Agarwal Feb. 21, 2024, 6:48 a.m. UTC 
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(-)

Comments

Serge Semin Feb. 21, 2024, 12:52 p.m. UTC | #1 
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/


>  
> -	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.

> +		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/

-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
>
Ajay Agarwal Feb. 21, 2024, 3:28 p.m. UTC | #2 
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
> >
diff mbox series

Patch

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 = 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");
+		ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+		if (!ret)
+			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;