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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 27 Jun 2025 12:19:55 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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.
> 
> 
LGTM. I thought about just applying this with David's message version but
given you are doing another spin for the other patches in the series I'll
pick this one up with those.

Thanks,

Jonathan

>