| Message ID | 20240122-dev_dma_safety_stm-v1-1-3a021614cbfb@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: commom: st_sensors: ensure proper DMA alignment | expand |
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á >
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á
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; };