Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 26 May 2020 12:15:56 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote:
> > On 5/25/20 7:52 PM, Andy Shevchenko wrote:  
> > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote:  
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> ...
> 
> > > > -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
> > > > +	/* Ensure natural alignment for buffer elements */
> > > > +	struct {
> > > > +		s16 channel;
> > > > +		s64 ts;
> > > > +	} scan;  
> > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
> > > iio_push_to_buffers_with_timestamp() point of view?  
> > 
> > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks
> > like we can't rely on implicit padding, but need to always explicitly
> > specify it.
> > 
> > Or maybe we can typedef and IIO timestamp type with an explicit __aligned
> > attribute. I wonder if that works... After having a quick look, the kernel
> > already defines aligned_u64, so maybe using that is an option.  
> 
> Another way is simple to provide offset of timestamp member as a parameter.
> Though, if it's an ABI, then alas, we need to align it properly.
> 
> Also, wouldn't be better to explicitly show the padding?
> 
> 	struct {
> 		s16 channel;
> 		s16 padding[3];
> 		s64 ts;
> 	} scan;
> 
> (matter of style though, just saying).
> 

gah.  Thanks for pointing this out Andy.  I wanted to avoid explicitly
calling out empty padding because it seemed to me to be more likely to
be error prone than filling it in.

I was trying to avoid using __aligned on the stack as it only works for
more recent kernels (due to gcc version changes) and some of these predate
that point.

I guess we just do it explicitly in all these cases.

The two patches that have already gone to Greg both have sufficient
data to ensure the structure is big enough (only 16 bytes padding in one and
none in the other).

I think we are also fine for the original question as well as it won't
matter if the whole structure is aligned to 4 bytes on x86_32 and
similar as an 8 byte write will be fine.

So fun question - do we want to enforce 8 byte alignment of the whole
structure, or simply the padding?

Maybe better to just do the padding explicitly as Andy suggested.

Jonathan