Message ID | 20200831032006.1019978-1-warthog618@gmail.com |
---|---|
Headers | show |
Series | gpio: cdev: add uAPI v2 | expand |
On Thu, Sep 03, 2020 at 10:02:04AM +0200, Bartosz Golaszewski wrote: > On Mon, Aug 31, 2020 at 5:21 AM 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. > > > > Cheers, > > Kent. > > > > Changes for v6: > > - flags variable in linereq_create() should be u64 not unsigned long > > (patch 07) > > - remove restrictions on configuration changes - any change from one > > valid state to another valid state is allowed. (patches 09, 10, 12) > > > > 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 | 1273 +++++++++++++++++++++++++++++++++-- > > drivers/gpio/gpiolib-cdev.h | 15 + > > drivers/gpio/gpiolib.c | 5 + > > 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, 1871 insertions(+), 230 deletions(-) > > > > > > base-commit: feeaefd378cae2f6840f879d6123ef265f8aee79 > > -- > > 2.28.0 > > > > To me it looks good, just a couple nits here and there and some questions. > > I think it's worth deciding whether we want to keep the selftests in > tools/testing/selftests/gpio/ and then maybe consider porting > gpio-mockup-chardev.c to V2 or simply outsource it entirely to > libgpiod. > Ooops - I wasn't even aware they existed - though it had crossed my mind that the kernel should have some selftests somewhere - I use the libgpiod tests, from my libgpiod port, and my own Go based test suite for my testing, as well as some smoke tests with the tools/gpio. The libgpiod tests only cover v1 equivalent functionality, while my Go tests cover the complete uAPI, and both v1 and v2. It would be good for the kernel to at least have some smoke tests to confirm basic functionality, even thorough testing is left to a userspace library. So the existing tests should be ported to v2, though should also retain the v1 tests if v1 is still compiled in. Cheers, Kent.
On Thu, Sep 03, 2020 at 04:37:50PM +0800, Kent Gibson wrote: > On Thu, Sep 03, 2020 at 10:02:04AM +0200, Bartosz Golaszewski wrote: > > On Mon, Aug 31, 2020 at 5:21 AM Kent Gibson <warthog618@gmail.com> wrote: > > > [snip] > > > > To me it looks good, just a couple nits here and there and some questions. > > > > I think it's worth deciding whether we want to keep the selftests in > > tools/testing/selftests/gpio/ and then maybe consider porting > > gpio-mockup-chardev.c to V2 or simply outsource it entirely to > > libgpiod. > > > > Ooops - I wasn't even aware they existed - though it had crossed my mind > that the kernel should have some selftests somewhere - I use the libgpiod > tests, from my libgpiod port, and my own Go based test suite for my testing, > as well as some smoke tests with the tools/gpio. > > The libgpiod tests only cover v1 equivalent functionality, while my Go > tests cover the complete uAPI, and both v1 and v2. > > It would be good for the kernel to at least have some smoke tests to > confirm basic functionality, even thorough testing is left to a > userspace library. So the existing tests should be ported to v2, though > should also retain the v1 tests if v1 is still compiled in. > I've got a v7 ready to submit that includes a couple of patches for the gpio-mockup selftests (their primary purpose appears to be testing the mockup module, rather than the GPIO ABI), but I now notice that the selftests/gpio section of the tree has a different maintainer: scripts/get_maintainer.pl 0021-selftests-gpio-port-to-GPIO-uAPI-v2.patch Bamvor Jian Zhang <bamv2005@gmail.com> (maintainer:GPIO MOCKUP DRIVER) Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK) linux-gpio@vger.kernel.org (open list:GPIO MOCKUP DRIVER) linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) linux-kernel@vger.kernel.org (open list) The v7 patch up to that point restores the functions that the selftests are using so that they build and run again. So I should hold off on the selftest patches and submit them separately after the GPIO changes are in? Cheers, Kent.
On Tue, Sep 8, 2020 at 5:24 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 9/4/20 7:02 AM, Bartosz Golaszewski wrote: > > On Fri, Sep 4, 2020 at 2:52 PM Kent Gibson <warthog618@gmail.com> wrote: > >> > >> On Thu, Sep 03, 2020 at 04:37:50PM +0800, Kent Gibson wrote: > >>> On Thu, Sep 03, 2020 at 10:02:04AM +0200, Bartosz Golaszewski wrote: > >>>> On Mon, Aug 31, 2020 at 5:21 AM Kent Gibson <warthog618@gmail.com> wrote: > >>>>> > >> [snip] > >>>> > >>>> To me it looks good, just a couple nits here and there and some questions. > >>>> > >>>> I think it's worth deciding whether we want to keep the selftests in > >>>> tools/testing/selftests/gpio/ and then maybe consider porting > >>>> gpio-mockup-chardev.c to V2 or simply outsource it entirely to > >>>> libgpiod. > >>>> > >>> > >>> Ooops - I wasn't even aware they existed - though it had crossed my mind > >>> that the kernel should have some selftests somewhere - I use the libgpiod > >>> tests, from my libgpiod port, and my own Go based test suite for my testing, > >>> as well as some smoke tests with the tools/gpio. > >>> > >>> The libgpiod tests only cover v1 equivalent functionality, while my Go > >>> tests cover the complete uAPI, and both v1 and v2. > >>> > >>> It would be good for the kernel to at least have some smoke tests to > >>> confirm basic functionality, even thorough testing is left to a > >>> userspace library. So the existing tests should be ported to v2, though > >>> should also retain the v1 tests if v1 is still compiled in. > >>> > >> > >> I've got a v7 ready to submit that includes a couple of patches for the > >> gpio-mockup selftests (their primary purpose appears to be testing the > >> mockup module, rather than the GPIO ABI), but I now notice that the > >> selftests/gpio section of the tree has a different maintainer: > >> > >> scripts/get_maintainer.pl 0021-selftests-gpio-port-to-GPIO-uAPI-v2.patch > >> Bamvor Jian Zhang <bamv2005@gmail.com> (maintainer:GPIO MOCKUP DRIVER) > >> Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK) > >> linux-gpio@vger.kernel.org (open list:GPIO MOCKUP DRIVER) > >> linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) > >> linux-kernel@vger.kernel.org (open list) > > > > Bamvor, Shuah: do you still have interest in maintaining these, or can > > we update MAINTAINERS? > > > > I maintain kselftests and gpio selftest falls under that. Please send > selftest patches to me so I can review them. > > As for the gpio mock driver and test itself, you will have to wait for > Bamvor to respond. > Hi Shuah, I've been de facto maintaining gpio-mockup for a couple years now. Bamvor has been quite inactive as far as gpio testing goes. I think it's fine if you ack the selftests changes. In fact: I don't want selftests to block getting V2 uAPI upstream so if that'll look like it's going to take more time then I'm for merging V2 without any changes to selftests - in the end we have tests in user-space already. Bart > >> > >> The v7 patch up to that point restores the functions that the selftests > >> are using so that they build and run again. > > This test has been problematic because of its dependency on tools/gpio. > > >> So I should hold off on the selftest patches and submit them separately > >> after the GPIO changes are in? > >> > > Please send me the selftest patches. Also see the comments in > selftests/Makefile about excluding the gpio test from default run. > > thanks, > -- Shuah > >
On 9/8/20 9:54 AM, Bartosz Golaszewski wrote: > On Tue, Sep 8, 2020 at 5:24 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 9/4/20 7:02 AM, Bartosz Golaszewski wrote: >>> On Fri, Sep 4, 2020 at 2:52 PM Kent Gibson <warthog618@gmail.com> wrote: >>>> >>>> On Thu, Sep 03, 2020 at 04:37:50PM +0800, Kent Gibson wrote: >>>>> On Thu, Sep 03, 2020 at 10:02:04AM +0200, Bartosz Golaszewski wrote: >>>>>> On Mon, Aug 31, 2020 at 5:21 AM Kent Gibson <warthog618@gmail.com> wrote: >>>>>>> >>>> [snip] >>>>>> >>>>>> To me it looks good, just a couple nits here and there and some questions. >>>>>> >>>>>> I think it's worth deciding whether we want to keep the selftests in >>>>>> tools/testing/selftests/gpio/ and then maybe consider porting >>>>>> gpio-mockup-chardev.c to V2 or simply outsource it entirely to >>>>>> libgpiod. >>>>>> >>>>> >>>>> Ooops - I wasn't even aware they existed - though it had crossed my mind >>>>> that the kernel should have some selftests somewhere - I use the libgpiod >>>>> tests, from my libgpiod port, and my own Go based test suite for my testing, >>>>> as well as some smoke tests with the tools/gpio. >>>>> >>>>> The libgpiod tests only cover v1 equivalent functionality, while my Go >>>>> tests cover the complete uAPI, and both v1 and v2. >>>>> >>>>> It would be good for the kernel to at least have some smoke tests to >>>>> confirm basic functionality, even thorough testing is left to a >>>>> userspace library. So the existing tests should be ported to v2, though >>>>> should also retain the v1 tests if v1 is still compiled in. >>>>> >>>> >>>> I've got a v7 ready to submit that includes a couple of patches for the >>>> gpio-mockup selftests (their primary purpose appears to be testing the >>>> mockup module, rather than the GPIO ABI), but I now notice that the >>>> selftests/gpio section of the tree has a different maintainer: >>>> >>>> scripts/get_maintainer.pl 0021-selftests-gpio-port-to-GPIO-uAPI-v2.patch >>>> Bamvor Jian Zhang <bamv2005@gmail.com> (maintainer:GPIO MOCKUP DRIVER) >>>> Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK) >>>> linux-gpio@vger.kernel.org (open list:GPIO MOCKUP DRIVER) >>>> linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) >>>> linux-kernel@vger.kernel.org (open list) >>> >>> Bamvor, Shuah: do you still have interest in maintaining these, or can >>> we update MAINTAINERS? >>> >> >> I maintain kselftests and gpio selftest falls under that. Please send >> selftest patches to me so I can review them. >> >> As for the gpio mock driver and test itself, you will have to wait for >> Bamvor to respond. >> > > Hi Shuah, > > I've been de facto maintaining gpio-mockup for a couple years now. > Bamvor has been quite inactive as far as gpio testing goes. I think > it's fine if you ack the selftests changes. > That is fine. I can do quick review and Ack so you can take them through gpio tree. > In fact: I don't want selftests to block getting V2 uAPI upstream so > if that'll look like it's going to take more time then I'm for merging > V2 without any changes to selftests - in the end we have tests in > user-space already. > Tests and features go through subsystem trees to avoid delays. Please make sure the test doesn't break the default kselftest build/run. In the future it would help if you include all the maintainers on the patch series, so I can review the tests from the framework angle to see if they build/run correctly. thanks, -- Shuah
On Tue, Sep 08, 2020 at 10:04:05AM -0600, Shuah Khan wrote: > On 9/8/20 9:54 AM, Bartosz Golaszewski wrote: > > On Tue, Sep 8, 2020 at 5:24 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > > > > > On 9/4/20 7:02 AM, Bartosz Golaszewski wrote: > > > > On Fri, Sep 4, 2020 at 2:52 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > On Thu, Sep 03, 2020 at 04:37:50PM +0800, Kent Gibson wrote: > > > > > > On Thu, Sep 03, 2020 at 10:02:04AM +0200, Bartosz Golaszewski wrote: > > > > > > > On Mon, Aug 31, 2020 at 5:21 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > To me it looks good, just a couple nits here and there and some questions. > > > > > > > > > > > > > > I think it's worth deciding whether we want to keep the selftests in > > > > > > > tools/testing/selftests/gpio/ and then maybe consider porting > > > > > > > gpio-mockup-chardev.c to V2 or simply outsource it entirely to > > > > > > > libgpiod. > > > > > > > > > > > > > > > > > > > Ooops - I wasn't even aware they existed - though it had crossed my mind > > > > > > that the kernel should have some selftests somewhere - I use the libgpiod > > > > > > tests, from my libgpiod port, and my own Go based test suite for my testing, > > > > > > as well as some smoke tests with the tools/gpio. > > > > > > > > > > > > The libgpiod tests only cover v1 equivalent functionality, while my Go > > > > > > tests cover the complete uAPI, and both v1 and v2. > > > > > > > > > > > > It would be good for the kernel to at least have some smoke tests to > > > > > > confirm basic functionality, even thorough testing is left to a > > > > > > userspace library. So the existing tests should be ported to v2, though > > > > > > should also retain the v1 tests if v1 is still compiled in. > > > > > > > > > > > > > > > > I've got a v7 ready to submit that includes a couple of patches for the > > > > > gpio-mockup selftests (their primary purpose appears to be testing the > > > > > mockup module, rather than the GPIO ABI), but I now notice that the > > > > > selftests/gpio section of the tree has a different maintainer: > > > > > > > > > > scripts/get_maintainer.pl 0021-selftests-gpio-port-to-GPIO-uAPI-v2.patch > > > > > Bamvor Jian Zhang <bamv2005@gmail.com> (maintainer:GPIO MOCKUP DRIVER) > > > > > Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK) > > > > > linux-gpio@vger.kernel.org (open list:GPIO MOCKUP DRIVER) > > > > > linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) > > > > > linux-kernel@vger.kernel.org (open list) > > > > > > > > Bamvor, Shuah: do you still have interest in maintaining these, or can > > > > we update MAINTAINERS? > > > > > > > > > > I maintain kselftests and gpio selftest falls under that. Please send > > > selftest patches to me so I can review them. > > > > > > As for the gpio mock driver and test itself, you will have to wait for > > > Bamvor to respond. > > > > > > > Hi Shuah, > > > > I've been de facto maintaining gpio-mockup for a couple years now. > > Bamvor has been quite inactive as far as gpio testing goes. I think > > it's fine if you ack the selftests changes. > > > > That is fine. I can do quick review and Ack so you can take them > through gpio tree. > > > In fact: I don't want selftests to block getting V2 uAPI upstream so > > if that'll look like it's going to take more time then I'm for merging > > V2 without any changes to selftests - in the end we have tests in > > user-space already. > > > > Tests and features go through subsystem trees to avoid delays. Please > make sure the test doesn't break the default kselftest build/run. > > In the future it would help if you include all the maintainers on the > patch series, so I can review the tests from the framework angle to > see if they build/run correctly. > To clarify, the patches have been submitted to the correct maintainers. While this v6, and those before, inadvertently broke the gpio selftests by removing code they depend on, there have been no code changes in the selftest tree, and so nothing for you to review. The v7 of this series restored the functions that the selftests use so that they again build and run - still nothing for you to review. While I had patches for the selftests available for v7, I pulled them from the patch series as I didn't want to bother you or Bamvor with the other patches that you wouldn't be interested in. Further, the gpio selftests are intended to test the gpio-mockup, as evidenced by Bamvor being their maintainer and the code itself. There have been no changes to the mockup here, and the existing selftests remain valid without being ported to the latest GPIO uAPI. Porting them to the latest uAPI, and then removing the resulting dead code from tools/gpio, is a nice to have that can wait and shouldn't block getting the uAPI changes in tree. Cheers, Kent.