* [RESEND PATCH v6 1/7] kfifo: provide noirqsave variants of spinlocked in and out helpers
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 2/7] kfifo: provide kfifo_is_empty_spinlocked() Bartosz Golaszewski
` (6 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, Stefani Seibold
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Provide variants of spinlocked kfifo_in() and kfifo_out() routines which
don't disable interrupts.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Stefani Seibold <stefani@seibold.net>
---
include/linux/kfifo.h | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index fc4b0b10210f..123c200ed7cb 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -517,6 +517,26 @@ __kfifo_uint_must_check_helper( \
__ret; \
})
+/**
+ * kfifo_in_spinlocked_noirqsave - put data into fifo using a spinlock for
+ * locking, don't disable interrupts
+ * @fifo: address of the fifo to be used
+ * @buf: the data to be added
+ * @n: number of elements to be added
+ * @lock: pointer to the spinlock to use for locking
+ *
+ * This is a variant of kfifo_in_spinlocked() but uses spin_lock/unlock()
+ * for locking and doesn't disable interrupts.
+ */
+#define kfifo_in_spinlocked_noirqsave(fifo, buf, n, lock) \
+({ \
+ unsigned int __ret; \
+ spin_lock(lock); \
+ __ret = kfifo_in(fifo, buf, n); \
+ spin_unlock(lock); \
+ __ret; \
+})
+
/* alias for kfifo_in_spinlocked, will be removed in a future release */
#define kfifo_in_locked(fifo, buf, n, lock) \
kfifo_in_spinlocked(fifo, buf, n, lock)
@@ -569,6 +589,28 @@ __kfifo_uint_must_check_helper( \
}) \
)
+/**
+ * kfifo_out_spinlocked_noirqsave - get data from the fifo using a spinlock
+ * for locking, don't disable interrupts
+ * @fifo: address of the fifo to be used
+ * @buf: pointer to the storage buffer
+ * @n: max. number of elements to get
+ * @lock: pointer to the spinlock to use for locking
+ *
+ * This is a variant of kfifo_out_spinlocked() which uses spin_lock/unlock()
+ * for locking and doesn't disable interrupts.
+ */
+#define kfifo_out_spinlocked_noirqsave(fifo, buf, n, lock) \
+__kfifo_uint_must_check_helper( \
+({ \
+ unsigned int __ret; \
+ spin_lock(lock); \
+ __ret = kfifo_out(fifo, buf, n); \
+ spin_unlock(lock); \
+ __ret; \
+}) \
+)
+
/* alias for kfifo_out_spinlocked, will be removed in a future release */
#define kfifo_out_locked(fifo, buf, n, lock) \
kfifo_out_spinlocked(fifo, buf, n, lock)
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 2/7] kfifo: provide kfifo_is_empty_spinlocked()
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 1/7] kfifo: provide noirqsave variants of spinlocked in and out helpers Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 3/7] gpiolib: rework the locking mechanism for lineevent kfifo Bartosz Golaszewski
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, Stefani Seibold
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Provide two spinlocked versions of kfifo_is_empty() to be used with
spinlocked variants of kfifo_in() and kfifo_out().
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Stefani Seibold <stefani@seibold.net>
---
include/linux/kfifo.h | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 123c200ed7cb..86249476b57f 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -246,6 +246,37 @@ __kfifo_int_must_check_helper(int val)
__tmpq->kfifo.in == __tmpq->kfifo.out; \
})
+/**
+ * kfifo_is_empty_spinlocked - returns true if the fifo is empty using
+ * a spinlock for locking
+ * @fifo: address of the fifo to be used
+ * @lock: spinlock to be used for locking
+ */
+#define kfifo_is_empty_spinlocked(fifo, lock) \
+({ \
+ unsigned long __flags; \
+ bool __ret; \
+ spin_lock_irqsave(lock, __flags); \
+ __ret = kfifo_is_empty(fifo); \
+ spin_unlock_irqrestore(lock, __flags); \
+ __ret; \
+})
+
+/**
+ * kfifo_is_empty_spinlocked_noirqsave - returns true if the fifo is empty
+ * using a spinlock for locking, doesn't disable interrupts
+ * @fifo: address of the fifo to be used
+ * @lock: spinlock to be used for locking
+ */
+#define kfifo_is_empty_spinlocked_noirqsave(fifo, lock) \
+({ \
+ bool __ret; \
+ spin_lock(lock); \
+ __ret = kfifo_is_empty(fifo); \
+ spin_unlock(lock); \
+ __ret; \
+})
+
/**
* kfifo_is_full - returns true if the fifo is full
* @fifo: address of the fifo to be used
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 3/7] gpiolib: rework the locking mechanism for lineevent kfifo
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 1/7] kfifo: provide noirqsave variants of spinlocked in and out helpers Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 2/7] kfifo: provide kfifo_is_empty_spinlocked() Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 4/7] gpiolib: emit a debug message when adding events to a full kfifo Bartosz Golaszewski
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
The read_lock mutex is supposed to prevent collisions between reading
and writing to the line event kfifo but it's actually only taken when
the events are being read from it.
Drop the mutex entirely and reuse the spinlock made available to us in
the waitqueue struct. Take the lock whenever the fifo is modified or
inspected. Drop the call to kfifo_to_user() and instead first extract
the new element from kfifo when the lock is taken and only then pass
it on to the user after the spinlock is released.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib.c | 64 +++++++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 29 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 753283486037..43d98309e725 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -787,8 +787,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
* @irq: the interrupt that trigger in response to events on this GPIO
* @wait: wait queue that handles blocking reads of events
* @events: KFIFO for the GPIO events
- * @read_lock: mutex lock to protect reads from colliding with adding
- * new events to the FIFO
* @timestamp: cache for the timestamp storing it between hardirq
* and IRQ thread, used to bring the timestamp close to the actual
* event
@@ -801,7 +799,6 @@ struct lineevent_state {
int irq;
wait_queue_head_t wait;
DECLARE_KFIFO(events, struct gpioevent_data, 16);
- struct mutex read_lock;
u64 timestamp;
};
@@ -817,7 +814,7 @@ static __poll_t lineevent_poll(struct file *filep,
poll_wait(filep, &le->wait, wait);
- if (!kfifo_is_empty(&le->events))
+ if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock))
events = EPOLLIN | EPOLLRDNORM;
return events;
@@ -830,43 +827,52 @@ static ssize_t lineevent_read(struct file *filep,
loff_t *f_ps)
{
struct lineevent_state *le = filep->private_data;
- unsigned int copied;
+ struct gpioevent_data event;
+ ssize_t bytes_read = 0;
int ret;
- if (count < sizeof(struct gpioevent_data))
+ if (count < sizeof(event))
return -EINVAL;
do {
+ spin_lock(&le->wait.lock);
if (kfifo_is_empty(&le->events)) {
- if (filep->f_flags & O_NONBLOCK)
+ if (bytes_read) {
+ spin_unlock(&le->wait.lock);
+ return bytes_read;
+ }
+
+ if (filep->f_flags & O_NONBLOCK) {
+ spin_unlock(&le->wait.lock);
return -EAGAIN;
+ }
- ret = wait_event_interruptible(le->wait,
+ ret = wait_event_interruptible_locked(le->wait,
!kfifo_is_empty(&le->events));
- if (ret)
+ if (ret) {
+ spin_unlock(&le->wait.lock);
return ret;
+ }
}
- if (mutex_lock_interruptible(&le->read_lock))
- return -ERESTARTSYS;
- ret = kfifo_to_user(&le->events, buf, count, &copied);
- mutex_unlock(&le->read_lock);
-
- if (ret)
- return ret;
-
- /*
- * If we couldn't read anything from the fifo (a different
- * thread might have been faster) we either return -EAGAIN if
- * the file descriptor is non-blocking, otherwise we go back to
- * sleep and wait for more data to arrive.
- */
- if (copied == 0 && (filep->f_flags & O_NONBLOCK))
- return -EAGAIN;
+ ret = kfifo_out(&le->events, &event, 1);
+ spin_unlock(&le->wait.lock);
+ if (ret != 1) {
+ /*
+ * This should never happen - we were holding the lock
+ * from the moment we learned the fifo is no longer
+ * empty until now.
+ */
+ ret = -EIO;
+ break;
+ }
- } while (copied == 0);
+ if (copy_to_user(buf + bytes_read, &event, sizeof(event)))
+ return -EFAULT;
+ bytes_read += sizeof(event);
+ } while (count >= bytes_read + sizeof(event));
- return copied;
+ return bytes_read;
}
static int lineevent_release(struct inode *inode, struct file *filep)
@@ -968,7 +974,8 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
return IRQ_NONE;
}
- ret = kfifo_put(&le->events, ge);
+ ret = kfifo_in_spinlocked_noirqsave(&le->events, &ge,
+ 1, &le->wait.lock);
if (ret)
wake_up_poll(&le->wait, EPOLLIN);
@@ -1083,7 +1090,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
INIT_KFIFO(le->events);
init_waitqueue_head(&le->wait);
- mutex_init(&le->read_lock);
/* Request a thread to read the events */
ret = request_threaded_irq(le->irq,
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 4/7] gpiolib: emit a debug message when adding events to a full kfifo
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
` (2 preceding siblings ...)
2020-02-11 9:19 ` [RESEND PATCH v6 3/7] gpiolib: rework the locking mechanism for lineevent kfifo Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-11 9:19 ` [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo Bartosz Golaszewski
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Currently if the line-event kfifo is full, we just silently drop any new
events. Add a ratelimited debug message so that we at least have some
trace in the kernel log of event overflow.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/gpio/gpiolib.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 43d98309e725..36afe0b2b150 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -978,6 +978,8 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
1, &le->wait.lock);
if (ret)
wake_up_poll(&le->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited("event FIFO is full - event dropped\n");
return IRQ_HANDLED;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
` (3 preceding siblings ...)
2020-02-11 9:19 ` [RESEND PATCH v6 4/7] gpiolib: emit a debug message when adding events to a full kfifo Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-03-16 16:20 ` Geert Uytterhoeven
2020-02-11 9:19 ` [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info Bartosz Golaszewski
` (2 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
We'll soon be filling out the gpioline_info structure in multiple
places. Add a separate function that given a gpio_desc sets all relevant
fields.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib.c | 98 ++++++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 43 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 36afe0b2b150..443321f9cf63 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1147,6 +1147,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
return ret;
}
+static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
+ struct gpioline_info *info)
+{
+ struct gpio_chip *chip = desc->gdev->chip;
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ if (desc->name) {
+ strncpy(info->name, desc->name, sizeof(info->name));
+ info->name[sizeof(info->name) - 1] = '\0';
+ } else {
+ info->name[0] = '\0';
+ }
+
+ if (desc->label) {
+ strncpy(info->consumer, desc->label, sizeof(info->consumer));
+ info->consumer[sizeof(info->consumer) - 1] = '\0';
+ } else {
+ info->consumer[0] = '\0';
+ }
+
+ /*
+ * Userspace only need to know that the kernel is using this GPIO so
+ * it can't use it.
+ */
+ info->flags = 0;
+ if (test_bit(FLAG_REQUESTED, &desc->flags) ||
+ test_bit(FLAG_IS_HOGGED, &desc->flags) ||
+ test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
+ test_bit(FLAG_EXPORT, &desc->flags) ||
+ test_bit(FLAG_SYSFS, &desc->flags) ||
+ !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
+ info->flags |= GPIOLINE_FLAG_KERNEL;
+ if (test_bit(FLAG_IS_OUT, &desc->flags))
+ info->flags |= GPIOLINE_FLAG_IS_OUT;
+ if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+ info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+ if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+ info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
+ GPIOLINE_FLAG_IS_OUT);
+ if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+ info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
+ GPIOLINE_FLAG_IS_OUT);
+ if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+ info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+ if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+ info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+ if (test_bit(FLAG_PULL_UP, &desc->flags))
+ info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+
+ spin_unlock_irqrestore(&gpio_lock, flags);
+}
+
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
@@ -1187,49 +1241,7 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (IS_ERR(desc))
return PTR_ERR(desc);
- if (desc->name) {
- strncpy(lineinfo.name, desc->name,
- sizeof(lineinfo.name));
- lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
- } else {
- lineinfo.name[0] = '\0';
- }
- if (desc->label) {
- strncpy(lineinfo.consumer, desc->label,
- sizeof(lineinfo.consumer));
- lineinfo.consumer[sizeof(lineinfo.consumer)-1] = '\0';
- } else {
- lineinfo.consumer[0] = '\0';
- }
-
- /*
- * Userspace only need to know that the kernel is using
- * this GPIO so it can't use it.
- */
- lineinfo.flags = 0;
- if (test_bit(FLAG_REQUESTED, &desc->flags) ||
- test_bit(FLAG_IS_HOGGED, &desc->flags) ||
- test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
- test_bit(FLAG_EXPORT, &desc->flags) ||
- test_bit(FLAG_SYSFS, &desc->flags) ||
- !pinctrl_gpio_can_use_line(chip->base + lineinfo.line_offset))
- lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
- if (test_bit(FLAG_IS_OUT, &desc->flags))
- lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
- if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
- lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
- if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
- lineinfo.flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
- GPIOLINE_FLAG_IS_OUT);
- if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
- lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
- GPIOLINE_FLAG_IS_OUT);
- if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
- lineinfo.flags |= GPIOLINE_FLAG_BIAS_DISABLE;
- if (test_bit(FLAG_PULL_DOWN, &desc->flags))
- lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
- if (test_bit(FLAG_PULL_UP, &desc->flags))
- lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+ gpio_desc_to_lineinfo(desc, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo
2020-02-11 9:19 ` [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo Bartosz Golaszewski
@ 2020-03-16 16:20 ` Geert Uytterhoeven
2020-03-17 13:40 ` Bartosz Golaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2020-03-16 16:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Bartosz Golaszewski
Hi Bartosz,
On Tue, Feb 11, 2020 at 10:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> We'll soon be filling out the gpioline_info structure in multiple
> places. Add a separate function that given a gpio_desc sets all relevant
> fields.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
This is now commit d2ac25798208fb85 ("gpiolib: provide a dedicated
function for setting lineinfo") in gpio/for-next.
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1147,6 +1147,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> return ret;
> }
>
> +static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> + struct gpioline_info *info)
> +{
> + struct gpio_chip *chip = desc->gdev->chip;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpio_lock, flags);
spinlock taken
> +
> + if (desc->name) {
> + strncpy(info->name, desc->name, sizeof(info->name));
> + info->name[sizeof(info->name) - 1] = '\0';
> + } else {
> + info->name[0] = '\0';
> + }
> +
> + if (desc->label) {
> + strncpy(info->consumer, desc->label, sizeof(info->consumer));
> + info->consumer[sizeof(info->consumer) - 1] = '\0';
> + } else {
> + info->consumer[0] = '\0';
> + }
> +
> + /*
> + * Userspace only need to know that the kernel is using this GPIO so
> + * it can't use it.
> + */
> + info->flags = 0;
> + if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> + test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> + test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
> + test_bit(FLAG_EXPORT, &desc->flags) ||
> + test_bit(FLAG_SYSFS, &desc->flags) ||
> + !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
pinctrl_gpio_can_use_line(), and pinctrl_get_device_gpio_range() called
from it, call mutex_lock():
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:281
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 652, name: lsgpio
CPU: 1 PID: 652 Comm: lsgpio Not tainted
5.6.0-rc1-koelsch-00008-gd2ac25798208fb85 #755
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[<c020e3f0>] (unwind_backtrace) from [<c020a5b8>] (show_stack+0x10/0x14)
[<c020a5b8>] (show_stack) from [<c07d31b4>] (dump_stack+0x88/0xa8)
[<c07d31b4>] (dump_stack) from [<c0241318>] (___might_sleep+0xf8/0x168)
[<c0241318>] (___might_sleep) from [<c07ec13c>] (mutex_lock+0x24/0x7c)
[<c07ec13c>] (mutex_lock) from [<c046f47c>]
(pinctrl_get_device_gpio_range+0x1c/0xb4)
[<c046f47c>] (pinctrl_get_device_gpio_range) from [<c046f5e8>]
(pinctrl_gpio_can_use_line+0x24/0x88)
[<c046f5e8>] (pinctrl_gpio_can_use_line) from [<c0478bd0>]
(gpio_ioctl+0x270/0x584)
[<c0478bd0>] (gpio_ioctl) from [<c03194c0>] (vfs_ioctl+0x20/0x38)
Reproducer is "lsgpio" with CONFIG_DEBUG_ATOMIC_SLEEP=y.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo
2020-03-16 16:20 ` Geert Uytterhoeven
@ 2020-03-17 13:40 ` Bartosz Golaszewski
0 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-03-17 13:40 UTC (permalink / raw)
To: Geert Uytterhoeven, Linus Walleij
Cc: Kent Gibson, Andy Shevchenko, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, Bartosz Golaszewski
pon., 16 mar 2020 o 17:21 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Bartosz,
>
> On Tue, Feb 11, 2020 at 10:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > We'll soon be filling out the gpioline_info structure in multiple
> > places. Add a separate function that given a gpio_desc sets all relevant
> > fields.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> This is now commit d2ac25798208fb85 ("gpiolib: provide a dedicated
> function for setting lineinfo") in gpio/for-next.
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1147,6 +1147,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > return ret;
> > }
> >
> > +static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > + struct gpioline_info *info)
> > +{
> > + struct gpio_chip *chip = desc->gdev->chip;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&gpio_lock, flags);
>
> spinlock taken
>
> > +
> > + if (desc->name) {
> > + strncpy(info->name, desc->name, sizeof(info->name));
> > + info->name[sizeof(info->name) - 1] = '\0';
> > + } else {
> > + info->name[0] = '\0';
> > + }
> > +
> > + if (desc->label) {
> > + strncpy(info->consumer, desc->label, sizeof(info->consumer));
> > + info->consumer[sizeof(info->consumer) - 1] = '\0';
> > + } else {
> > + info->consumer[0] = '\0';
> > + }
> > +
> > + /*
> > + * Userspace only need to know that the kernel is using this GPIO so
> > + * it can't use it.
> > + */
> > + info->flags = 0;
> > + if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> > + test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> > + test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
> > + test_bit(FLAG_EXPORT, &desc->flags) ||
> > + test_bit(FLAG_SYSFS, &desc->flags) ||
> > + !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
>
> pinctrl_gpio_can_use_line(), and pinctrl_get_device_gpio_range() called
> from it, call mutex_lock():
>
> BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:281
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 652, name: lsgpio
> CPU: 1 PID: 652 Comm: lsgpio Not tainted
> 5.6.0-rc1-koelsch-00008-gd2ac25798208fb85 #755
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [<c020e3f0>] (unwind_backtrace) from [<c020a5b8>] (show_stack+0x10/0x14)
> [<c020a5b8>] (show_stack) from [<c07d31b4>] (dump_stack+0x88/0xa8)
> [<c07d31b4>] (dump_stack) from [<c0241318>] (___might_sleep+0xf8/0x168)
> [<c0241318>] (___might_sleep) from [<c07ec13c>] (mutex_lock+0x24/0x7c)
> [<c07ec13c>] (mutex_lock) from [<c046f47c>]
> (pinctrl_get_device_gpio_range+0x1c/0xb4)
> [<c046f47c>] (pinctrl_get_device_gpio_range) from [<c046f5e8>]
> (pinctrl_gpio_can_use_line+0x24/0x88)
> [<c046f5e8>] (pinctrl_gpio_can_use_line) from [<c0478bd0>]
> (gpio_ioctl+0x270/0x584)
> [<c0478bd0>] (gpio_ioctl) from [<c03194c0>] (vfs_ioctl+0x20/0x38)
>
> Reproducer is "lsgpio" with CONFIG_DEBUG_ATOMIC_SLEEP=y.
>
Hi Geert,
thanks for the report. I added the locking around this code because it
seemed wrong to me that this part wasn't protected in any way before.
Now the question is how do we deal with this.
Linus: do you think it's safe to move the call to
pinctrl_gpio_can_use_line() before the spinlock is taken? I'd say yes
but I'm not sure if I'm not missing some inter-framework intricacies.
Best regards,
Bartosz Golaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
` (4 preceding siblings ...)
2020-02-11 9:19 ` [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-12 10:47 ` Linus Walleij
2020-02-11 9:19 ` [RESEND PATCH v6 7/7] tools: gpio: implement gpio-watch Bartosz Golaszewski
2020-02-12 10:49 ` [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Linus Walleij
7 siblings, 1 reply; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Currently there is no way for user-space to be informed about changes
in status of GPIO lines e.g. when someone else requests the line or its
config changes. We can only periodically re-read the line-info. This
is fine for simple one-off user-space tools, but any daemon that provides
a centralized access to GPIO chips would benefit hugely from an event
driven line info synchronization.
This patch adds a new ioctl() that allows user-space processes to reuse
the file descriptor associated with the character device for watching
any changes in line properties. Every such event contains the updated
line information.
Currently the events are generated on three types of status changes: when
a line is requested, when it's released and when its config is changed.
The first two are self-explanatory. For the third one: this will only
happen when another user-space process calls the new SET_CONFIG ioctl()
as any changes that can happen from within the kernel (i.e.
set_transitory() or set_debounce()) are of no interest to user-space.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/gpio/gpiolib.c | 186 ++++++++++++++++++++++++++++++++++++--
drivers/gpio/gpiolib.h | 1 +
include/uapi/linux/gpio.h | 30 ++++++
3 files changed, 209 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 443321f9cf63..f73077f26eff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -546,6 +546,9 @@ static long linehandle_set_config(struct linehandle_state *lh,
if (ret)
return ret;
}
+
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_CONFIG, desc);
}
return 0;
}
@@ -1201,14 +1204,25 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
spin_unlock_irqrestore(&gpio_lock, flags);
}
+struct gpio_chardev_data {
+ struct gpio_device *gdev;
+ wait_queue_head_t wait;
+ DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
+ struct notifier_block lineinfo_changed_nb;
+ unsigned long *watched_lines;
+};
+
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
- struct gpio_device *gdev = filp->private_data;
+ struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_device *gdev = priv->gdev;
struct gpio_chip *chip = gdev->chip;
void __user *ip = (void __user *)arg;
+ struct gpio_desc *desc;
+ __u32 offset;
/* We fail any subsequent ioctl():s when the chip is gone */
if (!chip)
@@ -1230,9 +1244,9 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
return -EFAULT;
return 0;
- } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+ } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
+ cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
struct gpioline_info lineinfo;
- struct gpio_desc *desc;
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -1245,11 +1259,25 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
+
+ if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL)
+ set_bit(desc_to_gpio(desc), priv->watched_lines);
+
return 0;
} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
return linehandle_create(gdev, ip);
} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
return lineevent_create(gdev, ip);
+ } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
+ if (copy_from_user(&offset, ip, sizeof(offset)))
+ return -EFAULT;
+
+ desc = gpiochip_get_desc(chip, offset);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ clear_bit(desc_to_gpio(desc), &desc->flags);
+ return 0;
}
return -EINVAL;
}
@@ -1262,6 +1290,101 @@ static long gpio_ioctl_compat(struct file *filp, unsigned int cmd,
}
#endif
+static struct gpio_chardev_data *
+to_gpio_chardev_data(struct notifier_block *nb)
+{
+ return container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
+}
+
+static int lineinfo_changed_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct gpio_chardev_data *priv = to_gpio_chardev_data(nb);
+ struct gpioline_info_changed chg;
+ struct gpio_desc *desc = data;
+ int ret;
+
+ if (!test_bit(desc_to_gpio(desc), priv->watched_lines))
+ return NOTIFY_DONE;
+
+ memset(&chg, 0, sizeof(chg));
+ chg.info.line_offset = gpio_chip_hwgpio(desc);
+ chg.event_type = action;
+ chg.timestamp = ktime_get_ns();
+ gpio_desc_to_lineinfo(desc, &chg.info);
+
+ ret = kfifo_in_spinlocked(&priv->events, &chg, 1, &priv->wait.lock);
+ if (ret)
+ wake_up_poll(&priv->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+
+ return NOTIFY_OK;
+}
+
+static __poll_t lineinfo_watch_poll(struct file *filep,
+ struct poll_table_struct *pollt)
+{
+ struct gpio_chardev_data *priv = filep->private_data;
+ __poll_t events = 0;
+
+ poll_wait(filep, &priv->wait, pollt);
+
+ if (!kfifo_is_empty_spinlocked_noirqsave(&priv->events,
+ &priv->wait.lock))
+ events = EPOLLIN | EPOLLRDNORM;
+
+ return events;
+}
+
+static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
+ size_t count, loff_t *off)
+{
+ struct gpio_chardev_data *priv = filep->private_data;
+ struct gpioline_info_changed event;
+ ssize_t bytes_read = 0;
+ int ret;
+
+ if (count < sizeof(event))
+ return -EINVAL;
+
+ do {
+ spin_lock(&priv->wait.lock);
+ if (kfifo_is_empty(&priv->events)) {
+ if (bytes_read) {
+ spin_unlock(&priv->wait.lock);
+ return bytes_read;
+ }
+
+ if (filep->f_flags & O_NONBLOCK) {
+ spin_unlock(&priv->wait.lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_locked(priv->wait,
+ !kfifo_is_empty(&priv->events));
+ if (ret) {
+ spin_unlock(&priv->wait.lock);
+ return ret;
+ }
+ }
+
+ ret = kfifo_out(&priv->events, &event, 1);
+ spin_unlock(&priv->wait.lock);
+ if (ret != 1) {
+ ret = -EIO;
+ break;
+ /* We should never get here. See lineevent_read(). */
+ }
+
+ if (copy_to_user(buf + bytes_read, &event, sizeof(event)))
+ return -EFAULT;
+ bytes_read += sizeof(event);
+ } while (count >= bytes_read + sizeof(event));
+
+ return bytes_read;
+}
+
/**
* gpio_chrdev_open() - open the chardev for ioctl operations
* @inode: inode for this chardev
@@ -1272,14 +1395,48 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
{
struct gpio_device *gdev = container_of(inode->i_cdev,
struct gpio_device, chrdev);
+ struct gpio_chardev_data *priv;
+ int ret = -ENOMEM;
/* Fail on open if the backing gpiochip is gone */
if (!gdev->chip)
return -ENODEV;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
+ if (!priv->watched_lines)
+ goto out_free_priv;
+
+ init_waitqueue_head(&priv->wait);
+ INIT_KFIFO(priv->events);
+ priv->gdev = gdev;
+
+ priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
+ ret = atomic_notifier_chain_register(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
+ if (ret)
+ goto out_free_bitmap;
+
get_device(&gdev->dev);
- filp->private_data = gdev;
+ filp->private_data = priv;
- return nonseekable_open(inode, filp);
+ ret = nonseekable_open(inode, filp);
+ if (ret)
+ goto out_unregister_notifier;
+
+ return ret;
+
+out_unregister_notifier:
+ atomic_notifier_chain_unregister(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
+out_free_bitmap:
+ bitmap_free(priv->watched_lines);
+out_free_priv:
+ kfree(priv);
+ return ret;
}
/**
@@ -1290,17 +1447,23 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
*/
static int gpio_chrdev_release(struct inode *inode, struct file *filp)
{
- struct gpio_device *gdev = container_of(inode->i_cdev,
- struct gpio_device, chrdev);
+ struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_device *gdev = priv->gdev;
+ bitmap_free(priv->watched_lines);
+ atomic_notifier_chain_unregister(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
put_device(&gdev->dev);
+ kfree(priv);
+
return 0;
}
-
static const struct file_operations gpio_fileops = {
.release = gpio_chrdev_release,
.open = gpio_chrdev_open,
+ .poll = lineinfo_watch_poll,
+ .read = lineinfo_watch_read,
.owner = THIS_MODULE,
.llseek = no_llseek,
.unlocked_ioctl = gpio_ioctl,
@@ -1511,6 +1674,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
spin_unlock_irqrestore(&gpio_lock, flags);
+ ATOMIC_INIT_NOTIFIER_HEAD(&gdev->notifier);
+
#ifdef CONFIG_PINCTRL
INIT_LIST_HEAD(&gdev->pin_ranges);
#endif
@@ -2843,6 +3008,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
}
done:
spin_unlock_irqrestore(&gpio_lock, flags);
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_REQUESTED, desc);
return ret;
}
@@ -2940,6 +3107,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
}
spin_unlock_irqrestore(&gpio_lock, flags);
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_RELEASED, desc);
+
return ret;
}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3e0aab2945d8..5ab90746b519 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -56,6 +56,7 @@ struct gpio_device {
const char *label;
void *data;
struct list_head list;
+ struct atomic_notifier_head notifier;
#ifdef CONFIG_PINCTRL
/*
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 799cf823d493..dca320764e4d 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -59,6 +59,34 @@ struct gpioline_info {
/* Maximum number of requested handles */
#define GPIOHANDLES_MAX 64
+/* Possible line status change events */
+enum {
+ GPIOLINE_CHANGED_REQUESTED = 1,
+ GPIOLINE_CHANGED_RELEASED,
+ GPIOLINE_CHANGED_CONFIG,
+};
+
+/**
+ * struct gpioline_info_changed - Information about a change in status
+ * of a GPIO line
+ * @info: updated line information
+ * @timestamp: estimate of time of status change occurrence, in nanoseconds
+ * and GPIOLINE_CHANGED_CONFIG
+ * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
+ *
+ * Note: struct gpioline_info embedded here has 32-bit alignment on its own,
+ * but it works fine with 64-bit alignment too. With its 72 byte size, we can
+ * guarantee there are no implicit holes between it and subsequent members.
+ * The 20-byte padding at the end makes sure we don't add any implicit padding
+ * at the end of the structure on 64-bit architectures.
+ */
+struct gpioline_info_changed {
+ struct gpioline_info info;
+ __u64 timestamp;
+ __u32 event_type;
+ __u32 padding[5]; /* for future use */
+};
+
/* Linerequest flags */
#define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
#define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
@@ -176,6 +204,8 @@ struct gpioevent_data {
#define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
#define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
+#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
+#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
#define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info
2020-02-11 9:19 ` [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info Bartosz Golaszewski
@ 2020-02-12 10:47 ` Linus Walleij
2020-02-12 11:00 ` Bartosz Golaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2020-02-12 10:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Andy Shevchenko, open list:GPIO SUBSYSTEM,
linux-kernel, Bartosz Golaszewski
On Tue, Feb 11, 2020 at 10:19 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Currently there is no way for user-space to be informed about changes
> in status of GPIO lines e.g. when someone else requests the line or its
> config changes. We can only periodically re-read the line-info. This
> is fine for simple one-off user-space tools, but any daemon that provides
> a centralized access to GPIO chips would benefit hugely from an event
> driven line info synchronization.
>
> This patch adds a new ioctl() that allows user-space processes to reuse
> the file descriptor associated with the character device for watching
> any changes in line properties. Every such event contains the updated
> line information.
>
> Currently the events are generated on three types of status changes: when
> a line is requested, when it's released and when its config is changed.
> The first two are self-explanatory. For the third one: this will only
> happen when another user-space process calls the new SET_CONFIG ioctl()
> as any changes that can happen from within the kernel (i.e.
> set_transitory() or set_debounce()) are of no interest to user-space.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Looks good to me. This got really slim and clean after
the reviews, and I am of course also impressed by the kfifo
improvement this brings.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
A question:
Bartosz, since you know about possible impacts on userspace,
since this code use the preferred ktime_get_ns() rather than
ktime_get_ns_real(), what happens if we just patch the other
event timestamp to use ktime_get_ns() instead, so we use the
same everywhere?
If it's fine I'd like to just toss in a patch for that as well.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info
2020-02-12 10:47 ` Linus Walleij
@ 2020-02-12 11:00 ` Bartosz Golaszewski
2020-02-20 15:03 ` Linus Walleij
0 siblings, 1 reply; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-12 11:00 UTC (permalink / raw)
To: Linus Walleij, Arnd Bergmann
Cc: Kent Gibson, Andy Shevchenko, open list:GPIO SUBSYSTEM,
linux-kernel, Bartosz Golaszewski
śr., 12 lut 2020 o 11:47 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Tue, Feb 11, 2020 at 10:19 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Currently there is no way for user-space to be informed about changes
> > in status of GPIO lines e.g. when someone else requests the line or its
> > config changes. We can only periodically re-read the line-info. This
> > is fine for simple one-off user-space tools, but any daemon that provides
> > a centralized access to GPIO chips would benefit hugely from an event
> > driven line info synchronization.
> >
> > This patch adds a new ioctl() that allows user-space processes to reuse
> > the file descriptor associated with the character device for watching
> > any changes in line properties. Every such event contains the updated
> > line information.
> >
> > Currently the events are generated on three types of status changes: when
> > a line is requested, when it's released and when its config is changed.
> > The first two are self-explanatory. For the third one: this will only
> > happen when another user-space process calls the new SET_CONFIG ioctl()
> > as any changes that can happen from within the kernel (i.e.
> > set_transitory() or set_debounce()) are of no interest to user-space.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Looks good to me. This got really slim and clean after
> the reviews, and I am of course also impressed by the kfifo
> improvement this brings.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> A question:
>
> Bartosz, since you know about possible impacts on userspace,
> since this code use the preferred ktime_get_ns() rather than
> ktime_get_ns_real(), what happens if we just patch the other
> event timestamp to use ktime_get_ns() instead, so we use the
> same everywhere?
>
> If it's fine I'd like to just toss in a patch for that as well.
>
Arnd pointed out it would be an incompatible ABI change[1].
However - I asked Khouloud who's working on v2 of the line event
interface to use ktime_get_ns().
Cheers
Bart
[1] https://marc.info/?l=linux-gpio&m=151661955709074&w=2
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info
2020-02-12 11:00 ` Bartosz Golaszewski
@ 2020-02-20 15:03 ` Linus Walleij
2020-02-20 15:06 ` Bartosz Golaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2020-02-20 15:03 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Arnd Bergmann, Kent Gibson, Andy Shevchenko,
open list:GPIO SUBSYSTEM, linux-kernel, Bartosz Golaszewski
On Wed, Feb 12, 2020 at 12:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Feb 11, 2020 at 10:19 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > A question:
> >
> > Bartosz, since you know about possible impacts on userspace,
> > since this code use the preferred ktime_get_ns() rather than
> > ktime_get_ns_real(), what happens if we just patch the other
> > event timestamp to use ktime_get_ns() instead, so we use the
> > same everywhere?
> >
> > If it's fine I'd like to just toss in a patch for that as well.
> >
>
> Arnd pointed out it would be an incompatible ABI change[1].
Yeah, I was thinking more about this specific answer from Arnd:
> "It is an incompatible ABI change, the question here is whether anyone
> actually cares. If nothing relies on the timestamps being in
> CLOCK_REALTIME domain, then it can be changed, the question
> is just how you want to prove that this is the case."
So the question is if userspace really cares.
What happens with libgpiod or users of it? Are they assuming
the weirdness of CLOCK_REALTIME, or are they simply assuming
something that is monotonic increasing and just lucky that they
didn't run into anything jumping backwards in time even though
they *could*.
I think I'll propose a change and see what people say.
> However - I asked Khouloud who's working on v2 of the line event
> interface to use ktime_get_ns().
That's great!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info
2020-02-20 15:03 ` Linus Walleij
@ 2020-02-20 15:06 ` Bartosz Golaszewski
0 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-20 15:06 UTC (permalink / raw)
To: Linus Walleij
Cc: Arnd Bergmann, Kent Gibson, Andy Shevchenko,
open list:GPIO SUBSYSTEM, linux-kernel, Bartosz Golaszewski
czw., 20 lut 2020 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Wed, Feb 12, 2020 at 12:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Tue, Feb 11, 2020 at 10:19 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> > > A question:
> > >
> > > Bartosz, since you know about possible impacts on userspace,
> > > since this code use the preferred ktime_get_ns() rather than
> > > ktime_get_ns_real(), what happens if we just patch the other
> > > event timestamp to use ktime_get_ns() instead, so we use the
> > > same everywhere?
> > >
> > > If it's fine I'd like to just toss in a patch for that as well.
> > >
> >
> > Arnd pointed out it would be an incompatible ABI change[1].
>
> Yeah, I was thinking more about this specific answer from Arnd:
>
> > "It is an incompatible ABI change, the question here is whether anyone
> > actually cares. If nothing relies on the timestamps being in
> > CLOCK_REALTIME domain, then it can be changed, the question
> > is just how you want to prove that this is the case."
>
> So the question is if userspace really cares.
>
> What happens with libgpiod or users of it? Are they assuming
> the weirdness of CLOCK_REALTIME, or are they simply assuming
> something that is monotonic increasing and just lucky that they
> didn't run into anything jumping backwards in time even though
> they *could*.
>
> I think I'll propose a change and see what people say.
>
Libgpiod doesn't care about the value really - it just forwards
whatever it reads.
Bart
> > However - I asked Khouloud who's working on v2 of the line event
> > interface to use ktime_get_ns().
>
> That's great!
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND PATCH v6 7/7] tools: gpio: implement gpio-watch
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
` (5 preceding siblings ...)
2020-02-11 9:19 ` [RESEND PATCH v6 6/7] gpiolib: add new ioctl() for monitoring changes in line info Bartosz Golaszewski
@ 2020-02-11 9:19 ` Bartosz Golaszewski
2020-02-12 10:49 ` [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Linus Walleij
7 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-02-11 9:19 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add a simple program that allows to test the new LINECHANGED_FD ioctl().
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
tools/gpio/.gitignore | 1 +
tools/gpio/Build | 1 +
tools/gpio/Makefile | 11 ++++-
tools/gpio/gpio-watch.c | 99 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 111 insertions(+), 1 deletion(-)
create mode 100644 tools/gpio/gpio-watch.c
diff --git a/tools/gpio/.gitignore b/tools/gpio/.gitignore
index a94c0e83b209..eab36c6d7751 100644
--- a/tools/gpio/.gitignore
+++ b/tools/gpio/.gitignore
@@ -1,4 +1,5 @@
gpio-event-mon
gpio-hammer
+gpio-watch
lsgpio
include/linux/gpio.h
diff --git a/tools/gpio/Build b/tools/gpio/Build
index 4141f35837db..67c7b7f6a717 100644
--- a/tools/gpio/Build
+++ b/tools/gpio/Build
@@ -2,3 +2,4 @@ gpio-utils-y += gpio-utils.o
lsgpio-y += lsgpio.o gpio-utils.o
gpio-hammer-y += gpio-hammer.o gpio-utils.o
gpio-event-mon-y += gpio-event-mon.o gpio-utils.o
+gpio-watch-y += gpio-watch.o
diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
index 6080de58861f..842287e42c83 100644
--- a/tools/gpio/Makefile
+++ b/tools/gpio/Makefile
@@ -18,7 +18,7 @@ MAKEFLAGS += -r
override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
-ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon
+ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon gpio-watch
ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
all: $(ALL_PROGRAMS)
@@ -66,6 +66,15 @@ $(GPIO_EVENT_MON_IN): prepare FORCE $(OUTPUT)gpio-utils-in.o
$(OUTPUT)gpio-event-mon: $(GPIO_EVENT_MON_IN)
$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+#
+# gpio-watch
+#
+GPIO_WATCH_IN := $(OUTPUT)gpio-watch-in.o
+$(GPIO_WATCH_IN): prepare FORCE
+ $(Q)$(MAKE) $(build)=gpio-watch
+$(OUTPUT)gpio-watch: $(GPIO_WATCH_IN)
+ $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
clean:
rm -f $(ALL_PROGRAMS)
rm -f $(OUTPUT)include/linux/gpio.h
diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
new file mode 100644
index 000000000000..5cea24fddfa7
--- /dev/null
+++ b/tools/gpio/gpio-watch.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * gpio-watch - monitor unrequested lines for property changes using the
+ * character device
+ *
+ * Copyright (C) 2019 BayLibre SAS
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ */
+
+#include <ctype.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <poll.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+int main(int argc, char **argv)
+{
+ struct gpioline_info_changed chg;
+ struct gpioline_info req;
+ struct pollfd pfd;
+ int fd, i, j, ret;
+ char *event, *end;
+ ssize_t rd;
+
+ if (argc < 3)
+ goto err_usage;
+
+ fd = open(argv[1], O_RDWR | O_CLOEXEC);
+ if (fd < 0) {
+ perror("unable to open gpiochip");
+ return EXIT_FAILURE;
+ }
+
+ for (i = 0, j = 2; i < argc - 2; i++, j++) {
+ memset(&req, 0, sizeof(req));
+
+ req.line_offset = strtoul(argv[j], &end, 0);
+ if (*end != '\0')
+ goto err_usage;
+
+ ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
+ if (ret) {
+ perror("unable to set up line watch");
+ return EXIT_FAILURE;
+ }
+ }
+
+ pfd.fd = fd;
+ pfd.events = POLLIN | POLLPRI;
+
+ for (;;) {
+ ret = poll(&pfd, 1, 5000);
+ if (ret < 0) {
+ perror("error polling the linechanged fd");
+ return EXIT_FAILURE;
+ } else if (ret > 0) {
+ memset(&chg, 0, sizeof(chg));
+ rd = read(pfd.fd, &chg, sizeof(chg));
+ if (rd < 0 || rd != sizeof(chg)) {
+ if (rd != sizeof(chg))
+ errno = EIO;
+
+ perror("error reading line change event");
+ return EXIT_FAILURE;
+ }
+
+ switch (chg.event_type) {
+ case GPIOLINE_CHANGED_REQUESTED:
+ event = "requested";
+ break;
+ case GPIOLINE_CHANGED_RELEASED:
+ event = "released";
+ break;
+ case GPIOLINE_CHANGED_CONFIG:
+ event = "config changed";
+ break;
+ default:
+ fprintf(stderr,
+ "invalid event type received from the kernel\n");
+ return EXIT_FAILURE;
+ }
+
+ printf("line %u: %s at %llu\n",
+ chg.info.line_offset, event, chg.timestamp);
+ }
+ }
+
+ return 0;
+
+err_usage:
+ printf("%s: <gpiochip> <line0> <line1> ...\n", argv[0]);
+ return EXIT_FAILURE;
+}
--
2.25.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes
2020-02-11 9:19 [RESEND PATCH v6 0/7] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
` (6 preceding siblings ...)
2020-02-11 9:19 ` [RESEND PATCH v6 7/7] tools: gpio: implement gpio-watch Bartosz Golaszewski
@ 2020-02-12 10:49 ` Linus Walleij
7 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2020-02-12 10:49 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Andy Shevchenko, open list:GPIO SUBSYSTEM,
linux-kernel, Bartosz Golaszewski
Hi Bartosz,
I'm very happy with this version and any remaining nits can surely
be ironed out in-tree.
Since we have ACKs from the kfifo maintainer, can you send me a
pull request for this so we get it into -next early?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 15+ messages in thread