| Message ID | 20250417-iio-more-timestamp-alignment-v1-7-eafac1e22318@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: more timestamp alignment | expand |
On Thu, Apr 17, 2025 at 11:52:39AM -0500, David Lechner wrote: > Align the buffer used with iio_push_to_buffers_with_timestamp() to > ensure the s64 timestamp is aligned to 8 bytes. Same question as per previous patch.
On Thu, 17 Apr 2025 20:00:05 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Thu, Apr 17, 2025 at 11:52:39AM -0500, David Lechner wrote: > > Align the buffer used with iio_push_to_buffers_with_timestamp() to > > ensure the s64 timestamp is aligned to 8 bytes. > > Same question as per previous patch. > In this case I don't think we know the position of the timestamp so a structure would be misleading. The comment above the define certainly suggests it is variable.. /* * Maximum of 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8. * May be less if fewer channels are enabled, as long as the timestamp * remains 8 byte aligned */ #define INV_MPU6050_OUTPUT_DATA_SIZE 32
On Thu, 17 Apr 2025 19:46:00, Jonathan Cameron wrote: > On Thu, 17 Apr 2025 20:00:05 +0300 > Andy Shevchenko <andy@kernel.org> wrote: > > > On Thu, Apr 17, 2025 at 11:52:39AM -0500, David Lechner wrote: > > > Align the buffer used with iio_push_to_buffers_with_timestamp() to > > > ensure the s64 timestamp is aligned to 8 bytes. > > > > Same question as per previous patch. > > > In this case I don't think we know the position of the timestamp > so a structure would be misleading. > > The comment above the define certainly suggests it is variable.. I confirm timestamp position is changing depending on channels enabled. It can be at address 8, 16 or 24. If there is only 1 sensor enabled (6 bytes of data), timestamp is at address 8. 2 sensors (12 bytes of data), timestamp will be at address 16. 3 sensors for MPU-9xxx (19 bytes of data), timestamp will be at address 24. If the buffer is aligned on 8 bytes, it will always work without any problem. > > /* > * Maximum of 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8. > * May be less if fewer channels are enabled, as long as the timestamp > * remains 8 byte aligned > */ > #define INV_MPU6050_OUTPUT_DATA_SIZE 32 Thanks, JB
On Fri, 18 Apr 2025 11:26:39 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > On Thu, 17 Apr 2025 19:46:00, Jonathan Cameron wrote: > > On Thu, 17 Apr 2025 20:00:05 +0300 > > Andy Shevchenko <andy@kernel.org> wrote: > > > > > On Thu, Apr 17, 2025 at 11:52:39AM -0500, David Lechner wrote: > > > > Align the buffer used with iio_push_to_buffers_with_timestamp() to > > > > ensure the s64 timestamp is aligned to 8 bytes. > > > > > > Same question as per previous patch. > > > > > In this case I don't think we know the position of the timestamp > > so a structure would be misleading. > > > > The comment above the define certainly suggests it is variable.. > > I confirm timestamp position is changing depending on channels enabled. It > can be at address 8, 16 or 24. > > If there is only 1 sensor enabled (6 bytes of data), timestamp is at address > 8. 2 sensors (12 bytes of data), timestamp will be at address 16. 3 sensors > for MPU-9xxx (19 bytes of data), timestamp will be at address 24. > > If the buffer is aligned on 8 bytes, it will always work without any problem. > > > > > /* > > * Maximum of 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8. > > * May be less if fewer channels are enabled, as long as the timestamp > > * remains 8 byte aligned > > */ > > #define INV_MPU6050_OUTPUT_DATA_SIZE 32 > > Thanks, > JB I applied this one as it stands with fixes tag and +CC stable. Fixes: 0829edc43e0a ("iio: imu: inv_mpu6050: read the full fifo when processing data") I thought about seeing if all the cases that are fixes are separable enough to take through togreg-fixes whilst the with_ts() series goes through togreg in parallel. I might see if that is doable easily. Jonathan
On Fri, 18 Apr 2025 16:02:38 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 18 Apr 2025 11:26:39 +0000 > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > On Thu, 17 Apr 2025 19:46:00, Jonathan Cameron wrote: > > > On Thu, 17 Apr 2025 20:00:05 +0300 > > > Andy Shevchenko <andy@kernel.org> wrote: > > > > > > > On Thu, Apr 17, 2025 at 11:52:39AM -0500, David Lechner wrote: > > > > > Align the buffer used with iio_push_to_buffers_with_timestamp() to > > > > > ensure the s64 timestamp is aligned to 8 bytes. > > > > > > > > Same question as per previous patch. > > > > > > > In this case I don't think we know the position of the timestamp > > > so a structure would be misleading. > > > > > > The comment above the define certainly suggests it is variable.. > > > > I confirm timestamp position is changing depending on channels enabled. It > > can be at address 8, 16 or 24. > > > > If there is only 1 sensor enabled (6 bytes of data), timestamp is at address > > 8. 2 sensors (12 bytes of data), timestamp will be at address 16. 3 sensors > > for MPU-9xxx (19 bytes of data), timestamp will be at address 24. > > > > If the buffer is aligned on 8 bytes, it will always work without any problem. > > > > > > > > /* > > > * Maximum of 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8. > > > * May be less if fewer channels are enabled, as long as the timestamp > > > * remains 8 byte aligned > > > */ > > > #define INV_MPU6050_OUTPUT_DATA_SIZE 32 > > > > Thanks, > > JB > > I applied this one as it stands with fixes tag and +CC stable. > > Fixes: 0829edc43e0a ("iio: imu: inv_mpu6050: read the full fifo when processing data") > > I thought about seeing if all the cases that are fixes are separable enough > to take through togreg-fixes whilst the with_ts() series goes through togreg > in parallel. I might see if that is doable easily. > I have done so and it seems fine as we didn't rename anything... 52d349884738 (HEAD -> fixes-togreg) iio: adc: ad7266: Fix potential timestamp alignment issue. ffbc26bc91c1 iio: adc: ad7768-1: Fix insufficient alignment of timestamp. 5097eaae98e5 iio: adc: dln2: Use aligned_s64 for timestamp 1bb942287e05 iio: accel: adxl355: Make timestamp 64-bit aligned using aligned_s64 f79aeb6c631b iio: temp: maxim-thermocouple: Fix potential lack of DMA safe buffer. 6ffa69867405 iio: chemical: pms7003: use aligned_s64 for timestamp bb49d940344b iio: chemical: sps30: use aligned_s64 for timestamp Now on the fixes-togreg branch of iio.git. > Jonathan
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index 3d3b27f28c9d1c94aba93678261ce0d63099e1dc..273196e647a2b5a4860e18cfa34a088c773540e4 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -50,7 +50,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) u16 fifo_count; u32 fifo_period; s64 timestamp; - u8 data[INV_MPU6050_OUTPUT_DATA_SIZE]; + u8 data[INV_MPU6050_OUTPUT_DATA_SIZE] __aligned(8); size_t i, nb; mutex_lock(&st->lock);
Align the buffer used with iio_push_to_buffers_with_timestamp() to ensure the s64 timestamp is aligned to 8 bytes. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)