[RFC,v1,02/20] gpio: Add GPIO polling interface to GPIO lib

From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>

Some Intel Timed I/O devices do not implement IRQ functionality. Augment
read() interface to allow polling.

Add two GPIO device methods: setup_poll() and poll():
- setup_poll() configures the GPIO interface e.g. capture rising edges
- poll() checks for events on the interface

To implement polling, the driver must implement the two functions above
and should either leave to_irq() method NULL or return irq 0.

setup_poll() should configure the hardware to 'listen' for input events.

poll() driver implementation must return the realtime timestamp
corresponding to the event and -EAGAIN if no data is available.

Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
Signed-off-by: Tamal Saha <tamal.saha@intel.com>
Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Reviewed-by: Mark Gross <mgross@linux.intel.com>
---
 drivers/gpio/gpiolib-cdev.c | 28 ++++++++++++++++++++++++++--
 include/linux/gpio/driver.h | 19 +++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
>
> From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
>
> Some Intel Timed I/O devices do not implement IRQ functionality. Augment
> read() interface to allow polling.
>
> Add two GPIO device methods: setup_poll() and poll():
> - setup_poll() configures the GPIO interface e.g. capture rising edges
> - poll() checks for events on the interface
>
> To implement polling, the driver must implement the two functions above
> and should either leave to_irq() method NULL or return irq 0.
>
> setup_poll() should configure the hardware to 'listen' for input events.
>
> poll() driver implementation must return the realtime timestamp
> corresponding to the event and -EAGAIN if no data is available.
>
> Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>
> ---

Interesting. So the idea is to allow user-space to read line events as
if they were generated by interrupts handled in the kernel. While this
whole series has a long way to go and this patch looks wrong to me in
several places at first glance, I find the idea interesting. Cc'ing
Kent who's the author of most of this code - Kent: what do you think?

Bart

>  drivers/gpio/gpiolib-cdev.c | 28 ++++++++++++++++++++++++++--
>  include/linux/gpio/driver.h | 19 +++++++++++++++++++
>  2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index c7b5446d01fd..4741bf34750b 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1227,13 +1227,34 @@ static ssize_t linereq_read(struct file *file,
>                             loff_t *f_ps)

Why would you do this in linereq_read()? Userspace ends up in
linereq_poll() when it calls poll().

>  {
>         struct linereq *lr = file->private_data;
> +       struct gpioevent_poll_data poll_data;
>         struct gpio_v2_line_event le;
>         ssize_t bytes_read = 0;
> -       int ret;
> +       int ret, offset;
>
>         if (count < sizeof(le))
>                 return -EINVAL;
>
> +       /* Without an IRQ, we can only poll */
> +       offset = gpio_chip_hwgpio(lr->gdev->descs);
> +       if (lr->lines[offset].irq == 0) {
> +               struct gpio_v2_line_event *event;
> +
> +               if (!(file->f_flags & O_NONBLOCK))
> +                       return -ENODEV;
> +
> +               ret = lr->gdev->chip->do_poll(lr->gdev->chip, offset,
> +                                             lr->lines[offset].eflags, &poll_data);

What if the driver doesn't implement do_poll()?

> +               if (ret)
> +                       return ret;
> +               event = kzalloc(sizeof(*event), GFP_KERNEL);
> +               event->timestamp_ns = poll_data.timestamp;
> +               event->id = poll_data.id;
> +               if (copy_to_user(buf, (void *)&event, sizeof(event)))
> +                       return -EFAULT;
> +               return sizeof(event);
> +       }
> +
>         do {
>                 spin_lock(&lr->wait.lock);
>                 if (kfifo_is_empty(&lr->events)) {
> @@ -1314,6 +1335,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>  {
>         struct gpio_v2_line_request ulr;
>         struct gpio_v2_line_config *lc;
> +       unsigned int file_flags;
>         struct linereq *lr;
>         struct file *file;
>         u64 flags;
> @@ -1411,6 +1433,8 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>                                 goto out_free_linereq;
>                 }
>
> +               file_flags = O_RDONLY | O_CLOEXEC;
> +
>                 blocking_notifier_call_chain(&desc->gdev->notifier,
>                                              GPIO_V2_LINE_CHANGED_REQUESTED, desc);
>
> @@ -1425,7 +1449,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>         }
>
>         file = anon_inode_getfile("gpio-line", &line_fileops, lr,
> -                                 O_RDONLY | O_CLOEXEC);
> +                                 file_flags);
>         if (IS_ERR(file)) {
>                 ret = PTR_ERR(file);
>                 goto out_put_unused_fd;
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 3a268781fcec..f5b971ad40bc 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -17,6 +17,7 @@ struct device_node;
>  struct seq_file;
>  struct gpio_device;
>  struct module;
> +struct gpioevent_poll_data;
>  enum gpiod_flags;
>  enum gpio_lookup_flags;
>
> @@ -304,6 +305,11 @@ struct gpio_irq_chip {
>   * @add_pin_ranges: optional routine to initialize pin ranges, to be used when
>   *     requires special mapping of the pins that provides GPIO functionality.
>   *     It is called after adding GPIO chip and before adding IRQ chip.
> + * @setup_poll: optional routine for devices that don't support interrupts.
> + *     Takes flags argument as in/out parameter, where caller requests
> + *     event flags and driver returns accepted flags.
> + * @do_poll: optional routine for devices that don't support interrupts.
> + *     Returns event specification in data parameter.
>   * @base: identifies the first GPIO number handled by this chip;
>   *     or, if negative during registration, requests dynamic ID allocation.
>   *     DEPRECATION: providing anything non-negative and nailing the base
> @@ -396,6 +402,14 @@ struct gpio_chip {
>
>         int                     (*add_pin_ranges)(struct gpio_chip *gc);
>
> +       int                     (*setup_poll)(struct gpio_chip *chip,
> +                                             unsigned int offset,
> +                                             u32 *eflags);

Does anyone even call this?

> +
> +       int                     (*do_poll)(struct gpio_chip *chip,
> +                                          unsigned int offset, u32 eflags,
> +                                          struct gpioevent_poll_data *data);
> +
>         int                     base;
>         u16                     ngpio;
>         const char              *const *names;
> @@ -471,6 +485,11 @@ struct gpio_chip {
>  #endif /* CONFIG_OF_GPIO */
>  };
>
> +struct gpioevent_poll_data {
> +       __u64 timestamp;
> +       __u32 id;
> +};
> +
>  extern const char *gpiochip_is_requested(struct gpio_chip *gc,
>                         unsigned int offset);
>
> --
> 2.17.1
>

This patch doesn't look good - or even tested - but as I said - the
idea itself sounds reasonable in general.

Bart

On Wed, Sep 22, 2021 at 12:03:53PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> >
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> >
> > Some Intel Timed I/O devices do not implement IRQ functionality. Augment
> > read() interface to allow polling.
> >
> > Add two GPIO device methods: setup_poll() and poll():
> > - setup_poll() configures the GPIO interface e.g. capture rising edges
> > - poll() checks for events on the interface
> >
> > To implement polling, the driver must implement the two functions above
> > and should either leave to_irq() method NULL or return irq 0.
> >
> > setup_poll() should configure the hardware to 'listen' for input events.
> >
> > poll() driver implementation must return the realtime timestamp
> > corresponding to the event and -EAGAIN if no data is available.
> >
> > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> > ---
> 
> Interesting. So the idea is to allow user-space to read line events as
> if they were generated by interrupts handled in the kernel. While this
> whole series has a long way to go and this patch looks wrong to me in
> several places at first glance, I find the idea interesting. Cc'ing
> Kent who's the author of most of this code - Kent: what do you think?
> 

It is interesting that we're seeing more hardware that provides more
detailed edge info than we get from irq.  The hte patch series can also
provide hardware timestamps, but the Timed I/O could even provide the
sequence numbers.
It might be worth abstracting the edge detection so edge events could be
more easily driven by subsystems other than irq, without festooning cdev
with special cases.

I'm not a fan of the polling here though, particularly from userspace.
If polling can't be avoided (why did they not provide an irq??) then I
would do the polling in kernel and place any resulting events in the
cdev kfifo for userspace to read as per the existing events.

Of course that is without knowing a whole lot about the hardware or use
cases.  The Intel datasheet doesn't provide much in the way of data :|.

Cheers,
Kent.