On Tue, May 26, 2020 at 05:43:28PM +0100, Jonathan Cameron wrote:
> On Tue, 26 May 2020 12:15:56 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
...
> > > > > + 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.
I have talked to colleague of mine, and we concluded (but without any
documentation proved evidence, one needs basically to read C standard followed
by ABI of all architectures supported by Linux) that the following will work.
Consider your patch, which introduces natural alignment via struct:
struct scan {
s16 ...;
...
s64 ts;
};
When we access ts as struct member like scan->ts, compiler makes sure that
there will be no hardware exception (due to unaligned access). Now, we _assume_
that dereferencing like
void *buf = &scan;
(int64_t *)buf[ts_offset] = value;
will work flawlessly because above.
If it's indeed a case, what we simple need is to pass ts offset into
iio_push_to_buffers_with_timestamp().
If it's not the case, we _additionally_ will need to replace
(int64_t *)buf[ts_offset] = value;
by
put_unaligned(value, (int64_t *)...);
--
With Best Regards,
Andy Shevchenko
