mbox series

[v5,0/7] gpiolib: add an ioctl() for monitoring line status changes

Message ID 20200109115010.27814-1-brgl@bgdev.pl
Headers show
Series gpiolib: add an ioctl() for monitoring line status changes | expand

Message

Bartosz Golaszewski Jan. 9, 2020, 11:50 a.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

When discussing the recent user-space changes with Kent and while working
on dbus API for libgpiod I noticed that we really don't have any way of
keeping the line info synchronized between the kernel and user-space
processes. We can of course periodically re-read the line information or
even do it every time we want to read a property but this isn't optimal.

This series adds a new ioctl() that allows user-space to set up a watch on
the GPIO chardev file-descriptor which can then be polled for events
emitted by the kernel when the line is requested, released or its status
changed. This of course doesn't require the line to be requested. Multiple
user-space processes can watch the same lines.

This series also includes a variety of minor tweaks & fixes for problems
discovered during development. For instance it addresses a race-condition
in current line event fifo.

First two patches add new helpers to kfifo, that are used in the later
parts of the series.

v1: https://lkml.org/lkml/2019/11/27/327

v1 -> v2:
- rework the main patch of the series: re-use the existing file-descriptor
  associated with an open character device
- add a patch adding a debug message when the line event kfifo is full and
  we're discarding another event
- rework the locking mechanism for lineevent kfifo: reuse the spinlock
  from the waitqueue structure
- other minor changes

v2 -> v3:
- added patches providing new implementation for some kfifo macros
- fixed a regression in the patch reworking the line event fifo: reading
  multiple events is now still possible
- reworked the structure for new ioctl: it's now padded such that there
  be no alignment issues if running a 64-bit kernel on 32-bit userspace
- fixed a bug where one process could disable the status watch of another
- use kstrtoul() instead of atoi() in gpio-watch for string validation

v3 -> v4:
- removed a binary file checked in by mistake
- drop __func__ from debug messages
- restructure the code in the notifier call
- add comments about the alignment of the new uAPI structure
- remove a stray new line that doesn't belong in this series
- tested the series on 32-bit user-space with 64-bit kernel

v4 -> v5:
- dropped patches already merged upstream
- collected review tags

Bartosz Golaszewski (7):
  kfifo: provide noirqsave variants of spinlocked in and out helpers
  kfifo: provide kfifo_is_empty_spinlocked()
  gpiolib: rework the locking mechanism for lineevent kfifo
  gpiolib: emit a debug message when adding events to a full kfifo
  gpiolib: provide a dedicated function for setting lineinfo
  gpiolib: add new ioctl() for monitoring changes in line info
  tools: gpio: implement gpio-watch

 drivers/gpio/gpiolib.c    | 351 +++++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.h    |   1 +
 include/linux/kfifo.h     |  73 ++++++++
 include/uapi/linux/gpio.h |  30 ++++
 tools/gpio/.gitignore     |   1 +
 tools/gpio/Build          |   1 +
 tools/gpio/Makefile       |  11 +-
 tools/gpio/gpio-watch.c   |  99 +++++++++++
 8 files changed, 486 insertions(+), 81 deletions(-)
 create mode 100644 tools/gpio/gpio-watch.c

Comments

Bartosz Golaszewski Jan. 14, 2020, 7:44 a.m. UTC | #1
czw., 9 sty 2020 o 12:50 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> When discussing the recent user-space changes with Kent and while working
> on dbus API for libgpiod I noticed that we really don't have any way of
> keeping the line info synchronized between the kernel and user-space
> processes. We can of course periodically re-read the line information or
> even do it every time we want to read a property but this isn't optimal.
>
> This series adds a new ioctl() that allows user-space to set up a watch on
> the GPIO chardev file-descriptor which can then be polled for events
> emitted by the kernel when the line is requested, released or its status
> changed. This of course doesn't require the line to be requested. Multiple
> user-space processes can watch the same lines.
>
> This series also includes a variety of minor tweaks & fixes for problems
> discovered during development. For instance it addresses a race-condition
> in current line event fifo.
>
> First two patches add new helpers to kfifo, that are used in the later
> parts of the series.
>
> v1: https://lkml.org/lkml/2019/11/27/327
>
> v1 -> v2:
> - rework the main patch of the series: re-use the existing file-descriptor
>   associated with an open character device
> - add a patch adding a debug message when the line event kfifo is full and
>   we're discarding another event
> - rework the locking mechanism for lineevent kfifo: reuse the spinlock
>   from the waitqueue structure
> - other minor changes
>
> v2 -> v3:
> - added patches providing new implementation for some kfifo macros
> - fixed a regression in the patch reworking the line event fifo: reading
>   multiple events is now still possible
> - reworked the structure for new ioctl: it's now padded such that there
>   be no alignment issues if running a 64-bit kernel on 32-bit userspace
> - fixed a bug where one process could disable the status watch of another
> - use kstrtoul() instead of atoi() in gpio-watch for string validation
>
> v3 -> v4:
> - removed a binary file checked in by mistake
> - drop __func__ from debug messages
> - restructure the code in the notifier call
> - add comments about the alignment of the new uAPI structure
> - remove a stray new line that doesn't belong in this series
> - tested the series on 32-bit user-space with 64-bit kernel
>
> v4 -> v5:
> - dropped patches already merged upstream
> - collected review tags
>
> Bartosz Golaszewski (7):
>   kfifo: provide noirqsave variants of spinlocked in and out helpers
>   kfifo: provide kfifo_is_empty_spinlocked()
>   gpiolib: rework the locking mechanism for lineevent kfifo
>   gpiolib: emit a debug message when adding events to a full kfifo
>   gpiolib: provide a dedicated function for setting lineinfo
>   gpiolib: add new ioctl() for monitoring changes in line info
>   tools: gpio: implement gpio-watch

Hi Andrew,

could you Ack the first two patches in this series if you're fine with
them? The code they modify lives in lib/ and doesn't have an assigned
maintainer, so I've been told to Cc you on this series. It would be
great if we could get it in for v5.6 merge window.

Best regards,
Bartosz Golaszewski
Bartosz Golaszewski Jan. 23, 2020, 10:15 a.m. UTC | #2
wt., 14 sty 2020 o 08:44 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> czw., 9 sty 2020 o 12:50 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > When discussing the recent user-space changes with Kent and while working
> > on dbus API for libgpiod I noticed that we really don't have any way of
> > keeping the line info synchronized between the kernel and user-space
> > processes. We can of course periodically re-read the line information or
> > even do it every time we want to read a property but this isn't optimal.
> >
> > This series adds a new ioctl() that allows user-space to set up a watch on
> > the GPIO chardev file-descriptor which can then be polled for events
> > emitted by the kernel when the line is requested, released or its status
> > changed. This of course doesn't require the line to be requested. Multiple
> > user-space processes can watch the same lines.
> >
> > This series also includes a variety of minor tweaks & fixes for problems
> > discovered during development. For instance it addresses a race-condition
> > in current line event fifo.
> >
> > First two patches add new helpers to kfifo, that are used in the later
> > parts of the series.
> >
> > v1: https://lkml.org/lkml/2019/11/27/327
> >
> > v1 -> v2:
> > - rework the main patch of the series: re-use the existing file-descriptor
> >   associated with an open character device
> > - add a patch adding a debug message when the line event kfifo is full and
> >   we're discarding another event
> > - rework the locking mechanism for lineevent kfifo: reuse the spinlock
> >   from the waitqueue structure
> > - other minor changes
> >
> > v2 -> v3:
> > - added patches providing new implementation for some kfifo macros
> > - fixed a regression in the patch reworking the line event fifo: reading
> >   multiple events is now still possible
> > - reworked the structure for new ioctl: it's now padded such that there
> >   be no alignment issues if running a 64-bit kernel on 32-bit userspace
> > - fixed a bug where one process could disable the status watch of another
> > - use kstrtoul() instead of atoi() in gpio-watch for string validation
> >
> > v3 -> v4:
> > - removed a binary file checked in by mistake
> > - drop __func__ from debug messages
> > - restructure the code in the notifier call
> > - add comments about the alignment of the new uAPI structure
> > - remove a stray new line that doesn't belong in this series
> > - tested the series on 32-bit user-space with 64-bit kernel
> >
> > v4 -> v5:
> > - dropped patches already merged upstream
> > - collected review tags
> >
> > Bartosz Golaszewski (7):
> >   kfifo: provide noirqsave variants of spinlocked in and out helpers
> >   kfifo: provide kfifo_is_empty_spinlocked()
> >   gpiolib: rework the locking mechanism for lineevent kfifo
> >   gpiolib: emit a debug message when adding events to a full kfifo
> >   gpiolib: provide a dedicated function for setting lineinfo
> >   gpiolib: add new ioctl() for monitoring changes in line info
> >   tools: gpio: implement gpio-watch
>
> Hi Andrew,
>
> could you Ack the first two patches in this series if you're fine with
> them? The code they modify lives in lib/ and doesn't have an assigned
> maintainer, so I've been told to Cc you on this series. It would be
> great if we could get it in for v5.6 merge window.
>
> Best regards,
> Bartosz Golaszewski

Gentle ping.

Greg: could you maybe ack the kfifo patches so that we can get this in for v5.6?

Bart
Greg Kroah-Hartman Jan. 23, 2020, 11:37 a.m. UTC | #3
On Thu, Jan 23, 2020 at 11:15:05AM +0100, Bartosz Golaszewski wrote:
> wt., 14 sty 2020 o 08:44 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> >
> > czw., 9 sty 2020 o 12:50 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > When discussing the recent user-space changes with Kent and while working
> > > on dbus API for libgpiod I noticed that we really don't have any way of
> > > keeping the line info synchronized between the kernel and user-space
> > > processes. We can of course periodically re-read the line information or
> > > even do it every time we want to read a property but this isn't optimal.
> > >
> > > This series adds a new ioctl() that allows user-space to set up a watch on
> > > the GPIO chardev file-descriptor which can then be polled for events
> > > emitted by the kernel when the line is requested, released or its status
> > > changed. This of course doesn't require the line to be requested. Multiple
> > > user-space processes can watch the same lines.
> > >
> > > This series also includes a variety of minor tweaks & fixes for problems
> > > discovered during development. For instance it addresses a race-condition
> > > in current line event fifo.
> > >
> > > First two patches add new helpers to kfifo, that are used in the later
> > > parts of the series.
> > >
> > > v1: https://lkml.org/lkml/2019/11/27/327
> > >
> > > v1 -> v2:
> > > - rework the main patch of the series: re-use the existing file-descriptor
> > >   associated with an open character device
> > > - add a patch adding a debug message when the line event kfifo is full and
> > >   we're discarding another event
> > > - rework the locking mechanism for lineevent kfifo: reuse the spinlock
> > >   from the waitqueue structure
> > > - other minor changes
> > >
> > > v2 -> v3:
> > > - added patches providing new implementation for some kfifo macros
> > > - fixed a regression in the patch reworking the line event fifo: reading
> > >   multiple events is now still possible
> > > - reworked the structure for new ioctl: it's now padded such that there
> > >   be no alignment issues if running a 64-bit kernel on 32-bit userspace
> > > - fixed a bug where one process could disable the status watch of another
> > > - use kstrtoul() instead of atoi() in gpio-watch for string validation
> > >
> > > v3 -> v4:
> > > - removed a binary file checked in by mistake
> > > - drop __func__ from debug messages
> > > - restructure the code in the notifier call
> > > - add comments about the alignment of the new uAPI structure
> > > - remove a stray new line that doesn't belong in this series
> > > - tested the series on 32-bit user-space with 64-bit kernel
> > >
> > > v4 -> v5:
> > > - dropped patches already merged upstream
> > > - collected review tags
> > >
> > > Bartosz Golaszewski (7):
> > >   kfifo: provide noirqsave variants of spinlocked in and out helpers
> > >   kfifo: provide kfifo_is_empty_spinlocked()
> > >   gpiolib: rework the locking mechanism for lineevent kfifo
> > >   gpiolib: emit a debug message when adding events to a full kfifo
> > >   gpiolib: provide a dedicated function for setting lineinfo
> > >   gpiolib: add new ioctl() for monitoring changes in line info
> > >   tools: gpio: implement gpio-watch
> >
> > Hi Andrew,
> >
> > could you Ack the first two patches in this series if you're fine with
> > them? The code they modify lives in lib/ and doesn't have an assigned
> > maintainer, so I've been told to Cc you on this series. It would be
> > great if we could get it in for v5.6 merge window.
> >
> > Best regards,
> > Bartosz Golaszewski
> 
> Gentle ping.
> 
> Greg: could you maybe ack the kfifo patches so that we can get this in for v5.6?

I am not the kfifo maintainer :(
Bartosz Golaszewski Jan. 23, 2020, 2:06 p.m. UTC | #4
czw., 23 sty 2020 o 12:37 Greg Kroah-Hartman
<gregkh@linuxfoundation.org> napisał(a):
>
> > > > Bartosz Golaszewski (7):
> > > >   kfifo: provide noirqsave variants of spinlocked in and out helpers
> > > >   kfifo: provide kfifo_is_empty_spinlocked()
> > > >   gpiolib: rework the locking mechanism for lineevent kfifo
> > > >   gpiolib: emit a debug message when adding events to a full kfifo
> > > >   gpiolib: provide a dedicated function for setting lineinfo
> > > >   gpiolib: add new ioctl() for monitoring changes in line info
> > > >   tools: gpio: implement gpio-watch
> > >
> > > Hi Andrew,
> > >
> > > could you Ack the first two patches in this series if you're fine with
> > > them? The code they modify lives in lib/ and doesn't have an assigned
> > > maintainer, so I've been told to Cc you on this series. It would be
> > > great if we could get it in for v5.6 merge window.
> > >
> > > Best regards,
> > > Bartosz Golaszewski
> >
> > Gentle ping.
> >
> > Greg: could you maybe ack the kfifo patches so that we can get this in for v5.6?
>
> I am not the kfifo maintainer :(

Nobody is... :(

I resent the series with some people who acked kfifo patches in the past.

Bartosz