Skip to contentSkip to conversation

gpiolib: add an ioctl() for monitoring line status changes

PATCH Superseded Bartosz Golaszewski 4 participants 22 messages Dec 19, 2019 – Dec 24, 2019

Subsystems (3)

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v3 cover gpiolib: add an ioctl() for monitoring line status changes

From: Bartosz Golaszewski <[email protected]>

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

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      | 402 +++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.h      |   4 +-
 include/linux/gpio/driver.h |   3 +-
 include/linux/kfifo.h       |  73 +++++++
 include/uapi/linux/gpio.h   |  24 +++
 tools/gpio/.gitignore       |   1 +
 tools/gpio/Build            |   1 +
 tools/gpio/Makefile         |  11 +-
 tools/gpio/gpio-watch       | Bin 0 -> 26528 bytes
 tools/gpio/gpio-watch.c     |  99 +++++++++
 10 files changed, 514 insertions(+), 104 deletions(-)
 create mode 100755 tools/gpio/gpio-watch
 create mode 100644 tools/gpio/gpio-watch.c
Signature · 1 line
2.23.0
#1 lore raw

PATCH v3 1/13 gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()

From: Bartosz Golaszewski <[email protected]>

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

---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib.c +1 -1
index 21b02a0064f8..a31797fe78fa 100644
@@ -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;
Signature · 1 line
2.23.0
#2 lore raw

PATCH v3 2/13 gpiolib: have a single place of calling set_config()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

---
 drivers/gpio/gpiolib.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
drivers/gpio/gpiolib.c +12 -9
index a31797fe78fa..72211407469f 100644
@@ -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);
Signature · 1 line
2.23.0
#3 lore raw

PATCH v3 3/13 gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

---
 drivers/gpio/gpiolib.c      | 5 +++--
 drivers/gpio/gpiolib.h      | 3 ++-
 include/linux/gpio/driver.h | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)
drivers/gpio/gpiolib.c +3 -2
index 72211407469f..b3ffb079e323 100644
@@ -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)
drivers/gpio/gpiolib.h +2 -1
index ca9bc1e4803c..a1cbeabadc69 100644
@@ -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,
include/linux/gpio/driver.h +2 -1
index e2480ef94c55..4f032de10bae 100644
@@ -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);
Signature · 1 line
2.23.0
#4 lore raw

PATCH v3 4/13 gpiolib: use gpiochip_get_desc() in linehandle_create()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

---
 drivers/gpio/gpiolib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/gpio/gpiolib.c +3 -4
index b3ffb079e323..6ef55cc1188b 100644
@@ -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;
Signature · 1 line
2.23.0
#5 lore raw

PATCH v3 5/13 gpiolib: use gpiochip_get_desc() in lineevent_create()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib.c +3 -3
index 6ef55cc1188b..17796437d7be 100644
@@ -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;
Signature · 1 line
2.23.0
#6 lore raw

PATCH v3 6/13 gpiolib: use gpiochip_get_desc() in gpio_ioctl()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib.c +4 -3
index 17796437d7be..b7043946c029 100644
@@ -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));
Signature · 1 line
2.23.0
#7 lore raw

PATCH v3 7/13 kfifo: provide noirqsave variants of spinlocked in and out helpers

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Provide variants of spinlocked kfifo_in() and kfifo_out() routines which don't disable interrupts.

---
 include/linux/kfifo.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
include/linux/kfifo.h +42 -0
index fc4b0b10210f..123c200ed7cb 100644
@@ -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)
Signature · 1 line
2.23.0
#8 lore raw

PATCH v3 8/13 kfifo: provide kfifo_is_empty_spinlocked()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Provide two spinlocked versions of kfifo_is_empty() to be used with spinlocked variants of kfifo_in() and kfifo_out().

---
 include/linux/kfifo.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
include/linux/kfifo.h +31 -0
index 123c200ed7cb..86249476b57f 100644
@@ -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
Signature · 1 line
2.23.0
#9 lore raw

PATCH v3 9/13 gpiolib: rework the locking mechanism for lineevent kfifo

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

---
 drivers/gpio/gpiolib.c | 64 +++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 29 deletions(-)
drivers/gpio/gpiolib.c +35 -29
index b7043946c029..073bc75b3d45 100644
@@ -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,
Signature · 1 line
2.23.0
#10 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpio/gpiolib.c +3 -0
index 073bc75b3d45..e27cf2113fed 100644
@@ -979,6 +979,9 @@ 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(
+			"%s: event FIFO is full - event dropped\n", __func__);
 
 	return IRQ_HANDLED;
 }
Signature · 1 line
2.23.0
#11 lore raw

PATCH v3 11/13 gpiolib: provide a dedicated function for setting lineinfo

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

---
 drivers/gpio/gpiolib.c | 98 ++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 43 deletions(-)
drivers/gpio/gpiolib.c +55 -43
index e27cf2113fed..41abf17640de 100644
@@ -1149,6 +1149,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
  */
@@ -1189,49 +1243,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;
Signature · 1 line
2.23.0
#12 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

---
 drivers/gpio/gpiolib.c    | 191 ++++++++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h    |   1 +
 include/uapi/linux/gpio.h |  24 +++++
 tools/gpio/gpio-watch     | Bin 0 -> 26528 bytes
 4 files changed, 208 insertions(+), 8 deletions(-)
 create mode 100755 tools/gpio/gpio-watch
drivers/gpio/gpiolib.c +183 -8
index 41abf17640de..a422086e6973 100644
@@ -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;
 }
@@ -1203,14 +1206,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)
@@ -1232,9 +1246,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;
@@ -1247,11 +1261,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;
 }
@@ -1264,6 +1292,105 @@ 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 = NOTIFY_DONE;
+
+	if (test_bit(desc_to_gpio(desc), priv->watched_lines)) {
+		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(
+				"%s: lineinfo event FIFO is full - event dropped\n",
+				__func__);
+
+		ret = NOTIFY_OK;
+	}
+
+	return ret;
+}
+
+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
@@ -1274,14 +1401,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;
+
+	ret = nonseekable_open(inode, filp);
+	if (ret)
+		goto out_unregister_notifier;
+
+	return ret;
 
-	return nonseekable_open(inode, filp);
+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;
 }
 
 /**
@@ -1292,17 +1453,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,
@@ -1513,6 +1680,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
@@ -2852,6 +3021,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;
 }
 
@@ -2949,6 +3120,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;
 }
 
@@ -3111,6 +3285,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
 		if (ret != -ENOTSUPP)
 			return ret;
 	}
+
 	return 0;
 }
 
drivers/gpio/gpiolib.h +1 -0
index a1cbeabadc69..8e3969616cfe 100644
@@ -54,6 +54,7 @@ struct gpio_device {
 	const char		*label;
 	void			*data;
 	struct list_head        list;
+	struct atomic_notifier_head notifier;
 
 #ifdef CONFIG_PINCTRL
 	/*
include/uapi/linux/gpio.h +24 -0
index 799cf823d493..a37ad3d97c8d 100644
@@ -59,6 +59,28 @@ 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
+ */
+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 +198,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)
 
tools/gpio/gpio-watch new file binary +0 -0
new file mode 100755
index 0000000000000000000000000000000000000000..d67bf021afad6b6eec40cf10b673c5127e55b0da
GIT binary patch
Signature · 1 line
2.23.0
#13 lore raw

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

Quoted 21 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 6 lines
---
 drivers/gpio/gpiolib.c    | 191 ++++++++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h    |   1 +
 include/uapi/linux/gpio.h |  24 +++++
 tools/gpio/gpio-watch     | Bin 0 -> 26528 bytes

Did you mean for this binary file to be checked in?

#14 lore raw

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

czw., 19 gru 2019 o 18:24 Greg Kroah-Hartman
<[email protected]> napisał(a):

Quoted 23 lines · Bartosz Golaszewski
Quoted 21 lines

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 9 lines
Quoted 6 lines
---
 drivers/gpio/gpiolib.c    | 191 ++++++++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h    |   1 +
 include/uapi/linux/gpio.h |  24 +++++
 tools/gpio/gpio-watch     | Bin 0 -> 26528 bytes

Did you mean for this binary file to be checked in?

No, of course not, it's a mistake. I'll remove it in v4.

Bart

#15 lore raw

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

In reply to Bartosz Golaszewski
Quoted 22 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

drivers/gpio/gpiolib.c static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 1250

Quoted 3 lines
drivers/gpio/gpiolib.c +2 -1
-       } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+       } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
+                  cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {

Wouldn't be better for maintenance to have them separated from the day 1?

...

Quoted 1 line

+ if (test_bit(desc_to_gpio(desc), priv->watched_lines)) {

if (!test_bit(...))
return NOTIFY_DONE;

?

drivers/gpio/gpiolib.c static long gpio_ioctl_compat(struct file *filp, unsigned int cmd, · line 1323

Quoted 4 lines
drivers/gpio/gpiolib.c +3 -0
+                       pr_debug_ratelimited(
+                               "%s: lineinfo event FIFO is full - event dropped\n",
 
+                               __func__);

This is in 99.9% cases redundant in *_dbg() calls.

drivers/gpio/gpiolib.c static long gpio_ioctl_compat(struct file *filp, unsigned int cmd, · line 1328

Quoted 4 lines
drivers/gpio/gpiolib.c +4 -0
+               ret = NOTIFY_OK;
+       }
+
+       return ret;

return NOTIFY_OK;
?

Quoted 1 line

+}

...

drivers/gpio/gpiolib.c static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc) · line 3291

Quoted 8 lines
drivers/gpio/gpiolib.c +1 -0
@@ -3111,6 +3285,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
                if (ret != -ENOTSUPP)
                        return ret;
        }
+
        return 0;
 }
 

This hunk doesn't belong to this patch.

...

include/uapi/linux/gpio.h struct gpioline_info { · line 78

Quoted 11 lines
include/uapi/linux/gpio.h +10 -0
+/**
+ * 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
+ */
+struct gpioline_info_changed {
 
+       struct gpioline_info info;

Is this guaranteed to be always 8 byte aligned?
I'm expecting to see some comments there and / or here about it.

Quoted 4 lines
include/uapi/linux/gpio.h +4 -0
+       __u64 timestamp;
+       __u32 event_type;
+       __u32 padding[5]; /* for future use */
+};
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#16 lore raw

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

czw., 19 gru 2019 o 19:17 Andy Shevchenko <[email protected]> napisał(a):

Quoted 24 lines · Bartosz Golaszewski
Quoted 21 lines

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 6 lines

drivers/gpio/gpiolib.c static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 1250

Quoted 3 lines
drivers/gpio/gpiolib.c +2 -1
-       } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+       } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
+                  cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {

Wouldn't be better for maintenance to have them separated from the day 1?

Sure. Will fix in v4.

Quoted 14 lines

...

Quoted 1 line

+ if (test_bit(desc_to_gpio(desc), priv->watched_lines)) {

if (!test_bit(...))
return NOTIFY_DONE;

?

drivers/gpio/gpiolib.c static long gpio_ioctl_compat(struct file *filp, unsigned int cmd, · line 1323

Quoted 4 lines
drivers/gpio/gpiolib.c +3 -0
+                       pr_debug_ratelimited(
+                               "%s: lineinfo event FIFO is full - event dropped\n",
 
+                               __func__);
Quoted 8 lines

This is in 99.9% cases redundant in *_dbg() calls.

drivers/gpio/gpiolib.c static long gpio_ioctl_compat(struct file *filp, unsigned int cmd, · line 1328

Quoted 4 lines
drivers/gpio/gpiolib.c +4 -0
+               ret = NOTIFY_OK;
+       }
+
+       return ret;

return NOTIFY_OK;

Yeah, makes sense.

Quoted 6 lines

?

Quoted 1 line

+}

...

Quoted 11 lines

drivers/gpio/gpiolib.c static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc) · line 3291

Quoted 8 lines
drivers/gpio/gpiolib.c +1 -0
@@ -3111,6 +3285,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
                if (ret != -ENOTSUPP)
                        return ret;
        }
+
        return 0;
 }
 

This hunk doesn't belong to this patch.

Ha! Of course it doesn't. :)

Quoted 12 lines

...

Quoted 9 lines
include/uapi/linux/gpio.h +9 -0
+/**
+ * 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
+ */
+struct gpioline_info_changed {
Quoted 5 lines
Quoted 1 line

+ struct gpioline_info info;

Is this guaranteed to be always 8 byte aligned?
I'm expecting to see some comments there and / or here about it.

struct gpioline_info alone is 32-bit aligned but its size is 72 bytes which works for 64-bit alignment. This new structure's biggest element in 64-bit, so it's 64-bit aligned on 64-bit arch. We have 72 bytes of gpioline_info, 8 bytes of timestamp, 32 bytes of event type and 5 * 32 bytes of padding. Should be fine, but I'll add comments to the header.

Bart

Quoted 8 lines

include/uapi/linux/gpio.h struct gpioline_info { · line 82

Quoted 4 lines
include/uapi/linux/gpio.h +4 -0
+       __u64 timestamp;
+       __u32 event_type;
+       __u32 padding[5]; /* for future use */
+};

--
With Best Regards,
Andy Shevchenko

#17 lore raw

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

Quoted 3 lines · Bartosz Golaszewski

czw., 19 gru 2019 o 19:17 Andy Shevchenko <[email protected]> napisał(a):

Quoted 1 line · Bartosz Golaszewski

...

Quoted 16 lines
Quoted 10 lines
Quoted 9 lines
include/uapi/linux/gpio.h +9 -0
+/**
+ * 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
+ */
+struct gpioline_info_changed {
Quoted 5 lines
Quoted 1 line

+ struct gpioline_info info;

Is this guaranteed to be always 8 byte aligned?
I'm expecting to see some comments there and / or here about it.

Quoted 5 lines

struct gpioline_info alone is 32-bit aligned but its size is 72 bytes which works for 64-bit alignment. This new structure's biggest element in 64-bit, so it's 64-bit aligned on 64-bit arch. We have 72 bytes of gpioline_info, 8 bytes of timestamp, 32 bytes of event type and 5 * 32 bytes of padding. Should be fine, but I'll add comments to the header.

Yes, what I meant is to add comment at least to struct gpioline_info definition that if somebody would like to change it there (which also might be a problematic here, if there is no versioning scheme / length member).

Quoted 4 lines
Quoted 4 lines

include/uapi/linux/gpio.h struct gpioline_info { · line 82

Quoted 4 lines
include/uapi/linux/gpio.h +4 -0
+       __u64 timestamp;
+       __u32 event_type;
+       __u32 padding[5]; /* for future use */
+};

Offtopic a bit, had you had a chance to look at Buildroot and our scripts I shared?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#18 lore raw

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

pt., 20 gru 2019 o 13:15 Andy Shevchenko <[email protected]> napisał(a):

Quoted 34 lines · Bartosz Golaszewski
Quoted 3 lines

czw., 19 gru 2019 o 19:17 Andy Shevchenko <[email protected]> napisał(a):

Quoted 1 line · Bartosz Golaszewski

...

Quoted 16 lines
Quoted 10 lines
Quoted 9 lines
include/uapi/linux/gpio.h +9 -0
+/**
+ * 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
+ */
+struct gpioline_info_changed {
Quoted 5 lines
Quoted 1 line

+ struct gpioline_info info;

Is this guaranteed to be always 8 byte aligned?
I'm expecting to see some comments there and / or here about it.

Quoted 5 lines

struct gpioline_info alone is 32-bit aligned but its size is 72 bytes which works for 64-bit alignment. This new structure's biggest element in 64-bit, so it's 64-bit aligned on 64-bit arch. We have 72 bytes of gpioline_info, 8 bytes of timestamp, 32 bytes of event type and 5 * 32 bytes of padding. Should be fine, but I'll add comments to the header.

Yes, what I meant is to add comment at least to struct gpioline_info definition that if somebody would like to change it there (which also might be a problematic here, if there is no versioning scheme / length member).

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines

include/uapi/linux/gpio.h struct gpioline_info { · line 82

Quoted 4 lines
include/uapi/linux/gpio.h +4 -0
+       __u64 timestamp;
+       __u32 event_type;
+       __u32 padding[5]; /* for future use */
+};

Offtopic a bit, had you had a chance to look at Buildroot and our scripts I shared?

Not yet, I'll do that over the weekend.

Bart

#19 lore raw

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

pt., 20 gru 2019 o 18:29 Bartosz Golaszewski
<[email protected]> napisał(a):

Quoted 16 lines
Quoted 16 lines
Quoted 6 lines

struct gpioline_info alone is 32-bit aligned but its size is 72 bytes which works for 64-bit alignment. This new structure's biggest element in 64-bit, so it's 64-bit aligned on 64-bit arch. We have 72 bytes of gpioline_info, 8 bytes of timestamp, 32 bytes of event type and 5 * 32 bytes of padding. Should be fine, but I'll add comments to the header.

Yes, what I meant is to add comment at least to struct gpioline_info definition that if somebody would like to change it there (which also might be a problematic here, if there is no versioning scheme / length member).

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
include/uapi/linux/gpio.h +4 -0
+       __u64 timestamp;
+       __u32 event_type;
+       __u32 padding[5]; /* for future use */
+};
Quoted 5 lines
Quoted 3 lines

Offtopic a bit, had you had a chance to look at Buildroot and our scripts I shared?

Not yet, I'll do that over the weekend.

Now tested using the buildroot image you posted. Thanks for that, I'll make sure to use it to test the v2 API for lineevents.

Bart

#20 lore raw

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

In reply to Andy Shevchenko

czw., 19 gru 2019 o 19:17 Andy Shevchenko <[email protected]> napisał(a):

Quoted 24 lines · Bartosz Golaszewski
Quoted 21 lines

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 6 lines

drivers/gpio/gpiolib.c static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 1250

Quoted 3 lines
drivers/gpio/gpiolib.c +2 -1
-       } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+       } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
+                  cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {

Wouldn't be better for maintenance to have them separated from the day 1?

I think this would lead to a lot of code duplication. I left it as it is is v4.

Bart

#21 lore raw

PATCH v3 13/13 tools: gpio: implement gpio-watch

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Add a simple program that allows to test the new LINECHANGED_FD ioctl().

---
 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
tools/gpio/.gitignore +1 -0
index a94c0e83b209..eab36c6d7751 100644
@@ -1,4 +1,5 @@
 gpio-event-mon
 gpio-hammer
+gpio-watch
 lsgpio
 include/linux/gpio.h
tools/gpio/Build +1 -0
index 4141f35837db..67c7b7f6a717 100644
@@ -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
tools/gpio/Makefile +10 -1
index 6080de58861f..842287e42c83 100644
@@ -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
tools/gpio/gpio-watch.c new file +99 -0

99 lines not on this page · raw message

Signature · 1 line
2.23.0