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@baylibre.com>
---
v4 changes:
* Drop the static_asserts(). Some 32-bit arches were triggering one, so
we had to address the problem instead of hoping that it didn't exist.
The other made a multi-statement macro, which isn't the best practice
and didn't have a way to make a really helpful error message. The
condition we were trying to catch is still caught by -Wvla.
* Changed __align(IIO_DMA_MINALIGN) to __align(MAX(IIO_DMA_MINALIGN,
sizeof(s64))). As the build-bots found, there are some 32-bit arches
where IIO_DMA_MINALIGN is 4, but we still need 8-byte alignment for
the timestamp.
v3 changes:
* Use leading double-underscore for "private" macro to match "private"
functions that do the same.
* Use static_assert() from linux/build_bug.h instead of _Static_assert()
* Fix incorrectly using sizeof(IIO_DMA_MINALIGN).
* Add check that count argument is constant. (Note, I didn't include a
message in this static assert because it already gives a reasonable
message.)
/home/david/work/bl/linux/drivers/iio/accel/sca3300.c:482:51: error: expression in static assertion is not constant
482 | IIO_DECLARE_BUFFER_WITH_TS(s16, channels, val);
| ^~~
v2 changes:
* Add 2nd macro for DMA alignment
---
include/linux/iio/iio.h | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..95d90740b4ffb3ebd819d03ad2bd05ecf40d4384 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -7,9 +7,12 @@
#ifndef _INDUSTRIAL_IO_H_
#define _INDUSTRIAL_IO_H_
+#include <linux/align.h>
+#include <linux/build_bug.h>
#include <linux/device.h>
#include <linux/cdev.h>
#include <linux/compiler_types.h>
+#include <linux/minmax.h>
#include <linux/slab.h>
#include <linux/iio/types.h>
/* IIO TODO LIST */
@@ -777,6 +780,40 @@ 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) \
+ /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \
+ __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
+
struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
/* The information at the returned address is guaranteed to be cacheline aligned */
--
2.43.0On 4/28/25 3:23 PM, 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@baylibre.com> > --- ... > +/** > + * 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) \ > + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ > + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) I just realized my logic behind this is faulty. It assumes sizeof(s64) == __alignof__(s64), but that isn't always true and that is what caused the builds to hit the static_assert() on v3. We should be able to leave this as __aligned(IIO_DMA_MINALIGN) And have this (with better error message): static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);
On 4/28/25 9:12 PM, David Lechner wrote: > On 4/28/25 3:23 PM, 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@baylibre.com> >> --- > > ... > >> +/** >> + * 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) \ >> + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ >> + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) > > I just realized my logic behind this is faulty. It assumes sizeof(s64) == > __alignof__(s64), but that isn't always true and that is what caused the builds > to hit the static_assert() on v3. > > We should be able to leave this as __aligned(IIO_DMA_MINALIGN) > > And have this (with better error message): > > static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0); I was working late yesterday and should have saved that reply until morning to think about it more! We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that aligned_s64 exists and always uses 8-byte alignment). So I think this patch is correct as-is after all.
On Tue, 2025-04-29 at 14:31 -0500, David Lechner wrote: > On 4/28/25 9:12 PM, David Lechner wrote: > > On 4/28/25 3:23 PM, 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@baylibre.com> > > > --- > > > > ... > > > > > +/** > > > + * 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) \ > > > + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ > > > + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) > > > > I just realized my logic behind this is faulty. It assumes sizeof(s64) == > > __alignof__(s64), but that isn't always true and that is what caused the > > builds > > to hit the static_assert() on v3. > > > > We should be able to leave this as __aligned(IIO_DMA_MINALIGN) > > > > And have this (with better error message): > > > > static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0); > > I was working late yesterday and should have saved that reply until morning > to think about it more! > > We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid > issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that > aligned_s64 exists and always uses 8-byte alignment). What issues could we have? In your inner macros I think you still make sure we pad everything up to sizeof(s64) right? Am I missing any subtle issue? but... > > So I think this patch is correct as-is after all. FWIW, I do prefer this approach or what Andy suggest (min as sizeof(s64)). - Nuno Sá
On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote: > On 4/28/25 9:12 PM, David Lechner wrote: > > On 4/28/25 3:23 PM, 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(). ... > >> +/** > >> + * 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) \ > >> + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ > >> + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) > > > > I just realized my logic behind this is faulty. It assumes sizeof(s64) == > > __alignof__(s64), but that isn't always true and that is what caused the builds > > to hit the static_assert() on v3. > > > > We should be able to leave this as __aligned(IIO_DMA_MINALIGN) > > > > And have this (with better error message): > > > > static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0); > > I was working late yesterday and should have saved that reply until morning > to think about it more! > > We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid > issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that > aligned_s64 exists and always uses 8-byte alignment). > > So I think this patch is correct as-is after all. I'm wondering, shouldn't it be better just to make sure that IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)? -- With Best Regards, Andy Shevchenko
On 4/29/25 2:36 PM, Andy Shevchenko wrote: > On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote: >> On 4/28/25 9:12 PM, David Lechner wrote: >>> On 4/28/25 3:23 PM, 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(). > > ... > >>>> +/** >>>> + * 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) \ >>>> + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ >>>> + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) >>> >>> I just realized my logic behind this is faulty. It assumes sizeof(s64) == >>> __alignof__(s64), but that isn't always true and that is what caused the builds >>> to hit the static_assert() on v3. >>> >>> We should be able to leave this as __aligned(IIO_DMA_MINALIGN) >>> >>> And have this (with better error message): >>> >>> static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0); >> >> I was working late yesterday and should have saved that reply until morning >> to think about it more! >> >> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid >> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that >> aligned_s64 exists and always uses 8-byte alignment). >> >> So I think this patch is correct as-is after all. > > I'm wondering, shouldn't it be better just to make sure that > IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)? > Sounds reasonable to me. From what I have seen while working on this is that there are quite a few drivers using IIO_DMA_MINALIGN expecting it to be sufficient for timestamp alignment, which as it seems is not always the case. I'll wait for Jonathan to weigh in though before spinning up a new patch.
On Tue, 29 Apr 2025 14:47:41 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 4/29/25 2:36 PM, Andy Shevchenko wrote: > > On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote: > >> On 4/28/25 9:12 PM, David Lechner wrote: > >>> On 4/28/25 3:23 PM, 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(). > > > > ... > > > >>>> +/** > >>>> + * 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) \ > >>>> + /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */ \ > >>>> + __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64))) > >>> > >>> I just realized my logic behind this is faulty. It assumes sizeof(s64) == > >>> __alignof__(s64), but that isn't always true and that is what caused the builds > >>> to hit the static_assert() on v3. > >>> > >>> We should be able to leave this as __aligned(IIO_DMA_MINALIGN) > >>> > >>> And have this (with better error message): > >>> > >>> static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0); > >> > >> I was working late yesterday and should have saved that reply until morning > >> to think about it more! > >> > >> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid > >> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that > >> aligned_s64 exists and always uses 8-byte alignment). > >> > >> So I think this patch is correct as-is after all. > > > > I'm wondering, shouldn't it be better just to make sure that > > IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)? > > > > Sounds reasonable to me. From what I have seen while working on this is that > there are quite a few drivers using IIO_DMA_MINALIGN expecting it to be > sufficient for timestamp alignment, which as it seems is not always the case. > > I'll wait for Jonathan to weigh in though before spinning up a new patch. > It would be very odd if we ever see a platform with a DMA alignment requirement that is not a multiple of sizeof(s64) just forcing IIO_DMA_MINALIGN to max of the arch constraint and sizeof(s64) seems fine to me and fixes up all those other drivers that were assuming this was true already... I thought it was and only got fussy for this macro :( Jonathan
© 2016 - 2026 Red Hat, Inc.