Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210824164801.28896-1-lakshmi.sowjanya.d@intel.com> <20210824164801.28896-3-lakshmi.sowjanya.d@intel.com>
In-Reply-To: <20210824164801.28896-3-lakshmi.sowjanya.d@intel.com>
From:   Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Wed, 22 Sep 2021 12:03:53 +0200
Message-ID: <CAMpxmJWeZP-f-3BoWwX7PkWNZySn5RP=rc4cVyLEwYmSb6if+w@mail.gmail.com>
Subject: Re: [RFC PATCH v1 02/20] gpio: Add GPIO polling interface to GPIO lib
To:     lakshmi.sowjanya.d@intel.com
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio <linux-gpio@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, mgross@linux.intel.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        tamal.saha@intel.com, bala.senthil@intel.com,
        Kent Gibson <warthog618@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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