core: Basic port to uAPI v2

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.

+ * have a larger buffer. So need to rethink the allocation here,

+ * or at least the comment above...

         */

Quoted 4 lines · Kent Gibson

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.

Cool!

Quoted 4 lines

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.

...

lib/core.c static bool line_request_direction_is_valid(int direction) · line 605

Quoted 8 lines

lib/core.c +8 -0

+       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE)
+               bias_flags++;
+       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP)
+               bias_flags++;
+       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN)
+               bias_flags++;
+       if (bias_flags > 1)
+               return false;

Seems to me like an emulation of hweight(), although I don't know if we have in standard libraries such.

...

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 832

Quoted 7 lines

lib/core.c +3 -3

-       if (values) {
-               for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
-                       data.values[i] = (uint8_t)!!values[i];
+       for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++) {
+               lines_bitmap_set_bit(&lv.mask, i);
+               lines_bitmap_assign_bit(&lv.bits, i, values && values[i]);
        }

Hmm... What about

       for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
               lines_bitmap_set_bit(&lv.mask, i);

  if (values) {
       for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
               lines_bitmap_assign_bit(&lv.bits, i, values[i]);
  }

?

...

lib/core.c int gpiod_line_event_read_fd_multiple(int fd, struct gpiod_line_event *events, · line 1078

Quoted 6 lines

/*
* 16 is the maximum number of events the kernel can store in the FIFO
* so we can allocate the buffer on the stack.

lib/core.c +3 -0

+        *
+        * NOTE: This is no longer strictly true for uAPI v2.  While 16 is
+        * the default for single line, a request with multiple lines will

for a single

Quoted 1 line

+ * have a larger buffer. So need to rethink the allocation here,

So we (I, ...?) need

Quoted 6 lines · Andy Shevchenko

Quoted 4 lines · Kent Gibson

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.

Quoted 2 lines

Cool!

Not terribly - I needed a libgpiod-based testbed for my kernel changes, and this was the easiest way to do it.
By happy coincidence that aligns with the path Bart wants to take for libgpiod 2.0, so here we are.

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 <[email protected]>

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 <[email protected]>

---

Quoted 5 lines · Kent Gibson

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.

Quoted 7 lines

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 <[email protected]>

---

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.

Quoted 5 lines · Kent Gibson

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.

Quoted 7 lines

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 <[email protected]>

---

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.

Quoted 14 lines · Bartosz Golaszewski

Quoted 5 lines · Kent Gibson

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.

Quoted 7 lines

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 <[email protected]>

---

Quoted 8 lines

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.

Quoted 63 lines · Kent Gibson

Quoted 14 lines · Bartosz Golaszewski

Quoted 5 lines · Kent Gibson

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.

Quoted 7 lines

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 <[email protected]>

---

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 80 lines · Bartosz Golaszewski

Quoted 63 lines · Kent Gibson

Quoted 14 lines · Bartosz Golaszewski

Quoted 5 lines · Kent Gibson

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.

Quoted 7 lines

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 <[email protected]>

---

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.