Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end

[Jean-Baptiste Maneyrol]

>
>________________________________________
>From: Jonathan Cameron <jic23@xxxxxxxxxx>
>Sent: Thursday, June 26, 2025 20:48
>To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>
>Cc: David Lechner <dlechner@xxxxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Nuno Sá <nuno.sa@xxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
>Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Tue, 24 Jun 2025 08:43:37 +0000
>Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx> wrote:
>
>> >
>> >________________________________________
>> >From: David Lechner <dlechner@xxxxxxxxxxxx>
>> >Sent: Tuesday, June 24, 2025 00:13
>> >To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>
>> >Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Nuno Sá <nuno.sa@xxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
>> >Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
>> > 
>> >This Message Is From an External Sender
>> >This message came from outside your organization.
>> > 
>> >On Mon, Jun 23, 2025 at 6:56 AM Jean-Baptiste Maneyrol via B4 Relay
>> ><devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx> wrote:  
>> >>
>> >> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>> >>
>> >> Move DMA buffers at the end of the structure to avoid overflow
>> >> bugs with unexpected effect.  
>> >
>> >If there is an overflow bug, we should fix that rather than hiding it.
>> >
>> >If I misunderstood the problem and timestamp and fifo should not be in
>> >the DMA aligned area and there is a problem with DMA cache writing
>> >over them, then I think we should reword the commit message.
>> >  
>> Hello David,
>> 
>> this change was asked by Jonathan so that potential DMA overflow would be more
>> easily caught by writing outside of the structure rather than writing inside
>> and do unexpected behavior.
>> 
>> >>
>> >> struct inv_icm42600_fifo has a DMA buffer at the end.
>> >>
>> >> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>> >> ---
>> >>  drivers/iio/imu/inv_icm42600/inv_icm42600.h | 8 ++++----
>> >>  1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> index 55ed1ddaa8cb5dd410d17db3866fa0f22f18e9d2..9b2cce172670c5513f18d5979a5ff563e9af4cb3 100644
>> >> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> @@ -148,9 +148,9 @@ struct inv_icm42600_suspended {
>> >>   *  @suspended:                suspended sensors configuration.
>> >>   *  @indio_gyro:       gyroscope IIO device.
>> >>   *  @indio_accel:      accelerometer IIO device.
>> >> - *  @buffer:           data transfer buffer aligned for DMA.
>> >> - *  @fifo:             FIFO management structure.
>> >>   *  @timestamp:                interrupt timestamps.
>> >> + *  @fifo:             FIFO management structure.
>> >> + *  @buffer:           data transfer buffer aligned for DMA.
>> >>   */
>> >>  struct inv_icm42600_state {
>> >>         struct mutex lock;
>> >> @@ -164,12 +164,12 @@ struct inv_icm42600_state {
>> >>         struct inv_icm42600_suspended suspended;
>> >>         struct iio_dev *indio_gyro;
>> >>         struct iio_dev *indio_accel;
>> >> -       u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >> -       struct inv_icm42600_fifo fifo;
>> >>         struct {
>> >>                 s64 gyro;
>> >>                 s64 accel;
>> >>         } timestamp;
>> >> +       struct inv_icm42600_fifo fifo;  
>> >
>> >I didn't look at how the drivers use timestamp and fifo, but if they
>> >are passed as a buffer to SPI, then they need to stay in the DMA
>> >aligned area of the struct.  
>> 
>> struct inv_icm42600_fifo has a buffer at its end that is passed to SPI.
>> Same things for buffer below. That's why both buffers are DMA
>> aligned.
>
>It's a tiny bit esoteric that this is relying on structure alignment rules
>that says (iirc) the structure element will be aligned to maximum of it's
>elements and there is tail padding to that size as well.  Thus the whole
>struct inv_icm42600 is __aligned(IIO_DMA_MINALIGN) and the buffer in there
>is itself after padding to ensure that it is __aligned(IIO_DMA_MINALIGN)
>
>
>Anyhow, all I think this actually does is avoid one lot of padding
>(as well as making it slightly easier to reason about!)
>
>outer struct {
>stuff
>padding to align #1
>fifo {
>	stuff
>	padding to align
>	buffer
>	padding to fill up align
>}
>struct timestamp;
>///this bit will go away as it can fit in the padding #1 (probably)
>Padding to align
>////
>u8 buffer[2] __ailgned(IIO_DMA_MINALIGN)
>
>

Hello David and Jonathan,

what should I changed for this patch? Rewrite the commit message?

Thanks for your feedback,
JB

>	
>> 
>> >  
>> >> +       u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >>  };
>> >>
>> >>
>> >>
>> >> --
>> >> 2.49.0
>> >>
>> >>  
>> >  
>> 
>> Thanks,
>> JB
>
>