[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6152da6f-84a1-40e0-9fe8-248a2835de6e@baylibre.com>
Date: Fri, 27 Jun 2025 12:19:55 -0500
From: David Lechner <dlechner@...libre.com>
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>,
Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers
at the end
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.
Powered by blists - more mailing lists