Re: [PATCH v6 3/7] iio: imu: st_lsm6dsx: Fix check for invalid samples from FIFO

[Jonathan Cameron]

On Thu, 26 Feb 2026 11:56:01 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> 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@xxxxxxxxx> 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 = { };
> 
> ?
>