Re: [libgpiod][PATCH] core: Basic port to uAPI v2

[Bartosz Golaszewski <bgolaszewski@baylibre.com>]

On Wed, Oct 14, 2020 at 10:51 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 09:37:38AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Oct 14, 2020 at 5:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Tue, Oct 13, 2020 at 09:08:44AM +0200, Bartosz Golaszewski wrote:
> > > > On Fri, Oct 2, 2020 at 8:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > Port existing implementation from GPIO uAPI v1 to v2.
> > > > > The libgpiod external interface remains unchanged, only the internal
> > > > > implementation switches from uAPI v1 to v2.
> > > > >
> > > > > This is a minimal port - uAPI v2 features are only used where it
> > > > > simplifies the implementation, specifically multiple events on a bulk can
> > > > > now be handled directly by the kernel in a single v2 line request rather
> > > > > than being emulated by multiple v1 event requests.
> > > > >
> > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > > > ---
> > > >
> > > > Hi Kent!
> > > >
> > > > I just noticed that this broke the tests in Python and C++ bindings.
> > > > Something to do with the RISING and FALLING edge values and how
> > > > they're translated in the bindings, it seems. Let me know if you'd
> > > > have time to take a look at it and see if it's something obvious.
> > > > Otherwise I'll investigate that tomorrow.
> > > >
> > >
> > > Lots of problems here, so where to start...
> > >
> > > Firstly, I foolishly assumed that the coverage of the CXX and Python
> > > tests was aimed at testing the binding, and that wouldn't provide
> > > additional coverage to the core API to that tested by the C tests.
> > > Not the case :(.
> > >
> > > Most of the problems are due to the switch from an event request per
> > > line to a single line request.  This breaks tests that assume an fd per
> > > line and test the fds using poll.  As they are now the same, all line fds
> > > in the bulk become ready to read at once, where previously it was only one.
> > > These are only present in CXX and Python test cases :(.
> > >
> > > I also misunderstood the use case for gpiod_line_event_wait_bulk(),
> > > and used the event from the fd to determine the line (incorrectly as it
> > > turns out).  As a consequence tests that then read the event from the
> > > line fd find the subsequent event, or the wrong number of events.
> > > Again, these are only present in CXX and Python tests :(.
> > >
> > > And I misunderstood the offset passed in gpiod_line_bulk_get_line(),
> > > using the chip offset not the bulk offset (there are too many damn
> > > offsets here), so that only happened to work in the C tests as ALL lines
> > > on the test chip were requested so the chip and bulk offsets match.
> > > Changing the test to mismatch those offsets makes the C tests fail as
> > > well :).
> > > To fix that we'd need to peek into the file to get the event
> > > offset without actually removing the event from the file, and be able to
> > > find the line in a bulk based on chip offset.
> > >
> > > Oh, and gpiod_line_bulk_get_line() doesn't do range checking, so will
> > > happily return garbage if offset >= bulk.num_lines.  It happened to
> > > return NULL in my testing, resulting an exception when trying to add the
> > > line to the bulk.  But if it had returned something else...
> > >
> > > So, in short, the switch to using one line request for all events in the
> > > bulk is problematic with the exposing of the line fd, and the
> > > current implementation of gpiod_line_event_wait_bulk() is broken.
> > >
> > > Where to go from here depends on where you want to go with the API.
> > > As mentioned yesterday, my preference would be for
> > > gpiod_line_event_wait_bulk() to return the next event that occured on the
> > > bulk rather than a bulk of lines with events.
> > >
> > > And it needs to be made clear that the fd returned by
> > > line.event_get_fd() applies to the bulk - assuming we switch to the one
> > > request per bulk.
> >
> > Thanks for the detailed explanation. It all makes sense now. I thought
> > it's possible to just use the new uAPI while keeping the old library
> > interface for now. It does not seem to be the case (at least for
> > events) in which case I'll just back out your port and I'll start
> > working on a new API bit by bit, changing separate parts and
> > introducing non-compatible changes where it's required.
> >
> > If you want to work on this too - just let me know which parts so we
> > don't do the same thing twice.
> >
>
> I originally did the port in multiple steps, so I should be able to put
> togther a patch that retains the old event behaviour without much
> trouble.  That would switch you totally over to uAPI v2 while still
> doing everything as per v1.  Then you could workout the best way to
> integrate the new features from there.
>

Yes, that would be great! Thanks!

Bart