On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>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().
Realistically it might work but doesn't help us. We need all receivers of that buffer to
know the location of ts. It has always been implicit like any other channel.
Rules are natural alignment so alignment is same as size of element.
Hence we need to ensure padding, but I'm not sure about alignment. If padding was
right for 8 byte alignment but whole structure had 4 byte alignment then I think we
should be fine. So slightly more relaxed the ensuring ts is 8 byte aligned.
Might be easier to just align it though than explain this subtlety.
J
>
>If it's not the case, we _additionally_ will need to replace
> (int64_t *)buf[ts_offset] = value;
>by
> put_unaligned(value, (int64_t *)...);
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
