Re: [RFC PATCH] gpio: uapi: v2 proposal

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, May 25, 2020 at 06:24:04PM +0200, Bartosz Golaszewski wrote:
> sob., 16 maj 2020 o 08:45 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> >
> > Add a new version of the uAPI to address existing 32/64bit alignment
> > issues, add support for debounce, and provide some future proofing by
> > adding padding reserved for future use.
> >
> > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> >
> > ---
> >
> > This patch is a proposal to replace the majority of the uAPI, so some
> > background and justification is in order.
> >
> > The alignment issue relates to the gpioevent_data, which packs to different
> > sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> > running on 64bit kernels.  The patch addresses that particular issue, and
> > the problem more generally, by adding pad fields that explicitly pad
> > structs out to 64bit boundaries, so they will pack to the same size now,
> > and even if some of the reserved padding is used for __u64 fields in the
> > future.
> >
> > The lack of future proofing in v1 makes it impossible to, for example,
> > add the debounce feature that is included in v2.
> > The future proofing is addressed by providing reserved padding in all
> > structs for future features.  Specifically, the line request,
> > config and info structs get updated versions and ioctls.
> >
> > I haven't added any padding to gpiochip_info, as I haven't seen any calls
> > for new features for the corresponding ioctl, but I'm open to updating that
> > as well.
> >
> > As the majority of the structs and ioctls were being replaced, it seemed
> > opportune to rework some of the other aspects of the uAPI.
> >
> > Firstly, I've reworked the flags field throughout.  v1 has three different
> > flags fields, each with their own separate bit definitions.  In v2 that is
> > collapsed to one.  Further, the bits of the v2 flags field are used
> > as feature enable flags, with any other necessary configuration fields encoded
> > separately.  This is simpler and clearer, while also providing a foundation
> > for adding features in the future.
> >
> > I've also merged the handle and event requests into a single request, the
> > line request, as the two requests where mostly the same, other than the
> > edge detection provided by event requests.  As a byproduct, the v2 uAPI
> > allows for multiple lines producing edge events on the same line handle.
> > This is a new capability as v1 only supports a single line in an event request.
> >
> > This means there are now only two types of file handle to be concerned with,
> > the chip and the line, and it is clearer which ioctls apply to which type
> > of handle.
> >
> > There is also some minor renaming of fields for consistency compared to their
> > v1 counterparts, e.g. offset rather than lineoffset or line_offset, and
> > consumer rather than consumer_label.
> >
> > And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and
> > gpioline_values for v2 - the only change being the renaming for clarity.
> >
> > The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
> > particularly libgpiod, should easily port to it.
> >
> > This patch is obviously only one patch in a much bigger series that
> > will actually implement it, but I would appreciate a review and any feedback,
> > as it is foundational to the rest of that series.
> >
> > Thanks,
> > Kent.
> >
> 
> Hi Kent,
> 
> Thanks for posting this. I like the general direction a lot. I'll
> review this in detail later this week.
> 
> Seeing the speed at which you make progress I think I won't be
> implementing support for the v1 of the watch ioctl() in libgpiod after
> all. Once the v2 is live I will probably bump the API version in
> libgpiod to v2.0.0 and make some non-compatible changes anyway.
> 

Yeah, libgpiod has similar problems to the v1 uAPI - there is no
easy way to extend it without causing breakage.

I've got a patch that ports libgpiod to the v2 uAPI, though it only goes
as far as v1 parity.  That is passing all existing tests.  The patch
doesn't address debounce as that will require libgpiod API changes.  Nor
does it make use of the bulk event capability, as that would change
behaviour and break some of the tests - the wait_multiple test was the
sticking point there.  Those are probably best combined with the v2.0.0
changes you are planning.

I've also got a couple of patches for minor things that I noticed while I
was in there.  I'll post them shortly.

Cheers,
Kent.