| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMpxmJWeZP-f-3BoWwX7PkWNZySn5RP=rc4cVyLEwYmSb6if+w@mail.gmail.com>
Date: Wed, 22 Sep 2021 12:03:53 +0200
From: Bartosz Golaszewski <bgolaszewski@...libre.com>
To: lakshmi.sowjanya.d@...el.com
Cc: Linus Walleij <linus.walleij@...aro.org>,
linux-gpio <linux-gpio@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, mgross@...ux.intel.com,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
tamal.saha@...el.com, bala.senthil@...el.com,
Kent Gibson <warthog618@...il.com>
Subject: Re: [RFC PATCH v1 02/20] gpio: Add GPIO polling interface to GPIO lib
On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@...el.com> wrote:
>
> From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.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@...el.com>
> Signed-off-by: Christopher Hall <christopher.s.hall@...el.com>
> Signed-off-by: Tamal Saha <tamal.saha@...el.com>
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.com>
> Reviewed-by: Mark Gross <mgross@...ux.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
Powered by blists - more mailing lists