The DRDY_MASK feature implemented in sensor chips marks gyroscope and
accelerometer invalid samples (i.e. samples that have been acquired during
the settling time of sensor filters) with the special values 0x7FFFh,
0x7FFE, and 0x7FFD.
The driver checks FIFO samples against these special values in order to
discard invalid samples; however, it does the check regardless of the type
of samples being processed, whereas this feature is specific to gyroscope
and accelerometer data. This could cause valid samples to be discarded.
Fix the above check so that it takes into account the type of samples being
processed. To avoid casting to __le16 * when checking sample values, clean
up the type representation for data read from the FIFO.
Fixes: 960506ed2c69 ("iio: imu: st_lsm6dsx: enable drdy-mask if available")
Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
.../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 23 +++++++++++--------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 5b28a3ffcc3d..a6ee2da5a06c 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -365,8 +365,6 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 addr,
return 0;
}
-#define ST_LSM6DSX_IIO_BUFF_SIZE (ALIGN(ST_LSM6DSX_SAMPLE_SIZE, \
- sizeof(s64)) + sizeof(s64))
/**
* st_lsm6dsx_read_fifo() - hw FIFO read routine
* @hw: Pointer to instance of struct st_lsm6dsx_hw.
@@ -539,14 +537,14 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
#define ST_LSM6DSX_INVALID_SAMPLE 0x7ffd
static int
st_lsm6dsx_push_tagged_data(struct st_lsm6dsx_hw *hw, u8 tag,
- u8 *data, s64 ts)
+ __le16 *data, s64 ts)
{
- s16 val = le16_to_cpu(*(__le16 *)data);
struct st_lsm6dsx_sensor *sensor;
struct iio_dev *iio_dev;
/* invalid sample during bootstrap phase */
- if (val >= ST_LSM6DSX_INVALID_SAMPLE)
+ if ((tag == ST_LSM6DSX_GYRO_TAG || tag == ST_LSM6DSX_ACC_TAG) &&
+ (s16)le16_to_cpup(data) >= ST_LSM6DSX_INVALID_SAMPLE)
return -EINVAL;
/*
@@ -609,7 +607,13 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
* must be passed a buffer that is aligned to 8 bytes so
* as to allow insertion of a naturally aligned timestamp.
*/
- u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
+ struct {
+ union {
+ __le16 data[3];
+ __le32 fifo_ts;
+ };
+ aligned_s64 timestamp;
+ } iio_buff = { };
u8 tag;
bool reset_ts = false;
int i, err, read_len;
@@ -648,7 +652,7 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
for (i = 0; i < pattern_len;
i += ST_LSM6DSX_TAGGED_SAMPLE_SIZE) {
- memcpy(iio_buff, &hw->buff[i + ST_LSM6DSX_TAG_SIZE],
+ memcpy(&iio_buff, &hw->buff[i + ST_LSM6DSX_TAG_SIZE],
ST_LSM6DSX_SAMPLE_SIZE);
tag = hw->buff[i] >> 3;
@@ -659,7 +663,7 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
* B0 = ts[7:0], B1 = ts[15:8], B2 = ts[23:16],
* B3 = ts[31:24]
*/
- ts = le32_to_cpu(*((__le32 *)iio_buff));
+ ts = le32_to_cpu(iio_buff.fifo_ts);
/*
* check if hw timestamp engine is going to
* reset (the sensor generates an interrupt
@@ -670,7 +674,8 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
reset_ts = true;
ts *= hw->ts_gain;
} else {
- st_lsm6dsx_push_tagged_data(hw, tag, iio_buff,
+ st_lsm6dsx_push_tagged_data(hw, tag,
+ iio_buff.data,
ts);
}
}
--
2.39.5On Wed, Feb 25, 2026 at 11:17:11AM +0100, Francesco Lavra wrote:
> The DRDY_MASK feature implemented in sensor chips marks gyroscope and
> accelerometer invalid samples (i.e. samples that have been acquired during
> the settling time of sensor filters) with the special values 0x7FFFh,
> 0x7FFE, and 0x7FFD.
> The driver checks FIFO samples against these special values in order to
> discard invalid samples; however, it does the check regardless of the type
> of samples being processed, whereas this feature is specific to gyroscope
> and accelerometer data. This could cause valid samples to be discarded.
>
> Fix the above check so that it takes into account the type of samples being
> processed. To avoid casting to __le16 * when checking sample values, clean
> up the type representation for data read from the FIFO.
...
> - u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> + struct {
> + union {
> + __le16 data[3];
> + __le32 fifo_ts;
> + };
> + aligned_s64 timestamp;
> + } iio_buff = { };
Hmm... Have you considered m68k case? IIRC there the alignment is 2 (even for
64-bit members), so theoretically the beginning of the structure can be aligned
to address 2. and hence the __le16 data[3] will have no gap to the following
timestamp. In current code it's 64-bit and not 48-bit item.
I believe it's easy to check, though.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, 25 Feb 2026 at 12:42, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Wed, Feb 25, 2026 at 11:17:11AM +0100, Francesco Lavra wrote:
> > The DRDY_MASK feature implemented in sensor chips marks gyroscope and
> > accelerometer invalid samples (i.e. samples that have been acquired during
> > the settling time of sensor filters) with the special values 0x7FFFh,
> > 0x7FFE, and 0x7FFD.
> > The driver checks FIFO samples against these special values in order to
> > discard invalid samples; however, it does the check regardless of the type
> > of samples being processed, whereas this feature is specific to gyroscope
> > and accelerometer data. This could cause valid samples to be discarded.
> >
> > Fix the above check so that it takes into account the type of samples being
> > processed. To avoid casting to __le16 * when checking sample values, clean
> > up the type representation for data read from the FIFO.
>
> ...
>
> > - u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> > + struct {
> > + union {
> > + __le16 data[3];
> > + __le32 fifo_ts;
> > + };
> > + aligned_s64 timestamp;
> > + } iio_buff = { };
>
> Hmm... Have you considered m68k case? IIRC there the alignment is 2 (even for
> 64-bit members), so theoretically the beginning of the structure can be aligned
> to address 2. and hence the __le16 data[3] will have no gap to the following
> timestamp. In current code it's 64-bit and not 48-bit item.
Fortunately, the layout of a structure does not change because the
first member could be stored at a less-aligned address, as that would
make it incompatible with a different instance that is stored at a
differently-aligned address ;-)
The alignment of a structure is the maximum of the alignments of its
members. So that will be 8 bytes, as the aligned_s64 definition has
an internal "__attribute__((aligned(8)))".
It would be wise to add explicit padding ("u16 pad") just before
timestamp though, for documentation purpose.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Feb 26, 2026 at 10:43:34AM +0100, Geert Uytterhoeven wrote:
> On Wed, 25 Feb 2026 at 12:42, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Feb 25, 2026 at 11:17:11AM +0100, Francesco Lavra wrote:
...
> > > - u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> > > + struct {
> > > + union {
> > > + __le16 data[3];
> > > + __le32 fifo_ts;
> > > + };
> > > + aligned_s64 timestamp;
> > > + } iio_buff = { };
> >
> > Hmm... Have you considered m68k case? IIRC there the alignment is 2 (even for
> > 64-bit members), so theoretically the beginning of the structure can be aligned
> > to address 2. and hence the __le16 data[3] will have no gap to the following
> > timestamp. In current code it's 64-bit and not 48-bit item.
>
> Fortunately, the layout of a structure does not change because the
> first member could be stored at a less-aligned address, as that would
> make it incompatible with a different instance that is stored at a
> differently-aligned address ;-)
>
> The alignment of a structure is the maximum of the alignments of its
> members. So that will be 8 bytes, as the aligned_s64 definition has
> an internal "__attribute__((aligned(8)))".
Thanks for the elaboration on this case, my knowledge about m68k is too shallow!
> It would be wise to add explicit padding ("u16 pad") just before
> timestamp though, for documentation purpose.
__le16 :-) Something like this:
struct {
union {
__le16 data[3];
__le32 fifo_ts;
};
__le16 reserved; // ...compatibility with the previous impl...
aligned_s64 timestamp;
} iio_buff = { };
?
--
With Best Regards,
Andy Shevchenko
On Thu, 26 Feb 2026 11:56:01 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Thu, Feb 26, 2026 at 10:43:34AM +0100, Geert Uytterhoeven wrote:
> > On Wed, 25 Feb 2026 at 12:42, Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Wed, Feb 25, 2026 at 11:17:11AM +0100, Francesco Lavra wrote:
>
> ...
>
> > > > - u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> > > > + struct {
> > > > + union {
> > > > + __le16 data[3];
> > > > + __le32 fifo_ts;
> > > > + };
> > > > + aligned_s64 timestamp;
> > > > + } iio_buff = { };
> > >
> > > Hmm... Have you considered m68k case? IIRC there the alignment is 2 (even for
> > > 64-bit members), so theoretically the beginning of the structure can be aligned
> > > to address 2. and hence the __le16 data[3] will have no gap to the following
> > > timestamp. In current code it's 64-bit and not 48-bit item.
> >
> > Fortunately, the layout of a structure does not change because the
> > first member could be stored at a less-aligned address, as that would
> > make it incompatible with a different instance that is stored at a
> > differently-aligned address ;-)
> >
> > The alignment of a structure is the maximum of the alignments of its
> > members. So that will be 8 bytes, as the aligned_s64 definition has
> > an internal "__attribute__((aligned(8)))".
>
> Thanks for the elaboration on this case, my knowledge about m68k is too shallow!
>
> > It would be wise to add explicit padding ("u16 pad") just before
> > timestamp though, for documentation purpose.
>
> __le16 :-) Something like this:
>
> struct {
> union {
> __le16 data[3];
> __le32 fifo_ts;
> };
> __le16 reserved; // ...compatibility with the previous impl...
We typically haven't done this. I'm not sure why this driver justifies it
as IIO timestamp placement always relies on the set of rules Geert has
laid out.
Also if we did do it, applying an endianness to reserve data is a an odd
thing to do.
So as far a I'm concerned the approach in this patch seems fine.
Thanks,
Jonathan
> aligned_s64 timestamp;
> } iio_buff = { };
>
> ?
>
© 2016 - 2026 Red Hat, Inc.