Message-ID: <b4218efe-3785-4065-a3f7-57824e882f09@baylibre.com>
Date: Wed, 23 Apr 2025 09:51:25 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Eugen Hristev <eugen.hristev@...aro.org>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

On 4/23/25 4:18 AM, Nuno Sá wrote:
> Hi David,
> 
> Nice patch, I really think these will be very helpful... Just one comment bellow
> 
> On Tue, 2025-04-22 at 17:07 -0500, David Lechner wrote:
>> Add new macros to help with the common case of declaring a buffer that
>> is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
>> to do correctly because of the alignment requirements of the timestamp.
>> This will make it easier for both authors and reviewers.
>>
>> To avoid double __align() attributes in cases where we also need DMA
>> alignment, add a 2nd variant IIO_DECLARE_DMA_BUFFER_WITH_TS.
>>
>> Signed-off-by: David Lechner <dlechner@...libre.com>
>> ---
>>  include/linux/iio/iio.h | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index
>> 638cf2420fbd85cf2924d09d061df601d1d4bb2a..4dd811e3530e228a6fadbd80cfb2f5068c3d
>> 6a9a 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -7,6 +7,7 @@
>>  #ifndef _INDUSTRIAL_IO_H_
>>  #define _INDUSTRIAL_IO_H_
>>  
>> +#include <linux/align.h>
>>  #include <linux/device.h>
>>  #include <linux/cdev.h>
>>  #include <linux/compiler_types.h>
>> @@ -777,6 +778,41 @@ static inline void *iio_device_get_drvdata(const struct
>> iio_dev *indio_dev)
>>   * them safe for use with non-coherent DMA.
>>   */
>>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
>> +
>> +#define _IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
>> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) + sizeof(s64) /
>> sizeof(type)]
>> +
>> +/**
>> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
>> + * @type: element type of the buffer
>> + * @name: identifier name of the buffer
>> + * @count: number of elements in the buffer
>> + *
>> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts().
>> In
>> + * addition to allocating enough space for @count elements of @type, it also
>> + * allocates space for a s64 timestamp at the end of the buffer and ensures
>> + * proper alignment of the timestamp.
>> + */
>> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
>> +	_IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(sizeof(s64))
>> +
>> +/**
>> + * IIO_DECLARE_DMA_BUFFER_WITH_TS() - Declare a DMA-aligned buffer with
>> timestamp
>> + * @type: element type of the buffer
>> + * @name: identifier name of the buffer
>> + * @count: number of elements in the buffer
>> + *
>> + * Same as IIO_DECLARE_BUFFER_WITH_TS(), but is uses
>> __aligned(IIO_DMA_MINALIGN)
>> + * to ensure that the buffer doesn't share cachelines with anything that
>> comes
>> + * before it in a struct. This should not be used for stack-allocated buffers
>> + * as stack memory cannot generally be used for DMA.
>> + */
>> +#define IIO_DECLARE_DMA_BUFFER_WITH_TS(type, name, count) \
>> +	_IIO_DECLARE_BUFFER_WITH_TS(type, name, count)
>> __aligned(IIO_DMA_MINALIGN)
>> +
>> +_Static_assert(sizeof(IIO_DMA_MINALIGN) % sizeof(s64) == 0,
>> +	"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp
>> alignment");
>>
> 
> I wonder about the usefulness of the above assert... AFAICT, the default

Jonathan seemed minorly concerned that a strange new architecture might have
IIO_DMA_MINALIGN is < 8 some day, so I threw it in there. But agree, it seems
highly unlikely to actually happen.

> alignment is 8 bytes and I could not find any arch defining ARCH_DMA_MINALIGN
> smaller than that (would be very odd to have a cacheline smaller than that these
> days). For bigger values, nowadays they are all power of 2 and I would be
> surprised otherwise. But the more important question to me is what if the above
> assert fails? Will we not allow IIO or some drivers to be used in that
> architecture? It can become a very "painful" situation (assuming these macros
> get widely used). So, IMHO, either we assume the above can happen and rework the
> macros to make it work for that hypotetical case or we assume the above is
> always true and drop the assert. TBH, I think it would be a fair assumption...
> 
> On top of that the assertion is wrong:
> 
> sizeof(IIO_DMA_MINALIGN) != IIO_DMA_MINALIGN :)

Doh!

> 
> On the other hand, as I mentioned in V1, I think that an assertion or
> BUILD_BUG_ON_MSG for making sure 'count' is a compile time constant expression
> would be helpful. Sure, we'll get -Wvla but some developers might still ignore
> the warning and send patches with these arrays. So, it would be neater if we
> fail to build and force them to fix their code.

BUILD_BUG_ON_MSG() won't work because it expands to a do/while loop which won't
work in static struct declarations. But I can try to see if we can come up with
something that works.

Powered by blists - more mailing lists