[PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Bartosz Golaszewski]

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.

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

Bartosz Golaszewski (13):
  gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()
  gpiolib: have a single place of calling set_config()
  gpiolib: convert the type of hwnum to unsigned int in
    gpiochip_get_desc()
  gpiolib: use gpiochip_get_desc() in linehandle_create()
  gpiolib: use gpiochip_get_desc() in lineevent_create()
  gpiolib: use gpiochip_get_desc() in gpio_ioctl()
  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      | 397 +++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.h      |   4 +-
 include/linux/gpio/driver.h |   3 +-
 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 +++++++++
 9 files changed, 515 insertions(+), 104 deletions(-)
 create mode 100644 tools/gpio/gpio-watch.c

-- 
2.23.0

[PATCH v4 01/13] gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Checkpatch complains about using 'unsigned' instead of 'unsigned int'.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9913886ede90..e5d101ee9ada 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3042,7 +3042,7 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
-static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
+static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 			   enum pin_config_param mode)
 {
 	unsigned long config;
-- 
2.23.0

[PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Instead of calling the gpiochip's set_config() callback directly and
checking its existence every time - just add a new routine that performs
this check internally. Call it in gpio_set_config() and
gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
the check for chip->set() as it's irrelevant to this config option.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e5d101ee9ada..616e431039fc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
+static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset,
+			      enum pin_config_param mode)
+{
+	if (!gc->set_config)
+		return -ENOTSUPP;
+
+	return gc->set_config(gc, offset, mode);
+}
+
 static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 			   enum pin_config_param mode)
 {
@@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 	}
 
 	config = PIN_CONF_PACKED(mode, arg);
-	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
+	return gpio_do_set_config(gc, offset, mode);
 }
 
 static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
@@ -3294,15 +3303,9 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 
 	VALIDATE_DESC(desc);
 	chip = desc->gdev->chip;
-	if (!chip->set || !chip->set_config) {
-		gpiod_dbg(desc,
-			  "%s: missing set() or set_config() operations\n",
-			  __func__);
-		return -ENOTSUPP;
-	}
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -3339,7 +3342,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
 					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = chip->set_config(chip, gpio, packed);
+	rc = gpio_do_set_config(chip, gpio, packed);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);
-- 
2.23.0

[PATCH v4 03/13] gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

gpiochip_get_desc() takes a u16 hwnum, but it turns out most users don't
respect that and usually pass an unsigned int. Since implicit casting to
a smaller type is dangerous - let's change the type of hwnum to unsigned
int in gpiochip_get_desc() and in gpiochip_request_own_desc() where the
size of hwnum is not respected either and who's a user of the former.

This is safe as we then check the hwnum against the number of lines
before proceeding in gpiochip_get_desc().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c      | 5 +++--
 drivers/gpio/gpiolib.h      | 3 ++-
 include/linux/gpio/driver.h | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 616e431039fc..68adbd2179a0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -140,7 +140,7 @@ EXPORT_SYMBOL_GPL(gpio_to_desc);
  * in the given chip for the specified hardware number.
  */
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
-				    u16 hwnum)
+				    unsigned int hwnum)
 {
 	struct gpio_device *gdev = chip->gpiodev;
 
@@ -2990,7 +2990,8 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * A pointer to the GPIO descriptor, or an ERR_PTR()-encoded negative error
  * code on failure.
  */
-struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip,
+					    unsigned int hwnum,
 					    const char *label,
 					    enum gpio_lookup_flags lflags,
 					    enum gpiod_flags dflags)
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ca9bc1e4803c..a1cbeabadc69 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -78,7 +78,8 @@ struct gpio_array {
 	unsigned long		invert_mask[];
 };
 
-struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
+struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
+				    unsigned int hwnum);
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e2480ef94c55..4f032de10bae 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -715,7 +715,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
 
 #endif /* CONFIG_PINCTRL */
 
-struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip,
+					    unsigned int hwnum,
 					    const char *label,
 					    enum gpio_lookup_flags lflags,
 					    enum gpiod_flags dflags);
-- 
2.23.0

[PATCH v4 04/13] gpiolib: use gpiochip_get_desc() in linehandle_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
checking its return value.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 68adbd2179a0..fcec8b090677 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -678,14 +678,13 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	/* Request each GPIO */
 	for (i = 0; i < handlereq.lines; i++) {
 		u32 offset = handlereq.lineoffsets[i];
-		struct gpio_desc *desc;
+		struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
 
-		if (offset >= gdev->ngpio) {
-			ret = -EINVAL;
+		if (IS_ERR(desc)) {
+			ret = PTR_ERR(desc);
 			goto out_free_descs;
 		}
 
-		desc = &gdev->descs[offset];
 		ret = gpiod_request(desc, lh->label);
 		if (ret)
 			goto out_free_descs;
-- 
2.23.0

[PATCH v4 05/13] gpiolib: use gpiochip_get_desc() in lineevent_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
checking its return value.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fcec8b090677..007f16fdf782 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1009,8 +1009,9 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	lflags = eventreq.handleflags;
 	eflags = eventreq.eventflags;
 
-	if (offset >= gdev->ngpio)
-		return -EINVAL;
+	desc = gpiochip_get_desc(gdev->chip, offset);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
 
 	/* Return an error if a unknown flag is set */
 	if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
@@ -1048,7 +1049,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 		}
 	}
 
-	desc = &gdev->descs[offset];
 	ret = gpiod_request(desc, le->label);
 	if (ret)
 		goto out_free_label;
-- 
2.23.0

[PATCH v4 06/13] gpiolib: use gpiochip_get_desc() in gpio_ioctl()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Unduplicate the offset check by simply calling gpiochip_get_desc() and
checking its return value.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 007f16fdf782..81d5eda4de7d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1175,10 +1175,11 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
 			return -EFAULT;
-		if (lineinfo.line_offset >= gdev->ngpio)
-			return -EINVAL;
 
-		desc = &gdev->descs[lineinfo.line_offset];
+		desc = gpiochip_get_desc(chip, lineinfo.line_offset);
+		if (IS_ERR(desc))
+			return PTR_ERR(desc);
+
 		if (desc->name) {
 			strncpy(lineinfo.name, desc->name,
 				sizeof(lineinfo.name));
-- 
2.23.0

[PATCH v4 07/13] kfifo: provide noirqsave variants of spinlocked in and out helpers

[Bartosz Golaszewski]

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>
---
 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.23.0

[PATCH v4 08/13] kfifo: provide kfifo_is_empty_spinlocked()

[Bartosz Golaszewski]

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>
---
 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.23.0

[PATCH v4 09/13] gpiolib: rework the locking mechanism for lineevent kfifo

[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>
---
 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 81d5eda4de7d..a859c0813e0d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -788,8 +788,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
@@ -802,7 +800,6 @@ struct lineevent_state {
 	int irq;
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
-	struct mutex read_lock;
 	u64 timestamp;
 };
 
@@ -818,7 +815,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;
@@ -831,43 +828,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)
@@ -969,7 +975,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);
 
@@ -1084,7 +1091,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.23.0

[PATCH v4 10/13] gpiolib: emit a debug message when adding events to a full kfifo

[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 a859c0813e0d..543244355e1c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -979,6 +979,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.23.0

[PATCH v4 11/13] gpiolib: provide a dedicated function for setting lineinfo

[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>
---
 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 543244355e1c..276a7068f23c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1148,6 +1148,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
  */
@@ -1188,49 +1242,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.23.0

[PATCH v4 12/13] gpiolib: add new ioctl() for monitoring changes in line info

[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    | 187 ++++++++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h    |   1 +
 include/uapi/linux/gpio.h |  30 ++++++
 3 files changed, 210 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 276a7068f23c..9952405deb7d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -547,6 +547,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;
 }
@@ -1202,14 +1205,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)
@@ -1231,9 +1245,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;
@@ -1246,11 +1260,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;
 }
@@ -1263,6 +1291,102 @@ 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_real_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
@@ -1273,14 +1397,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;
 }
 
 /**
@@ -1291,17 +1449,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,
@@ -1512,6 +1676,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 	for (i = 0; i < chip->ngpio; i++)
 		gdev->descs[i].gdev = gdev;
 
+	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->notifier);
+
 #ifdef CONFIG_PINCTRL
 	INIT_LIST_HEAD(&gdev->pin_ranges);
 #endif
@@ -2851,6 +3017,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;
 }
 
@@ -2948,6 +3116,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 a1cbeabadc69..8e3969616cfe 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -54,6 +54,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..10183d4d4276 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 5-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.23.0

[PATCH v4 13/13] tools: gpio: implement gpio-watch

[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>
---
 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.23.0

Re: [PATCH v4 01/13] gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Checkpatch complains about using 'unsigned' instead of 'unsigned int'.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Instead of calling the gpiochip's set_config() callback directly and
> checking its existence every time - just add a new routine that performs
> this check internally. Call it in gpio_set_config() and
> gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> the check for chip->set() as it's irrelevant to this config option.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 03/13] gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> gpiochip_get_desc() takes a u16 hwnum, but it turns out most users don't
> respect that and usually pass an unsigned int. Since implicit casting to
> a smaller type is dangerous - let's change the type of hwnum to unsigned
> int in gpiochip_get_desc() and in gpiochip_request_own_desc() where the
> size of hwnum is not respected either and who's a user of the former.
>
> This is safe as we then check the hwnum against the number of lines
> before proceeding in gpiochip_get_desc().
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 04/13] gpiolib: use gpiochip_get_desc() in linehandle_create()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
> checking its return value.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 05/13] gpiolib: use gpiochip_get_desc() in lineevent_create()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
> checking its return value.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 06/13] gpiolib: use gpiochip_get_desc() in gpio_ioctl()

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the offset check by simply calling gpiochip_get_desc() and
> checking its return value.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Linus Walleij]

On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> 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.

The patch set overall looks good to me, I don't understand the kfifo
parts but I trust you on this, though we need review from a FIFO
maintainer.

Could you send me a pull request of the first patches before the
FIFO changes start, they are good cleanups on their own, also
it brings down the size of your patch stack.

Yours,
Linus Walleij

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Bartosz Golaszewski]

wt., 7 sty 2020 o 11:07 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > 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.
>
> The patch set overall looks good to me, I don't understand the kfifo
> parts but I trust you on this, though we need review from a FIFO
> maintainer.

Ha! This may be a problem - there doesn't seem to be one. This is why
I Cc'd Greg.

>
> Could you send me a pull request of the first patches before the
> FIFO changes start, they are good cleanups on their own, also
> it brings down the size of your patch stack.

Sure, will do.

>
> Yours,
> Linus Walleij

Bart

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Linus Walleij]

On Tue, Jan 7, 2020 at 11:38 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> wt., 7 sty 2020 o 11:07 Linus Walleij <linus.walleij@linaro.org> napisał(a):

> > The patch set overall looks good to me, I don't understand the kfifo
> > parts but I trust you on this, though we need review from a FIFO
> > maintainer.
>
> Ha! This may be a problem - there doesn't seem to be one. This is why
> I Cc'd Greg.

I was under the impression that KFIFO was part of the driver core.
Let's try to CC the actual author (Stefani Seibold) and see if the mail
address works and if he can look at it. Or did you already talk to
Stefani?

(git blame is always my best friend in cases like this, hehe)

Yours,
Linus Walleij

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Stefani Seibold]

Am Dienstag, den 07.01.2020, 13:50 +0100 schrieb Linus Walleij:
> On Tue, Jan 7, 2020 at 11:38 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > wt., 7 sty 2020 o 11:07 Linus Walleij <linus.walleij@linaro.org>
> > napisał(a):
> > > The patch set overall looks good to me, I don't understand the
> > > kfifo
> > > parts but I trust you on this, though we need review from a FIFO
> > > maintainer.
> > 
> > Ha! This may be a problem - there doesn't seem to be one. This is
> > why
> > I Cc'd Greg.
> 
> I was under the impression that KFIFO was part of the driver core.
> Let's try to CC the actual author (Stefani Seibold) and see if the
> mail
> address works and if he can look at it. Or did you already talk to
> Stefani?
> 
> (git blame is always my best friend in cases like this, hehe)
> 
> Yours,
> Linus Walleij

I have looked around for the patches and I found the following patches 

[PATCH v4 07/13] kfifo: provide noirqsave variants of spinlocked in and out helpers
[PATCH v4 08/13] kfifo: provide kfifo_is_empty_spinlocked()

dated on 24 Dec 2019.

Both seems to be okay. The patch is non intrusive to KFIFO adding only
spinlock wrapper functions for the contemporary kfifo functions.

So...

Acked by Stefani Seibold <stefani@seibold.net>

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Andy Shevchenko]

On Tue, Jan 07, 2020 at 01:50:28PM +0100, Linus Walleij wrote:

...

> Let's try to CC the actual author (Stefani Seibold) and see if the mail
> address works and if he can look at it. Or did you already talk to
> Stefani?
> 
> (git blame is always my best friend in cases like this, hehe)

Recently I started to be smarted in such cases, i.e. I run also
`git log --author='$AUTHOR'` to see if they are still active and
what address had been used lately.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Andy Shevchenko]

On Tue, Jan 07, 2020 at 04:44:55PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 07, 2020 at 01:50:28PM +0100, Linus Walleij wrote:
> 
> ...
> 
> > Let's try to CC the actual author (Stefani Seibold) and see if the mail
> > address works and if he can look at it. Or did you already talk to
> > Stefani?
> > 
> > (git blame is always my best friend in cases like this, hehe)
> 
> Recently I started to be smarted in such cases, i.e. I run also
> `git log --author='$AUTHOR'` to see if they are still active and
> what address had been used lately.

...and another possibility to `git log --grep '$AUTHOR'`.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Bartosz Golaszewski]

wt., 7 sty 2020 o 15:45 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Tue, Jan 07, 2020 at 04:44:55PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 07, 2020 at 01:50:28PM +0100, Linus Walleij wrote:
> >
> > ...
> >
> > > Let's try to CC the actual author (Stefani Seibold) and see if the mail
> > > address works and if he can look at it. Or did you already talk to
> > > Stefani?
> > >
> > > (git blame is always my best friend in cases like this, hehe)
> >
> > Recently I started to be smarted in such cases, i.e. I run also
> > `git log --author='$AUTHOR'` to see if they are still active and
> > what address had been used lately.
>
> ...and another possibility to `git log --grep '$AUTHOR'`.
>
> --
> With Best Regards,
> Andy Shevchenko
>

So if some module doesn't have an official maintainer listed in
MAINTAINERS, we should still get a review from the original author?
KFIFO lives in lib/ - is there even an official maintainer for all
library helpers?

Bart

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Andy Shevchenko]

On Tue, Jan 07, 2020 at 04:19:59PM +0100, Bartosz Golaszewski wrote:
> wt., 7 sty 2020 o 15:45 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > On Tue, Jan 07, 2020 at 04:44:55PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 07, 2020 at 01:50:28PM +0100, Linus Walleij wrote:
> > >
> > > ...
> > >
> > > > Let's try to CC the actual author (Stefani Seibold) and see if the mail
> > > > address works and if he can look at it. Or did you already talk to
> > > > Stefani?
> > > >
> > > > (git blame is always my best friend in cases like this, hehe)
> > >
> > > Recently I started to be smarted in such cases, i.e. I run also
> > > `git log --author='$AUTHOR'` to see if they are still active and
> > > what address had been used lately.
> >
> > ...and another possibility to `git log --grep '$AUTHOR'`.

> So if some module doesn't have an official maintainer listed in
> MAINTAINERS, we should still get a review from the original author?

If you asking me, I do it in a way of playing good citizen. It's not required,
but may give a good feedback.

> KFIFO lives in lib/ - is there even an official maintainer for all
> library helpers?

lib/ is (in most cases) under akpm@ realm.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 00/13] gpiolib: add an ioctl() for monitoring line status changes

[Bartosz Golaszewski]

wt., 7 sty 2020 o 16:58 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Tue, Jan 07, 2020 at 04:19:59PM +0100, Bartosz Golaszewski wrote:
> > wt., 7 sty 2020 o 15:45 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > >
> > > On Tue, Jan 07, 2020 at 04:44:55PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Jan 07, 2020 at 01:50:28PM +0100, Linus Walleij wrote:
> > > >
> > > > ...
> > > >
> > > > > Let's try to CC the actual author (Stefani Seibold) and see if the mail
> > > > > address works and if he can look at it. Or did you already talk to
> > > > > Stefani?
> > > > >
> > > > > (git blame is always my best friend in cases like this, hehe)
> > > >
> > > > Recently I started to be smarted in such cases, i.e. I run also
> > > > `git log --author='$AUTHOR'` to see if they are still active and
> > > > what address had been used lately.
> > >
> > > ...and another possibility to `git log --grep '$AUTHOR'`.
>
> > So if some module doesn't have an official maintainer listed in
> > MAINTAINERS, we should still get a review from the original author?
>
> If you asking me, I do it in a way of playing good citizen. It's not required,
> but may give a good feedback.
>
> > KFIFO lives in lib/ - is there even an official maintainer for all
> > library helpers?
>
> lib/ is (in most cases) under akpm@ realm.
>

Once the first part of the series is in Linus' branch, I'll resend the
remaining patches with akpm in Cc.

Bart

Re: [PATCH v4 09/13] gpiolib: rework the locking mechanism for lineevent kfifo

[Andy Shevchenko]

On Tue, Dec 24, 2019 at 01:07:05PM +0100, Bartosz Golaszewski wrote:
> 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.

Sounds like a plausible approach.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.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 81d5eda4de7d..a859c0813e0d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -788,8 +788,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
> @@ -802,7 +800,6 @@ struct lineevent_state {
>  	int irq;
>  	wait_queue_head_t wait;
>  	DECLARE_KFIFO(events, struct gpioevent_data, 16);
> -	struct mutex read_lock;
>  	u64 timestamp;
>  };
>  
> @@ -818,7 +815,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;
> @@ -831,43 +828,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)
> @@ -969,7 +975,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);
>  
> @@ -1084,7 +1091,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.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 11/13] gpiolib: provide a dedicated function for setting lineinfo

[Andy Shevchenko]

On Tue, Dec 24, 2019 at 01:07:07PM +0100, Bartosz Golaszewski 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.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.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 543244355e1c..276a7068f23c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1148,6 +1148,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
>   */
> @@ -1188,49 +1242,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.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 12/13] gpiolib: add new ioctl() for monitoring changes in line info

[Andy Shevchenko]

On Tue, Dec 24, 2019 at 01:07:08PM +0100, Bartosz Golaszewski 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.

...

> -	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> +	} else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
> +		   cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {

What about to split the functionality to something like lineinfo_get() and...

>  		struct gpioline_info lineinfo;

> -		struct gpio_desc *desc;

Hmm... Is it correct patch for this change?

>  
>  		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
>  			return -EFAULT;
> @@ -1246,11 +1260,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);
> +

...simple use

	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
		return lineinfo_get();
	} else if {cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
		ret = lineinfo_get()
		set_bit();
		return ret;
	}

?

>  		return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 13/13] tools: gpio: implement gpio-watch

[Andy Shevchenko]

On Tue, Dec 24, 2019 at 01:07:09PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add a simple program that allows to test the new LINECHANGED_FD ioctl().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.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.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 12/13] gpiolib: add new ioctl() for monitoring changes in line info

[Bartosz Golaszewski]

śr., 8 sty 2020 o 13:46 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Tue, Dec 24, 2019 at 01:07:08PM +0100, Bartosz Golaszewski wrote:
> ...
>
> > -     } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> > +     } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
> > +                cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
>
> What about to split the functionality to something like lineinfo_get() and...
>
> >               struct gpioline_info lineinfo;
>
> > -             struct gpio_desc *desc;
>
> Hmm... Is it correct patch for this change?

Yes, because instead of declaring *desc in every branch, we declare it
once and use it twice.

>
> >
> >               if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
> >                       return -EFAULT;
> > @@ -1246,11 +1260,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);
> > +
>
> ...simple use
>
>         } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
>                 return lineinfo_get();
>         } else if {cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
>                 ret = lineinfo_get()
>                 set_bit();
>                 return ret;
>         }
>
> ?

You still need to retrieve *desc outside of lineinfo_get(). I think
it's not worth the hassle.

Bart

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Geert Uytterhoeven]

Hi Bartosz,

On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Instead of calling the gpiochip's set_config() callback directly and
> checking its existence every time - just add a new routine that performs
> this check internally. Call it in gpio_set_config() and
> gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> the check for chip->set() as it's irrelevant to this config option.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
>   * rely on gpio_request() having been called beforehand.
>   */
>
> +static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset,
> +                             enum pin_config_param mode)
> +{
> +       if (!gc->set_config)
> +               return -ENOTSUPP;
> +
> +       return gc->set_config(gc, offset, mode);
> +}
> +
>  static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
>                            enum pin_config_param mode)
>  {
> @@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
>         }
>
>         config = PIN_CONF_PACKED(mode, arg);
> -       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
> +       return gpio_do_set_config(gc, offset, mode);

These two lines are not equivalent: the new code no longer uses the
packed value of mode and arg!
Hence this leads to subsequent cleanups in commits e5e42ad224a040f9
("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796
("gpiolib: Remove duplicated function gpio_do_set_config()").

However, what was the purpose of the PIN_CONF_PACKED() translation?
Why is it no longer needed?

Thanks!

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

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Andy Shevchenko]

On Mon, Jan 20, 2020 at 09:44:43AM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Instead of calling the gpiochip's set_config() callback directly and
> > checking its existence every time - just add a new routine that performs
> > this check internally. Call it in gpio_set_config() and
> > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> > the check for chip->set() as it's irrelevant to this config option.

...

> >         config = PIN_CONF_PACKED(mode, arg);
> > -       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
> > +       return gpio_do_set_config(gc, offset, mode);
> 
> These two lines are not equivalent: the new code no longer uses the
> packed value of mode and arg!

Good catch!
It's a regression (pin control drivers expects arg to be 1 in case it has been
called thru GPIO framework to set "default" values in terms of certain driver)
and below mentioned commits must be reverted. This one seems has a typo which
must be fixed.

> Hence this leads to subsequent cleanups in commits e5e42ad224a040f9
> ("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796
> ("gpiolib: Remove duplicated function gpio_do_set_config()").

> However, what was the purpose of the PIN_CONF_PACKED() translation?
> Why is it no longer needed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Bartosz Golaszewski]

pon., 20 sty 2020 o 09:44 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Bartosz,
>
> On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Instead of calling the gpiochip's set_config() callback directly and
> > checking its existence every time - just add a new routine that performs
> > this check internally. Call it in gpio_set_config() and
> > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> > the check for chip->set() as it's irrelevant to this config option.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>

[snip!]

>
> These two lines are not equivalent: the new code no longer uses the
> packed value of mode and arg!
> Hence this leads to subsequent cleanups in commits e5e42ad224a040f9
> ("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796
> ("gpiolib: Remove duplicated function gpio_do_set_config()").
>
> However, what was the purpose of the PIN_CONF_PACKED() translation?
> Why is it no longer needed?
>

Thanks for catching this. I was OoO for a couple days. I'll try to get
through the mail today and address this as well.

Bartosz

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Guenter Roeck]

On Tue, Dec 24, 2019 at 01:06:58PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Instead of calling the gpiochip's set_config() callback directly and
> checking its existence every time - just add a new routine that performs
> this check internally. Call it in gpio_set_config() and
> gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> the check for chip->set() as it's irrelevant to this config option.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

This patch made it into mainline, even though a regression was reported
against it by Geert. Please note that it is not just a theoretic problem
but _does_ indeed cause regressions.

Guenter

> ---
>  drivers/gpio/gpiolib.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e5d101ee9ada..616e431039fc 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
>   * rely on gpio_request() having been called beforehand.
>   */
>  
> +static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset,
> +			      enum pin_config_param mode)
> +{
> +	if (!gc->set_config)
> +		return -ENOTSUPP;
> +
> +	return gc->set_config(gc, offset, mode);
> +}
> +
>  static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
>  			   enum pin_config_param mode)
>  {
> @@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
>  	}
>  
>  	config = PIN_CONF_PACKED(mode, arg);
> -	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
> +	return gpio_do_set_config(gc, offset, mode);
>  }
>  
>  static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
> @@ -3294,15 +3303,9 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  
>  	VALIDATE_DESC(desc);
>  	chip = desc->gdev->chip;
> -	if (!chip->set || !chip->set_config) {
> -		gpiod_dbg(desc,
> -			  "%s: missing set() or set_config() operations\n",
> -			  __func__);
> -		return -ENOTSUPP;
> -	}
>  
>  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> +	return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
> @@ -3339,7 +3342,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
>  	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
>  					  !transitory);
>  	gpio = gpio_chip_hwgpio(desc);
> -	rc = chip->set_config(chip, gpio, packed);
> +	rc = gpio_do_set_config(chip, gpio, packed);
>  	if (rc == -ENOTSUPP) {
>  		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
>  				gpio);

Re: [PATCH v4 02/13] gpiolib: have a single place of calling set_config()

[Bartosz Golaszewski]

sob., 1 lut 2020 o 20:52 Guenter Roeck <linux@roeck-us.net> napisał(a):
>
> On Tue, Dec 24, 2019 at 01:06:58PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Instead of calling the gpiochip's set_config() callback directly and
> > checking its existence every time - just add a new routine that performs
> > this check internally. Call it in gpio_set_config() and
> > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop
> > the check for chip->set() as it's irrelevant to this config option.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> This patch made it into mainline, even though a regression was reported
> against it by Geert. Please note that it is not just a theoretic problem
> but _does_ indeed cause regressions.
>
> Guenter
>

Hi Guenter,

I'm sorry for this, I was still largely unavailable for the past two
weeks. I'll address it today, this time for real.

Best regards,
Bartosz Golaszewski

Re: [PATCH v4 12/13] gpiolib: add new ioctl() for monitoring changes in line info

[Kent Gibson]

On Tue, Dec 24, 2019 at 01:07:08PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 

[snip!]

> +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_real_ns();
> +	gpio_desc_to_lineinfo(desc, &chg.info);
> +

Is this call legal?  It can sleep - in fact you recently changed that
very function to move a mutex call outside of a spinlock protected section.
Here it is being called within an RCU lock, as lineinfo_changed_notify
is at the end of an atomic_notifier_call_chain.

I was looking at adding a chg.seqno here and considering what level of
locking would be appropriate for the source counter when I noticed that
call. Hopefully I'm missing something.

Cheers,
Kent.

Re: [PATCH v4 12/13] gpiolib: add new ioctl() for monitoring changes in line info

[Bartosz Golaszewski]

wt., 9 cze 2020 o 02:23 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Tue, Dec 24, 2019 at 01:07:08PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
>
> [snip!]
>
> > +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_real_ns();
> > +     gpio_desc_to_lineinfo(desc, &chg.info);
> > +
>
> Is this call legal?  It can sleep - in fact you recently changed that
> very function to move a mutex call outside of a spinlock protected section.
> Here it is being called within an RCU lock, as lineinfo_changed_notify
> is at the end of an atomic_notifier_call_chain.
>

Yeah, this is clearly wrong and lockdep would complain about invalid
wait context. I'm trying to remember why I went for an atomic notifier
chain here though... At first glance it doesn't look like the chain
could be called from atomic context anywhere.

Bart