[PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Ajay Agarwal]

If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
will fail to allocate the memory if there are no 32-bit addresses
available. This will lead to the PCIe RC probe failure.
Fix this by setting the DMA mask to 32 bits only if the kernel
configuration enables DMA32 zone. Else, leave the mask as is.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 7991f0e179b2..163a78c5f9d8 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -377,10 +377,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	 * Note until there is a better alternative found the reservation is
 	 * done by allocating from the artificially limited DMA-coherent
 	 * memory.
+	 *
+	 * Set the coherent DMA mask to 32 bits only if the DMA32 zone is
+	 * supported. Otherwise, leave the mask as is.
+	 * This ensures that the dmam_alloc_coherent is successful in
+	 * allocating the memory.
 	 */
+#ifdef CONFIG_ZONE_DMA32
 	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");
+#endif
 
 	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
 					GFP_KERNEL);
-- 
2.43.0.195.gebba966016-goog

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Ajay Agarwal]

Adding Robin and Serge.

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Ajay Agarwal]

Adding Robin and Serge.

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Ajay Agarwal]

Adding Robin and Serge.

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Serge Semin]

Hi Ajay

On Thu, Dec 21, 2023 at 11:10:51PM +0530, Ajay Agarwal wrote:
> If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
> will fail to allocate the memory if there are no 32-bit addresses
> available. This will lead to the PCIe RC probe failure.
> Fix this by setting the DMA mask to 32 bits only if the kernel
> configuration enables DMA32 zone. Else, leave the mask as is.
> 
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 7991f0e179b2..163a78c5f9d8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -377,10 +377,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  	 * Note until there is a better alternative found the reservation is
>  	 * done by allocating from the artificially limited DMA-coherent
>  	 * memory.
> +	 *
> +	 * Set the coherent DMA mask to 32 bits only if the DMA32 zone is
> +	 * supported. Otherwise, leave the mask as is.
> +	 * This ensures that the dmam_alloc_coherent is successful in
> +	 * allocating the memory.
>  	 */
> +#ifdef CONFIG_ZONE_DMA32
>  	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");
> +#endif

Without setting the mask the allocation below may cause having MSI
data from above 4GB which in its turn will cause MSI not working for
peripheral PCI-devices supporting 32-bit MSI only. That's the gist of
this questionable code above and below.

The discussion around it can be found here:
https://lore.kernel.org/linux-pci/20220825185026.3816331-2-willmcvicker@google.com
and a problem similar to what you described was reported here:
https://lore.kernel.org/linux-pci/decae9e4-3446-2384-4fc5-4982b747ac03@yadro.com/

The easiest solution in this case is to somehow pre-define
pp->msi_data with a PCI-bus address free from RAM behind and avoid
allocation below at all. The only question is how to do that. See the
discussions above for details.

-Serge(y)

>  
>  	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>  					GFP_KERNEL);
> -- 
> 2.43.0.195.gebba966016-goog
> 
> 

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Christoph Hellwig]

On Thu, Dec 21, 2023 at 11:10:51PM +0530, Ajay Agarwal wrote:
> If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
> will fail to allocate the memory if there are no 32-bit addresses
> available. This will lead to the PCIe RC probe failure.
> Fix this by setting the DMA mask to 32 bits only if the kernel
> configuration enables DMA32 zone. Else, leave the mask as is.

And that's why you never must disable ZONE_DMA32 on devices that
can have more than >32bit memory and don't use an iommu for
everything.

NAK.

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Robin Murphy]

On 2023-12-21 6:33 pm, Serge Semin wrote:
> Hi Ajay
> 
> On Thu, Dec 21, 2023 at 11:10:51PM +0530, Ajay Agarwal wrote:
>> If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
>> will fail to allocate the memory if there are no 32-bit addresses
>> available. This will lead to the PCIe RC probe failure.
>> Fix this by setting the DMA mask to 32 bits only if the kernel
>> configuration enables DMA32 zone. Else, leave the mask as is.
>>
>> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 7991f0e179b2..163a78c5f9d8 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -377,10 +377,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   	 * Note until there is a better alternative found the reservation is
>>   	 * done by allocating from the artificially limited DMA-coherent
>>   	 * memory.
>> +	 *
>> +	 * Set the coherent DMA mask to 32 bits only if the DMA32 zone is
>> +	 * supported. Otherwise, leave the mask as is.
>> +	 * This ensures that the dmam_alloc_coherent is successful in
>> +	 * allocating the memory.
>>   	 */
>> +#ifdef CONFIG_ZONE_DMA32
>>   	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");
>> +#endif
> 
> Without setting the mask the allocation below may cause having MSI
> data from above 4GB which in its turn will cause MSI not working for
> peripheral PCI-devices supporting 32-bit MSI only. That's the gist of
> this questionable code above and below.

Right, this change is wrong on several levels, as it would end up hiding 
the warning in cases where it would be most relevant.

> The discussion around it can be found here:
> https://lore.kernel.org/linux-pci/20220825185026.3816331-2-willmcvicker@google.com
> and a problem similar to what you described was reported here:
> https://lore.kernel.org/linux-pci/decae9e4-3446-2384-4fc5-4982b747ac03@yadro.com/
> 
> The easiest solution in this case is to somehow pre-define
> pp->msi_data with a PCI-bus address free from RAM behind and avoid
> allocation below at all. The only question is how to do that. See the
> discussions above for details.

FWIW there's also the potential question of whether failing to obtain a 
32-bit address needs to be entirely fatal to probe at all. Perhaps it 
might be reasonable to just continue without MSI support, or maybe retry 
with a larger mask to attempt limited 64-bit-only MSI support - IIRC the 
latter was proposed originally, but Will's initial use-case didn't 
actually need it so we concluded it was hard to justify the complexity 
at the time. The main thing is not to quietly go ahead and do something 
which we can't guarantee to fully work, without at least letting the 
user know.

Thanks,
Robin.

> 
> -Serge(y)
> 
>>   
>>   	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>>   					GFP_KERNEL);
>> -- 
>> 2.43.0.195.gebba966016-goog
>>
>>

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Serge Semin]

On Mon, Jan 08, 2024 at 05:50:26PM +0000, Robin Murphy wrote:
> On 2023-12-21 6:33 pm, Serge Semin wrote:
> > Hi Ajay
> > 
> > On Thu, Dec 21, 2023 at 11:10:51PM +0530, Ajay Agarwal wrote:
> > > If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
> > > will fail to allocate the memory if there are no 32-bit addresses
> > > available. This will lead to the PCIe RC probe failure.
> > > Fix this by setting the DMA mask to 32 bits only if the kernel
> > > configuration enables DMA32 zone. Else, leave the mask as is.
> > > 
> > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 7991f0e179b2..163a78c5f9d8 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -377,10 +377,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > >   	 * Note until there is a better alternative found the reservation is
> > >   	 * done by allocating from the artificially limited DMA-coherent
> > >   	 * memory.
> > > +	 *
> > > +	 * Set the coherent DMA mask to 32 bits only if the DMA32 zone is
> > > +	 * supported. Otherwise, leave the mask as is.
> > > +	 * This ensures that the dmam_alloc_coherent is successful in
> > > +	 * allocating the memory.
> > >   	 */
> > > +#ifdef CONFIG_ZONE_DMA32
> > >   	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");
> > > +#endif
> > 
> > Without setting the mask the allocation below may cause having MSI
> > data from above 4GB which in its turn will cause MSI not working for
> > peripheral PCI-devices supporting 32-bit MSI only. That's the gist of
> > this questionable code above and below.
> 
> Right, this change is wrong on several levels, as it would end up hiding the
> warning in cases where it would be most relevant.
> 
> > The discussion around it can be found here:
> > https://lore.kernel.org/linux-pci/20220825185026.3816331-2-willmcvicker@google.com
> > and a problem similar to what you described was reported here:
> > https://lore.kernel.org/linux-pci/decae9e4-3446-2384-4fc5-4982b747ac03@yadro.com/
> > 
> > The easiest solution in this case is to somehow pre-define
> > pp->msi_data with a PCI-bus address free from RAM behind and avoid
> > allocation below at all. The only question is how to do that. See the
> > discussions above for details.
> 

> FWIW there's also the potential question of whether failing to obtain a
> 32-bit address needs to be entirely fatal to probe at all. Perhaps it might
> be reasonable to just continue without MSI support, or maybe retry with a
> larger mask to attempt limited 64-bit-only MSI support - IIRC the latter was
> proposed originally, but Will's initial use-case didn't actually need it so
> we concluded it was hard to justify the complexity at the time. The main
> thing is not to quietly go ahead and do something which we can't guarantee
> to fully work, without at least letting the user know.

Hm, I guess you are right. It may be an overkill to halt the probe
procedure if 32-bit mask failed to be specified. Printing some big fat
warning looks better at least until it is possible to reserve a
PCIe-bus memory within lowest 4GB irrespective to the system RAM
availability and the device DMA capabilities.

Regarding the Will's patch. His solution wasn't entirely correct. It
implied to use the DW PCIe Root-Port MSI capability as a "storage" of
the MSI 64-bit flag. It was wrong from two perspective. First DW PCIe
iMSI-RX engine always supports 64-bit MSI addresses, so having the MSI
64-bit flag cleared would be at least misleading. Second it was much
easier and more correct to just define a flag with the same semantics
in the driver private data.

In anyway trying 32-bit mask and then falling back to the 64-bit one
sound reasonable indeed.

-Serge(y)

> 
> Thanks,
> Robin.
> 
> > 
> > -Serge(y)
> > 
> > >   	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > >   					GFP_KERNEL);
> > > -- 
> > > 2.43.0.195.gebba966016-goog
> > > 
> > > 

Re: [PATCH] PCI: dwc: Set DMA mask to 32 only if ZONE_DMA32 is enabled

[Ajay Agarwal]

On Wed, Jan 10, 2024 at 01:47:39AM +0300, Serge Semin wrote:
> On Mon, Jan 08, 2024 at 05:50:26PM +0000, Robin Murphy wrote:
> > On 2023-12-21 6:33 pm, Serge Semin wrote:
> > > Hi Ajay
> > > 
> > > On Thu, Dec 21, 2023 at 11:10:51PM +0530, Ajay Agarwal wrote:
> > > > If CONFIG_ZONE_DMA32 is disabled, then the dmam_alloc_coherent
> > > > will fail to allocate the memory if there are no 32-bit addresses
> > > > available. This will lead to the PCIe RC probe failure.
> > > > Fix this by setting the DMA mask to 32 bits only if the kernel
> > > > configuration enables DMA32 zone. Else, leave the mask as is.
> > > > 
> > > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++++
> > > >   1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index 7991f0e179b2..163a78c5f9d8 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -377,10 +377,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > > >   	 * Note until there is a better alternative found the reservation is
> > > >   	 * done by allocating from the artificially limited DMA-coherent
> > > >   	 * memory.
> > > > +	 *
> > > > +	 * Set the coherent DMA mask to 32 bits only if the DMA32 zone is
> > > > +	 * supported. Otherwise, leave the mask as is.
> > > > +	 * This ensures that the dmam_alloc_coherent is successful in
> > > > +	 * allocating the memory.
> > > >   	 */
> > > > +#ifdef CONFIG_ZONE_DMA32
> > > >   	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");
> > > > +#endif
> > > 
> > > Without setting the mask the allocation below may cause having MSI
> > > data from above 4GB which in its turn will cause MSI not working for
> > > peripheral PCI-devices supporting 32-bit MSI only. That's the gist of
> > > this questionable code above and below.
> > 
> > Right, this change is wrong on several levels, as it would end up hiding the
> > warning in cases where it would be most relevant.
> > 
> > > The discussion around it can be found here:
> > > https://lore.kernel.org/linux-pci/20220825185026.3816331-2-willmcvicker@google.com
> > > and a problem similar to what you described was reported here:
> > > https://lore.kernel.org/linux-pci/decae9e4-3446-2384-4fc5-4982b747ac03@yadro.com/
> > > 
> > > The easiest solution in this case is to somehow pre-define
> > > pp->msi_data with a PCI-bus address free from RAM behind and avoid
> > > allocation below at all. The only question is how to do that. See the
> > > discussions above for details.
> > 
> 
> > FWIW there's also the potential question of whether failing to obtain a
> > 32-bit address needs to be entirely fatal to probe at all. Perhaps it might
> > be reasonable to just continue without MSI support, or maybe retry with a
> > larger mask to attempt limited 64-bit-only MSI support - IIRC the latter was
> > proposed originally, but Will's initial use-case didn't actually need it so
> > we concluded it was hard to justify the complexity at the time. The main
> > thing is not to quietly go ahead and do something which we can't guarantee
> > to fully work, without at least letting the user know.
> 
> Hm, I guess you are right. It may be an overkill to halt the probe
> procedure if 32-bit mask failed to be specified. Printing some big fat
> warning looks better at least until it is possible to reserve a
> PCIe-bus memory within lowest 4GB irrespective to the system RAM
> availability and the device DMA capabilities.
>
Thanks Robin and Serge for your inputs. In the next version of the
patch, I will try to use the reserved memory, if it exists, to set up
the MSI data. If the reserved memory does not exist, the patch will try
to use the standard DMA API to allocate the msi_data.

> Regarding the Will's patch. His solution wasn't entirely correct. It
> implied to use the DW PCIe Root-Port MSI capability as a "storage" of
> the MSI 64-bit flag. It was wrong from two perspective. First DW PCIe
> iMSI-RX engine always supports 64-bit MSI addresses, so having the MSI
> 64-bit flag cleared would be at least misleading. Second it was much
> easier and more correct to just define a flag with the same semantics
> in the driver private data.
> 
> In anyway trying 32-bit mask and then falling back to the 64-bit one
> sound reasonable indeed.
> 
Yes, I will add the fallback to 64-bit if the 32-bit allocation fails.
This fallback will be irrespective of whether the connected/intended EP
supports 64-bit MSI or not.

> -Serge(y)
> 
> > 
> > Thanks,
> > Robin.
> > 
> > > 
> > > -Serge(y)
> > > 
> > > >   	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > > >   					GFP_KERNEL);
> > > > -- 
> > > > 2.43.0.195.gebba966016-goog
> > > > 
> > > >