linux-kernel - Re: [PATCH v2 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <701bfc6a715046044dbc789f1c11c7f85395c7a8.camel@gmail.com>
Date: Wed, 23 Apr 2025 10:18:55 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.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

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
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 :)

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.

- Nuno Sá