[PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[David Lechner]

The main point of this series is to fix a regression reported in
hid-sensor-rotation where the alignment of the quaternion field in the
data was inadvertently changed from 16 bytes to 8 bytes. This is an
unusually case (one of only 2 in the kernel) where the .repeat field of
struct iio_scan_type is used and we have such a requirement. (The other
case uses u16 instead of u32, so it wasn't affected.)

To make the reason for the alignment more explicit to future readers,
we introduce a new macro, IIO_DECLARE_QUATERNION(), to declare the
array with proper alignment. This is meant to follow the pattern of
the similar IIO_DECLARE_BUFFER_WITH_TS() macro.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
Changes in v2:
- Rename IIO_DECLARE_REPEATED_ELEMENT() to IIO_DECLARE_QUATERNION().
- Link to v1: https://lore.kernel.org/r/20260214-iio-fix-repeat-alignment-v1-0-47f01288c803@baylibre.com

---
David Lechner (2):
      iio: add IIO_DECLARE_QUATERNION() macro
      iio: orientation: hid-sensor-rotation: fix quaternion alignment

 drivers/iio/orientation/hid-sensor-rotation.c |  2 +-
 include/linux/iio/iio.h                       | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
---
base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
change-id: 20260214-iio-fix-repeat-alignment-575b2c009e25

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v2 1/2] iio: add IIO_DECLARE_QUATERNION() macro

[David Lechner]

Add a new IIO_DECLARE_QUATERNION() macro that is used to declare the
field in an IIO buffer struct that contains a quaternion vector.

Quaternions are currently the only IIO data type that uses the .repeat
feature of struct iio_scan_type. This has an implicit rule that the
element in the buffer must be aligned to the entire size of the repeated
element. This macro will make that requirement explicit. Since this is
the only user, we just call the macro IIO_DECLARE_QUATERNION() instead
of something more generic.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 include/linux/iio/iio.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index a9ecff191bd9..2c91b7659ce9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -931,6 +931,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
 #define IIO_DECLARE_DMA_BUFFER_WITH_TS(type, name, count) \
 	__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(IIO_DMA_MINALIGN)
 
+/**
+ * IIO_DECLARE_QUATERNION() - Declare a quaternion element
+ * @type: element type of the individual vectors
+ * @name: identifier name
+ *
+ * Quaternions are a vector composed of 4 elements (W, X, Y, Z). Use this macro
+ * to declare a quaternion element in a struct to ensure proper alignment in
+ * an IIO buffer.
+ */
+#define IIO_DECLARE_QUATERNION(type, name) \
+	type name[4] __aligned(sizeof(type) * 4)
+
 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.0

Re: [PATCH v2 1/2] iio: add IIO_DECLARE_QUATERNION() macro

[David Lechner]

On 2/28/26 2:02 PM, David Lechner wrote:
> Add a new IIO_DECLARE_QUATERNION() macro that is used to declare the
> field in an IIO buffer struct that contains a quaternion vector.
> 
> Quaternions are currently the only IIO data type that uses the .repeat
> feature of struct iio_scan_type. This has an implicit rule that the
> element in the buffer must be aligned to the entire size of the repeated
> element. This macro will make that requirement explicit. Since this is
> the only user, we just call the macro IIO_DECLARE_QUATERNION() instead
> of something more generic.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  include/linux/iio/iio.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index a9ecff191bd9..2c91b7659ce9 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -931,6 +931,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>  #define IIO_DECLARE_DMA_BUFFER_WITH_TS(type, name, count) \
>  	__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(IIO_DMA_MINALIGN)
>  
> +/**
> + * IIO_DECLARE_QUATERNION() - Declare a quaternion element
> + * @type: element type of the individual vectors
> + * @name: identifier name
> + *
> + * Quaternions are a vector composed of 4 elements (W, X, Y, Z). Use this macro
> + * to declare a quaternion element in a struct to ensure proper alignment in
> + * an IIO buffer.
> + */
> +#define IIO_DECLARE_QUATERNION(type, name) \
> +	type name[4] __aligned(sizeof(type) * 4)
> +
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
>  /* The information at the returned address is guaranteed to be cacheline aligned */
> 

Hi Francesco,

I should have cc'ed you on this one. We'll want to add another macro
like this for IIO_DECLARE_QUATERNION_AXIS(), I imagine. (This has been
applied to the fixes-togreg branch since it is a dependency to a fix.)

What to do on that for the alignment though is an open question since
we've never had a repeat of 3 before. The question is if it is OK to
have an alignment that isn't a power of two bytes. For your case, since
the data is 3 x 16-bit, it would be 3 * 2 = 6 bytes.

Re: [PATCH v2 1/2] iio: add IIO_DECLARE_QUATERNION() macro

[Francesco Lavra]

On Sat, 2026-03-07 at 18:35 -0600, David Lechner wrote:
> On 2/28/26 2:02 PM, David Lechner wrote:
> > Add a new IIO_DECLARE_QUATERNION() macro that is used to declare the
> > field in an IIO buffer struct that contains a quaternion vector.
> > 
> > Quaternions are currently the only IIO data type that uses the .repeat
> > feature of struct iio_scan_type. This has an implicit rule that the
> > element in the buffer must be aligned to the entire size of the
> > repeated
> > element. This macro will make that requirement explicit. Since this is
> > the only user, we just call the macro IIO_DECLARE_QUATERNION() instead
> > of something more generic.

...


> Hi Francesco,
> 
> I should have cc'ed you on this one. We'll want to add another macro
> like this for IIO_DECLARE_QUATERNION_AXIS(), I imagine. (This has been
> applied to the fixes-togreg branch since it is a dependency to a fix.)
> 
> What to do on that for the alignment though is an open question since
> we've never had a repeat of 3 before. The question is if it is OK to
> have an alignment that isn't a power of two bytes. For your case, since
> the data is 3 x 16-bit, it would be 3 * 2 = 6 bytes.

Hi David. Differently from the hid-sensor driver (where the `scan` field in
struct dev_rot_state is used exclusively for quaternion data), the lsm6dsx
driver uses a data structure (see the `iio_buff` variable in
st_lsm6dsx_buffer.c:st_lsm6dsx_read_tagged_fifo()) that is shared between
all the different data types that can be read from the hardware FIFO
(accel, gyro, quaternion, external sensor data), so changing this structure
to a quaternion-specific type is not ideal. So I think for the time being
there wouldn't be any users of an IIO_DECLARE_QUATERNION_AXIS() macro.

As for the alignment, according to your patch at [1], when the repeat
number is not a power of two, it is (will be) rounded up to the next power
of two (and this is consistent with what the lsm6dsx driver expects), so
the alignment will be 8 bytes.

[1]
https://lore.kernel.org/linux-iio/20260307-iio-fix-timestamp-alignment-v2-4-d1d48fbadbbf@baylibre.com/

Re: [PATCH v2 1/2] iio: add IIO_DECLARE_QUATERNION() macro

[David Lechner]

On 3/16/26 2:03 PM, Francesco Lavra wrote:
> On Sat, 2026-03-07 at 18:35 -0600, David Lechner wrote:
>> On 2/28/26 2:02 PM, David Lechner wrote:
>>> Add a new IIO_DECLARE_QUATERNION() macro that is used to declare the
>>> field in an IIO buffer struct that contains a quaternion vector.
>>>
>>> Quaternions are currently the only IIO data type that uses the .repeat
>>> feature of struct iio_scan_type. This has an implicit rule that the
>>> element in the buffer must be aligned to the entire size of the
>>> repeated
>>> element. This macro will make that requirement explicit. Since this is
>>> the only user, we just call the macro IIO_DECLARE_QUATERNION() instead
>>> of something more generic.
> 
> ...
> 
> 
>> Hi Francesco,
>>
>> I should have cc'ed you on this one. We'll want to add another macro
>> like this for IIO_DECLARE_QUATERNION_AXIS(), I imagine. (This has been
>> applied to the fixes-togreg branch since it is a dependency to a fix.)
>>
>> What to do on that for the alignment though is an open question since
>> we've never had a repeat of 3 before. The question is if it is OK to
>> have an alignment that isn't a power of two bytes. For your case, since
>> the data is 3 x 16-bit, it would be 3 * 2 = 6 bytes.
> 
> Hi David. Differently from the hid-sensor driver (where the `scan` field in
> struct dev_rot_state is used exclusively for quaternion data), the lsm6dsx
> driver uses a data structure (see the `iio_buff` variable in
> st_lsm6dsx_buffer.c:st_lsm6dsx_read_tagged_fifo()) that is shared between
> all the different data types that can be read from the hardware FIFO
> (accel, gyro, quaternion, external sensor data), so changing this structure
> to a quaternion-specific type is not ideal. So I think for the time being
> there wouldn't be any users of an IIO_DECLARE_QUATERNION_AXIS() macro.

Makes sense.

> 
> As for the alignment, according to your patch at [1], when the repeat
> number is not a power of two, it is (will be) rounded up to the next power
> of two (and this is consistent with what the lsm6dsx driver expects), so
> the alignment will be 8 bytes.

I think you are referring to the 8-byte alignment for the timestamp?

Patch [1] should make a difference when the timestamp is not enabled
in a buffered read though.

When the timestamp is enabled, the buffer is going to be 16 bytes per
sample no matter what because of the 8-byte alignment of the timestamp.

But if the timestamp is not enabled, then for 16-bit storagebits and
repeat of 3, before the patch, the buffer would only be 6 bytes per
sample, but after the patch would be 8 bytes per sample. This doesn't
make a difference in the driver itself, but does make a difference to
userspace that is reading the buffer.

> 
> [1]
> https://lore.kernel.org/linux-iio/20260307-iio-fix-timestamp-alignment-v2-4-d1d48fbadbbf@baylibre.com/

Re: [PATCH v2 1/2] iio: add IIO_DECLARE_QUATERNION() macro

[Francesco Lavra]

On Mon, 2026-03-16 at 14:56 -0500, David Lechner wrote:
...
> > As for the alignment, according to your patch at [1], when the repeat
> > number is not a power of two, it is (will be) rounded up to the next
> > power
> > of two (and this is consistent with what the lsm6dsx driver expects),
> > so
> > the alignment will be 8 bytes.
> 
> I think you are referring to the 8-byte alignment for the timestamp?
> 
> Patch [1] should make a difference when the timestamp is not enabled
> in a buffered read though.
> 
> When the timestamp is enabled, the buffer is going to be 16 bytes per
> sample no matter what because of the 8-byte alignment of the timestamp.
> 
> But if the timestamp is not enabled, then for 16-bit storagebits and
> repeat of 3, before the patch, the buffer would only be 6 bytes per
> sample, but after the patch would be 8 bytes per sample. This doesn't
> make a difference in the driver itself, but does make a difference to
> userspace that is reading the buffer.

I was referring to alignment in general, regardless of the timestamp being
enabled. From my understanding, the computed storage size is also used for
alignment. Anyway, yes, after [1] the storage size for the quaternion scan
element will be 8 bytes, with the last 2 bytes of each sample zeroed out.

> 
> > [1]
> > https://lore.kernel.org/linux-iio/20260307-iio-fix-timestamp-alignment-v2-4-d1d48fbadbbf@baylibre.com/

[PATCH v2 2/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[David Lechner]

Restore the alignment of sampled_vals to 16 bytes by using
IIO_DECLARE_QUATERNION(). This field contains a quaternion value which
has scan_type.repeat = 4 and storagebits = 32. So the alignment must
be 16 bytes to match the assumptions of iio_storage_bytes_for_si() and
also to not break userspace.

Reported-by: Lixu Zhang <lixu.zhang@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221077
Fixes: b31a74075cb4 ("iio: orientation: hid-sensor-rotation: remove unnecessary alignment")
Tested-by: Lixu Zhang <lixu.zhang@intel.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/orientation/hid-sensor-rotation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
index e759f91a710a..6806481873be 100644
--- a/drivers/iio/orientation/hid-sensor-rotation.c
+++ b/drivers/iio/orientation/hid-sensor-rotation.c
@@ -19,7 +19,7 @@ struct dev_rot_state {
 	struct hid_sensor_common common_attributes;
 	struct hid_sensor_hub_attribute_info quaternion;
 	struct {
-		s32 sampled_vals[4];
+		IIO_DECLARE_QUATERNION(s32, sampled_vals);
 		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;

-- 
2.43.0

Re: [PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[Andy Shevchenko]

On Sat, Feb 28, 2026 at 02:02:21PM -0600, David Lechner wrote:
> The main point of this series is to fix a regression reported in
> hid-sensor-rotation where the alignment of the quaternion field in the
> data was inadvertently changed from 16 bytes to 8 bytes. This is an
> unusually case (one of only 2 in the kernel) where the .repeat field of
> struct iio_scan_type is used and we have such a requirement. (The other
> case uses u16 instead of u32, so it wasn't affected.)
> 
> To make the reason for the alignment more explicit to future readers,
> we introduce a new macro, IIO_DECLARE_QUATERNION(), to declare the
> array with proper alignment. This is meant to follow the pattern of
> the similar IIO_DECLARE_BUFFER_WITH_TS() macro.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

But this is in conflict with the other hack-patch in another series.
I'm a bit lost what patch 2 fixes here and that hack-patch fixes
in the same driver. Shouldn't survive only one?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[Nuno Sá]

On Mon, 2026-03-02 at 10:58 +0200, Andy Shevchenko wrote:
> On Sat, Feb 28, 2026 at 02:02:21PM -0600, David Lechner wrote:
> > The main point of this series is to fix a regression reported in
> > hid-sensor-rotation where the alignment of the quaternion field in the
> > data was inadvertently changed from 16 bytes to 8 bytes. This is an
> > unusually case (one of only 2 in the kernel) where the .repeat field of
> > struct iio_scan_type is used and we have such a requirement. (The other
> > case uses u16 instead of u32, so it wasn't affected.)
> > 
> > To make the reason for the alignment more explicit to future readers,
> > we introduce a new macro, IIO_DECLARE_QUATERNION(), to declare the
> > array with proper alignment. This is meant to follow the pattern of
> > the similar IIO_DECLARE_BUFFER_WITH_TS() macro.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> But this is in conflict with the other hack-patch in another series.
> I'm a bit lost what patch 2 fixes here and that hack-patch fixes
> in the same driver. Shouldn't survive only one?

+1.

I see this is older so should we only look at the other series?

- Nuno Sá

Re: [PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[David Lechner]

On 3/2/26 6:32 AM, Nuno Sá wrote:
> On Mon, 2026-03-02 at 10:58 +0200, Andy Shevchenko wrote:
>> On Sat, Feb 28, 2026 at 02:02:21PM -0600, David Lechner wrote:
>>> The main point of this series is to fix a regression reported in
>>> hid-sensor-rotation where the alignment of the quaternion field in the
>>> data was inadvertently changed from 16 bytes to 8 bytes. This is an
>>> unusually case (one of only 2 in the kernel) where the .repeat field of
>>> struct iio_scan_type is used and we have such a requirement. (The other
>>> case uses u16 instead of u32, so it wasn't affected.)
>>>
>>> To make the reason for the alignment more explicit to future readers,
>>> we introduce a new macro, IIO_DECLARE_QUATERNION(), to declare the
>>> array with proper alignment. This is meant to follow the pattern of
>>> the similar IIO_DECLARE_BUFFER_WITH_TS() macro.
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>>
>> But this is in conflict with the other hack-patch in another series.
>> I'm a bit lost what patch 2 fixes here and that hack-patch fixes
>> in the same driver. Shouldn't survive only one?
> 
> +1.
> 
> I see this is older so should we only look at the other series?
> 
> - Nuno Sá

They are fixing two separate bugs.

This bug (which is a recent regression) is that the size of the struct is
wrong. It is supposed to be 32 bytes, but a recent change make it only 24.
This causes iio_push_to_buffers_with_ts() to write over data past the end
of the struct.

The other bug (which has been a bug for 6 years) is that the the timestamp
is in the wrong place. That patch also has an effect of making the struct
the right size, but that is only a side-effect.

So yes, I should have mentioned that in the cover letter that this series
should probably be applied first since it is the worse bug. And it looks
like I'll be doing a v2 of the other series anyway, so I can properly rebase
it and declare the dependency.

Re: [PATCH v2 0/2] iio: orientation: hid-sensor-rotation: fix quaternion alignment

[Jonathan Cameron]

On Mon, 2 Mar 2026 09:52:58 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 3/2/26 6:32 AM, Nuno Sá wrote:
> > On Mon, 2026-03-02 at 10:58 +0200, Andy Shevchenko wrote:  
> >> On Sat, Feb 28, 2026 at 02:02:21PM -0600, David Lechner wrote:  
> >>> The main point of this series is to fix a regression reported in
> >>> hid-sensor-rotation where the alignment of the quaternion field in the
> >>> data was inadvertently changed from 16 bytes to 8 bytes. This is an
> >>> unusually case (one of only 2 in the kernel) where the .repeat field of
> >>> struct iio_scan_type is used and we have such a requirement. (The other
> >>> case uses u16 instead of u32, so it wasn't affected.)
> >>>
> >>> To make the reason for the alignment more explicit to future readers,
> >>> we introduce a new macro, IIO_DECLARE_QUATERNION(), to declare the
> >>> array with proper alignment. This is meant to follow the pattern of
> >>> the similar IIO_DECLARE_BUFFER_WITH_TS() macro.  
> >>
> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> >>
> >> But this is in conflict with the other hack-patch in another series.
> >> I'm a bit lost what patch 2 fixes here and that hack-patch fixes
> >> in the same driver. Shouldn't survive only one?  
> > 
> > +1.
> > 
> > I see this is older so should we only look at the other series?
> > 
> > - Nuno Sá  
> 
> They are fixing two separate bugs.
> 
> This bug (which is a recent regression) is that the size of the struct is
> wrong. It is supposed to be 32 bytes, but a recent change make it only 24.
> This causes iio_push_to_buffers_with_ts() to write over data past the end
> of the struct.
> 
> The other bug (which has been a bug for 6 years) is that the the timestamp
> is in the wrong place. That patch also has an effect of making the struct
> the right size, but that is only a side-effect.
> 
> So yes, I should have mentioned that in the cover letter that this series
> should probably be applied first since it is the worse bug. And it looks
> like I'll be doing a v2 of the other series anyway, so I can properly rebase
> it and declare the dependency.
> 
> 
I've applied these two to the fixes-togreg branch with them marked for stable
inclusion.  For the first one I added a comment to the tags block to say
it's stable as a precusor for the second.

Thanks,

Jonathan