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

[Kent Gibson <warthog618@gmail.com>]

On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 17:55 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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?
> 

Fixing it in API v2 makes the most sense to me.

Kent.