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

[David Lechner <dlechner@baylibre.com>]

On 6/27/25 10:27 AM, Jean-Baptiste Maneyrol wrote:

...

>>>>> 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?
> 

I had to go digging through the source code to understand any of this, but
now I finally do.

What would have made this clear to me in the commit message would be to say
that:

1. The timestamp <anonymous> struct is not used with DMA so it doesn't
   belong after __aligned(IIO_DMA_MINALIGN).
2. struct inv_icm42600_fifo contains it's own __aligned(IIO_DMA_MINALIGN)
   within it so it should not be after __ailgned(IIO_DMA_MINALIGN) in
   the outer struct either.
3. Normally 1 would have been considered a bug, but because of the extra
   alignment from 2, it actually was OK, but we shouldn't be relying on
   such quirks.