[PATCH] iio: commom: st_sensors: ensure proper DMA alignment

[PATCH] iio: commom: st_sensors: ensure proper DMA alignment

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Aligning the buffer to the L1 cache is not sufficient in some platforms
as they might have larger cacheline sizes for caches after L1 and thus,
we can't guarantee DMA safety.

That was the whole reason to introduce IIO_DMA_MINALIGN in [1]. Do the same
for st_sensors common buffer.

[1]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/

Fixes: e031d5f558f1 ("iio:st_sensors: remove buffer allocation at each buffer enable")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/common/st_sensors.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 607c3a89a647..a02652cf4862 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -258,7 +258,7 @@ struct st_sensor_data {
 	bool hw_irq_trigger;
 	s64 hw_timestamp;
 
-	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
+	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
 
 	struct mutex odr_lock;
 };

---
base-commit: f9c0358aadcba16d04d139a5412b413eeee87afe
change-id: 20240122-dev_dma_safety_stm-c7016673f4ec
--

Thanks!
- Nuno Sá

Re: [PATCH] iio: commom: st_sensors: ensure proper DMA alignment

[Jonathan Cameron]

On Mon, 22 Jan 2024 15:15:41 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Aligning the buffer to the L1 cache is not sufficient in some platforms
> as they might have larger cacheline sizes for caches after L1 and thus,
> we can't guarantee DMA safety.
> 
> That was the whole reason to introduce IIO_DMA_MINALIGN in [1]. Do the same
> for st_sensors common buffer.
> 
> [1]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
> 
> Fixes: e031d5f558f1 ("iio:st_sensors: remove buffer allocation at each buffer enable")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  include/linux/iio/common/st_sensors.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 607c3a89a647..a02652cf4862 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -258,7 +258,7 @@ struct st_sensor_data {
>  	bool hw_irq_trigger;
>  	s64 hw_timestamp;
>  
> -	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
> +	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
>  
>  	struct mutex odr_lock;
Hi Nuno.

This is another problem.  There should be nothing after a DMA safe buffer embedded
in a structure like we do here.  We rely on C structure padding to ensure the
rest of the __aligned(IIO_DMA_MINALIGN) region is unused and that doesn't work
if the buffer isn't the last element.

My guess is we are safe to just reorder this before the buffer.
Nuno, do you mind spinning a v2 that does that as well as the size change.

Jonathan

>  };
> 
> ---
> base-commit: f9c0358aadcba16d04d139a5412b413eeee87afe
> change-id: 20240122-dev_dma_safety_stm-c7016673f4ec
> --
> 
> Thanks!
> - Nuno Sá
> 

Re: [PATCH] iio: commom: st_sensors: ensure proper DMA alignment

[Nuno Sá]

On Sat, 2024-01-27 at 15:56 +0000, Jonathan Cameron wrote:
> On Mon, 22 Jan 2024 15:15:41 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Aligning the buffer to the L1 cache is not sufficient in some platforms
> > as they might have larger cacheline sizes for caches after L1 and thus,
> > we can't guarantee DMA safety.
> > 
> > That was the whole reason to introduce IIO_DMA_MINALIGN in [1]. Do the same
> > for st_sensors common buffer.
> > 
> > [1]:
> > https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
> > 
> > Fixes: e031d5f558f1 ("iio:st_sensors: remove buffer allocation at each
> > buffer enable")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  include/linux/iio/common/st_sensors.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/iio/common/st_sensors.h
> > b/include/linux/iio/common/st_sensors.h
> > index 607c3a89a647..a02652cf4862 100644
> > --- a/include/linux/iio/common/st_sensors.h
> > +++ b/include/linux/iio/common/st_sensors.h
> > @@ -258,7 +258,7 @@ struct st_sensor_data {
> >  	bool hw_irq_trigger;
> >  	s64 hw_timestamp;
> >  
> > -	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
> > +	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE]
> > __aligned(IIO_DMA_MINALIGN);
> >  
> >  	struct mutex odr_lock;
> Hi Nuno.
> 
> This is another problem.  There should be nothing after a DMA safe buffer
> embedded
> in a structure like we do here.  We rely on C structure padding to ensure the
> rest of the __aligned(IIO_DMA_MINALIGN) region is unused and that doesn't work
> if the buffer isn't the last element.
> 
> My guess is we are safe to just reorder this before the buffer.
> Nuno, do you mind spinning a v2 that does that as well as the size change.
> 

Hi Jonathan,

Somehow I failed to see your reply and was so focused on just having the proper
alignment that completely forgot the mutex after the buffer is obviously
problematic. Good catch!

Will spin a v2.

- Nuno Sá