[PATCH 0/4] iio: buffer: fix timestamp alignment (in rare case)

[PATCH 0/4] iio: buffer: fix timestamp alignment (in rare case)

[David Lechner]

In [1], it was pointed out that the iio_push_to_buffers_with_timestamp()
function is not putting the timestamp at the correct offset in the scan
buffer in rare cases where the largest scan element size is larger than
sizeof(int64_t).

[1]: https://lore.kernel.org/linux-iio/20260215162351.79f40b32@jic23-huawei/

This only affected one driver, namely hid-sensor-rotation since it is
the only driver that meets the condition. To fix things up, first we
fix the hid-sensor-rotation driver in a way that preserves compatibility
with the broken timestamp alignment. Then we are free to fix the core
IIO code without affecting any users.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
David Lechner (4):
      iio: orientation: hid-sensor-rotation: add timestamp hack to not break userspace
      iio: buffer: check return value of iio_compute_scan_bytes()
      iio: buffer: cache largest scan element size
      iio: buffer: fix timestamp alignment when quaternion in scan

 drivers/iio/industrialio-buffer.c             | 44 ++++++++++++++++++++-------
 drivers/iio/orientation/hid-sensor-rotation.c | 20 ++++++++++--
 include/linux/iio/buffer.h                    | 12 ++++++--
 include/linux/iio/iio.h                       |  3 ++
 4 files changed, 63 insertions(+), 16 deletions(-)
---
base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
change-id: 20260228-iio-fix-timestamp-alignment-89ade1af458b

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

[PATCH 1/4] iio: orientation: hid-sensor-rotation: add timestamp hack to not break userspace

[David Lechner]

Add a hack to push two timestamps in the hid-sensor-rotation scan data
to avoid breaking userspace applications that depend on the timestamp
being at the incorrect location in the scan data due to unintentional
misalignment in older kernels.

When this driver was written, the timestamp was in the correct location
because of the way iio_compute_scan_bytes() was implemented at the time.
(Samples were 24 bytes each.) Then commit 883f61653069 ("iio: buffer:
align the size of scan bytes to size of the largest element") changed
the computed scan_bytes to be a different size (32 bytes), which caused
iio_push_to_buffers_with_timestamp() to place the timestamp at an
incorrect offset.

There have been long periods of time (6 years each) where the timestamp
was in either location, so to not break either case, we open-code the
timestamps to be pushed to both locations in the scan data.

Reported-by: Jonathan Cameron <jic23@kernel.org>
Closes: https://lore.kernel.org/linux-iio/20260215162351.79f40b32@jic23-huawei/
Fixes: 883f61653069 ("iio: buffer: align the size of scan bytes to size of the largest element")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---

I found that I could emulate this thanks to /dev/uhid. And thanks to AI
code generators, I was able to reasonably quickly make a script that
worked for emulating "HID-SENSOR-20008a". I'll include the script at the
end.

I set up the buffer like this:

cd /sys/bus/iio/devices/iio:device1/buffer0
echo 1 > in_rot_quaternion_en
echo 1 > in_timestamp_en
echo 1 > enable

Before this series is applied, we can see that the timestamp (group of 8
ending in "98 18") is at offset of 24 in the 32-byte data.

hd /dev/iio\:device1

00000000  6a 18 00 00 ac f3 ff ff  83 2d 00 00 02 d3 ff ff  |j........-......|
00000010  00 00 00 00 00 00 00 00  5a 17 a0 2a 73 cb 98 18  |........Z..*s...|

00000020  ad 17 00 00 6a f4 ff ff  35 2b 00 00 ca d0 ff ff  |....j...5+......|
00000030  00 00 00 00 00 00 00 00  2a a6 bb 30 73 cb 98 18  |........*..0s...|

00000040  92 1e 00 00 50 ec ff ff  ea c1 ff ff 78 f0 ff ff  |....P.......x...|
00000050  00 00 00 00 00 00 00 00  8f 3b a7 39 77 cb 98 18  |.........;.9w...|

After the first patch, we can see that the timestamp is now repeated at
both the correct and previous incorrect offsets (24 and 32). (Normally,
the last 8 bytes would be all 00 for padding.)

00000000  dd e0 ff ff 0e e0 ff ff  75 07 00 00 90 3f 00 00  |........u....?..|
00000010  f4 38 82 d0 3a cc 98 18  f4 38 82 d0 3a cc 98 18  |.8..:....8..:...|

00000020  a0 e0 ff ff 1d e0 ff ff  a0 0a 00 00 1c 3f 00 00  |.............?..|
00000030  3a 29 9f d6 3a cc 98 18  3a 29 9f d6 3a cc 98 18  |:)..:...:)..:...|

00000040  a9 e1 ff ff 1e 14 00 00  6c c1 ff ff 98 f2 ff ff  |........l.......|
00000050  39 21 77 11 55 cc 98 18  39 21 77 11 55 cc 98 18  |9!w.U...9!w.U...|

Test script:

import math
import os
import struct
import time

UHID_DESTROY = 1
UHID_CREATE2 = 11
UHID_INPUT2 = 12

UHID_DATA_MAX = 4096
BUS_USB = 0x03

class UHIDRotationSensor:
    def __init__(self, device_path: str = "/dev/uhid") -> None:
        """Initialize the virtual UHID rotation sensor.

        Args:
            device_path: Path to the UHID character device.
        """
        self.device_path = device_path
        self.fd = -1

    def open_device(self) -> None:
        """Open the UHID device."""
        self.fd = os.open(self.device_path, os.O_RDWR)

    def close_device(self) -> None:
        """Close the UHID device."""
        if self.fd >= 0:
            os.close(self.fd)
            self.fd = -1

    def create_device(self) -> None:
        """Create and register a virtual HID rotation sensor."""
        # HID descriptor for Sensor Device Orientation (HID-SENSOR-20008a)
        report_desc = bytes(
            [
                0x05,
                0x20,  # Usage Page: Sensor
                0x09,
                0x8A,  # Usage: Device Orientation (0x20008A)
                0xA1,
                0x01,  # Collection: Application

                # Input report (Report ID 1): quaternion x, y, z, w (s16)
                0x85,
                0x01,  # Report ID 1
                0x0A,
                0x83,
                0x04,  # Usage: Orientation Quaternion (0x200483)
                0x16,
                0x00,
                0x80,  # Logical Minimum: -32768
                0x26,
                0xFF,
                0x7F,  # Logical Maximum: 32767
                0x75,
                0x10,  # Report Size: 16
                0x95,
                0x04,  # Report Count: 4
                0x81,
                0x02,  # Input: Data, Variable, Absolute

                # Feature report (Report ID 2): report state, power state,
                # and report interval
                0x85,
                0x02,  # Report ID 2
                0x0A,
                0x16,
                0x03,  # Usage: Property Report State (0x200316)
                0x15,
                0x01,  # Logical Minimum: 1
                0x25,
                0x05,  # Logical Maximum: 5
                0x75,
                0x08,
                0x95,
                0x01,
                0xB1,
                0x02,  # Feature: Data, Variable, Absolute

                0x0A,
                0x19,
                0x03,  # Usage: Property Power State (0x200319)
                0x15,
                0x01,
                0x25,
                0x05,
                0x75,
                0x08,
                0x95,
                0x01,
                0xB1,
                0x02,  # Feature: Data, Variable, Absolute

                0x0A,
                0x0E,
                0x03,  # Usage: Property Report Interval (0x20030E)
                0x15,
                0x00,  # Logical Minimum: 0
                0x27,
                0xFF,
                0xFF,
                0xFF,
                0x7F,  # Logical Maximum: 2147483647
                0x75,
                0x20,  # Report Size: 32
                0x95,
                0x01,  # Report Count: 1
                0xB1,
                0x02,  # Feature: Data, Variable, Absolute

                0xC0,  # End Collection
            ]
        )

        # Build struct uhid_create2_req payload
        create_req_header = struct.pack(
            "<128s64s64sHHIIII",
            b"HID-SENSOR-20008a",  # name
            b"AD Inc",  # phys
            b"",  # uniq
            len(report_desc),  # rd_size
            BUS_USB,  # bus
            0x0001,  # vendor
            0x0001,  # product
            0,  # version
            0,  # country
        )

        create_req = (
            create_req_header
            + report_desc
            + b"\x00" * (UHID_DATA_MAX - len(report_desc))
        )
        event = struct.pack("<I", UHID_CREATE2) + create_req

        if self.fd < 0:
            raise RuntimeError("UHID device is not open")

        os.write(self.fd, event)

    def send_rotation_data(self, qx: int, qy: int, qz: int, qw: int) -> None:
        """Send one quaternion sample report.

        Args:
            qx: Quaternion X component (signed 16-bit).
            qy: Quaternion Y component (signed 16-bit).
            qz: Quaternion Z component (signed 16-bit).
            qw: Quaternion W component (signed 16-bit).
        """
        report_id = 1
        report = struct.pack(
            "<Bhhhh",
            report_id,
            max(-32768, min(32767, qx)),
            max(-32768, min(32767, qy)),
            max(-32768, min(32767, qz)),
            max(-32768, min(32767, qw)),
        )

        input_req = (
            struct.pack("<H", len(report))
            + report
            + b"\x00" * (UHID_DATA_MAX - len(report))
        )
        event = struct.pack("<I", UHID_INPUT2) + input_req

        if self.fd < 0:
            raise RuntimeError("UHID device is not open")

        os.write(self.fd, event)

    def run(self) -> None:
        """Run the virtual sensor simulation loop."""
        try:
            self.open_device()
            self.create_device()
            time.sleep(0.5)

            angle = 0.0
            while True:
                # Simulate changing orientation in quaternion form.
                half_angle = angle * 0.5
                qx = int(8192 * math.sin(half_angle * 0.7))
                qy = int(8192 * math.cos(half_angle * 0.5))
                qz = int(16384 * math.sin(half_angle))
                qw = int(16384 * math.cos(half_angle))

                self.send_rotation_data(qx, qy, qz, qw)
                angle += 0.1
                time.sleep(0.1)

        except KeyboardInterrupt:
            print("\nStopping...")
        finally:
            self.close_device()

if __name__ == "__main__":
    sensor = UHIDRotationSensor()
    sensor.run()
---
 drivers/iio/orientation/hid-sensor-rotation.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
index e759f91a710a..a8bce5151dcb 100644
--- a/drivers/iio/orientation/hid-sensor-rotation.c
+++ b/drivers/iio/orientation/hid-sensor-rotation.c
@@ -20,7 +20,11 @@ struct dev_rot_state {
 	struct hid_sensor_hub_attribute_info quaternion;
 	struct {
 		s32 sampled_vals[4];
-		aligned_s64 timestamp;
+		/*
+		 * HACK: There are two copies of the same timestamp in case of
+		 * userspace depending on broken alignment from older kernels.
+		 */
+		aligned_s64 timestamp[2];
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
@@ -154,8 +158,18 @@ static int dev_rot_proc_event(struct hid_sensor_hub_device *hsdev,
 		if (!rot_state->timestamp)
 			rot_state->timestamp = iio_get_time_ns(indio_dev);
 
-		iio_push_to_buffers_with_timestamp(indio_dev, &rot_state->scan,
-						   rot_state->timestamp);
+		/*
+		 * HACK: IIO previously had an incorrect implementation of
+		 * iio_push_to_buffers_with_timestamp() that put the timestamp
+		 * in the last 8 bytes of the buffer, which was incorrect
+		 * according to the IIO ABI. To avoid breaking userspace that
+		 * depended on this broken behavior, we put the timestamp in
+		 * both the correct place and the old incorrect place.
+		 */
+		rot_state->scan.timestamp[0] = rot_state->timestamp;
+		rot_state->scan.timestamp[1] = rot_state->timestamp;
+
+		iio_push_to_buffers(indio_dev, &rot_state->scan);
 
 		rot_state->timestamp = 0;
 	}

-- 
2.43.0

[PATCH 2/4] iio: buffer: check return value of iio_compute_scan_bytes()

[David Lechner]

Check return value of iio_compute_scan_bytes() as it can return an
error.

The result is moved to an output parameter while we are touching this
as we will need to add a second output parameter in a later change.

The return type of iio_buffer_update_bytes_per_datum() also had to be
changed to propagate the error.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/industrialio-buffer.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 46f36a6ed271..71dfc81cb9e5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -764,7 +764,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
 }
 
 static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
-				  const unsigned long *mask, bool timestamp)
+				  const unsigned long *mask, bool timestamp,
+				  unsigned int *scan_bytes)
 {
 	unsigned int bytes = 0;
 	int length, i, largest = 0;
@@ -790,8 +791,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 		largest = max(largest, length);
 	}
 
-	bytes = ALIGN(bytes, largest);
-	return bytes;
+	*scan_bytes = ALIGN(bytes, largest);
+
+	return 0;
 }
 
 static void iio_buffer_activate(struct iio_dev *indio_dev,
@@ -836,18 +838,23 @@ static int iio_buffer_disable(struct iio_buffer *buffer,
 	return buffer->access->disable(buffer, indio_dev);
 }
 
-static void iio_buffer_update_bytes_per_datum(struct iio_dev *indio_dev,
-					      struct iio_buffer *buffer)
+static int iio_buffer_update_bytes_per_datum(struct iio_dev *indio_dev,
+					     struct iio_buffer *buffer)
 {
 	unsigned int bytes;
+	int ret;
 
 	if (!buffer->access->set_bytes_per_datum)
-		return;
+		return 0;
 
-	bytes = iio_compute_scan_bytes(indio_dev, buffer->scan_mask,
-				       buffer->scan_timestamp);
+	ret = iio_compute_scan_bytes(indio_dev, buffer->scan_mask,
+				     buffer->scan_timestamp, &bytes);
+	if (ret)
+		return ret;
 
 	buffer->access->set_bytes_per_datum(buffer, bytes);
+
+	return 0;
 }
 
 static int iio_buffer_request_update(struct iio_dev *indio_dev,
@@ -855,7 +862,10 @@ static int iio_buffer_request_update(struct iio_dev *indio_dev,
 {
 	int ret;
 
-	iio_buffer_update_bytes_per_datum(indio_dev, buffer);
+	ret = iio_buffer_update_bytes_per_datum(indio_dev, buffer);
+	if (ret)
+		return ret;
+
 	if (buffer->access->request_update) {
 		ret = buffer->access->request_update(buffer);
 		if (ret) {
@@ -898,6 +908,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	struct iio_buffer *buffer;
 	bool scan_timestamp;
 	unsigned int modes;
+	int ret;
 
 	if (insert_buffer &&
 	    bitmap_empty(insert_buffer->scan_mask, masklength)) {
@@ -985,8 +996,11 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 		scan_mask = compound_mask;
 	}
 
-	config->scan_bytes = iio_compute_scan_bytes(indio_dev,
-						    scan_mask, scan_timestamp);
+	ret = iio_compute_scan_bytes(indio_dev, scan_mask, scan_timestamp,
+				     &config->scan_bytes);
+	if (ret)
+		return ret;
+
 	config->scan_mask = scan_mask;
 	config->scan_timestamp = scan_timestamp;
 

-- 
2.43.0

[PATCH 3/4] iio: buffer: cache largest scan element size

[David Lechner]

Cache the largest scan element size of elements enabled in a scan
buffer. This will be used later to ensure proper alignment of the
timestamp element in the scan buffer.

The new field could not be placed in struct iio_dev_opaque because we
will need to access it in a static inline function later, so we make it
__private instead. It is only intended to be used by core IIO code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/industrialio-buffer.c | 14 +++++++++++---
 include/linux/iio/iio.h           |  3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 71dfc81cb9e5..83e9392f949f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -765,7 +765,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
 
 static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				  const unsigned long *mask, bool timestamp,
-				  unsigned int *scan_bytes)
+				  unsigned int *scan_bytes,
+				  unsigned int *largest_element_size)
 {
 	unsigned int bytes = 0;
 	int length, i, largest = 0;
@@ -793,6 +794,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 
 	*scan_bytes = ALIGN(bytes, largest);
 
+	if (largest_element_size)
+		*largest_element_size = largest;
+
 	return 0;
 }
 
@@ -848,7 +852,7 @@ static int iio_buffer_update_bytes_per_datum(struct iio_dev *indio_dev,
 		return 0;
 
 	ret = iio_compute_scan_bytes(indio_dev, buffer->scan_mask,
-				     buffer->scan_timestamp, &bytes);
+				     buffer->scan_timestamp, &bytes, NULL);
 	if (ret)
 		return ret;
 
@@ -892,6 +896,7 @@ struct iio_device_config {
 	unsigned int watermark;
 	const unsigned long *scan_mask;
 	unsigned int scan_bytes;
+	unsigned int largest_scan_element_size;
 	bool scan_timestamp;
 };
 
@@ -997,7 +1002,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	}
 
 	ret = iio_compute_scan_bytes(indio_dev, scan_mask, scan_timestamp,
-				     &config->scan_bytes);
+				     &config->scan_bytes,
+				     &config->largest_scan_element_size);
 	if (ret)
 		return ret;
 
@@ -1155,6 +1161,8 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	indio_dev->active_scan_mask = config->scan_mask;
 	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
 	indio_dev->scan_bytes = config->scan_bytes;
+	ACCESS_PRIVATE(indio_dev, largest_scan_element_size) =
+		config->largest_scan_element_size;
 	iio_dev_opaque->currentmode = config->mode;
 
 	iio_update_demux(indio_dev);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index a9ecff191bd9..85bcb5f8ae15 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -584,6 +584,8 @@ struct iio_buffer_setup_ops {
  *			and owner
  * @buffer:		[DRIVER] any buffer present
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
+ * @largest_scan_element_size: [INTERN] cache of the largest scan element size
+ *			       among the channels selected in the scan mask
  * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
  *			   array in order of preference, the most preferred
  *			   masks first.
@@ -610,6 +612,7 @@ struct iio_dev {
 
 	struct iio_buffer		*buffer;
 	int				scan_bytes;
+	unsigned int			__private largest_scan_element_size;
 
 	const unsigned long		*available_scan_masks;
 	unsigned int			__private masklength;

-- 
2.43.0

[PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[David Lechner]

Fix timestamp alignment when a scan buffer contains an element larger
than sizeof(int64_t). Currently s32 quaternions are the only such
element, and the one driver that has this (hid-sensor-rotation) has a
workaround in place already so this change does not affect it.

Previously, we assumed that the timestamp would always be 8-byte aligned
relative to the end of the scan buffer, but in the case of a scan buffer
a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
to be placed at offset 16, not 24.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

To test this, I used hid-sensor-rotation minus the first patch in the
series so that we can see that the timestamp actually moved to the
correct location.

Before this patch, the timestamp (8 bytes ending with "98 18") is in the
wrong location.

00000000  6a 18 00 00 ac f3 ff ff  83 2d 00 00 02 d3 ff ff  |j........-......|
00000010  00 00 00 00 00 00 00 00  5a 17 a0 2a 73 cb 98 18  |........Z..*s...|

00000020  ad 17 00 00 6a f4 ff ff  35 2b 00 00 ca d0 ff ff  |....j...5+......|
00000030  00 00 00 00 00 00 00 00  2a a6 bb 30 73 cb 98 18  |........*..0s...|

00000040  92 1e 00 00 50 ec ff ff  ea c1 ff ff 78 f0 ff ff  |....P.......x...|
00000050  00 00 00 00 00 00 00 00  8f 3b a7 39 77 cb 98 18  |.........;.9w...|

After this patch, timestamp is now in the correct location.

00000000  55 0f 00 00 dd 1f 00 00  af 0b 00 00 ec 3e 00 00  |U............>..|
00000010  c7 17 68 42 6d d0 98 18  00 00 00 00 00 00 00 00  |..hBm...........|

00000020  57 0e 00 00 c8 1f 00 00  d1 0e 00 00 42 3e 00 00  |W...........B>..|
00000030  56 a2 87 48 6d d0 98 18  00 00 00 00 00 00 00 00  |V..Hm...........|

00000040  a3 e2 ff ff d3 1b 00 00  0b c9 ff ff cc 20 00 00  |............. ..|
00000050  27 59 4d b3 72 d0 98 18  00 00 00 00 00 00 00 00  |'YM.r...........|

I also tested this with a different driver not affected by this bug to
make sure that the timestamp is still in the correct location for all
other drivers.
---
 include/linux/iio/buffer.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index d37f82678f71..ac19b39bdbe4 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -34,8 +34,16 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
 	void *data, int64_t timestamp)
 {
 	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
-		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
-		((int64_t *)data)[ts_offset] = timestamp;
+		size_t ts_offset = indio_dev->scan_bytes -
+			ACCESS_PRIVATE(indio_dev, largest_scan_element_size);
+
+		/*
+		 * The size of indio_dev->scan_bytes is always aligned to
+		 * largest_scan_element_size (see iio_compute_scan_bytes()).
+		 * And this size is always going to be >= sizeof(timestamp).
+		 * So to correctly place the timestamp, it goes at this offset.
+		 */
+		*(int64_t *)(data + ts_offset) = timestamp;
 	}
 
 	return iio_push_to_buffers(indio_dev, data);

-- 
2.43.0

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[Andy Shevchenko]

On Sun, Mar 01, 2026 at 02:24:53PM -0600, David Lechner wrote:
> Fix timestamp alignment when a scan buffer contains an element larger
> than sizeof(int64_t). Currently s32 quaternions are the only such
> element, and the one driver that has this (hid-sensor-rotation) has a
> workaround in place already so this change does not affect it.
> 
> Previously, we assumed that the timestamp would always be 8-byte aligned
> relative to the end of the scan buffer, but in the case of a scan buffer
> a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
> to be placed at offset 16, not 24.

...

> -		((int64_t *)data)[ts_offset] = timestamp;

> +		*(int64_t *)(data + ts_offset) = timestamp;

What's the point in modifying this? The comment you added suffice for
the original line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: orientation: hid-sensor-rotation: add timestamp hack to not break userspace

[Andy Shevchenko]

On Sun, Mar 01, 2026 at 02:24:50PM -0600, David Lechner wrote:
> Add a hack to push two timestamps in the hid-sensor-rotation scan data
> to avoid breaking userspace applications that depend on the timestamp
> being at the incorrect location in the scan data due to unintentional
> misalignment in older kernels.
> 
> When this driver was written, the timestamp was in the correct location
> because of the way iio_compute_scan_bytes() was implemented at the time.
> (Samples were 24 bytes each.) Then commit 883f61653069 ("iio: buffer:
> align the size of scan bytes to size of the largest element") changed
> the computed scan_bytes to be a different size (32 bytes), which caused
> iio_push_to_buffers_with_timestamp() to place the timestamp at an
> incorrect offset.
> 
> There have been long periods of time (6 years each) where the timestamp
> was in either location, so to not break either case, we open-code the
> timestamps to be pushed to both locations in the scan data.

...

> +		/*
> +		 * HACK: There are two copies of the same timestamp in case of

Usually we use FIXME in such cases. HACK is something which goes with
"do not apply".

Does it mean it will stay forever?

> +		 * userspace depending on broken alignment from older kernels.
> +		 */
> +		aligned_s64 timestamp[2];

...

> +		/*
> +		 * HACK: IIO previously had an incorrect implementation of

Ditto.

> +		 * iio_push_to_buffers_with_timestamp() that put the timestamp
> +		 * in the last 8 bytes of the buffer, which was incorrect
> +		 * according to the IIO ABI. To avoid breaking userspace that
> +		 * depended on this broken behavior, we put the timestamp in
> +		 * both the correct place and the old incorrect place.
> +		 */
> +		rot_state->scan.timestamp[0] = rot_state->timestamp;
> +		rot_state->scan.timestamp[1] = rot_state->timestamp;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[Nuno Sá]

On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:
> Fix timestamp alignment when a scan buffer contains an element larger
> than sizeof(int64_t). Currently s32 quaternions are the only such
> element, and the one driver that has this (hid-sensor-rotation) has a
> workaround in place already so this change does not affect it.
> 
> Previously, we assumed that the timestamp would always be 8-byte aligned
> relative to the end of the scan buffer, but in the case of a scan buffer
> a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
> to be placed at offset 16, not 24.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> To test this, I used hid-sensor-rotation minus the first patch in the
> series so that we can see that the timestamp actually moved to the
> correct location.
> 
> Before this patch, the timestamp (8 bytes ending with "98 18") is in the
> wrong location.
> 
> 00000000  6a 18 00 00 ac f3 ff ff  83 2d 00 00 02 d3 ff ff  |j........-......|
> 00000010  00 00 00 00 00 00 00 00  5a 17 a0 2a 73 cb 98 18  |........Z..*s...|
> 
> 00000020  ad 17 00 00 6a f4 ff ff  35 2b 00 00 ca d0 ff ff  |....j...5+......|
> 00000030  00 00 00 00 00 00 00 00  2a a6 bb 30 73 cb 98 18  |........*..0s...|
> 
> 00000040  92 1e 00 00 50 ec ff ff  ea c1 ff ff 78 f0 ff ff  |....P.......x...|
> 00000050  00 00 00 00 00 00 00 00  8f 3b a7 39 77 cb 98 18  |.........;.9w...|
> 
> After this patch, timestamp is now in the correct location.
> 
> 00000000  55 0f 00 00 dd 1f 00 00  af 0b 00 00 ec 3e 00 00  |U............>..|
> 00000010  c7 17 68 42 6d d0 98 18  00 00 00 00 00 00 00 00  |..hBm...........|
> 
> 00000020  57 0e 00 00 c8 1f 00 00  d1 0e 00 00 42 3e 00 00  |W...........B>..|
> 00000030  56 a2 87 48 6d d0 98 18  00 00 00 00 00 00 00 00  |V..Hm...........|
> 
> 00000040  a3 e2 ff ff d3 1b 00 00  0b c9 ff ff cc 20 00 00  |............. ..|
> 00000050  27 59 4d b3 72 d0 98 18  00 00 00 00 00 00 00 00  |'YM.r...........|
> 
> I also tested this with a different driver not affected by this bug to
> make sure that the timestamp is still in the correct location for all
> other drivers.
> ---
>  include/linux/iio/buffer.h | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index d37f82678f71..ac19b39bdbe4 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -34,8 +34,16 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
>  	void *data, int64_t timestamp)
>  {
>  	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
> -		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
> -		((int64_t *)data)[ts_offset] = timestamp;
> +		size_t ts_offset = indio_dev->scan_bytes -
> +			ACCESS_PRIVATE(indio_dev, largest_scan_element_size);

Given that we're adding a new private member, maybe we could just directly cache the ts_offset
in iio_compute_scan_bytes()? Would make the code a bit easier to follow IMHO

- Nuno Sá
> 

Re: [PATCH 3/4] iio: buffer: cache largest scan element size

[Nuno Sá]

On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:
> Cache the largest scan element size of elements enabled in a scan
> buffer. This will be used later to ensure proper alignment of the
> timestamp element in the scan buffer.
> 
> The new field could not be placed in struct iio_dev_opaque because we
> will need to access it in a static inline function later, so we make it
> __private instead. It is only intended to be used by core IIO code.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/industrialio-buffer.c | 14 +++++++++++---
>  include/linux/iio/iio.h           |  3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 71dfc81cb9e5..83e9392f949f 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -765,7 +765,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
>  
>  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  				  const unsigned long *mask, bool timestamp,
> -				  unsigned int *scan_bytes)
> +				  unsigned int *scan_bytes,
> +				  unsigned int *largest_element_size)
>  {
>  	unsigned int bytes = 0;
>  	int length, i, largest = 0;
> @@ -793,6 +794,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  
>  	*scan_bytes = ALIGN(bytes, largest);
>  
> +	if (largest_element_size)
> +		*largest_element_size = largest;

I might be missing something but it seems we now have two paths:

1. Go with 32 bytes
2. Go with 24 bytes (natural alignment)

ABI was not clear so I'm not sure if we do want to enforce/treat repeated values as one single
element? If so, nothing to change. But if not, we could re-think the approach and save some bytes. 
Marginal savings though so If having the smaller buffer is not straight enough I would be ok with
the simplicity tradeoff.

- Nuno Sá

> +
>  	return 0;
>  }
>  
> @@ -848,7 +852,7 @@ static int iio_buffer_update_bytes_per_datum(struct iio_dev *indio_dev,
>  		return 0;
>  
>  	ret = iio_compute_scan_bytes(indio_dev, buffer->scan_mask,
> -				     buffer->scan_timestamp, &bytes);
> +				     buffer->scan_timestamp, &bytes, NULL);
>  	if (ret)
>  		return ret;
>  
> @@ -892,6 +896,7 @@ struct iio_device_config {
>  	unsigned int watermark;
>  	const unsigned long *scan_mask;
>  	unsigned int scan_bytes;
> +	unsigned int largest_scan_element_size;
>  	bool scan_timestamp;
>  };
>  
> @@ -997,7 +1002,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	}
>  
>  	ret = iio_compute_scan_bytes(indio_dev, scan_mask, scan_timestamp,
> -				     &config->scan_bytes);
> +				     &config->scan_bytes,
> +				     &config->largest_scan_element_size);
>  	if (ret)
>  		return ret;
>  
> @@ -1155,6 +1161,8 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	indio_dev->active_scan_mask = config->scan_mask;
>  	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
>  	indio_dev->scan_bytes = config->scan_bytes;
> +	ACCESS_PRIVATE(indio_dev, largest_scan_element_size) =
> +		config->largest_scan_element_size;
>  	iio_dev_opaque->currentmode = config->mode;
>  
>  	iio_update_demux(indio_dev);
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index a9ecff191bd9..85bcb5f8ae15 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -584,6 +584,8 @@ struct iio_buffer_setup_ops {
>   *			and owner
>   * @buffer:		[DRIVER] any buffer present
>   * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
> + * @largest_scan_element_size: [INTERN] cache of the largest scan element size
> + *			       among the channels selected in the scan mask
>   * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
>   *			   array in order of preference, the most preferred
>   *			   masks first.
> @@ -610,6 +612,7 @@ struct iio_dev {
>  
>  	struct iio_buffer		*buffer;
>  	int				scan_bytes;
> +	unsigned int			__private largest_scan_element_size;
>  
>  	const unsigned long		*available_scan_masks;
>  	unsigned int			__private masklength;

Re: [PATCH 1/4] iio: orientation: hid-sensor-rotation: add timestamp hack to not break userspace

[David Lechner]

On 3/2/26 2:50 AM, Andy Shevchenko wrote:
> On Sun, Mar 01, 2026 at 02:24:50PM -0600, David Lechner wrote:
>> Add a hack to push two timestamps in the hid-sensor-rotation scan data
>> to avoid breaking userspace applications that depend on the timestamp
>> being at the incorrect location in the scan data due to unintentional
>> misalignment in older kernels.
>>
>> When this driver was written, the timestamp was in the correct location
>> because of the way iio_compute_scan_bytes() was implemented at the time.
>> (Samples were 24 bytes each.) Then commit 883f61653069 ("iio: buffer:
>> align the size of scan bytes to size of the largest element") changed
>> the computed scan_bytes to be a different size (32 bytes), which caused
>> iio_push_to_buffers_with_timestamp() to place the timestamp at an
>> incorrect offset.
>>
>> There have been long periods of time (6 years each) where the timestamp
>> was in either location, so to not break either case, we open-code the
>> timestamps to be pushed to both locations in the scan data.
> 
> ...
> 
>> +		/*
>> +		 * HACK: There are two copies of the same timestamp in case of
> 
> Usually we use FIXME in such cases. HACK is something which goes with
> "do not apply".
> 
> Does it mean it will stay forever?

Yes, it will have to stay forever because we can't break userspace.

My intention here was for HACK to mean "do not copy to another driver".


If we think that no one is depending on timestamps being in the wrong
offset, we could omit this change and apply only the rest of the series.
And then only apply this patch if anyone complains. I am just trying to
play it safe here and not risk breaking things.

Re: [PATCH 3/4] iio: buffer: cache largest scan element size

[David Lechner]

On 3/2/26 6:16 AM, Nuno Sá wrote:
> On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:
>> Cache the largest scan element size of elements enabled in a scan
>> buffer. This will be used later to ensure proper alignment of the
>> timestamp element in the scan buffer.
>>
>> The new field could not be placed in struct iio_dev_opaque because we
>> will need to access it in a static inline function later, so we make it
>> __private instead. It is only intended to be used by core IIO code.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/iio/industrialio-buffer.c | 14 +++++++++++---
>>  include/linux/iio/iio.h           |  3 +++
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 71dfc81cb9e5..83e9392f949f 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -765,7 +765,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
>>  
>>  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>>  				  const unsigned long *mask, bool timestamp,
>> -				  unsigned int *scan_bytes)
>> +				  unsigned int *scan_bytes,
>> +				  unsigned int *largest_element_size)
>>  {
>>  	unsigned int bytes = 0;
>>  	int length, i, largest = 0;
>> @@ -793,6 +794,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>>  
>>  	*scan_bytes = ALIGN(bytes, largest);
>>  
>> +	if (largest_element_size)
>> +		*largest_element_size = largest;
> 
> I might be missing something but it seems we now have two paths:
> 
> 1. Go with 32 bytes
> 2. Go with 24 bytes (natural alignment)

Hmm... You are right, but we are safe for now because the only repeat is
a quaternion, which has repeat of 4, so we don't have a case with e.g.
24 bytes right now.

I can do a follow-up to future-proof iio_storage_bytes_for_si() to
handle repeat = 3 (or any other less likely value).

Without the repeat, the size is based on .storagebits, and .storagebits /
BITS_PER_BYTE is always a power of 2, so no problem there.

> 
> ABI was not clear so I'm not sure if we do want to enforce/treat repeated values as one single
> element

It has always been this way and we had some recent breakage because I didn't
know about this. This patch series is a direct result of that [1] because we
identified more bugs when analyzing it.

[1]: https://lore.kernel.org/linux-iio/bug-221077-217253@https.bugzilla.kernel.org%2F/

> If so, nothing to change. But if not, we could re-think the approach and save some bytes. 

It's too late and breaks usespace as we have seen.

> Marginal savings though so If having the smaller buffer is not straight enough I would be ok with
> the simplicity tradeoff.
> 
> - Nuno Sá
> 

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[David Lechner]

On 3/2/26 2:47 AM, Andy Shevchenko wrote:
> On Sun, Mar 01, 2026 at 02:24:53PM -0600, David Lechner wrote:
>> Fix timestamp alignment when a scan buffer contains an element larger
>> than sizeof(int64_t). Currently s32 quaternions are the only such
>> element, and the one driver that has this (hid-sensor-rotation) has a
>> workaround in place already so this change does not affect it.
>>
>> Previously, we assumed that the timestamp would always be 8-byte aligned
>> relative to the end of the scan buffer, but in the case of a scan buffer
>> a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
>> to be placed at offset 16, not 24.
> 
> ...
> 
>> -		((int64_t *)data)[ts_offset] = timestamp;
> 
>> +		*(int64_t *)(data + ts_offset) = timestamp;
> 
> What's the point in modifying this? The comment you added suffice for
> the original line.
> 

ts_offset is now in bytes rather than number of int64_t elements.
So ((int64_t *)data)[ts_offset] would now 8x past the end of the array.

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[David Lechner]

On 3/2/26 6:04 AM, Nuno Sá wrote:
> On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:
>> Fix timestamp alignment when a scan buffer contains an element larger
>> than sizeof(int64_t). Currently s32 quaternions are the only such
>> element, and the one driver that has this (hid-sensor-rotation) has a
>> workaround in place already so this change does not affect it.
>>
>> Previously, we assumed that the timestamp would always be 8-byte aligned
>> relative to the end of the scan buffer, but in the case of a scan buffer
>> a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
>> to be placed at offset 16, not 24.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> To test this, I used hid-sensor-rotation minus the first patch in the
>> series so that we can see that the timestamp actually moved to the
>> correct location.
>>
>> Before this patch, the timestamp (8 bytes ending with "98 18") is in the
>> wrong location.
>>
>> 00000000  6a 18 00 00 ac f3 ff ff  83 2d 00 00 02 d3 ff ff  |j........-......|
>> 00000010  00 00 00 00 00 00 00 00  5a 17 a0 2a 73 cb 98 18  |........Z..*s...|
>>
>> 00000020  ad 17 00 00 6a f4 ff ff  35 2b 00 00 ca d0 ff ff  |....j...5+......|
>> 00000030  00 00 00 00 00 00 00 00  2a a6 bb 30 73 cb 98 18  |........*..0s...|
>>
>> 00000040  92 1e 00 00 50 ec ff ff  ea c1 ff ff 78 f0 ff ff  |....P.......x...|
>> 00000050  00 00 00 00 00 00 00 00  8f 3b a7 39 77 cb 98 18  |.........;.9w...|
>>
>> After this patch, timestamp is now in the correct location.
>>
>> 00000000  55 0f 00 00 dd 1f 00 00  af 0b 00 00 ec 3e 00 00  |U............>..|
>> 00000010  c7 17 68 42 6d d0 98 18  00 00 00 00 00 00 00 00  |..hBm...........|
>>
>> 00000020  57 0e 00 00 c8 1f 00 00  d1 0e 00 00 42 3e 00 00  |W...........B>..|
>> 00000030  56 a2 87 48 6d d0 98 18  00 00 00 00 00 00 00 00  |V..Hm...........|
>>
>> 00000040  a3 e2 ff ff d3 1b 00 00  0b c9 ff ff cc 20 00 00  |............. ..|
>> 00000050  27 59 4d b3 72 d0 98 18  00 00 00 00 00 00 00 00  |'YM.r...........|
>>
>> I also tested this with a different driver not affected by this bug to
>> make sure that the timestamp is still in the correct location for all
>> other drivers.
>> ---
>>  include/linux/iio/buffer.h | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
>> index d37f82678f71..ac19b39bdbe4 100644
>> --- a/include/linux/iio/buffer.h
>> +++ b/include/linux/iio/buffer.h
>> @@ -34,8 +34,16 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
>>  	void *data, int64_t timestamp)
>>  {
>>  	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
>> -		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
>> -		((int64_t *)data)[ts_offset] = timestamp;
>> +		size_t ts_offset = indio_dev->scan_bytes -
>> +			ACCESS_PRIVATE(indio_dev, largest_scan_element_size);
> 
> Given that we're adding a new private member, maybe we could just directly cache the ts_offset
> in iio_compute_scan_bytes()? Would make the code a bit easier to follow IMHO
> 
> - Nuno Sá
>>

Clever. :-)

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[Andy Shevchenko]

On Mon, Mar 02, 2026 at 09:39:30AM -0600, David Lechner wrote:
> On 3/2/26 2:47 AM, Andy Shevchenko wrote:
> > On Sun, Mar 01, 2026 at 02:24:53PM -0600, David Lechner wrote:

...

> >> -		((int64_t *)data)[ts_offset] = timestamp;
> > 
> >> +		*(int64_t *)(data + ts_offset) = timestamp;
> > 
> > What's the point in modifying this? The comment you added suffice for
> > the original line.
> 
> ts_offset is now in bytes rather than number of int64_t elements.
> So ((int64_t *)data)[ts_offset] would now 8x past the end of the array.

So semantic of the variable changed w/o name changing... OK.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: buffer: cache largest scan element size

[Nuno Sá]

On Mon, 2026-03-02 at 09:35 -0600, David Lechner wrote:
> On 3/2/26 6:16 AM, Nuno Sá wrote:
> > On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:
> > > Cache the largest scan element size of elements enabled in a scan
> > > buffer. This will be used later to ensure proper alignment of the
> > > timestamp element in the scan buffer.
> > > 
> > > The new field could not be placed in struct iio_dev_opaque because we
> > > will need to access it in a static inline function later, so we make it
> > > __private instead. It is only intended to be used by core IIO code.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 14 +++++++++++---
> > >  include/linux/iio/iio.h           |  3 +++
> > >  2 files changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index 71dfc81cb9e5..83e9392f949f 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -765,7 +765,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
> > >  
> > >  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
> > >  				  const unsigned long *mask, bool timestamp,
> > > -				  unsigned int *scan_bytes)
> > > +				  unsigned int *scan_bytes,
> > > +				  unsigned int *largest_element_size)
> > >  {
> > >  	unsigned int bytes = 0;
> > >  	int length, i, largest = 0;
> > > @@ -793,6 +794,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
> > >  
> > >  	*scan_bytes = ALIGN(bytes, largest);
> > >  
> > > +	if (largest_element_size)
> > > +		*largest_element_size = largest;
> > 
> > I might be missing something but it seems we now have two paths:
> > 
> > 1. Go with 32 bytes
> > 2. Go with 24 bytes (natural alignment)
> 
> Hmm... You are right, but we are safe for now because the only repeat is
> a quaternion, which has repeat of 4, so we don't have a case with e.g.
> 24 bytes right now.
> 
> I can do a follow-up to future-proof iio_storage_bytes_for_si() to
> handle repeat = 3 (or any other less likely value).
> 
> Without the repeat, the size is based on .storagebits, and .storagebits /
> BITS_PER_BYTE is always a power of 2, so no problem there.
> 
> > 
> > ABI was not clear so I'm not sure if we do want to enforce/treat repeated values as one single
> > element
> 
> It has always been this way and we had some recent breakage because I didn't
> know about this. This patch series is a direct result of that [1] because we
> identified more bugs when analyzing it.

Hmm true. If we shrink the buffer, userspace expecting timestamps at bit 24 will not work.


- Nuno Sá

> 
> [1]: https://lore.kernel.org/linux-iio/bug-221077-217253@https.bugzilla.kernel.org%2F/
> 
> > If so, nothing to change. But if not, we could re-think the approach and save some bytes. 
> 
> It's too late and breaks usespace as we have seen.
> 
> > Marginal savings though so If having the smaller buffer is not straight enough I would be ok
> > with
> > the simplicity tradeoff.
> > 
> > - Nuno Sá
> > 

Re: [PATCH 1/4] iio: orientation: hid-sensor-rotation: add timestamp hack to not break userspace

[Jonathan Cameron]

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

> On 3/2/26 2:50 AM, Andy Shevchenko wrote:
> > On Sun, Mar 01, 2026 at 02:24:50PM -0600, David Lechner wrote:  
> >> Add a hack to push two timestamps in the hid-sensor-rotation scan data
> >> to avoid breaking userspace applications that depend on the timestamp
> >> being at the incorrect location in the scan data due to unintentional
> >> misalignment in older kernels.
> >>
> >> When this driver was written, the timestamp was in the correct location
> >> because of the way iio_compute_scan_bytes() was implemented at the time.
> >> (Samples were 24 bytes each.) Then commit 883f61653069 ("iio: buffer:
> >> align the size of scan bytes to size of the largest element") changed
> >> the computed scan_bytes to be a different size (32 bytes), which caused
> >> iio_push_to_buffers_with_timestamp() to place the timestamp at an
> >> incorrect offset.
> >>
> >> There have been long periods of time (6 years each) where the timestamp
> >> was in either location, so to not break either case, we open-code the
> >> timestamps to be pushed to both locations in the scan data.  
> > 
> > ...
> >   
> >> +		/*
> >> +		 * HACK: There are two copies of the same timestamp in case of  
> > 
> > Usually we use FIXME in such cases. HACK is something which goes with
> > "do not apply".
> > 
> > Does it mean it will stay forever?  
> 
> Yes, it will have to stay forever because we can't break userspace.
> 
> My intention here was for HACK to mean "do not copy to another driver".
> 
> 
> If we think that no one is depending on timestamps being in the wrong
> offset, we could omit this change and apply only the rest of the series.
> And then only apply this patch if anyone complains. I am just trying to
> play it safe here and not risk breaking things.

I think it makes sense, but maybe label it something like

ABI regression avoidance: 

Good idea to just keep the wrongly aligned one as it removes the
risk and should do no harm as correct userspace should ignore the
second copy as it's in the padding.

Jonathan

Re: [PATCH 3/4] iio: buffer: cache largest scan element size

[Jonathan Cameron]

On Sun, 01 Mar 2026 14:24:52 -0600
David Lechner <dlechner@baylibre.com> wrote:

> Cache the largest scan element size of elements enabled in a scan
> buffer. This will be used later to ensure proper alignment of the
> timestamp element in the scan buffer.
Why can't we just stash the start of the timestamp? It's computed
a couple of lines up (or at least it's supposed to be computed there!)

	if (timestamp) {
		length = iio_storage_bytes_for_timestamp(indio_dev);
		if (length < 0)
			return length;

		bytes = ALIGN(bytes, length);

Or is that wrong currently?  If it is we need to fix that logic.

J
> 
> The new field could not be placed in struct iio_dev_opaque because we
> will need to access it in a static inline function later, so we make it
> __private instead. It is only intended to be used by core IIO code.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/industrialio-buffer.c | 14 +++++++++++---
>  include/linux/iio/iio.h           |  3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 71dfc81cb9e5..83e9392f949f 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -765,7 +765,8 @@ static int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
>  
>  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  				  const unsigned long *mask, bool timestamp,
> -				  unsigned int *scan_bytes)
> +				  unsigned int *scan_bytes,
> +				  unsigned int *largest_element_size)
>  {
>  	unsigned int bytes = 0;
>  	int length, i, largest = 0;
> @@ -793,6 +794,9 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  
>  	*scan_bytes = ALIGN(bytes, largest);
>  
> +	if (largest_element_size)
> +		*largest_element_size = largest;
> +
>  	return 0;
>  }
>  
> @@ -848,7 +852,7 @@ static int iio_buffer_update_bytes_per_datum(struct iio_dev *indio_dev,
>  		return 0;
>  
>  	ret = iio_compute_scan_bytes(indio_dev, buffer->scan_mask,
> -				     buffer->scan_timestamp, &bytes);
> +				     buffer->scan_timestamp, &bytes, NULL);
>  	if (ret)
>  		return ret;
>  
> @@ -892,6 +896,7 @@ struct iio_device_config {
>  	unsigned int watermark;
>  	const unsigned long *scan_mask;
>  	unsigned int scan_bytes;
> +	unsigned int largest_scan_element_size;
>  	bool scan_timestamp;
>  };
>  
> @@ -997,7 +1002,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	}
>  
>  	ret = iio_compute_scan_bytes(indio_dev, scan_mask, scan_timestamp,
> -				     &config->scan_bytes);
> +				     &config->scan_bytes,
> +				     &config->largest_scan_element_size);
>  	if (ret)
>  		return ret;
>  
> @@ -1155,6 +1161,8 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	indio_dev->active_scan_mask = config->scan_mask;
>  	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
>  	indio_dev->scan_bytes = config->scan_bytes;
> +	ACCESS_PRIVATE(indio_dev, largest_scan_element_size) =
> +		config->largest_scan_element_size;
>  	iio_dev_opaque->currentmode = config->mode;
>  
>  	iio_update_demux(indio_dev);
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index a9ecff191bd9..85bcb5f8ae15 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -584,6 +584,8 @@ struct iio_buffer_setup_ops {
>   *			and owner
>   * @buffer:		[DRIVER] any buffer present
>   * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
> + * @largest_scan_element_size: [INTERN] cache of the largest scan element size
> + *			       among the channels selected in the scan mask
>   * @available_scan_masks: [DRIVER] optional array of allowed bitmasks. Sort the
>   *			   array in order of preference, the most preferred
>   *			   masks first.
> @@ -610,6 +612,7 @@ struct iio_dev {
>  
>  	struct iio_buffer		*buffer;
>  	int				scan_bytes;
> +	unsigned int			__private largest_scan_element_size;
>  
>  	const unsigned long		*available_scan_masks;
>  	unsigned int			__private masklength;
> 

Re: [PATCH 4/4] iio: buffer: fix timestamp alignment when quaternion in scan

[Jonathan Cameron]

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

> On 3/2/26 6:04 AM, Nuno Sá wrote:
> > On Sun, 2026-03-01 at 14:24 -0600, David Lechner wrote:  
> >> Fix timestamp alignment when a scan buffer contains an element larger
> >> than sizeof(int64_t). Currently s32 quaternions are the only such
> >> element, and the one driver that has this (hid-sensor-rotation) has a
> >> workaround in place already so this change does not affect it.
> >>
> >> Previously, we assumed that the timestamp would always be 8-byte aligned
> >> relative to the end of the scan buffer, but in the case of a scan buffer
> >> a 16-byte quaternion vector, scan_bytes == 32, but the timestamp needs
> >> to be placed at offset 16, not 24.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---
> >>
> >> To test this, I used hid-sensor-rotation minus the first patch in the
> >> series so that we can see that the timestamp actually moved to the
> >> correct location.
> >>
> >> Before this patch, the timestamp (8 bytes ending with "98 18") is in the
> >> wrong location.
> >>
> >> 00000000  6a 18 00 00 ac f3 ff ff  83 2d 00 00 02 d3 ff ff  |j........-......|
> >> 00000010  00 00 00 00 00 00 00 00  5a 17 a0 2a 73 cb 98 18  |........Z..*s...|
> >>
> >> 00000020  ad 17 00 00 6a f4 ff ff  35 2b 00 00 ca d0 ff ff  |....j...5+......|
> >> 00000030  00 00 00 00 00 00 00 00  2a a6 bb 30 73 cb 98 18  |........*..0s...|
> >>
> >> 00000040  92 1e 00 00 50 ec ff ff  ea c1 ff ff 78 f0 ff ff  |....P.......x...|
> >> 00000050  00 00 00 00 00 00 00 00  8f 3b a7 39 77 cb 98 18  |.........;.9w...|
> >>
> >> After this patch, timestamp is now in the correct location.
> >>
> >> 00000000  55 0f 00 00 dd 1f 00 00  af 0b 00 00 ec 3e 00 00  |U............>..|
> >> 00000010  c7 17 68 42 6d d0 98 18  00 00 00 00 00 00 00 00  |..hBm...........|
> >>
> >> 00000020  57 0e 00 00 c8 1f 00 00  d1 0e 00 00 42 3e 00 00  |W...........B>..|
> >> 00000030  56 a2 87 48 6d d0 98 18  00 00 00 00 00 00 00 00  |V..Hm...........|
> >>
> >> 00000040  a3 e2 ff ff d3 1b 00 00  0b c9 ff ff cc 20 00 00  |............. ..|
> >> 00000050  27 59 4d b3 72 d0 98 18  00 00 00 00 00 00 00 00  |'YM.r...........|
> >>
> >> I also tested this with a different driver not affected by this bug to
> >> make sure that the timestamp is still in the correct location for all
> >> other drivers.
> >> ---
> >>  include/linux/iio/buffer.h | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> >> index d37f82678f71..ac19b39bdbe4 100644
> >> --- a/include/linux/iio/buffer.h
> >> +++ b/include/linux/iio/buffer.h
> >> @@ -34,8 +34,16 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
> >>  	void *data, int64_t timestamp)
> >>  {
> >>  	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
> >> -		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
> >> -		((int64_t *)data)[ts_offset] = timestamp;
> >> +		size_t ts_offset = indio_dev->scan_bytes -
> >> +			ACCESS_PRIVATE(indio_dev, largest_scan_element_size);  
> > 
> > Given that we're adding a new private member, maybe we could just directly cache the ts_offset
> > in iio_compute_scan_bytes()? Would make the code a bit easier to follow IMHO
> > 
> > - Nuno Sá  
> >>  
> 
> Clever. :-)
Ah. I should have read on!

J

Re: [PATCH 3/4] iio: buffer: cache largest scan element size

[David Lechner]

On 3/2/26 2:47 PM, Jonathan Cameron wrote:
> On Sun, 01 Mar 2026 14:24:52 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> Cache the largest scan element size of elements enabled in a scan
>> buffer. This will be used later to ensure proper alignment of the
>> timestamp element in the scan buffer.
> Why can't we just stash the start of the timestamp?

This is exactly what Nuno suggested. ;-)