diff mbox series

[v2,01/92] iio: core: Fix IIO_ALIGN and rename as it was not sufficiently large

Message ID 20220508175712.647246-2-jic23@kernel.org (mailing list archive)
State Accepted
Headers show
Series IIO: Fix alignment of buffers for DMA | expand

Commit Message

Jonathan Cameron May 8, 2022, 5:55 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Discussion of the series:
https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
mm, arm64: Reduce ARCH_KMALLOC_MINALIGN brought to my attention that
our current IIO usage of L1CACHE_ALIGN is insufficient as their are Arm
platforms out their with non coherent DMA and larger cache lines at
at higher levels of their cache hierarchy.

Rename the define to make it's purpose more explicit. It will be used
much more widely going forwards (to replace incorrect ____cacheline_aligned
markings.

Note this patch will greatly reduce the padding on some architectures
that have smaller requirements for DMA safe buffers.

The history of changing values of ARCH_KMALLOC_MINALIGN via
ARCH_DMA_MINALIGN on arm64 is rather complex. I'm not tagging this
as fixing a particular patch from that route as it's not clear what to tag.

Most recently a change to bring them back inline was reverted because
of some Qualcomm Kryo cores with an L2 cache with 128-byte lines
sitting above the point of coherency.

c1132702c71f Revert "arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES)"
That reverts:
65688d2a05de arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES) which
refers to the change originally being motivated by Thunder x1 performance
rather than correctness.

Fixes: 6f7c8ee585e9d ("staging:iio: Add ability to allocate private data space to iio_allocate_device")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c   |  7 ++++---
 drivers/iio/industrialio-core.c |  4 ++--
 include/linux/iio/iio.h         | 10 ++++++++--
 3 files changed, 14 insertions(+), 7 deletions(-)

Comments

Jonathan Cameron June 4, 2022, 4:56 p.m. UTC | #1 
On Sun,  8 May 2022 18:55:41 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Discussion of the series:
> https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> mm, arm64: Reduce ARCH_KMALLOC_MINALIGN brought to my attention that
> our current IIO usage of L1CACHE_ALIGN is insufficient as their are Arm
> platforms out their with non coherent DMA and larger cache lines at
> at higher levels of their cache hierarchy.
> 
> Rename the define to make it's purpose more explicit. It will be used
> much more widely going forwards (to replace incorrect ____cacheline_aligned
> markings.
> 
> Note this patch will greatly reduce the padding on some architectures
> that have smaller requirements for DMA safe buffers.
> 
> The history of changing values of ARCH_KMALLOC_MINALIGN via
> ARCH_DMA_MINALIGN on arm64 is rather complex. I'm not tagging this
> as fixing a particular patch from that route as it's not clear what to tag.
> 
> Most recently a change to bring them back inline was reverted because
> of some Qualcomm Kryo cores with an L2 cache with 128-byte lines
> sitting above the point of coherency.
> 
> c1132702c71f Revert "arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES)"
> That reverts:
> 65688d2a05de arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES) which
> refers to the change originally being motivated by Thunder x1 performance
> rather than correctness.
> 
> Fixes: 6f7c8ee585e9d ("staging:iio: Add ability to allocate private data space to iio_allocate_device")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Nuno Sá <nuno.sa@analog.com>

This crossed with a patch adding another use of IIO_ALIGN in bma400. I've fixed that
rename up whilst applying.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/adi-axi-adc.c   |  7 ++++---
>  drivers/iio/industrialio-core.c |  4 ++--
>  include/linux/iio/iio.h         | 10 ++++++++--
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index a73e3c2d212f..099be9d47223 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -84,7 +84,8 @@ void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
>  {
>  	struct adi_axi_adc_client *cl = conv_to_client(conv);
>  
> -	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
> +	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> +				  IIO_DMA_MINALIGN);
>  }
>  EXPORT_SYMBOL_GPL(adi_axi_adc_conv_priv);
>  
> @@ -169,9 +170,9 @@ static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
>  	struct adi_axi_adc_client *cl;
>  	size_t alloc_size;
>  
> -	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
> +	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_DMA_MINALIGN);
>  	if (sizeof_priv)
> -		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
> +		alloc_size += ALIGN(sizeof_priv, IIO_DMA_MINALIGN);
>  
>  	cl = kzalloc(alloc_size, GFP_KERNEL);
>  	if (!cl)
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index e1ed44dec2ab..b4218f3b1ac2 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1640,7 +1640,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  
>  	alloc_size = sizeof(struct iio_dev_opaque);
>  	if (sizeof_priv) {
> -		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> +		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN);
>  		alloc_size += sizeof_priv;
>  	}
>  
> @@ -1650,7 +1650,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  
>  	indio_dev = &iio_dev_opaque->indio_dev;
>  	indio_dev->priv = (char *)iio_dev_opaque +
> -		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> +		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
>  
>  	indio_dev->dev.parent = parent;
>  	indio_dev->dev.type = &iio_device_type;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index faf00f2c0be6..c4ce02293f1f 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -9,6 +9,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/slab.h>
>  #include <linux/iio/types.h>
>  #include <linux/of.h>
>  /* IIO TODO LIST */
> @@ -657,8 +658,13 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>  	return dev_get_drvdata(&indio_dev->dev);
>  }
>  
> -/* Can we make this smaller? */
> -#define IIO_ALIGN L1_CACHE_BYTES
> +/*
> + * Used to ensure the iio_priv() structure is aligned to allow that structure
> + * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
> + * must not share  cachelines with the rest of the structure, thus making
> + * them safe for use with non-coherent DMA.
> + */
> +#define IIO_DMA_MINALIGN ARCH_KMALLOC_MINALIGN
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
>  /* The information at the returned address is guaranteed to be cacheline aligned */
diff mbox series

Patch

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index a73e3c2d212f..099be9d47223 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -84,7 +84,8 @@  void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
 {
 	struct adi_axi_adc_client *cl = conv_to_client(conv);
 
-	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
+	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
+				  IIO_DMA_MINALIGN);
 }
 EXPORT_SYMBOL_GPL(adi_axi_adc_conv_priv);
 
@@ -169,9 +170,9 @@  static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
 	struct adi_axi_adc_client *cl;
 	size_t alloc_size;
 
-	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
+	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_DMA_MINALIGN);
 	if (sizeof_priv)
-		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
+		alloc_size += ALIGN(sizeof_priv, IIO_DMA_MINALIGN);
 
 	cl = kzalloc(alloc_size, GFP_KERNEL);
 	if (!cl)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e1ed44dec2ab..b4218f3b1ac2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1640,7 +1640,7 @@  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 
 	alloc_size = sizeof(struct iio_dev_opaque);
 	if (sizeof_priv) {
-		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
+		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN);
 		alloc_size += sizeof_priv;
 	}
 
@@ -1650,7 +1650,7 @@  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 
 	indio_dev = &iio_dev_opaque->indio_dev;
 	indio_dev->priv = (char *)iio_dev_opaque +
-		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
+		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
 
 	indio_dev->dev.parent = parent;
 	indio_dev->dev.type = &iio_device_type;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index faf00f2c0be6..c4ce02293f1f 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -9,6 +9,7 @@ 
 
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/slab.h>
 #include <linux/iio/types.h>
 #include <linux/of.h>
 /* IIO TODO LIST */
@@ -657,8 +658,13 @@  static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
 	return dev_get_drvdata(&indio_dev->dev);
 }
 
-/* Can we make this smaller? */
-#define IIO_ALIGN L1_CACHE_BYTES
+/*
+ * Used to ensure the iio_priv() structure is aligned to allow that structure
+ * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
+ * must not share  cachelines with the rest of the structure, thus making
+ * them safe for use with non-coherent DMA.
+ */
+#define IIO_DMA_MINALIGN ARCH_KMALLOC_MINALIGN
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
 
 /* The information at the returned address is guaranteed to be cacheline aligned */