[PATCH v3 0/6] gpio: notify user-space about config changes in the kernel

[PATCH v3 0/6] gpio: notify user-space about config changes in the kernel

[Bartosz Golaszewski]

We currently only emit events on changed line config to user-space on
changes made from user-space. Users have no way of getting notified
about in-kernel changes. This series improves the situation and also
contains a couple other related improvements.

This is a reworked approach which gets and stores as much info (all of
it actually, except for the pinctrl state) the moment the event occurrs
but moves the actual queueing of the event on the kfifo to a dedicated
high-priority, ordered workqueue.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v3:
- don't reach into pinctrl if the USED flag is already set
- reword the comment from patch 5 and move it one patch back
- don't abandon emitting the event if the GPIO chip is gone by the time
  the work is executed, just don't reach into pinctrl and return
  whatever data we already had
- Link to v2: https://lore.kernel.org/r/20241010-gpio-notify-in-kernel-events-v2-0-b560411f7c59@linaro.org

Changes in v2:
- put all line-info events emitting on a workqueue to allow queueing
  them from atomic context
- switch the notifier type used for emitting info events to atomic
- add a patch notifying user-space about drivers requesting their own
  descs
- drop patches that were picked up
- Link to v1: https://lore.kernel.org/r/20241004-gpio-notify-in-kernel-events-v1-0-8ac29e1df4fe@linaro.org

---
Bartosz Golaszewski (6):
      gpiolib: notify user-space when a driver requests its own desc
      gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic
      gpiolib: add a per-gpio_device line state notification workqueue
      gpio: cdev: put emitting the line state events on a workqueue
      gpiolib: switch the line state notifier to atomic
      gpiolib: notify user-space about in-kernel line state changes

 drivers/gpio/gpiolib-cdev.c | 127 +++++++++++++++++++++++++++++++++-----------
 drivers/gpio/gpiolib.c      |  79 +++++++++++++++++++++++----
 drivers/gpio/gpiolib.h      |   8 ++-
 3 files changed, 172 insertions(+), 42 deletions(-)
---
base-commit: b852e1e7a0389ed6168ef1d38eb0bad71a6b11e8
change-id: 20240924-gpio-notify-in-kernel-events-07cd912153e8

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v3 1/6] gpiolib: notify user-space when a driver requests its own desc

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We notify user-space about lines being requested from user-space or by
drivers calling gpiod_get() but not when drivers request their own lines
so add the missing call to gpiod_line_state_notify().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 97346b746ef5..c09464f70f73 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2532,6 +2532,8 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 		return ERR_PTR(ret);
 	}
 
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+
 	return desc;
 }
 EXPORT_SYMBOL_GPL(gpiochip_request_own_desc);

-- 
2.43.0

[PATCH v3 2/6] gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to prepare gpio_desc_to_lineinfo() to being called from atomic
context, add a new argument - bool atomic - which, if set, indicates
that no sleeping functions must be called (currently: only
pinctrl_gpio_can_use_line()).

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b0050250ac3a..b28751242835 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2361,7 +2361,7 @@ static void gpio_v2_line_info_changed_to_v1(
 #endif /* CONFIG_GPIO_CDEV_V1 */
 
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
-				  struct gpio_v2_line_info *info)
+				  struct gpio_v2_line_info *info, bool atomic)
 {
 	unsigned long dflags;
 	const char *label;
@@ -2402,9 +2402,12 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	    test_bit(FLAG_USED_AS_IRQ, &dflags) ||
 	    test_bit(FLAG_EXPORT, &dflags) ||
 	    test_bit(FLAG_SYSFS, &dflags) ||
-	    !gpiochip_line_is_valid(guard.gc, info->offset) ||
-	    !pinctrl_gpio_can_use_line(guard.gc, info->offset))
+	    !gpiochip_line_is_valid(guard.gc, info->offset)) {
 		info->flags |= GPIO_V2_LINE_FLAG_USED;
+	} else if (!atomic) {
+		if (!pinctrl_gpio_can_use_line(guard.gc, info->offset))
+			info->flags |= GPIO_V2_LINE_FLAG_USED;
+	}
 
 	if (test_bit(FLAG_IS_OUT, &dflags))
 		info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
@@ -2502,7 +2505,7 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
 			return -EBUSY;
 	}
 
-	gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+	gpio_desc_to_lineinfo(desc, &lineinfo_v2, false);
 	gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
 
 	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
@@ -2539,7 +2542,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
 		if (test_and_set_bit(lineinfo.offset, cdev->watched_lines))
 			return -EBUSY;
 	}
-	gpio_desc_to_lineinfo(desc, &lineinfo);
+	gpio_desc_to_lineinfo(desc, &lineinfo, false);
 	supinfo_to_lineinfo(desc, &lineinfo);
 
 	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
@@ -2632,7 +2635,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	memset(&chg, 0, sizeof(chg));
 	chg.event_type = action;
 	chg.timestamp_ns = ktime_get_ns();
-	gpio_desc_to_lineinfo(desc, &chg.info);
+	gpio_desc_to_lineinfo(desc, &chg.info, false);
 	supinfo_to_lineinfo(desc, &chg.info);
 
 	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);

-- 
2.43.0

[PATCH v3 3/6] gpiolib: add a per-gpio_device line state notification workqueue

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to prepare the line state notification mechanism for working in
atomic context as well, add a dedicated, high-priority, ordered
workqueue to GPIO device which will be used to queue the events fron any
context for them to be emitted always in process context.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 6 ++++++
 drivers/gpio/gpiolib.h      | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b28751242835..5b28a558c9d0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2868,6 +2868,11 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 	gdev->chrdev.owner = THIS_MODULE;
 	gdev->dev.devt = MKDEV(MAJOR(devt), gdev->id);
 
+	gdev->line_state_wq = alloc_ordered_workqueue(dev_name(&gdev->dev),
+						      WQ_HIGHPRI);
+	if (!gdev->line_state_wq)
+		return -ENOMEM;
+
 	ret = cdev_device_add(&gdev->chrdev, &gdev->dev);
 	if (ret)
 		return ret;
@@ -2884,6 +2889,7 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 
 void gpiolib_cdev_unregister(struct gpio_device *gdev)
 {
+	destroy_workqueue(gdev->line_state_wq);
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
 	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 067197d61d57..d24cd9e8b17c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/srcu.h>
+#include <linux/workqueue.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
 
@@ -44,6 +45,8 @@
  * @list: links gpio_device:s together for traversal
  * @line_state_notifier: used to notify subscribers about lines being
  *                       requested, released or reconfigured
+ * @line_state_wq: used to emit line state events from a separate thread in
+ *                 process context
  * @device_notifier: used to notify character device wait queues about the GPIO
  *                   device being unregistered
  * @srcu: protects the pointer to the underlying GPIO chip
@@ -70,6 +73,7 @@ struct gpio_device {
 	void			*data;
 	struct list_head        list;
 	struct blocking_notifier_head line_state_notifier;
+	struct workqueue_struct	*line_state_wq;
 	struct blocking_notifier_head device_notifier;
 	struct srcu_struct	srcu;
 

-- 
2.43.0

[PATCH v3 4/6] gpio: cdev: put emitting the line state events on a workqueue

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to allow line state notifications to be emitted from atomic
context (for instance: from gpiod_direction_input/output()), we must
stop calling any sleeping functions in lineinfo_changed_notify(). To
that end let's use the new workqueue.

Let's atomically allocate small structures containing the required data
and fill it with information immediately upon being notified about the
change except for the pinctrl state which will be retrieved later from
process context. We can pretty reliably do this as pin functions are
typically set once per boot.

Let's make sure to bump the reference count of GPIO device and the GPIO
character device file descriptor to keep both alive until the event was
queued.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 84 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5b28a558c9d0..a10d6ea22415 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2450,6 +2450,7 @@ struct gpio_chardev_data {
 #ifdef CONFIG_GPIO_CDEV_V1
 	atomic_t watch_abi_version;
 #endif
+	struct file *fp;
 };
 
 static int chipinfo_get(struct gpio_chardev_data *cdev, void __user *ip)
@@ -2620,29 +2621,87 @@ static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
 }
 #endif
 
+struct lineinfo_changed_ctx {
+	struct work_struct work;
+	struct gpio_v2_line_info_changed chg;
+	struct gpio_device *gdev;
+	struct gpio_chardev_data *cdev;
+};
+
+static void lineinfo_changed_func(struct work_struct *work)
+{
+	struct lineinfo_changed_ctx *ctx =
+			container_of(work, struct lineinfo_changed_ctx, work);
+	struct gpio_chip *gc;
+	int ret;
+
+	if (!(ctx->chg.info.flags & GPIO_V2_LINE_FLAG_USED)) {
+		/*
+		 * If nobody set the USED flag earlier, let's see with pinctrl
+		 * now. We're doing this late because it's a sleeping function.
+		 * Pin functions are in general much more static and while it's
+		 * not 100% bullet-proof, it's good enough for most cases.
+		 */
+		scoped_guard(srcu, &ctx->gdev->srcu) {
+			gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu);
+			if (gc &&
+			    !pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset))
+				ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED;
+		}
+	}
+
+	ret = kfifo_in_spinlocked(&ctx->cdev->events, &ctx->chg, 1,
+				  &ctx->cdev->wait.lock);
+	if (ret)
+		wake_up_poll(&ctx->cdev->wait, EPOLLIN);
+	else
+		pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+
+	gpio_device_put(ctx->gdev);
+	fput(ctx->cdev->fp);
+	kfree(ctx);
+}
+
 static int lineinfo_changed_notify(struct notifier_block *nb,
 				   unsigned long action, void *data)
 {
 	struct gpio_chardev_data *cdev =
 		container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
-	struct gpio_v2_line_info_changed chg;
+	struct lineinfo_changed_ctx *ctx;
 	struct gpio_desc *desc = data;
-	int ret;
 
 	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
 		return NOTIFY_DONE;
 
-	memset(&chg, 0, sizeof(chg));
-	chg.event_type = action;
-	chg.timestamp_ns = ktime_get_ns();
-	gpio_desc_to_lineinfo(desc, &chg.info, false);
-	supinfo_to_lineinfo(desc, &chg.info);
+	/*
+	 * If this is called from atomic context (for instance: with a spinlock
+	 * taken by the atomic notifier chain), any sleeping calls must be done
+	 * outside of this function in process context of the dedicated
+	 * workqueue.
+	 *
+	 * Let's gather as much info as possible from the descriptor and
+	 * postpone just the call to pinctrl_gpio_can_use_line() until the work
+	 * is executed.
+	 */
 
-	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
-	if (ret)
-		wake_up_poll(&cdev->wait, EPOLLIN);
-	else
-		pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+	ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC);
+	if (!ctx) {
+		pr_err("Failed to allocate memory for line info notification\n");
+		return NOTIFY_DONE;
+	}
+
+	ctx->chg.event_type = action;
+	ctx->chg.timestamp_ns = ktime_get_ns();
+	gpio_desc_to_lineinfo(desc, &ctx->chg.info, true);
+	supinfo_to_lineinfo(desc, &ctx->chg.info);
+	/* Keep the GPIO device alive until we emit the event. */
+	ctx->gdev = gpio_device_get(desc->gdev);
+	ctx->cdev = cdev;
+	/* Keep the file descriptor alive too. */
+	get_file(ctx->cdev->fp);
+
+	INIT_WORK(&ctx->work, lineinfo_changed_func);
+	queue_work(ctx->gdev->line_state_wq, &ctx->work);
 
 	return NOTIFY_OK;
 }
@@ -2802,6 +2861,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 		goto out_unregister_line_notifier;
 
 	file->private_data = cdev;
+	cdev->fp = file;
 
 	ret = nonseekable_open(inode, file);
 	if (ret)

-- 
2.43.0

[PATCH v3 5/6] gpiolib: switch the line state notifier to atomic

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With everything else ready, we can now switch to using the atomic
notifier for line state events which will allow us to notify user-space
about direction changes from atomic context.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 12 ++++++------
 drivers/gpio/gpiolib.c      |  6 +++---
 drivers/gpio/gpiolib.h      |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index a10d6ea22415..f8c69ef33888 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2848,8 +2848,8 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	cdev->gdev = gpio_device_get(gdev);
 
 	cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
-	ret = blocking_notifier_chain_register(&gdev->line_state_notifier,
-					       &cdev->lineinfo_changed_nb);
+	ret = atomic_notifier_chain_register(&gdev->line_state_notifier,
+					     &cdev->lineinfo_changed_nb);
 	if (ret)
 		goto out_free_bitmap;
 
@@ -2873,8 +2873,8 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	blocking_notifier_chain_unregister(&gdev->device_notifier,
 					   &cdev->device_unregistered_nb);
 out_unregister_line_notifier:
-	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
-					   &cdev->lineinfo_changed_nb);
+	atomic_notifier_chain_unregister(&gdev->line_state_notifier,
+					 &cdev->lineinfo_changed_nb);
 out_free_bitmap:
 	gpio_device_put(gdev);
 	bitmap_free(cdev->watched_lines);
@@ -2898,8 +2898,8 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
 
 	blocking_notifier_chain_unregister(&gdev->device_notifier,
 					   &cdev->device_unregistered_nb);
-	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
-					   &cdev->lineinfo_changed_nb);
+	atomic_notifier_chain_unregister(&gdev->line_state_notifier,
+					 &cdev->lineinfo_changed_nb);
 	bitmap_free(cdev->watched_lines);
 	gpio_device_put(gdev);
 	kfree(cdev);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c09464f70f73..fafa759ce743 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1026,7 +1026,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		}
 	}
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
+	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 
 	ret = init_srcu_struct(&gdev->srcu);
@@ -4089,8 +4089,8 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep);
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action)
 {
-	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
-				     action, desc);
+	atomic_notifier_call_chain(&desc->gdev->line_state_notifier,
+				   action, desc);
 }
 
 /**
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d24cd9e8b17c..2799157a1f6b 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -72,7 +72,7 @@ struct gpio_device {
 	const char		*label;
 	void			*data;
 	struct list_head        list;
-	struct blocking_notifier_head line_state_notifier;
+	struct atomic_notifier_head line_state_notifier;
 	struct workqueue_struct	*line_state_wq;
 	struct blocking_notifier_head device_notifier;
 	struct srcu_struct	srcu;

-- 
2.43.0

[PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We currently only notify user-space about line config changes that are
made from user-space. Any kernel config changes are not signalled.

Let's improve the situation by emitting the events closer to the source.
To that end let's call the relevant notifier chain from the functions
setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
gpiod_toggle_active_low(). This covers all the options that we can
inform the user-space about. We ignore events which don't have
corresponding flags exported to user-space on purpose - otherwise the
user would see a config-changed event but the associated line-info would
remain unchanged.

gpiod_direction_output/input() can be called from any context.
Fortunately, we now emit line state events using an atomic notifier
chain, so it's no longer an issue.

Let's also add non-notifying wrappers around the direction setters in
order to not emit superfluous reconfigure events when requesting the
lines as the initial config should be part of the request notification.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 12 +++-----
 drivers/gpio/gpiolib.c      | 71 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/gpio/gpiolib.h      |  2 ++
 3 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f8c69ef33888..1296e6cbcef7 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -196,8 +196,6 @@ static long linehandle_set_config(struct linehandle_state *lh,
 			if (ret)
 				return ret;
 		}
-
-		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
 	}
 	return 0;
 }
@@ -363,11 +361,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
 			int val = !!handlereq.default_values[i];
 
-			ret = gpiod_direction_output(desc, val);
+			ret = gpiod_direction_output_nonotify(desc, val);
 			if (ret)
 				goto out_free_lh;
 		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			if (ret)
 				goto out_free_lh;
 		}
@@ -1568,8 +1566,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 		}
 
 		WRITE_ONCE(line->edflags, edflags);
-
-		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
 	}
 	return 0;
 }
@@ -1826,11 +1822,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 		if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
 			int val = gpio_v2_line_config_output_value(lc, i);
 
-			ret = gpiod_direction_output(desc, val);
+			ret = gpiod_direction_output_nonotify(desc, val);
 			if (ret)
 				goto out_free_linereq;
 		} else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			if (ret)
 				goto out_free_linereq;
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fafa759ce743..4303b6a689af 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2673,6 +2673,18 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
  * 0 on success, or negative errno on failure.
  */
 int gpiod_direction_input(struct gpio_desc *desc)
+{
+	int ret;
+
+	ret = gpiod_direction_input_nonotify(desc);
+	if (ret == 0)
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_input);
+
+int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 {
 	int ret = 0;
 
@@ -2720,7 +2732,6 @@ int gpiod_direction_input(struct gpio_desc *desc)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
@@ -2782,8 +2793,15 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
  */
 int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
 {
+	int ret;
+
 	VALIDATE_DESC(desc);
-	return gpiod_direction_output_raw_commit(desc, value);
+
+	ret = gpiod_direction_output_raw_commit(desc, value);
+	if (ret == 0)
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
 
@@ -2801,6 +2819,18 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
  * 0 on success, or negative errno on failure.
  */
 int gpiod_direction_output(struct gpio_desc *desc, int value)
+{
+	int ret;
+
+	ret = gpiod_direction_output_nonotify(desc, value);
+	if (ret == 0)
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_output);
+
+int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
 {
 	unsigned long flags;
 	int ret;
@@ -2863,7 +2893,6 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 		set_bit(FLAG_IS_OUT, &desc->flags);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
 /**
  * gpiod_enable_hw_timestamp_ns - Enable hardware timestamp in nanoseconds.
@@ -2942,13 +2971,34 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
  */
 int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 {
+	int ret;
+
 	VALIDATE_DESC(desc);
 
 	CLASS(gpio_chip_guard, guard)(desc);
 	if (!guard.gc)
 		return -ENODEV;
 
-	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+	if (ret == 0) {
+		/* These are the only options we notify the userspace about. */
+		switch (pinconf_to_config_param(config)) {
+		case PIN_CONFIG_BIAS_DISABLE:
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+		case PIN_CONFIG_BIAS_PULL_UP:
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+		case PIN_CONFIG_INPUT_DEBOUNCE:
+			gpiod_line_state_notify(desc,
+						GPIO_V2_LINE_CHANGED_CONFIG);
+			break;
+		default:
+			break;
+		}
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiod_set_config);
 
@@ -3015,6 +3065,7 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
 {
 	VALIDATE_DESC_VOID(desc);
 	change_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
 }
 EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
 
@@ -3659,9 +3710,15 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
  */
 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
 {
+	int ret;
+
 	VALIDATE_DESC(desc);
 
-	return desc_set_label(desc, name);
+	ret = desc_set_label(desc, name);
+	if (ret == 0)
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
 
@@ -4539,10 +4596,10 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 
 	/* Process flags */
 	if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
-		ret = gpiod_direction_output(desc,
+		ret = gpiod_direction_output_nonotify(desc,
 				!!(dflags & GPIOD_FLAGS_BIT_DIR_VAL));
 	else
-		ret = gpiod_direction_input(desc);
+		ret = gpiod_direction_input_nonotify(desc);
 
 	return ret;
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2799157a1f6b..fc321d281346 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -155,6 +155,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
+int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value);
+int gpiod_direction_input_nonotify(struct gpio_desc *desc);
 
 struct gpio_desc_label {
 	struct rcu_head rh;

-- 
2.43.0

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We currently only notify user-space about line config changes that are
> made from user-space. Any kernel config changes are not signalled.
>
> Let's improve the situation by emitting the events closer to the source.
> To that end let's call the relevant notifier chain from the functions
> setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
> gpiod_toggle_active_low(). This covers all the options that we can
> inform the user-space about. We ignore events which don't have
> corresponding flags exported to user-space on purpose - otherwise the
> user would see a config-changed event but the associated line-info would
> remain unchanged.
>
> gpiod_direction_output/input() can be called from any context.
> Fortunately, we now emit line state events using an atomic notifier
> chain, so it's no longer an issue.
>
> Let's also add non-notifying wrappers around the direction setters in
> order to not emit superfluous reconfigure events when requesting the
> lines as the initial config should be part of the request notification.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib-cdev.c | 12 +++-----
>  drivers/gpio/gpiolib.c      | 71 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/gpio/gpiolib.h      |  2 ++
>  3 files changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f8c69ef33888..1296e6cbcef7 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -196,8 +196,6 @@ static long linehandle_set_config(struct linehandle_state *lh,
>  			if (ret)
>  				return ret;
>  		}
> -
> -		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  	}
>  	return 0;
>  }
> @@ -363,11 +361,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>  		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
>  			int val = !!handlereq.default_values[i];
>
> -			ret = gpiod_direction_output(desc, val);
> +			ret = gpiod_direction_output_nonotify(desc, val);
>  			if (ret)
>  				goto out_free_lh;
>  		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			if (ret)
>  				goto out_free_lh;
>  		}
> @@ -1568,8 +1566,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>  		}
>
>  		WRITE_ONCE(line->edflags, edflags);
> -
> -		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  	}
>  	return 0;
>  }
> @@ -1826,11 +1822,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>  		if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
>  			int val = gpio_v2_line_config_output_value(lc, i);
>
> -			ret = gpiod_direction_output(desc, val);
> +			ret = gpiod_direction_output_nonotify(desc, val);
>  			if (ret)
>  				goto out_free_linereq;
>  		} else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			if (ret)
>  				goto out_free_linereq;
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index fafa759ce743..4303b6a689af 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2673,6 +2673,18 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
>   * 0 on success, or negative errno on failure.
>   */
>  int gpiod_direction_input(struct gpio_desc *desc)
> +{
> +	int ret;
> +
> +	ret = gpiod_direction_input_nonotify(desc);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_direction_input);
> +
> +int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>  {
>  	int ret = 0;
>
> @@ -2720,7 +2732,6 @@ int gpiod_direction_input(struct gpio_desc *desc)
>
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(gpiod_direction_input);
>
>  static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>  {
> @@ -2782,8 +2793,15 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>   */
>  int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
> -	return gpiod_direction_output_raw_commit(desc, value);
> +
> +	ret = gpiod_direction_output_raw_commit(desc, value);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>
> @@ -2801,6 +2819,18 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>   * 0 on success, or negative errno on failure.
>   */
>  int gpiod_direction_output(struct gpio_desc *desc, int value)
> +{
> +	int ret;
> +
> +	ret = gpiod_direction_output_nonotify(desc, value);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_direction_output);
> +
> +int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
>  {
>  	unsigned long flags;
>  	int ret;
> @@ -2863,7 +2893,6 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
>  		set_bit(FLAG_IS_OUT, &desc->flags);
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(gpiod_direction_output);
>
>  /**
>   * gpiod_enable_hw_timestamp_ns - Enable hardware timestamp in nanoseconds.
> @@ -2942,13 +2971,34 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
>   */
>  int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
>
>  	CLASS(gpio_chip_guard, guard)(desc);
>  	if (!guard.gc)
>  		return -ENODEV;
>
> -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> +	if (ret == 0) {
> +		/* These are the only options we notify the userspace about. */
> +		switch (pinconf_to_config_param(config)) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		case PIN_CONFIG_INPUT_DEBOUNCE:
> +			gpiod_line_state_notify(desc,
> +						GPIO_V2_LINE_CHANGED_CONFIG);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }

Ah, the debounce - I forgot about that, and other features that cdev
might emulate.

What happens if userspace requests a line with debounce that is
supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
LINE_CONFIG_CHANGED when the line is requested.

Conversely, what if a config change impacts features that don't result in a
notification from gpiod_set_config(), like active low, or emulated
drive or debounce?

Other than those little wrinkles, it all looks good to me.
Though I haven't done any testing on it yet - will do as soon as I can.

Cheers,
Kent.

>  EXPORT_SYMBOL_GPL(gpiod_set_config);
>
> @@ -3015,6 +3065,7 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
>  {
>  	VALIDATE_DESC_VOID(desc);
>  	change_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
>
> @@ -3659,9 +3710,15 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
>   */
>  int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
>
> -	return desc_set_label(desc, name);
> +	ret = desc_set_label(desc, name);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
>
> @@ -4539,10 +4596,10 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>
>  	/* Process flags */
>  	if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
> -		ret = gpiod_direction_output(desc,
> +		ret = gpiod_direction_output_nonotify(desc,
>  				!!(dflags & GPIOD_FLAGS_BIT_DIR_VAL));
>  	else
> -		ret = gpiod_direction_input(desc);
> +		ret = gpiod_direction_input_nonotify(desc);
>
>  	return ret;
>  }
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 2799157a1f6b..fc321d281346 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -155,6 +155,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>  int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
>
>  void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
> +int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value);
> +int gpiod_direction_input_nonotify(struct gpio_desc *desc);
>
>  struct gpio_desc_label {
>  	struct rcu_head rh;
>
> --
> 2.43.0
>

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > +	if (ret == 0) {
> > +		/* These are the only options we notify the userspace about. */
> > +		switch (pinconf_to_config_param(config)) {
> > +		case PIN_CONFIG_BIAS_DISABLE:
> > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> > +		case PIN_CONFIG_BIAS_PULL_UP:
> > +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> > +		case PIN_CONFIG_INPUT_DEBOUNCE:
> > +			gpiod_line_state_notify(desc,
> > +						GPIO_V2_LINE_CHANGED_CONFIG);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> >  }
>
> Ah, the debounce - I forgot about that, and other features that cdev
> might emulate.
>
> What happens if userspace requests a line with debounce that is
> supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> LINE_CONFIG_CHANGED when the line is requested.
>

This is problematic for me to test at the moment, as gpiosim doesn't support
debounce. Any chance we could make that configurable?  Similarly drive.

> Conversely, what if a config change impacts features that don't result in a
> notification from gpiod_set_config(), like active low, or emulated
> drive or debounce?
>

Bah, drive is emulated in gpiolib itself, so that should be fine.

When changing config cdev always calls gpiod_direction_input/output(), so I
think that covers the active low case.

But I have a test taking a line from input to output|open_drain and I
get two change events.  The first is the most interesting as it reports
input|open_drain, the second then reports output|open_drain.
That is due to gpiod_direction_output() calling gpiod_set_config() to
set the drive, and later to set the direction, in that order.
Given it will be setting the direction, it should inhibit the event from
the drive setting?

Still haven't tested any debounce changes...

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

On Wed, Oct 16, 2024 at 9:27 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > -   return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > +   ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > +   if (ret == 0) {
> > > +           /* These are the only options we notify the userspace about. */
> > > +           switch (pinconf_to_config_param(config)) {
> > > +           case PIN_CONFIG_BIAS_DISABLE:
> > > +           case PIN_CONFIG_BIAS_PULL_DOWN:
> > > +           case PIN_CONFIG_BIAS_PULL_UP:
> > > +           case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > +           case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > +           case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > +           case PIN_CONFIG_INPUT_DEBOUNCE:
> > > +                   gpiod_line_state_notify(desc,
> > > +                                           GPIO_V2_LINE_CHANGED_CONFIG);
> > > +                   break;
> > > +           default:
> > > +                   break;
> > > +           }
> > > +   }
> > > +
> > > +   return ret;
> > >  }
> >
> > Ah, the debounce - I forgot about that, and other features that cdev
> > might emulate.
> >
> > What happens if userspace requests a line with debounce that is
> > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> > LINE_CONFIG_CHANGED when the line is requested.
> >
>
> This is problematic for me to test at the moment, as gpiosim doesn't support
> debounce. Any chance we could make that configurable?  Similarly drive.
>
> > Conversely, what if a config change impacts features that don't result in a
> > notification from gpiod_set_config(), like active low, or emulated
> > drive or debounce?
> >
>
> Bah, drive is emulated in gpiolib itself, so that should be fine.
>
> When changing config cdev always calls gpiod_direction_input/output(), so I
> think that covers the active low case.
>
> But I have a test taking a line from input to output|open_drain and I
> get two change events.  The first is the most interesting as it reports
> input|open_drain, the second then reports output|open_drain.
> That is due to gpiod_direction_output() calling gpiod_set_config() to

No, it never calls gpiod_set_config() but gpio_set_config() which
never emits an event.

> set the drive, and later to set the direction, in that order.
> Given it will be setting the direction, it should inhibit the event from
> the drive setting?

I think you're really hitting this:
https://github.com/brgl/linux/blob/b4/gpio-notify-in-kernel-events/drivers/gpio/gpiolib.c#L2863

These should be changed to nonotify variants too. Would you mind confirming?

Bart

>
> Still haven't tested any debounce changes...
>
> Cheers,
> Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 03:27:30PM +0800, Kent Gibson wrote:
> On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > +	if (ret == 0) {
> > > +		/* These are the only options we notify the userspace about. */
> > > +		switch (pinconf_to_config_param(config)) {
> > > +		case PIN_CONFIG_BIAS_DISABLE:
> > > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> > > +		case PIN_CONFIG_BIAS_PULL_UP:
> > > +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > +		case PIN_CONFIG_INPUT_DEBOUNCE:
> > > +			gpiod_line_state_notify(desc,
> > > +						GPIO_V2_LINE_CHANGED_CONFIG);
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return ret;
> > >  }
> >
> > Ah, the debounce - I forgot about that, and other features that cdev
> > might emulate.
> >
> > What happens if userspace requests a line with debounce that is
> > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> > LINE_CONFIG_CHANGED when the line is requested.
> >
>
> This is problematic for me to test at the moment, as gpiosim doesn't support
> debounce. Any chance we could make that configurable?  Similarly drive.
>
> > Conversely, what if a config change impacts features that don't result in a
> > notification from gpiod_set_config(), like active low, or emulated
> > drive or debounce?
> >
>
> Bah, drive is emulated in gpiolib itself, so that should be fine.
>
> When changing config cdev always calls gpiod_direction_input/output(), so I
> think that covers the active low case.
>
> But I have a test taking a line from input to output|open_drain and I
> get two change events.  The first is the most interesting as it reports
> input|open_drain, the second then reports output|open_drain.
> That is due to gpiod_direction_output() calling gpiod_set_config() to
> set the drive, and later to set the direction, in that order.
> Given it will be setting the direction, it should inhibit the event from
> the drive setting?
>

Ok, I oversimplified, it was actually

input -> output|active_low|open_drain

and

input -> output|open_source

fails the same way.

> Still haven't tested any debounce changes...
>

Have now.

input -> input|debounce

does not report the debounce.  Only get the one event and it does not
contain any debounce.

Similarly

input -> input|edge|debounce

which exercises a different path through cdev.

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

On Wed, 16 Oct 2024 10:37:47 +0200, Kent Gibson <warthog618@gmail.com> said:
> On Wed, Oct 16, 2024 at 03:27:30PM +0800, Kent Gibson wrote:
>> On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
>> > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
>> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> > >
>> > > -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
>> > > +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
>> > > +	if (ret == 0) {
>> > > +		/* These are the only options we notify the userspace about. */
>> > > +		switch (pinconf_to_config_param(config)) {
>> > > +		case PIN_CONFIG_BIAS_DISABLE:
>> > > +		case PIN_CONFIG_BIAS_PULL_DOWN:
>> > > +		case PIN_CONFIG_BIAS_PULL_UP:
>> > > +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
>> > > +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
>> > > +		case PIN_CONFIG_DRIVE_PUSH_PULL:
>> > > +		case PIN_CONFIG_INPUT_DEBOUNCE:
>> > > +			gpiod_line_state_notify(desc,
>> > > +						GPIO_V2_LINE_CHANGED_CONFIG);
>> > > +			break;
>> > > +		default:
>> > > +			break;
>> > > +		}
>> > > +	}
>> > > +
>> > > +	return ret;
>> > >  }
>> >
>> > Ah, the debounce - I forgot about that, and other features that cdev
>> > might emulate.
>> >
>> > What happens if userspace requests a line with debounce that is
>> > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
>> > LINE_CONFIG_CHANGED when the line is requested.
>> >
>>
>> This is problematic for me to test at the moment, as gpiosim doesn't support
>> debounce. Any chance we could make that configurable?  Similarly drive.
>>
>> > Conversely, what if a config change impacts features that don't result in a
>> > notification from gpiod_set_config(), like active low, or emulated
>> > drive or debounce?
>> >
>>
>> Bah, drive is emulated in gpiolib itself, so that should be fine.
>>
>> When changing config cdev always calls gpiod_direction_input/output(), so I
>> think that covers the active low case.
>>
>> But I have a test taking a line from input to output|open_drain and I
>> get two change events.  The first is the most interesting as it reports
>> input|open_drain, the second then reports output|open_drain.
>> That is due to gpiod_direction_output() calling gpiod_set_config() to
>> set the drive, and later to set the direction, in that order.
>> Given it will be setting the direction, it should inhibit the event from
>> the drive setting?
>>
>
> Ok, I oversimplified, it was actually
>
> input -> output|active_low|open_drain
>
> and
>
> input -> output|open_source
>
> fails the same way.
>

Does the following help?

@@ -2830,7 +2860,7 @@ int gpiod_direction_output(struct gpio_desc
*desc, int value)
 			goto set_output_value;
 		/* Emulate open drain by not actively driving the line high */
 		if (value) {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			goto set_output_flag;
 		}
 	} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
@@ -2839,7 +2869,7 @@ int gpiod_direction_output(struct gpio_desc
*desc, int value)
 			goto set_output_value;
 		/* Emulate open source by not actively driving the line low */
 		if (!value) {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			goto set_output_flag;
 		}
 	} else {

>> Still haven't tested any debounce changes...
>>
>
> Have now.
>
> input -> input|debounce
>
> does not report the debounce.  Only get the one event and it does not
> contain any debounce.
>

You mean, you get a CHANGED_CONFIG event but the debounce value is not
in the associated line info?

> Similarly
>
> input -> input|edge|debounce
>
> which exercises a different path through cdev.
>
> Cheers,
> Kent.
>

Bart

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 09:50:58AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 16, 2024 at 9:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> > > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > -   return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > +   ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > +   if (ret == 0) {
> > > > +           /* These are the only options we notify the userspace about. */
> > > > +           switch (pinconf_to_config_param(config)) {
> > > > +           case PIN_CONFIG_BIAS_DISABLE:
> > > > +           case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > +           case PIN_CONFIG_BIAS_PULL_UP:
> > > > +           case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > +           case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > > +           case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > +           case PIN_CONFIG_INPUT_DEBOUNCE:
> > > > +                   gpiod_line_state_notify(desc,
> > > > +                                           GPIO_V2_LINE_CHANGED_CONFIG);
> > > > +                   break;
> > > > +           default:
> > > > +                   break;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   return ret;
> > > >  }
> > >
> > > Ah, the debounce - I forgot about that, and other features that cdev
> > > might emulate.
> > >
> > > What happens if userspace requests a line with debounce that is
> > > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> > > LINE_CONFIG_CHANGED when the line is requested.
> > >
> >
> > This is problematic for me to test at the moment, as gpiosim doesn't support
> > debounce. Any chance we could make that configurable?  Similarly drive.
> >
> > > Conversely, what if a config change impacts features that don't result in a
> > > notification from gpiod_set_config(), like active low, or emulated
> > > drive or debounce?
> > >
> >
> > Bah, drive is emulated in gpiolib itself, so that should be fine.
> >
> > When changing config cdev always calls gpiod_direction_input/output(), so I
> > think that covers the active low case.
> >
> > But I have a test taking a line from input to output|open_drain and I
> > get two change events.  The first is the most interesting as it reports
> > input|open_drain, the second then reports output|open_drain.
> > That is due to gpiod_direction_output() calling gpiod_set_config() to
>
> No, it never calls gpiod_set_config() but gpio_set_config() which
> never emits an event.
>

Depends whether the driver supports drive or not.

If it does then
line 2858:

		ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_DRAIN);

will emit.

So your _nonotify function is calling a bunch of functions that can
emit.

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

On Wed, Oct 16, 2024 at 10:57 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Oct 16, 2024 at 09:50:58AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Oct 16, 2024 at 9:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> > > > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > -   return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > > +   ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > > +   if (ret == 0) {
> > > > > +           /* These are the only options we notify the userspace about. */
> > > > > +           switch (pinconf_to_config_param(config)) {
> > > > > +           case PIN_CONFIG_BIAS_DISABLE:
> > > > > +           case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > > +           case PIN_CONFIG_BIAS_PULL_UP:
> > > > > +           case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > > +           case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > > > +           case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > > +           case PIN_CONFIG_INPUT_DEBOUNCE:
> > > > > +                   gpiod_line_state_notify(desc,
> > > > > +                                           GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > +                   break;
> > > > > +           default:
> > > > > +                   break;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   return ret;
> > > > >  }
> > > >
> > > > Ah, the debounce - I forgot about that, and other features that cdev
> > > > might emulate.
> > > >
> > > > What happens if userspace requests a line with debounce that is
> > > > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> > > > LINE_CONFIG_CHANGED when the line is requested.
> > > >
> > >
> > > This is problematic for me to test at the moment, as gpiosim doesn't support
> > > debounce. Any chance we could make that configurable?  Similarly drive.
> > >
> > > > Conversely, what if a config change impacts features that don't result in a
> > > > notification from gpiod_set_config(), like active low, or emulated
> > > > drive or debounce?
> > > >
> > >
> > > Bah, drive is emulated in gpiolib itself, so that should be fine.
> > >
> > > When changing config cdev always calls gpiod_direction_input/output(), so I
> > > think that covers the active low case.
> > >
> > > But I have a test taking a line from input to output|open_drain and I
> > > get two change events.  The first is the most interesting as it reports
> > > input|open_drain, the second then reports output|open_drain.
> > > That is due to gpiod_direction_output() calling gpiod_set_config() to
> >
> > No, it never calls gpiod_set_config() but gpio_set_config() which
> > never emits an event.
> >
>
> Depends whether the driver supports drive or not.
>
> If it does then
> line 2858:
>
>                 ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_DRAIN);
>

I'm seeing this:

gpiod_direction_output_nonotify()
  gpio_set_config()
    gpio_set_config_with_argument()
      gpio_do_set_config()
        gc->set_config()

There's no call to gpiod_line_state_notify() in this path.

Bart

> will emit.
>
> So your _nonotify function is calling a bunch of functions that can
> emit.
>
> Cheers,
> Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 11:02:37AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 16, 2024 at 10:57 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, Oct 16, 2024 at 09:50:58AM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Oct 16, 2024 at 9:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> > > > > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > >
> > > > > > -   return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > > > +   ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> > > > > > +   if (ret == 0) {
> > > > > > +           /* These are the only options we notify the userspace about. */
> > > > > > +           switch (pinconf_to_config_param(config)) {
> > > > > > +           case PIN_CONFIG_BIAS_DISABLE:
> > > > > > +           case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > > > +           case PIN_CONFIG_BIAS_PULL_UP:
> > > > > > +           case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > > > +           case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > > > > +           case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > > > +           case PIN_CONFIG_INPUT_DEBOUNCE:
> > > > > > +                   gpiod_line_state_notify(desc,
> > > > > > +                                           GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > > +                   break;
> > > > > > +           default:
> > > > > > +                   break;
> > > > > > +           }
> > > > > > +   }
> > > > > > +
> > > > > > +   return ret;
> > > > > >  }
> > > > >
> > > > > Ah, the debounce - I forgot about that, and other features that cdev
> > > > > might emulate.
> > > > >
> > > > > What happens if userspace requests a line with debounce that is
> > > > > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> > > > > LINE_CONFIG_CHANGED when the line is requested.
> > > > >
> > > >
> > > > This is problematic for me to test at the moment, as gpiosim doesn't support
> > > > debounce. Any chance we could make that configurable?  Similarly drive.
> > > >
> > > > > Conversely, what if a config change impacts features that don't result in a
> > > > > notification from gpiod_set_config(), like active low, or emulated
> > > > > drive or debounce?
> > > > >
> > > >
> > > > Bah, drive is emulated in gpiolib itself, so that should be fine.
> > > >
> > > > When changing config cdev always calls gpiod_direction_input/output(), so I
> > > > think that covers the active low case.
> > > >
> > > > But I have a test taking a line from input to output|open_drain and I
> > > > get two change events.  The first is the most interesting as it reports
> > > > input|open_drain, the second then reports output|open_drain.
> > > > That is due to gpiod_direction_output() calling gpiod_set_config() to
> > >
> > > No, it never calls gpiod_set_config() but gpio_set_config() which
> > > never emits an event.
> > >
> >
> > Depends whether the driver supports drive or not.
> >
> > If it does then
> > line 2858:
> >
> >                 ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_DRAIN);
> >
>
> I'm seeing this:
>
> gpiod_direction_output_nonotify()
>   gpio_set_config()
>     gpio_set_config_with_argument()
>       gpio_do_set_config()
>         gc->set_config()
>
> There's no call to gpiod_line_state_notify() in this path.
>

Ah, my bad - thought it was gpiod_set_config().
So it is just the gpiod_direction_input() calls.

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 01:52:49AM -0700, Bartosz Golaszewski wrote:
> On Wed, 16 Oct 2024 10:37:47 +0200, Kent Gibson <warthog618@gmail.com> said:
> > On Wed, Oct 16, 2024 at 03:27:30PM +0800, Kent Gibson wrote:
> >> On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> >> > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> >> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> > >
> >> > > -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> >> > > +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> >> > > +	if (ret == 0) {
> >> > > +		/* These are the only options we notify the userspace about. */
> >> > > +		switch (pinconf_to_config_param(config)) {
> >> > > +		case PIN_CONFIG_BIAS_DISABLE:
> >> > > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> >> > > +		case PIN_CONFIG_BIAS_PULL_UP:
> >> > > +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> >> > > +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> >> > > +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> >> > > +		case PIN_CONFIG_INPUT_DEBOUNCE:
> >> > > +			gpiod_line_state_notify(desc,
> >> > > +						GPIO_V2_LINE_CHANGED_CONFIG);
> >> > > +			break;
> >> > > +		default:
> >> > > +			break;
> >> > > +		}
> >> > > +	}
> >> > > +
> >> > > +	return ret;
> >> > >  }
> >> >
> >> > Ah, the debounce - I forgot about that, and other features that cdev
> >> > might emulate.
> >> >
> >> > What happens if userspace requests a line with debounce that is
> >> > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> >> > LINE_CONFIG_CHANGED when the line is requested.
> >> >
> >>
> >> This is problematic for me to test at the moment, as gpiosim doesn't support
> >> debounce. Any chance we could make that configurable?  Similarly drive.
> >>
> >> > Conversely, what if a config change impacts features that don't result in a
> >> > notification from gpiod_set_config(), like active low, or emulated
> >> > drive or debounce?
> >> >
> >>
> >> Bah, drive is emulated in gpiolib itself, so that should be fine.
> >>
> >> When changing config cdev always calls gpiod_direction_input/output(), so I
> >> think that covers the active low case.
> >>
> >> But I have a test taking a line from input to output|open_drain and I
> >> get two change events.  The first is the most interesting as it reports
> >> input|open_drain, the second then reports output|open_drain.
> >> That is due to gpiod_direction_output() calling gpiod_set_config() to
> >> set the drive, and later to set the direction, in that order.
> >> Given it will be setting the direction, it should inhibit the event from
> >> the drive setting?
> >>
> >
> > Ok, I oversimplified, it was actually
> >
> > input -> output|active_low|open_drain
> >
> > and
> >
> > input -> output|open_source
> >
> > fails the same way.
> >
>
> Does the following help?
>
> @@ -2830,7 +2860,7 @@ int gpiod_direction_output(struct gpio_desc
> *desc, int value)
>  			goto set_output_value;
>  		/* Emulate open drain by not actively driving the line high */
>  		if (value) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			goto set_output_flag;
>  		}
>  	} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
> @@ -2839,7 +2869,7 @@ int gpiod_direction_output(struct gpio_desc
> *desc, int value)
>  			goto set_output_value;
>  		/* Emulate open source by not actively driving the line low */
>  		if (!value) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			goto set_output_flag;
>  		}
>  	} else {
>

That fixes the drive problems.

> >> Still haven't tested any debounce changes...
> >>
> >
> > Have now.
> >
> > input -> input|debounce
> >
> > does not report the debounce.  Only get the one event and it does not
> > contain any debounce.
> >
>
> You mean, you get a CHANGED_CONFIG event but the debounce value is not
> in the associated line info?
>

Correct.

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

On Wed, Oct 16, 2024 at 11:17 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> >
> > Does the following help?
> >
> > @@ -2830,7 +2860,7 @@ int gpiod_direction_output(struct gpio_desc
> > *desc, int value)
> >                       goto set_output_value;
> >               /* Emulate open drain by not actively driving the line high */
> >               if (value) {
> > -                     ret = gpiod_direction_input(desc);
> > +                     ret = gpiod_direction_input_nonotify(desc);
> >                       goto set_output_flag;
> >               }
> >       } else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
> > @@ -2839,7 +2869,7 @@ int gpiod_direction_output(struct gpio_desc
> > *desc, int value)
> >                       goto set_output_value;
> >               /* Emulate open source by not actively driving the line low */
> >               if (!value) {
> > -                     ret = gpiod_direction_input(desc);
> > +                     ret = gpiod_direction_input_nonotify(desc);
> >                       goto set_output_flag;
> >               }
> >       } else {
> >
>
> That fixes the drive problems.
>

Ok, thanks.

> > >> Still haven't tested any debounce changes...
> > >>
> > >
> > > Have now.
> > >
> > > input -> input|debounce
> > >
> > > does not report the debounce.  Only get the one event and it does not
> > > contain any debounce.
> > >
> >
> > You mean, you get a CHANGED_CONFIG event but the debounce value is not
> > in the associated line info?
> >
>
> Correct.
>

Ok, let me see.

Bart

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 11:22:07AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 16, 2024 at 11:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > > >
> > >
> > > You mean, you get a CHANGED_CONFIG event but the debounce value is not
> > > in the associated line info?
> > >
> >
> > Correct.
> >
>
> Ok, let me see.
>

When setting from userspace the issue is that linereq_set_config() setting the
direction will emit, quite possibly before the debounce has been set.  The
edge_detector_setup() that does set it can also emit, though only if the
hardware supports debounce.  And then there could be a race between the
notifier being called and the period being set in the supinfo.
(the set will probably win that one)

Debounce set from the kernel side is going to be an issue as cdev
catches and stores the value from userspace to report in the supinfo - that
isn't the case for kernel calls to gpiod_set_config().

Seems moving the debounce value out of the desc and into cdev, which seemed a
good idea at the time, might come back and bite now if it is no longer
restricted to being cdev specific.  Now there is an actual reason to
store it in the desc :(.

Cheers,
Kent.

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Bartosz Golaszewski]

On Wed, Oct 16, 2024 at 11:43 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Oct 16, 2024 at 11:22:07AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Oct 16, 2024 at 11:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > > >
> > > >
> > > > You mean, you get a CHANGED_CONFIG event but the debounce value is not
> > > > in the associated line info?
> > > >
> > >
> > > Correct.
> > >
> >
> > Ok, let me see.
> >
>
> When setting from userspace the issue is that linereq_set_config() setting the
> direction will emit, quite possibly before the debounce has been set.  The
> edge_detector_setup() that does set it can also emit, though only if the
> hardware supports debounce.  And then there could be a race between the
> notifier being called and the period being set in the supinfo.
> (the set will probably win that one)
>
> Debounce set from the kernel side is going to be an issue as cdev
> catches and stores the value from userspace to report in the supinfo - that
> isn't the case for kernel calls to gpiod_set_config().
>
> Seems moving the debounce value out of the desc and into cdev, which seemed a
> good idea at the time, might come back and bite now if it is no longer
> restricted to being cdev specific.  Now there is an actual reason to
> store it in the desc :(.
>

I'm seeing commit:

commit 9344e34e7992fec95ce6210d95ac01437dd327ab
Author: Kent Gibson <warthog618@gmail.com>
Date:   Tue Dec 19 08:41:54 2023 +0800

    gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

    Store the debounce period for a requested line locally, rather than in
    the debounce_period_us field in the gpiolib struct gpio_desc.

    Add a global tree of lines containing supplemental line information
    to make the debounce period available to be reported by the
    GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.

    Signed-off-by: Kent Gibson <warthog618@gmail.com>
    Reviewed-by: Andy Shevchenko <andy@kernel.org>
    Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

But it doesn't explain *why* we did this and I don't remember the
story behind this change.

How bad would it be to go back to storing the debounce setting in the
descriptor?

Bart

Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Wed, Oct 16, 2024 at 12:12:10PM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 16, 2024 at 11:43 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, Oct 16, 2024 at 11:22:07AM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Oct 16, 2024 at 11:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > > >
> > > > >
> > > > > You mean, you get a CHANGED_CONFIG event but the debounce value is not
> > > > > in the associated line info?
> > > > >
> > > >
> > > > Correct.
> > > >
> > >
> > > Ok, let me see.
> > >
> >
> > When setting from userspace the issue is that linereq_set_config() setting the
> > direction will emit, quite possibly before the debounce has been set.  The
> > edge_detector_setup() that does set it can also emit, though only if the
> > hardware supports debounce.  And then there could be a race between the
> > notifier being called and the period being set in the supinfo.
> > (the set will probably win that one)
> >
> > Debounce set from the kernel side is going to be an issue as cdev
> > catches and stores the value from userspace to report in the supinfo - that
> > isn't the case for kernel calls to gpiod_set_config().
> >
> > Seems moving the debounce value out of the desc and into cdev, which seemed a
> > good idea at the time, might come back and bite now if it is no longer
> > restricted to being cdev specific.  Now there is an actual reason to
> > store it in the desc :(.
> >
>
> I'm seeing commit:
>
> commit 9344e34e7992fec95ce6210d95ac01437dd327ab
> Author: Kent Gibson <warthog618@gmail.com>
> Date:   Tue Dec 19 08:41:54 2023 +0800
>
>     gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
>
>     Store the debounce period for a requested line locally, rather than in
>     the debounce_period_us field in the gpiolib struct gpio_desc.
>
>     Add a global tree of lines containing supplemental line information
>     to make the debounce period available to be reported by the
>     GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.
>
>     Signed-off-by: Kent Gibson <warthog618@gmail.com>
>     Reviewed-by: Andy Shevchenko <andy@kernel.org>
>     Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> But it doesn't explain *why* we did this and I don't remember the
> story behind this change.
>
> How bad would it be to go back to storing the debounce setting in the
> descriptor?
>

At the time it was only being used in cdev, and moving it into cdev was
just about not exporting cdev specific stuff to the rest of the kernel.
So it was just tidying up.
But if cdev is now reporting the configuration of the line independent
of whether it was set from userspace or the kernel then it actually
makes more sense for that state to be stored in the desc.

I don't have any objections to that commit being reverted.

Cheers,
Kent.