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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250426121921.24c89212@jic23-huawei>
Date: Sat, 26 Apr 2025 12:19:21 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Nuno Sá <noname.nuno@...il.com>, 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>,
 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 Wed, 23 Apr 2025 09:51:25 -0500
David Lechner <dlechner@...libre.com> wrote:

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

Yeah, it's unlikely.  Architectures using small sizes is not about cacheline
length any more but rather than they guarantee that the system will work fine irrespective
of the cacheline length.  (e.g. x86_64 where the min align has been 8 for a long
time - possibly always? and cachelines are generally 64 bytes)  It seems very unlikely
anyone will care about smaller than that so such a macro is really just
paranoia!

The ARCH_DMA_MINALIGN fallback is sizeof(unsigned long long).

Basically I want the assert so I don't have to pay attention to weird new architectures.
I'm not that fussed though if it is hard to do for some reason.

Jonathan