| Message ID | 20200827140020.159627-1-warthog618@gmail.com |
|---|---|
| Headers | show |
| Series | gpio: cdev: add uAPI v2 | expand |
On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <warthog618@gmail.com> wrote: > This patchset defines and implements a new version of the > GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add > support for debounce, event sequence numbers, and allow for requested > lines with different configurations. > It provides some future proofing by adding optional configuration fields > and padding reserved for future use. > > The series can be partitioned into three blocks; the first two patches > are minor fixes that impact later patches, the next eleven contain the > v2 uAPI definition and implementation, and the final seven port the GPIO > tools to the v2 uAPI and extend them to use new uAPI features. > > The more complicated patches include their own commentary where > appropriate. I'm ready to queue this now. Certainly any remaining snags can be fixed in-tree. It kind of keeps in tradition with proper software projects "plan to throw one away" which is what we have traditionally done several times: the first Bluetooh framework was tossed, JFFS was tossed for JFFS2, Video4Linux was tossed for V4L2. So let's do this. Anyone against? I will put it on an immutable branch and then merge that in for devel. Yours, Linus Walleij
On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <warthog618@gmail.com> wrote: > > > This patchset defines and implements a new version of the > > GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add > > support for debounce, event sequence numbers, and allow for requested > > lines with different configurations. > > It provides some future proofing by adding optional configuration fields > > and padding reserved for future use. > > > > The series can be partitioned into three blocks; the first two patches > > are minor fixes that impact later patches, the next eleven contain the > > v2 uAPI definition and implementation, and the final seven port the GPIO > > tools to the v2 uAPI and extend them to use new uAPI features. > > > > The more complicated patches include their own commentary where > > appropriate. > > I'm ready to queue this now. Certainly any remaining snags can be > fixed in-tree. > > It kind of keeps in tradition with proper software projects "plan to > throw one away" which is what we have traditionally done several > times: the first Bluetooh framework was tossed, JFFS was tossed > for JFFS2, Video4Linux was tossed for V4L2. So let's do this. > > Anyone against? I will put it on an immutable branch and then merge > that in for devel. > Hi Linus, please hold it maybe for one more week - I'd love to have some more people take a look at the user facing header at least. Andy is usually very thorough in his reviews so I'm Ccing him here. I'll too skim through the series one more time. Bart
On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote: > On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > This patchset defines and implements a new version of the > > > GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add > > > support for debounce, event sequence numbers, and allow for requested > > > lines with different configurations. > > > It provides some future proofing by adding optional configuration fields > > > and padding reserved for future use. > > > > > > The series can be partitioned into three blocks; the first two patches > > > are minor fixes that impact later patches, the next eleven contain the > > > v2 uAPI definition and implementation, and the final seven port the GPIO > > > tools to the v2 uAPI and extend them to use new uAPI features. > > > > > > The more complicated patches include their own commentary where > > > appropriate. > > > > I'm ready to queue this now. Certainly any remaining snags can be > > fixed in-tree. > > > > It kind of keeps in tradition with proper software projects "plan to > > throw one away" which is what we have traditionally done several > > times: the first Bluetooh framework was tossed, JFFS was tossed > > for JFFS2, Video4Linux was tossed for V4L2. So let's do this. > > > > Anyone against? I will put it on an immutable branch and then merge > > that in for devel. > > > > Hi Linus, > > please hold it maybe for one more week - I'd love to have some more > people take a look at the user facing header at least. Andy is usually > very thorough in his reviews so I'm Ccing him here. > > I'll too skim through the series one more time. > I'd be happier with more eyeballs over it before it goes in as well. I'm also wondering if it is worthwhile dropping the restriction on changing edge detection configuration, that is present in gpio_v2_line_config_change_validate() in patch 10, so long as userspace is made aware that changing it may result in lost interrupts as we may need to release and re-request the interrupt to effect the change. Similarly changing debounce while edge detection is enabled, that is disallowed in patch 12. My policy when writing the implementation was to disallow any operation that may have unanticiapted side-effects, and forcing userspace to release and re-request the line(s), but that may be overly restrictive? The particular use case I am considering is one I had been asked about - changing a requested line from input with edge detection to output, and vice versa. Losing interrupts isn't really an issue for this use case - it is expected. Yet the current implementation requires a re-request. Cheers, Kent.
On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <warthog618@gmail.com> wrote: > The particular use case I am considering is one I had been asked about - > changing a requested line from input with edge detection to output, and > vice versa. Losing interrupts isn't really an issue for this use case - > it is expected. Yet the current implementation requires a re-request. This is possible to do for in-kernel users, but I don't know if that makes sense for userspace. It is for one-offs and prototyping after all, there is no need (IMO) to make it overly convenient for users to implement all kind of weirdness in userspace unless there is a very real use case. Yours, Linus Walleij
On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote: > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <warthog618@gmail.com> wrote: > > > The particular use case I am considering is one I had been asked about - > > changing a requested line from input with edge detection to output, and > > vice versa. Losing interrupts isn't really an issue for this use case - > > it is expected. Yet the current implementation requires a re-request. > > This is possible to do for in-kernel users, but I don't know if that makes > sense for userspace. It is for one-offs and prototyping after all, there > is no need (IMO) to make it overly convenient for users to implement > all kind of weirdness in userspace unless there is a very real use case. > Fair point - in fact it is the same one that made me reconsider why I was so concerned about potentially losing an edge event in a few rare corner cases. Another point for this change are that it actually simplifies the kernel code, as it takes as much code to detect and filter these cases as it does to include them in the normal flow. I had a play with it yesterday and the change removes two whole functions, gpio_v2_line_config_change_validate() and gpio_v2_line_config_has_edge_detection() at the expense of making debounce_update() a little more complicated. I'm happy to put together a v6 that incorporates those changes if there aren't any strenuous objections - we can always revert to v5. Or I could mail the couple of patches I've made and if they seem reasonable then I could merge them into this set? Cheers, Kent.
On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote: > > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > The particular use case I am considering is one I had been asked about - > > > changing a requested line from input with edge detection to output, and > > > vice versa. Losing interrupts isn't really an issue for this use case - > > > it is expected. Yet the current implementation requires a re-request. > > > > This is possible to do for in-kernel users, but I don't know if that makes > > sense for userspace. It is for one-offs and prototyping after all, there > > is no need (IMO) to make it overly convenient for users to implement > > all kind of weirdness in userspace unless there is a very real use case. > > > > Fair point - in fact it is the same one that made me reconsider why I > was so concerned about potentially losing an edge event in a few rare > corner cases. > > Another point for this change are that it actually simplifies the kernel > code, as it takes as much code to detect and filter these cases as it > does to include them in the normal flow. > > I had a play with it yesterday and the change removes two whole > functions, gpio_v2_line_config_change_validate() and > gpio_v2_line_config_has_edge_detection() at the expense of making > debounce_update() a little more complicated. I'm happy to put together a > v6 that incorporates those changes if there aren't any strenuous > objections - we can always revert to v5. Or I could mail the couple of > patches I've made and if they seem reasonable then I could merge them > into this set? > > Cheers, > Kent. I personally like v6 more. The code is more elegant and we've also tried limiting GPIO chardev features before and now we're doing v2 so let's not make the same mistake twice. :) I'll try to review v6 in detail later today. Bartosz
On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote: > On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <linus.walleij@linaro.org> wrote: > please hold it maybe for one more week - I'd love to have some more > people take a look at the user facing header at least. Andy is usually > very thorough in his reviews so I'm Ccing him here. > > I'll too skim through the series one more time. Thank you, Bart. I think you, guys know how to do that best. Unfortunately I was almost squeezed under pile of several tasks and didn't find time for this. Meanwhile I have sent an updated fix for v1 as suggested by Arnd. It works for me.
On Thu, Sep 10, 2020 at 1:10 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote: > > On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > please hold it maybe for one more week - I'd love to have some more > > people take a look at the user facing header at least. Andy is usually > > very thorough in his reviews so I'm Ccing him here. > > > > I'll too skim through the series one more time. > > Thank you, Bart. > > I think you, guys know how to do that best. Unfortunately I was almost squeezed > under pile of several tasks and didn't find time for this. > If we knew, we wouldn't be needing a v2. :) > Meanwhile I have sent an updated fix for v1 as suggested by Arnd. It works for > me. > Cool, I'll take a look at that. Bart
On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote: > On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote: > > > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > The particular use case I am considering is one I had been asked about - > > > > changing a requested line from input with edge detection to output, and > > > > vice versa. Losing interrupts isn't really an issue for this use case - > > > > it is expected. Yet the current implementation requires a re-request. > > > > > > This is possible to do for in-kernel users, but I don't know if that makes > > > sense for userspace. It is for one-offs and prototyping after all, there > > > is no need (IMO) to make it overly convenient for users to implement > > > all kind of weirdness in userspace unless there is a very real use case. > > > > > > > Fair point - in fact it is the same one that made me reconsider why I > > was so concerned about potentially losing an edge event in a few rare > > corner cases. > > > > Another point for this change are that it actually simplifies the kernel > > code, as it takes as much code to detect and filter these cases as it > > does to include them in the normal flow. > > > > I had a play with it yesterday and the change removes two whole > > functions, gpio_v2_line_config_change_validate() and > > gpio_v2_line_config_has_edge_detection() at the expense of making > > debounce_update() a little more complicated. I'm happy to put together a > > v6 that incorporates those changes if there aren't any strenuous > > objections - we can always revert to v5. Or I could mail the couple of > > patches I've made and if they seem reasonable then I could merge them > > into this set? > > > > Cheers, > > Kent. > > I personally like v6 more. The code is more elegant and we've also > tried limiting GPIO chardev features before and now we're doing v2 so > let's not make the same mistake twice. :) > > I'll try to review v6 in detail later today. Let me briefly review to this. Can you remind which patch has a top level description of what features are provided in comparison to uAPI v1? (Btw, do we have some kind of comparison table?)
On Thu, Sep 10, 2020 at 4:09 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote: > > On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote: > > > > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > The particular use case I am considering is one I had been asked about - > > > > > changing a requested line from input with edge detection to output, and > > > > > vice versa. Losing interrupts isn't really an issue for this use case - > > > > > it is expected. Yet the current implementation requires a re-request. > > > > > > > > This is possible to do for in-kernel users, but I don't know if that makes > > > > sense for userspace. It is for one-offs and prototyping after all, there > > > > is no need (IMO) to make it overly convenient for users to implement > > > > all kind of weirdness in userspace unless there is a very real use case. > > > > > > > > > > Fair point - in fact it is the same one that made me reconsider why I > > > was so concerned about potentially losing an edge event in a few rare > > > corner cases. > > > > > > Another point for this change are that it actually simplifies the kernel > > > code, as it takes as much code to detect and filter these cases as it > > > does to include them in the normal flow. > > > > > > I had a play with it yesterday and the change removes two whole > > > functions, gpio_v2_line_config_change_validate() and > > > gpio_v2_line_config_has_edge_detection() at the expense of making > > > debounce_update() a little more complicated. I'm happy to put together a > > > v6 that incorporates those changes if there aren't any strenuous > > > objections - we can always revert to v5. Or I could mail the couple of > > > patches I've made and if they seem reasonable then I could merge them > > > into this set? > > > > > > Cheers, > > > Kent. > > > > I personally like v6 more. The code is more elegant and we've also > > tried limiting GPIO chardev features before and now we're doing v2 so > > let's not make the same mistake twice. :) > > > > I'll try to review v6 in detail later today. > > Let me briefly review to this. Can you remind which patch has a top level > description of what features are provided in comparison to uAPI v1? > (Btw, do we have some kind of comparison table?) We are now at v8 for this series. The cover letter contains a lot of info and patch 4/20 defining the uAPI header explains v2 even more. I think these are the most important parts. Any implementation details can be fixed later as opposed to the API itself. Bart
On Thu, Sep 10, 2020 at 04:12:08PM +0200, Bartosz Golaszewski wrote: > On Thu, Sep 10, 2020 at 4:09 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote: > > > On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <warthog618@gmail.com> wrote: ... > > > I personally like v6 more. The code is more elegant and we've also > > > tried limiting GPIO chardev features before and now we're doing v2 so > > > let's not make the same mistake twice. :) > > > > > > I'll try to review v6 in detail later today. > > > > Let me briefly review to this. Can you remind which patch has a top level > > description of what features are provided in comparison to uAPI v1? > > (Btw, do we have some kind of comparison table?) > > We are now at v8 for this series. The cover letter contains a lot of > info and patch 4/20 defining the uAPI header explains v2 even more. I > think these are the most important parts. Any implementation details > can be fixed later as opposed to the API itself. Right, thanks for pointers!
This patchset defines and implements a new version of the GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add support for debounce, event sequence numbers, and allow for requested lines with different configurations. It provides some future proofing by adding optional configuration fields and padding reserved for future use. The series can be partitioned into three blocks; the first two patches are minor fixes that impact later patches, the next eleven contain the v2 uAPI definition and implementation, and the final seven port the GPIO tools to the v2 uAPI and extend them to use new uAPI features. The more complicated patches include their own commentary where appropriate. Cheers, Kent. Changes for v5: All changes for v5 fix issues with the gpiolib-cdev.c implementation, in patches 07-12. The uAPI is unchanged from v4, as is the port of the tools. - use IS_ALIGNED in BUILD_BUG_ON checks (patch 07) - relocate BUILD_BUG_ON checks to gpiolib_cdev_register (patch 07) - s/requies/requires/ (patch 07) - use unsigned int for variables that are never negative - change lineinfo_get() parameter from cmd to bool watch (patch 08) - flagsv2 in gpio_v2_line_info_to_v1() should be u64, not int (patch 08) - change "_locked" suffixed function names to "_unlocked" (patch 10 and 11) - be less eager breaking long lines - move commentary into checkin comment where appropriate - particularly patch 12 - restructure the request/line split - rename struct line to struct linereq, and struct edge_detector to struct line, and relocate the desc field from linereq to line. The linereq name was selected over line_request as function names such as linereq_set_values() are more clearly associated with requests than line_request_set_values(), particularly as there is also a struct line. And linereq is as informative as linerequest, so I went with the shortened form. Changes for v4: - bitmap width clarification in gpiod.h (patch 04) - fix info offset initialisation bug (patch 08 and inserting patch 01) - replace strncpy with strscpy to remove compiler warnings (patch 08 and inserting patch 02) - fix mask handling in line_get_values (patch 07) Changes for v3: - disabling the character device from the build requires EXPERT - uAPI revisions (see patch 02) - replace padding_not_zeroed with calls to memchr_inv - don't use bitops on 64-bit flags as that doesn't work on BE-32 - accept first attribute matching a line in gpio_v2_line_config.attrs rather than the last - rework lsgpio port to uAPI v2 as flags reverted to v1 like layout (since patch v2) - swapped patches 17 and 18 to apply debounce to multiple monitored lines Changes for v2: - split out cleanup patches into a separate series. - split implementation patch into a patch for each ioctl or major feature. - split tool port patch into a patch per tool. - rework uAPI to allow requested lines with different configurations. Kent Gibson (20): gpiolib: cdev: desc_to_lineinfo should set info offset gpiolib: cdev: replace strncpy with strscpy gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes gpio: uapi: define uAPI v2 gpiolib: make cdev a build option gpiolib: add build option for CDEV v1 ABI gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL gpiolib: cdev: support edge detection for uAPI v2 gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL gpiolib: cdev: support setting debounce gpio: uapi: document uAPI v1 as deprecated tools: gpio: port lsgpio to v2 uAPI tools: gpio: port gpio-watch to v2 uAPI tools: gpio: rename nlines to num_lines tools: gpio: port gpio-hammer to v2 uAPI tools: gpio: port gpio-event-mon to v2 uAPI tools: gpio: add multi-line monitoring to gpio-event-mon tools: gpio: add debounce support to gpio-event-mon drivers/gpio/Kconfig | 29 +- drivers/gpio/Makefile | 2 +- drivers/gpio/gpiolib-cdev.c | 1315 +++++++++++++++++++++++++++++++++-- drivers/gpio/gpiolib-cdev.h | 15 + drivers/gpio/gpiolib.c | 2 + drivers/gpio/gpiolib.h | 6 + include/uapi/linux/gpio.h | 316 ++++++++- tools/gpio/gpio-event-mon.c | 146 ++-- tools/gpio/gpio-hammer.c | 56 +- tools/gpio/gpio-utils.c | 127 ++-- tools/gpio/gpio-utils.h | 50 +- tools/gpio/gpio-watch.c | 16 +- tools/gpio/lsgpio.c | 60 +- 13 files changed, 1910 insertions(+), 230 deletions(-) base-commit: 22cc422070d9a9a399f8a70b89f1b852945444cb