[PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports

[PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports

[Nikita Yushchenko]

> I reckon the easiest way forward would be to pass in some flag to
> arch_setup_dma_ops to indicate whether it's an explicitly-configured
> range or not - then simply initialising parent_dma_mask to ~0 for the
> default case *should* keep things working as before.

Tried to do that.

---

Nikita Yushchenko (2):
  dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()
  arm64: avoid increasing DMA masks above what hardware supports

 arch/arm/include/asm/dma-mapping.h             |  1 +
 arch/arm/mm/dma-mapping.c                      |  3 +-
 arch/arm64/Kconfig                             |  3 ++
 arch/arm64/include/asm/device.h                |  1 +
 arch/arm64/include/asm/dma-mapping.h           |  6 +++-
 arch/arm64/mm/dma-mapping.c                    | 43 +++++++++++++++++++++++++-
 arch/mips/include/asm/dma-mapping.h            |  3 +-
 drivers/acpi/scan.c                            |  2 +-
 drivers/iommu/rockchip-iommu.c                 |  2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c |  2 +-
 drivers/of/device.c                            |  5 ++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        |  2 +-
 12 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

There are cases when device is capable of wide DMA mask (and driver
issues corresponding dma_set_mask() call), but bus device sits on can't
support wide address. Example: NVMe device behind PCIe controller
sitting on 32-bit SoC bus.

To support such case, architecture needs information about such
limitations. Such information can originate from dma-ranges property
in device tree, and is passed to architecture via arch_setup_dma_ops()
call.

Problem is that in wide majority of cases, no dma range is defined.
E.g. ACPI has no means to define it. Thus default range (usually
full 32-bit range, i.e. 4G starting at zero address) is passed instead.

If architecture enforce this range, all setups currently using
wide DMA addresses without explicitly defining support for that via
device tree will break. This is bad, especially for ACPI based
platforms.

To avoid that, this patch adds additional boolean argument to
arch_setup_dma_ops() to show if range originates from authorative source
and thus should be enforced, or is just a guess and should be handled as
such.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 arch/arm/include/asm/dma-mapping.h             | 1 +
 arch/arm/mm/dma-mapping.c                      | 3 ++-
 arch/arm64/include/asm/dma-mapping.h           | 3 ++-
 arch/arm64/mm/dma-mapping.c                    | 3 ++-
 arch/mips/include/asm/dma-mapping.h            | 3 ++-
 drivers/acpi/scan.c                            | 2 +-
 drivers/iommu/rockchip-iommu.c                 | 2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
 drivers/of/device.c                            | 5 ++++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        | 2 +-
 10 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index bf02dbd..2a3863e 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -117,6 +117,7 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			       bool enforce_range,
 			       const struct iommu_ops *iommu, bool coherent);
 
 #define arch_teardown_dma_ops arch_teardown_dma_ops
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..b8b11f8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2380,7 +2380,8 @@ static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	struct dma_map_ops *dma_ops;
 
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ccea82c..ae1c23f 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -48,7 +48,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent);
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
 #ifdef CONFIG_IOMMU_DMA
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..ea295f1 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -953,7 +953,8 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 #endif  /* CONFIG_IOMMU_DMA */
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
index 7aa71b9..6af4d87 100644
--- a/arch/mips/include/asm/dma-mapping.h
+++ b/arch/mips/include/asm/dma-mapping.h
@@ -34,7 +34,8 @@ extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-				      u64 size, const struct iommu_ops *iommu,
+				      u64 size, bool enforce_range,
+				      const struct iommu_ops *iommu,
 				      bool coherent)
 {
 #ifdef CONFIG_DMA_PERDEV_COHERENT
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..dea17a5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1385,7 +1385,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 	 * Assume dma valid range starts at 0 and covers the whole
 	 * coherent_dma_mask.
 	 */
-	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
 			   attr == DEV_DMA_COHERENT);
 }
 EXPORT_SYMBOL_GPL(acpi_dma_configure);
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9afcbf7..0995ab3 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
-	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
+	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
 
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index c9b7ad6..19f70d8 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
 
 	/* device used for DMA mapping */
-	arch_setup_dma_ops(dev, 0, 0, NULL, false);
+	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
 	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
 	if (err) {
 		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad..1cc2115 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	bool coherent;
 	unsigned long offset;
 	const struct iommu_ops *iommu;
+	bool enforce_range = false;
 
 	/*
 	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
@@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 			return;
 		}
 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+
+		enforce_range = true;
 	}
 
 	dev->dma_pfn_offset = offset;
@@ -147,7 +150,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+	arch_setup_dma_ops(dev, dma_addr, size, enforce_range, iommu, coherent);
 }
 EXPORT_SYMBOL_GPL(of_dma_configure);
 
diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 5ac373c..480b644 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
 
 	/* Objects are coherent, unless 'no shareability' flag set. */
 	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
-		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
+		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
 
 	/*
 	 * The device-specific probe callback will get invoked by device_add()
-- 
2.1.4

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:51 PM CET Nikita Yushchenko wrote:

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 9afcbf7..0995ab3 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
> -	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
> +	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
>  
>  	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index c9b7ad6..19f70d8 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
>  	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
>  
>  	/* device used for DMA mapping */
> -	arch_setup_dma_ops(dev, 0, 0, NULL, false);
> +	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
>  	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
>  	if (err) {
>  		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> index 5ac373c..480b644 100644
> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>  
>  	/* Objects are coherent, unless 'no shareability' flag set. */
>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>  
>  	/*
>  	 * The device-specific probe callback will get invoked by device_add()

Why are these actually calling arch_setup_dma_ops() here in the first
place? Are these all devices that are DMA masters without an OF node?

> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad..1cc2115 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  	bool coherent;
>  	unsigned long offset;
>  	const struct iommu_ops *iommu;
> +	bool enforce_range = false;
>  
>  	/*
>  	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  			return;
>  		}
>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> +
> +		enforce_range = true;
>  	}
>  
>  	dev->dma_pfn_offset = offset;

Hmm, I think when the dma-ranges are missing, we should either enforce
a 32-bit mask, or disallow DMA completely. It's probably too late for
the latter, I wish we had done this earlier in order to force everyone
on ARM64 to have a valid dma-ranges property for any DMA master.

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> index 5ac373c..480b644 100644
>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>  
>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>  
>>  	/*
>>  	 * The device-specific probe callback will get invoked by device_add()
> 
> Why are these actually calling arch_setup_dma_ops() here in the first
> place? Are these all devices that are DMA masters without an OF node?

I don't know, but that's a different topic. This patch just adds
argument and sets it to false everywhere but in the location when range
should be definitely enforced.

>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>  			return;
>>  		}
>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> +
>> +		enforce_range = true;
>>  	}
>>  
>>  	dev->dma_pfn_offset = offset;
> 
> Hmm, I think when the dma-ranges are missing, we should either enforce
> a 32-bit mask, or disallow DMA completely. It's probably too late for
> the latter, I wish we had done this earlier in order to force everyone
> on ARM64 to have a valid dma-ranges property for any DMA master.

This can be done over time.

However the very idea of this version of patch is - keep working pieces
as-is, thus for now setting enforce_range to false in case of no defined
dma-ranges is intentional.

What I should re-check is - does rcar dtsi set dma-ranges, and add it if
it does not.

Nikita

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

12.01.2017 08:52, Nikita Yushchenko wrote:
>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> index 5ac373c..480b644 100644
>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>  
>>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>  
>>>  	/*
>>>  	 * The device-specific probe callback will get invoked by device_add()
>>
>> Why are these actually calling arch_setup_dma_ops() here in the first
>> place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.
> 
>>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>>  			return;
>>>  		}
>>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>> +
>>> +		enforce_range = true;
>>>  	}
>>>  
>>>  	dev->dma_pfn_offset = offset;
>>
>> Hmm, I think when the dma-ranges are missing, we should either enforce
>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>> the latter, I wish we had done this earlier in order to force everyone
>> on ARM64 to have a valid dma-ranges property for any DMA master.
> 
> This can be done over time.
> 
> However the very idea of this version of patch is - keep working pieces
> as-is, thus for now setting enforce_range to false in case of no defined
> dma-ranges is intentional.

What we can do is - check bus width (as it is defined in DT) and set
enforce_range to true if bus is 32-bit

> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> it does not.

It does not, will have to add.

In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
Maybe DT needs fixing.

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 9:33:32 AM CET Nikita Yushchenko wrote:
> >> Hmm, I think when the dma-ranges are missing, we should either enforce
> >> a 32-bit mask, or disallow DMA completely. It's probably too late for
> >> the latter, I wish we had done this earlier in order to force everyone
> >> on ARM64 to have a valid dma-ranges property for any DMA master.
> > 
> > This can be done over time.
> > 
> > However the very idea of this version of patch is - keep working pieces
> > as-is, thus for now setting enforce_range to false in case of no defined
> > dma-ranges is intentional.
> 
> What we can do is - check bus width (as it is defined in DT) and set
> enforce_range to true if bus is 32-bit
> 
> > What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> > it does not.
> 
> It does not, will have to add.
> 
> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
> Maybe DT needs fixing.

I think we always assumed that the lack of a dma-ranges property
implied a 32-bit width, as that is the safe fallback as well as the
most common case.

AFAICT, this means you are actually fine on rcar, and all other
platforms will keep working as we enforce it, but might get slowed
down if they relied on the unintended behavior of allowing 64-bit
DMA.

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>>>> Hmm, I think when the dma-ranges are missing, we should either enforce
>>>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>>>> the latter, I wish we had done this earlier in order to force everyone
>>>> on ARM64 to have a valid dma-ranges property for any DMA master.
>>>
>>> This can be done over time.
>>>
>>> However the very idea of this version of patch is - keep working pieces
>>> as-is, thus for now setting enforce_range to false in case of no defined
>>> dma-ranges is intentional.
>>
>> What we can do is - check bus width (as it is defined in DT) and set
>> enforce_range to true if bus is 32-bit
>>
>>> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
>>> it does not.
>>
>> It does not, will have to add.
>>
>> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
>> Maybe DT needs fixing.
> 
> I think we always assumed that the lack of a dma-ranges property
> implied a 32-bit width, as that is the safe fallback as well as the
> most common case.

Yes we assumed that, but that was combined with blindly accepting wider
dma_mask per driver's request.  Later is being changed.

> AFAICT, this means you are actually fine on rcar, and all other
> platforms will keep working as we enforce it, but might get slowed
> down if they relied on the unintended behavior of allowing 64-bit
> DMA.

Yesterday Robin raised issue that a change starting to enforce default
dma_mask will break existing setups - i.e. those that depend in 64bit
DMA being implicitly supported without manually declaring such support.

In reply to that, I suggested this version of patchset that should keep
existing behavior by default.

I'm fine with both approaches regarding behavior on hw that I don't have
- but I'm not in position to make any decisions on that.

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Will Deacon]

On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> index 5ac373c..480b644 100644
> >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> >>  
> >>  	/* Objects are coherent, unless 'no shareability' flag set. */
> >>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> >> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> >> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> >>  
> >>  	/*
> >>  	 * The device-specific probe callback will get invoked by device_add()
> > 
> > Why are these actually calling arch_setup_dma_ops() here in the first
> > place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.

I also wouldn't lose any sleep over a staging driver.

Will

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> index 5ac373c..480b644 100644
> > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> > >>  
> > >>    /* Objects are coherent, unless 'no shareability' flag set. */
> > >>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> > >> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> > >> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> > >>  
> > >>    /*
> > >>     * The device-specific probe callback will get invoked by device_add()
> > > 
> > > Why are these actually calling arch_setup_dma_ops() here in the first
> > > place? Are these all devices that are DMA masters without an OF node?
> > 
> > I don't know, but that's a different topic. This patch just adds
> > argument and sets it to false everywhere but in the location when range
> > should be definitely enforced.
> 
> I also wouldn't lose any sleep over a staging driver.

I think this is in the process of being moved out of staging, and
my question was about the other two as well:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
drivers/iommu/rockchip-iommu.c

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Robin Murphy]

On 12/01/17 13:25, Arnd Bergmann wrote:
> On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
>> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
>>>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> index 5ac373c..480b644 100644
>>>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>>>  
>>>>>    /* Objects are coherent, unless 'no shareability' flag set. */
>>>>>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>>>> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>>>> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>>>  
>>>>>    /*
>>>>>     * The device-specific probe callback will get invoked by device_add()
>>>>
>>>> Why are these actually calling arch_setup_dma_ops() here in the first
>>>> place? Are these all devices that are DMA masters without an OF node?
>>>
>>> I don't know, but that's a different topic. This patch just adds
>>> argument and sets it to false everywhere but in the location when range
>>> should be definitely enforced.
>>
>> I also wouldn't lose any sleep over a staging driver.
> 
> I think this is in the process of being moved out of staging, and
> my question was about the other two as well:

The fsl-mc is actually a sort-of-bus-controller probing and configuring
its (directly DMA-capable) child devices, so is in fact legitimate here.

> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c

That one is completely bogus, and should just go away.

> drivers/iommu/rockchip-iommu.c

That one is part of some ugly trickery involving creating a fake device
to represent multiple separate IOMMU devices. The driver could probably
be reworked to not need it (the Exynos IOMMU handles a similar situation
without such tricks), but it's non-trivial.

Robin.

> 
> 	Arnd
> 

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: x86_64-randconfig-u0-01131618 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/acpi/scan.c: In function 'acpi_dma_configure':
>> drivers/acpi/scan.c:1388:2: error: too many arguments to function 'arch_setup_dma_ops'
     arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
     ^~~~~~~~~~~~~~~~~~
   In file included from drivers/acpi/scan.c:15:0:
   include/linux/dma-mapping.h:611:20: note: declared here
    static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
                       ^~~~~~~~~~~~~~~~~~

vim +/arch_setup_dma_ops +1388 drivers/acpi/scan.c

  1382		iommu = iort_iommu_configure(dev);
  1383	
  1384		/*
  1385		 * Assume dma valid range starts at 0 and covers the whole
  1386		 * coherent_dma_mask.
  1387		 */
> 1388		arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
  1389				   attr == DEV_DMA_COHERENT);
  1390	}
  1391	EXPORT_SYMBOL_GPL(acpi_dma_configure);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25665 bytes --]

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

There are cases when device supports wide DMA addresses wider than
device's connection supports.

In this case driver sets DMA mask based on knowledge of device
capabilities. That must succeed to allow drivers to initialize.

However, swiotlb or iommu still need knowledge about actual device
capabilities. To avoid breakage, actual mask must not be set wider than
device connection allows.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Robin Murphy <robin.murphy@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                   |  3 +++
 arch/arm64/include/asm/device.h      |  1 +
 arch/arm64/include/asm/dma-mapping.h |  3 +++
 arch/arm64/mm/dma-mapping.c          | 40 ++++++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ae1c23f..46b53e6 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -52,6 +52,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
+#define HAVE_ARCH_DMA_SET_MASK 1
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+
 #ifdef CONFIG_IOMMU_DMA
 void arch_teardown_dma_ops(struct device *dev);
 #define arch_teardown_dma_ops	arch_teardown_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index ea295f1..31b96fd 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (ops->set_dma_mask)
+		return ops->set_dma_mask(dev, mask);
+
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 				     unsigned long offset, size_t size,
 				     enum dma_data_direction dir,
@@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	if (enforce_range)
+		dev->archdata.parent_dma_mask = size - 1;
+	else
+		dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:52 PM CET Nikita Yushchenko wrote:
> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>         if (!dev->archdata.dma_ops)
>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +       /*
> +        * Whatever the parent bus can set. A device must not set
> +        * a DMA mask larger than this.
> +        */
> +       if (enforce_range)
> +               dev->archdata.parent_dma_mask = size - 1;
> +       else
> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
> +
>         dev->archdata.dma_coherent = coherent;
>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
> 

Could we just pass the mask instead of the size here?

	Arnd

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

>> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>         if (!dev->archdata.dma_ops)
>>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>>  
>> +       /*
>> +        * Whatever the parent bus can set. A device must not set
>> +        * a DMA mask larger than this.
>> +        */
>> +       if (enforce_range)
>> +               dev->archdata.parent_dma_mask = size - 1;
>> +       else
>> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
>> +
>>         dev->archdata.dma_coherent = coherent;
>>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
>>
> 
> Could we just pass the mask instead of the size here?

We don't want to change API now.

Nikita

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   arch/arm64/mm/dma-mapping.c: In function 'dma_set_mask':
>> arch/arm64/mm/dma-mapping.c:210:6: error: 'mask' undeclared (first use in this function)
     if (mask > dev->archdata.parent_dma_mask)
         ^~~~
   arch/arm64/mm/dma-mapping.c:210:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/mask +210 arch/arm64/mm/dma-mapping.c

   204	}
   205	
   206	int dma_set_mask(struct device *dev, u64 dma_mask)
   207	{
   208		const struct dma_map_ops *ops = get_dma_ops(dev);
   209	
 > 210		if (mask > dev->archdata.parent_dma_mask)
   211			mask = dev->archdata.parent_dma_mask;
   212	
   213		if (ops->set_dma_mask)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33713 bytes --]