Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

wt., 10 gru 2019 o 17:55 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
>
> > For Go the structs are aligned based on the size of their components so
> > that arrays of struct are naturally aligned.  The struct is given a
> > hidden trailing pad so that a subsequent struct will be correctly aligned.
> > The sizeof the struct includes this hidden pad.
> > I'm pretty sure the same is true for gcc.
> >
> > The gpioevent_data contains a __u64 which causes the whole struct to be
> > 64 bit aligned on 64 bit, so it actually looks like this internally:
> >
> > struct gpioevent_data {
> >       __u64 timestamp;
> >       __u32 id;
> >     __u32 pad; // hidden
> > };
> >
> > so 16 bytes.
> >
> > On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
> > so 12 bytes. This causes grief for the read due to the size mismatch.
>
> Exactly.
>
> > (I'm sorry to say I had to add the pad to my Go gpiod library to get it
> > to read event data - but forgot to go back later and work out why -
> > until now :-()
> >
> > Your new info change struct has the same problem, as it also contains a
> > __u64 and ends up with an odd number of __u32s, so gets a trailing pad
> > on 64 bit.  Using __packed seems to inhibit the trailing pad.
> > Or you could explicitly add the pad so the struct will be 64bit aligned
> > even on 32bit.
>
> I spoke to colleague of mine and has been told that best option is to fill all
> gaps explicitly to have all members in the struct + 8 bytes alignment at the
> end (also with explicit member).
>
> > Neither of those options are available for the
> > gpioevent_data, as that would break the ABI.
>
> ABI needs v2 actually.
>

I finally sat down to integrate this with my series and figured that
this can't go on top of it. It's a bug-fix actually and maybe even
stable material.

On the other hand - if we have so few users of GPIO chardev with
32-bit user-space and 64-bit kernel - maybe we should just bite the
bullet, not fix this one, deprecate it and introduce a proper v2 of
the API?

Bart