[PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[Frank Li]

From: Guanhua Gao <guanhua.gao@nxp.com>

Set DMA mask and coherent DMA mask to enable 64-bit addressing.

Signed-off-by: Guanhua Gao <guanhua.gao@nxp.com>
Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index de4c1758a6c33..6fd0dea38a32c 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
 
+	/* set 64-bit DMA mask and coherent DMA mask */
+	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
+		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))
+			return -EIO;
+
 	platform_set_drvdata(pdev, pcie);
 
 	ret = dw_pcie_ep_init(&pci->ep);
-- 
2.34.1

Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[Christophe JAILLET]

Le 21/09/2023 à 17:37, Frank Li a écrit :
> From: Guanhua Gao <guanhua.gao@nxp.com>
> 
> Set DMA mask and coherent DMA mask to enable 64-bit addressing.
> 
> Signed-off-by: Guanhua Gao <guanhua.gao@nxp.com>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index de4c1758a6c33..6fd0dea38a32c 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>   
>   	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
>   
> +	/* set 64-bit DMA mask and coherent DMA mask */
> +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
> +		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))

As stated in [1], dma_set_mask() with a 64-bit mask will never
fail if dev->dma_mask is non-NULL.

So, if it fails, the 32 bits case will also fail for the same reason.
There is no need for the 2nd test.


See [1] for Christoph Hellwig comment about it.

CJ


[1]: https://lkml.org/lkml/2021/6/7/398

> +			return -EIO;
> +
>   	platform_set_drvdata(pdev, pcie);
>   
>   	ret = dw_pcie_ep_init(&pci->ep);

Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[Frank Li]

On Thu, Sep 21, 2023 at 07:59:51PM +0200, Christophe JAILLET wrote:
> Le 21/09/2023 à 17:37, Frank Li a écrit :
> > From: Guanhua Gao <guanhua.gao@nxp.com>
> > 
> > Set DMA mask and coherent DMA mask to enable 64-bit addressing.
> > 
> > Signed-off-by: Guanhua Gao <guanhua.gao@nxp.com>
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >   drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index de4c1758a6c33..6fd0dea38a32c 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
> >   	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> > +	/* set 64-bit DMA mask and coherent DMA mask */
> > +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
> > +		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))
> 
> As stated in [1], dma_set_mask() with a 64-bit mask will never
> fail if dev->dma_mask is non-NULL.
> 
> So, if it fails, the 32 bits case will also fail for the same reason.
> There is no need for the 2nd test.
> 
> 
> See [1] for Christoph Hellwig comment about it.

I don't think it is true. the below is dma_set_mask()'s implementation

int dma_set_mask(struct device *dev, u64 mask)
{
	/*
	 * Truncate the mask to the actually supported dma_addr_t width to
	 * avoid generating unsupportable addresses.
	 */
	mask = (dma_addr_t)mask;

	if (!dev->dma_mask || !dma_supported(dev, mask))
				^^^^^^^
		return -EIO;

	arch_dma_set_mask(dev, mask);
	*dev->dma_mask = mask;
	return 0;
}

dma_supported() may return failiure.

static int dma_supported(struct device *dev, u64 mask)
{
	const struct dma_map_ops *ops = get_dma_ops(dev);

	/*
	 * ->dma_supported sets the bypass flag, so we must always call
	 * into the method here unless the device is truly direct mapped.
	 */
	if (!ops)
		return dma_direct_supported(dev, mask);
	if (!ops->dma_supported)
		return 1;
	return ops->dma_supported(dev, mask);
                    ^^^^^^
			DMA driver or IOMMU driver may return failure.
}

 
Frank

> 
> CJ
> 
> 
> [1]: https://lkml.org/lkml/2021/6/7/398
> 
> > +			return -EIO;
> > +
> >   	platform_set_drvdata(pdev, pcie);
> >   	ret = dw_pcie_ep_init(&pci->ep);
> 

Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[Christophe JAILLET]

Le 21/09/2023 à 20:35, Frank Li a écrit :
> On Thu, Sep 21, 2023 at 07:59:51PM +0200, Christophe JAILLET wrote:
>> Le 21/09/2023 à 17:37, Frank Li a écrit :
>>> From: Guanhua Gao <guanhua.gao@nxp.com>
>>>
>>> Set DMA mask and coherent DMA mask to enable 64-bit addressing.
>>>
>>> Signed-off-by: Guanhua Gao <guanhua.gao@nxp.com>
>>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>> ---
>>>    drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> index de4c1758a6c33..6fd0dea38a32c 100644
>>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> @@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>>>    	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
>>> +	/* set 64-bit DMA mask and coherent DMA mask */
>>> +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
>>> +		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))
>>
>> As stated in [1], dma_set_mask() with a 64-bit mask will never
>> fail if dev->dma_mask is non-NULL.
>>
>> So, if it fails, the 32 bits case will also fail for the same reason.
>> There is no need for the 2nd test.
>>
>>
>> See [1] for Christoph Hellwig comment about it.
> 
> I don't think it is true. the below is dma_set_mask()'s implementation

I'll try to recollect a more detailled explanation from Christoph.

I also checked all paths some times ago, and the conclusion was that if 
dma_set_mask(64) failed, dma_set_mask(32) would fail for the exact same 
reasons.

I'll try to find the corresponding mail and come back to you.

I don't thing that implementation details have changed since that times, 
so the conclusion should still be valid.

Adding Christoph in cc, if he wants to give another look at it, or if he 
beats me finding the 1 or 2 years old mails.

CJ

> 
> int dma_set_mask(struct device *dev, u64 mask)
> {
> 	/*
> 	 * Truncate the mask to the actually supported dma_addr_t width to
> 	 * avoid generating unsupportable addresses.
> 	 */
> 	mask = (dma_addr_t)mask;
> 
> 	if (!dev->dma_mask || !dma_supported(dev, mask))
> 				^^^^^^^
> 		return -EIO;
> 
> 	arch_dma_set_mask(dev, mask);
> 	*dev->dma_mask = mask;
> 	return 0;
> }
> 
> dma_supported() may return failiure.
> 
> static int dma_supported(struct device *dev, u64 mask)
> {
> 	const struct dma_map_ops *ops = get_dma_ops(dev);
> 
> 	/*
> 	 * ->dma_supported sets the bypass flag, so we must always call
> 	 * into the method here unless the device is truly direct mapped.
> 	 */
> 	if (!ops)
> 		return dma_direct_supported(dev, mask);
> 	if (!ops->dma_supported)
> 		return 1;
> 	return ops->dma_supported(dev, mask);
>                      ^^^^^^
> 			DMA driver or IOMMU driver may return failure.
> }
> 
>   
> Frank
> 
>>
>> CJ
>>
>>
>> [1]: https://lkml.org/lkml/2021/6/7/398
>>
>>> +			return -EIO;
>>> +
>>>    	platform_set_drvdata(pdev, pcie);
>>>    	ret = dw_pcie_ep_init(&pci->ep);
>>
> 

Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

[Frank Li]

On Thu, Sep 21, 2023 at 10:04:31PM +0200, Christophe JAILLET wrote:
> Le 21/09/2023 à 20:35, Frank Li a écrit :
> > On Thu, Sep 21, 2023 at 07:59:51PM +0200, Christophe JAILLET wrote:
> > > Le 21/09/2023 à 17:37, Frank Li a écrit :
> > > > From: Guanhua Gao <guanhua.gao@nxp.com>
> > > > 
> > > > Set DMA mask and coherent DMA mask to enable 64-bit addressing.
> > > > 
> > > > Signed-off-by: Guanhua Gao <guanhua.gao@nxp.com>
> > > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >    drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > index de4c1758a6c33..6fd0dea38a32c 100644
> > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > @@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > >    	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> > > > +	/* set 64-bit DMA mask and coherent DMA mask */
> > > > +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
> > > > +		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))
> > > 
> > > As stated in [1], dma_set_mask() with a 64-bit mask will never
> > > fail if dev->dma_mask is non-NULL.
> > > 
> > > So, if it fails, the 32 bits case will also fail for the same reason.
> > > There is no need for the 2nd test.
> > > 
> > > 
> > > See [1] for Christoph Hellwig comment about it.
> > 
> > I don't think it is true. the below is dma_set_mask()'s implementation
> 
> I'll try to recollect a more detailled explanation from Christoph.
> 
> I also checked all paths some times ago, and the conclusion was that if
> dma_set_mask(64) failed, dma_set_mask(32) would fail for the exact same
> reasons.
> 
> I'll try to find the corresponding mail and come back to you.

I go through iommu driver and code carefully. You are right.
The dma_supported() actual means iommu require minimized dma capatiblity.

It is quite miss leading. There are many codes in driver like these pattern. 

A example: 
static int sba_dma_supported( struct device *dev, u64 mask)()
{
	...
	 * check if mask is >= than the current max IO Virt Address                                 
         * The max IO Virt address will *always* < 30 bits.                                         
         */                                                                                         
        return((int)(mask >= (ioc->ibase - 1 +                                                      
                        (ioc->pdir_size / sizeof(u64) * IOVP_SIZE) )));
	...
}

1 means supported. 0 means unsupported. 

So dma_set_mask(64) is enough. Let me send new patch.

Frank


> 
> I don't thing that implementation details have changed since that times, so
> the conclusion should still be valid.
> 
> Adding Christoph in cc, if he wants to give another look at it, or if he
> beats me finding the 1 or 2 years old mails.
> 
> CJ
> 
> > 
> > int dma_set_mask(struct device *dev, u64 mask)
> > {
> > 	/*
> > 	 * Truncate the mask to the actually supported dma_addr_t width to
> > 	 * avoid generating unsupportable addresses.
> > 	 */
> > 	mask = (dma_addr_t)mask;
> > 
> > 	if (!dev->dma_mask || !dma_supported(dev, mask))
> > 				^^^^^^^
> > 		return -EIO;
> > 
> > 	arch_dma_set_mask(dev, mask);
> > 	*dev->dma_mask = mask;
> > 	return 0;
> > }
> > 
> > dma_supported() may return failiure.
> > 
> > static int dma_supported(struct device *dev, u64 mask)
> > {
> > 	const struct dma_map_ops *ops = get_dma_ops(dev);
> > 
> > 	/*
> > 	 * ->dma_supported sets the bypass flag, so we must always call
> > 	 * into the method here unless the device is truly direct mapped.
> > 	 */
> > 	if (!ops)
> > 		return dma_direct_supported(dev, mask);
> > 	if (!ops->dma_supported)
> > 		return 1;
> > 	return ops->dma_supported(dev, mask);
> >                      ^^^^^^
> > 			DMA driver or IOMMU driver may return failure.
> > }
> > 
> > Frank
> > 
> > > 
> > > CJ
> > > 
> > > 
> > > [1]: https://lkml.org/lkml/2021/6/7/398
> > > 
> > > > +			return -EIO;
> > > > +
> > > >    	platform_set_drvdata(pdev, pcie);
> > > >    	ret = dw_pcie_ep_init(&pci->ep);
> > > 
> > 
>