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

[PATCH v5 0/8] 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 v5:
- only notify user-space about changed config AFTER the edge detector
  was updated (to that end keep the explicit notification and use the
  nonotify variants of direction setters)
- for consistency do the same for v1
- Link to v4: https://lore.kernel.org/r/20241017-gpio-notify-in-kernel-events-v4-0-64bc05f3be0c@linaro.org

Changes in v4:
- don't emit additional events when emulating open-source or open-drain
  from gpiod_direction_output(), to that end use the nonotify variants
  of gpiod_direction_input()
- store current debounce period in the GPIO line descriptor and remove
  the entire supinfo infrastructure from gpiolib-cdev.c
- fix all (hopefully) corner-cases where two reconfigure or
  requested/reconfigured events could be emitted for a single logical
  event
- emit the reconfigure event from gpio_set_debounce_timeout()
- add a patch refactoring gpio_do_set_config()
- Link to v3: https://lore.kernel.org/r/20241015-gpio-notify-in-kernel-events-v3-0-9edf05802271@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 (8):
      gpiolib: notify user-space when a driver requests its own desc
      gpiolib: unduplicate chip guard in set_config path
      gpio: cdev: go back to storing debounce period in the GPIO descriptor
      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 | 301 +++++++++++++++++++-------------------------
 drivers/gpio/gpiolib.c      | 130 ++++++++++++++-----
 drivers/gpio/gpiolib.h      |  14 ++-
 3 files changed, 243 insertions(+), 202 deletions(-)
---
base-commit: f2493655d2d3d5c6958ed996b043c821c23ae8d3
change-id: 20240924-gpio-notify-in-kernel-events-07cd912153e8

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

[PATCH v5 1/8] 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 v5 2/8] gpiolib: unduplicate chip guard in set_config path

[Bartosz Golaszewski]

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

We don't need to guard the GPIO chip until its first dereference in
gpio_do_set_config().

First: change the prototype of gpio_do_set_config() to take the GPIO
line descriptor as argument, then move the gpio_chip protection into it
and drop it in two places where it's done too early.

This has the added benefit of making gpio_go_set_config() safe to use
from outside of this compilation unit without taking the gdev SRCU read
lock and will come in handy when we'll want to make it available to the
character device code.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c09464f70f73..b1ce213d3a23 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2562,13 +2562,16 @@ 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,
-			      unsigned long config)
+static int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
 {
-	if (!gc->set_config)
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
+	if (!guard.gc->set_config)
 		return -ENOTSUPP;
 
-	return gc->set_config(gc, offset, config);
+	return guard.gc->set_config(guard.gc, gpio_chip_hwgpio(desc), config);
 }
 
 static int gpio_set_config_with_argument(struct gpio_desc *desc,
@@ -2577,12 +2580,8 @@ static int gpio_set_config_with_argument(struct gpio_desc *desc,
 {
 	unsigned long config;
 
-	CLASS(gpio_chip_guard, guard)(desc);
-	if (!guard.gc)
-		return -ENODEV;
-
 	config = pinconf_to_config_packed(mode, argument);
-	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+	return gpio_do_set_config(desc, config);
 }
 
 static int gpio_set_config_with_argument_optional(struct gpio_desc *desc,
@@ -2944,11 +2943,7 @@ int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 {
 	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);
+	return gpio_do_set_config(desc, config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_config);
 

-- 
2.43.0

[PATCH v5 3/8] gpio: cdev: go back to storing debounce period in the GPIO descriptor

[Bartosz Golaszewski]

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

This effectively reverts commits 9344e34e7992 ("gpiolib: cdev: relocate
debounce_period_us from struct gpio_desc") and d8543cbaf979 ("gpiolib:
remove debounce_period_us from struct gpio_desc") and goes back to
storing the debounce period in microseconds in the GPIO descriptor

We're doing it in preparation for notifying the user-space about
in-kernel line config changes.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 163 ++++++--------------------------------------
 drivers/gpio/gpiolib.c      |  18 ++++-
 drivers/gpio/gpiolib.h      |   5 ++
 3 files changed, 43 insertions(+), 143 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b0050250ac3a..d55d2a246d41 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -23,7 +23,6 @@
 #include <linux/overflow.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/poll.h>
-#include <linux/rbtree.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/timekeeping.h>
@@ -421,7 +420,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 
 /**
  * struct line - contains the state of a requested line
- * @node: to store the object in supinfo_tree if supplemental
  * @desc: the GPIO descriptor for this line.
  * @req: the corresponding line request
  * @irq: the interrupt triggered in response to events on this GPIO
@@ -434,7 +432,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @line_seqno: the seqno for the current edge event in the sequence of
  * events for this line.
  * @work: the worker that implements software debouncing
- * @debounce_period_us: the debounce period in microseconds
  * @sw_debounced: flag indicating if the software debouncer is active
  * @level: the current debounced physical level of the line
  * @hdesc: the Hardware Timestamp Engine (HTE) descriptor
@@ -443,7 +440,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @last_seqno: the last sequence number before debounce period expires
  */
 struct line {
-	struct rb_node node;
 	struct gpio_desc *desc;
 	/*
 	 * -- edge detector specific fields --
@@ -477,15 +473,6 @@ struct line {
 	 * -- debouncer specific fields --
 	 */
 	struct delayed_work work;
-	/*
-	 * debounce_period_us is accessed by debounce_irq_handler() and
-	 * process_hw_ts() which are disabled when modified by
-	 * debounce_setup(), edge_detector_setup() or edge_detector_stop()
-	 * or can live with a stale version when updated by
-	 * edge_detector_update().
-	 * The modifying functions are themselves mutually exclusive.
-	 */
-	unsigned int debounce_period_us;
 	/*
 	 * sw_debounce is accessed by linereq_set_config(), which is the
 	 * only setter, and linereq_get_values(), which can live with a
@@ -518,17 +505,6 @@ struct line {
 #endif /* CONFIG_HTE */
 };
 
-/*
- * a rbtree of the struct lines containing supplemental info.
- * Used to populate gpio_v2_line_info with cdev specific fields not contained
- * in the struct gpio_desc.
- * A line is determined to contain supplemental information by
- * line_has_supinfo().
- */
-static struct rb_root supinfo_tree = RB_ROOT;
-/* covers supinfo_tree */
-static DEFINE_SPINLOCK(supinfo_lock);
-
 /**
  * struct linereq - contains the state of a userspace line request
  * @gdev: the GPIO device the line request pertains to
@@ -542,8 +518,7 @@ static DEFINE_SPINLOCK(supinfo_lock);
  * this line request.  Note that this is not used when @num_lines is 1, as
  * the line_seqno is then the same and is cheaper to calculate.
  * @config_mutex: mutex for serializing ioctl() calls to ensure consistency
- * of configuration, particularly multi-step accesses to desc flags and
- * changes to supinfo status.
+ * of configuration, particularly multi-step accesses to desc flags.
  * @lines: the lines held by this line request, with @num_lines elements.
  */
 struct linereq {
@@ -559,103 +534,6 @@ struct linereq {
 	struct line lines[] __counted_by(num_lines);
 };
 
-static void supinfo_insert(struct line *line)
-{
-	struct rb_node **new = &(supinfo_tree.rb_node), *parent = NULL;
-	struct line *entry;
-
-	guard(spinlock)(&supinfo_lock);
-
-	while (*new) {
-		entry = container_of(*new, struct line, node);
-
-		parent = *new;
-		if (line->desc < entry->desc) {
-			new = &((*new)->rb_left);
-		} else if (line->desc > entry->desc) {
-			new = &((*new)->rb_right);
-		} else {
-			/* this should never happen */
-			WARN(1, "duplicate line inserted");
-			return;
-		}
-	}
-
-	rb_link_node(&line->node, parent, new);
-	rb_insert_color(&line->node, &supinfo_tree);
-}
-
-static void supinfo_erase(struct line *line)
-{
-	guard(spinlock)(&supinfo_lock);
-
-	rb_erase(&line->node, &supinfo_tree);
-}
-
-static struct line *supinfo_find(struct gpio_desc *desc)
-{
-	struct rb_node *node = supinfo_tree.rb_node;
-	struct line *line;
-
-	while (node) {
-		line = container_of(node, struct line, node);
-		if (desc < line->desc)
-			node = node->rb_left;
-		else if (desc > line->desc)
-			node = node->rb_right;
-		else
-			return line;
-	}
-	return NULL;
-}
-
-static void supinfo_to_lineinfo(struct gpio_desc *desc,
-				struct gpio_v2_line_info *info)
-{
-	struct gpio_v2_line_attribute *attr;
-	struct line *line;
-
-	guard(spinlock)(&supinfo_lock);
-
-	line = supinfo_find(desc);
-	if (!line)
-		return;
-
-	attr = &info->attrs[info->num_attrs];
-	attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
-	attr->debounce_period_us = READ_ONCE(line->debounce_period_us);
-	info->num_attrs++;
-}
-
-static inline bool line_has_supinfo(struct line *line)
-{
-	return READ_ONCE(line->debounce_period_us);
-}
-
-/*
- * Checks line_has_supinfo() before and after the change to avoid unnecessary
- * supinfo_tree access.
- * Called indirectly by linereq_create() or linereq_set_config() so line
- * is already protected from concurrent changes.
- */
-static void line_set_debounce_period(struct line *line,
-				     unsigned int debounce_period_us)
-{
-	bool was_suppl = line_has_supinfo(line);
-
-	WRITE_ONCE(line->debounce_period_us, debounce_period_us);
-
-	/* if supinfo status is unchanged then we're done */
-	if (line_has_supinfo(line) == was_suppl)
-		return;
-
-	/* supinfo status has changed, so update the tree */
-	if (was_suppl)
-		supinfo_erase(line);
-	else
-		supinfo_insert(line);
-}
-
 #define GPIO_V2_LINE_BIAS_FLAGS \
 	(GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
 	 GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
@@ -823,7 +701,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
 		line->total_discard_seq++;
 		line->last_seqno = ts->seq;
 		mod_delayed_work(system_wq, &line->work,
-		  usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
+		  usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
 	} else {
 		if (unlikely(ts->seq < line->line_seqno))
 			return HTE_CB_HANDLED;
@@ -964,7 +842,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p)
 	struct line *line = p;
 
 	mod_delayed_work(system_wq, &line->work,
-		usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
+		usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
 
 	return IRQ_HANDLED;
 }
@@ -1047,7 +925,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 	/* try hardware */
 	ret = gpiod_set_debounce(line->desc, debounce_period_us);
 	if (!ret) {
-		line_set_debounce_period(line, debounce_period_us);
+		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 		return ret;
 	}
 	if (ret != -ENOTSUPP)
@@ -1132,7 +1010,8 @@ static void edge_detector_stop(struct line *line)
 	cancel_delayed_work_sync(&line->work);
 	WRITE_ONCE(line->sw_debounced, 0);
 	WRITE_ONCE(line->edflags, 0);
-	line_set_debounce_period(line, 0);
+	if (line->desc)
+		WRITE_ONCE(line->desc->debounce_period_us, 0);
 	/* do not change line->level - see comment in debounced_value() */
 }
 
@@ -1165,7 +1044,7 @@ static int edge_detector_setup(struct line *line,
 		ret = debounce_setup(line, debounce_period_us);
 		if (ret)
 			return ret;
-		line_set_debounce_period(line, debounce_period_us);
+		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 	}
 
 	/* detection disabled or sw debouncer will provide edge detection */
@@ -1213,12 +1092,12 @@ static int edge_detector_update(struct line *line,
 			gpio_v2_line_config_debounce_period(lc, line_idx);
 
 	if ((active_edflags == edflags) &&
-	    (READ_ONCE(line->debounce_period_us) == debounce_period_us))
+	    (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
 		return 0;
 
 	/* sw debounced and still will be...*/
 	if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
-		line_set_debounce_period(line, debounce_period_us);
+		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 		/*
 		 * ensure event fifo is initialised if edge detection
 		 * is now enabled.
@@ -1677,7 +1556,6 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 
 static void linereq_free(struct linereq *lr)
 {
-	struct line *line;
 	unsigned int i;
 
 	if (lr->device_unregistered_nb.notifier_call)
@@ -1685,14 +1563,10 @@ static void linereq_free(struct linereq *lr)
 						   &lr->device_unregistered_nb);
 
 	for (i = 0; i < lr->num_lines; i++) {
-		line = &lr->lines[i];
-		if (!line->desc)
-			continue;
-
-		edge_detector_stop(line);
-		if (line_has_supinfo(line))
-			supinfo_erase(line);
-		gpiod_free(line->desc);
+		if (lr->lines[i].desc) {
+			edge_detector_stop(&lr->lines[i]);
+			gpiod_free(lr->lines[i].desc);
+		}
 	}
 	kfifo_free(&lr->events);
 	kfree(lr->label);
@@ -2363,6 +2237,7 @@ static void gpio_v2_line_info_changed_to_v1(
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 				  struct gpio_v2_line_info *info)
 {
+	u32 debounce_period_us;
 	unsigned long dflags;
 	const char *label;
 
@@ -2435,6 +2310,14 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 		info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME;
 	else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags))
 		info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
+
+	debounce_period_us = READ_ONCE(desc->debounce_period_us);
+	if (debounce_period_us) {
+		info->attrs[info->num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
+		info->attrs[info->num_attrs].debounce_period_us =
+							debounce_period_us;
+		info->num_attrs++;
+	}
 }
 
 struct gpio_chardev_data {
@@ -2540,7 +2423,6 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
 			return -EBUSY;
 	}
 	gpio_desc_to_lineinfo(desc, &lineinfo);
-	supinfo_to_lineinfo(desc, &lineinfo);
 
 	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
 		if (watch)
@@ -2633,7 +2515,6 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	chg.event_type = action;
 	chg.timestamp_ns = ktime_get_ns();
 	gpio_desc_to_lineinfo(desc, &chg.info);
-	supinfo_to_lineinfo(desc, &chg.info);
 
 	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
 	if (ret)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b1ce213d3a23..807bee86afd5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2432,7 +2432,9 @@ static void gpiod_free_commit(struct gpio_desc *desc)
 #endif
 		desc_set_label(desc, NULL);
 		WRITE_ONCE(desc->flags, flags);
-
+#ifdef CONFIG_GPIO_CDEV
+		WRITE_ONCE(desc->debounce_period_us, 0);
+#endif
 		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_RELEASED);
 	}
 }
@@ -2564,6 +2566,8 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
 
 static int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
 {
+	int ret;
+
 	CLASS(gpio_chip_guard, guard)(desc);
 	if (!guard.gc)
 		return -ENODEV;
@@ -2571,7 +2575,17 @@ static int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
 	if (!guard.gc->set_config)
 		return -ENOTSUPP;
 
-	return guard.gc->set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+	ret = guard.gc->set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+#ifdef CONFIG_GPIO_CDEV
+	/*
+	 * Special case - if we're setting debounce period, we need to store
+	 * it in the descriptor in case user-space wants to know it.
+	 */
+	if (!ret && pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE)
+		WRITE_ONCE(desc->debounce_period_us,
+			   pinconf_to_config_argument(config));
+#endif
+	return ret;
 }
 
 static int gpio_set_config_with_argument(struct gpio_desc *desc,
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 067197d61d57..8daba06eb472 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -165,6 +165,7 @@ struct gpio_desc_label {
  * @label:		Name of the consumer
  * @name:		Line name
  * @hog:		Pointer to the device node that hogs this line (if any)
+ * @debounce_period_us:	Debounce period in microseconds
  *
  * These are obtained using gpiod_get() and are preferable to the old
  * integer-based handles.
@@ -202,6 +203,10 @@ struct gpio_desc {
 #ifdef CONFIG_OF_DYNAMIC
 	struct device_node	*hog;
 #endif
+#ifdef CONFIG_GPIO_CDEV
+	/* debounce period in microseconds */
+	unsigned int		debounce_period_us;
+#endif
 };
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)

-- 
2.43.0

[PATCH v5 4/8] 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 d55d2a246d41..0cba74381687 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2235,7 +2235,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)
 {
 	u32 debounce_period_us;
 	unsigned long dflags;
@@ -2277,9 +2277,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;
@@ -2385,7 +2388,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))) {
@@ -2422,7 +2425,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);
 
 	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
 		if (watch)
@@ -2514,7 +2517,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);
 
 	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
 	if (ret)

-- 
2.43.0

[PATCH v5 5/8] 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 0cba74381687..b242fdb1ad28 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2749,6 +2749,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;
@@ -2765,6 +2770,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 8daba06eb472..8737e1641278 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 v5 6/8] 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 | 82 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b242fdb1ad28..7759dca92f8b 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2333,6 +2333,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)
@@ -2502,28 +2503,86 @@ 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);
+	/*
+	 * 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);
+	/* 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;
 }
@@ -2683,6 +2742,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 v5 7/8] 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 7759dca92f8b..cb4fb55e2696 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2729,8 +2729,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;
 
@@ -2754,8 +2754,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);
@@ -2779,8 +2779,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 807bee86afd5..83e85dbfdeed 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);
@@ -4098,8 +4098,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 8737e1641278..a54be597d21a 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 v5 8/8] 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.

Use gpio_do_set_config() instead of gpiod_set_debounce() for configuring
debouncing via hardware from the character device code to avoid multiple
reconfigure events.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index cb4fb55e2696..13d83675bf4f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -187,11 +187,11 @@ static long linehandle_set_config(struct linehandle_state *lh,
 		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
 			int val = !!gcnf.default_values[i];
 
-			ret = gpiod_direction_output(desc, val);
+			ret = gpiod_direction_output_nonotify(desc, val);
 			if (ret)
 				return ret;
 		} else {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			if (ret)
 				return ret;
 		}
@@ -362,11 +362,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;
 		}
@@ -922,8 +922,13 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 	int ret, level, irq;
 	char *label;
 
-	/* try hardware */
-	ret = gpiod_set_debounce(line->desc, debounce_period_us);
+	/*
+	 * Try hardware. Skip gpiod_set_config() to avoid emitting two
+	 * CHANGED_CONFIG line state events.
+	 */
+	ret = gpio_do_set_config(line->desc,
+			pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE,
+						 debounce_period_us));
 	if (!ret) {
 		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 		return ret;
@@ -1433,11 +1438,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 			int val = gpio_v2_line_config_output_value(&lc, i);
 
 			edge_detector_stop(line);
-			ret = gpiod_direction_output(desc, val);
+			ret = gpiod_direction_output_nonotify(desc, val);
 			if (ret)
 				return ret;
 		} else {
-			ret = gpiod_direction_input(desc);
+			ret = gpiod_direction_input_nonotify(desc);
 			if (ret)
 				return ret;
 
@@ -1700,11 +1705,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 83e85dbfdeed..ae758ba6dc3d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2564,7 +2564,7 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
-static int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
+int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
 {
 	int ret;
 
@@ -2670,9 +2670,15 @@ static int gpio_set_bias(struct gpio_desc *desc)
  */
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 {
-	return gpio_set_config_with_argument_optional(desc,
-						      PIN_CONFIG_INPUT_DEBOUNCE,
-						      debounce);
+	int ret;
+
+	ret = gpio_set_config_with_argument_optional(desc,
+						     PIN_CONFIG_INPUT_DEBOUNCE,
+						     debounce);
+	if (!ret)
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+	return ret;
 }
 
 /**
@@ -2686,6 +2692,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;
 
@@ -2733,7 +2751,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)
 {
@@ -2795,8 +2812,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);
 
@@ -2814,6 +2838,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;
@@ -2843,7 +2879,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)) {
@@ -2852,7 +2888,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 {
@@ -2876,7 +2912,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.
@@ -2955,9 +2990,30 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
  */
 int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 {
+	int ret;
+
 	VALIDATE_DESC(desc);
 
-	return gpio_do_set_config(desc, config);
+	ret = gpio_do_set_config(desc, config);
+	if (!ret) {
+		/* 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);
 
@@ -3024,6 +3080,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);
 
@@ -3668,9 +3725,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);
 
@@ -4548,10 +4611,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 a54be597d21a..83690f72f7e5 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;
@@ -258,6 +260,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 					 const char *label,
 					 bool platform_lookup_allowed);
 
+int gpio_do_set_config(struct gpio_desc *desc, unsigned long config);
 int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);

-- 
2.43.0

Re: [PATCH v5 0/8] gpio: notify user-space about config changes in the kernel

[Kent Gibson]

On Fri, Oct 18, 2024 at 11:10:08AM +0200, Bartosz Golaszewski wrote:
> 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>

Reviewed-by: Kent Gibson <warthog618@gmail.com>

Cheers,
Kent.

Re: [PATCH v5 0/8] gpio: notify user-space about config changes in the kernel

[Bartosz Golaszewski]

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


On Fri, 18 Oct 2024 11:10:08 +0200, Bartosz Golaszewski wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/8] gpiolib: notify user-space when a driver requests its own desc
      commit: 49182c87af377ec4ae0a5e116a9059e03b083574
[2/8] gpiolib: unduplicate chip guard in set_config path
      commit: dd26ffaa4d278d6e538c4ea68fb508a69f567827
[3/8] gpio: cdev: go back to storing debounce period in the GPIO descriptor
      commit: 81625f362497509526a2f9ac53843ae30b4709cc
[4/8] gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic
      commit: 8c44447bd76109e33a69fccda89c84715fbd5658
[5/8] gpiolib: add a per-gpio_device line state notification workqueue
      commit: 7b9b77a8bba9c1fd7bad3310dbf2cf382e1f8c25
[6/8] gpio: cdev: put emitting the line state events on a workqueue
      commit: 40b7c49950bd56c984b1f6722f865b922879260e
[7/8] gpiolib: switch the line state notifier to atomic
      commit: fcc8b637c542d1a0c19e5797ad72f9258e10464c
[8/8] gpiolib: notify user-space about in-kernel line state changes
      commit: 07c61d4da43fa3b34c152b28010d20be115a96db

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 14354 bytes --]

On Fri, Oct 18, 2024 at 11:10:16AM +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.

Today's -next is not booting on several of my platforms, including
beaglebone-black, i.MX8MP-EVK and pine64plus.  Bisects are pointing at
this commit, and i.MX8MP-EVK is actually giving some console output:

[    2.502208] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[    2.511036] Mem abort info:

...

[    2.679934] Call trace:
[    2.682379]  gpiod_direction_output+0x34/0x5c
[    2.686745]  i2c_register_adapter+0x59c/0x670
[    2.691111]  __i2c_add_numbered_adapter+0x58/0xa8
[    2.695822]  i2c_add_adapter+0xa0/0xd0
[    2.699578]  i2c_add_numbered_adapter+0x2c/0x38
[    2.704117]  i2c_imx_probe+0x2d0/0x640

which looks plausible given the change.

Full boot log for i.MX8MP-EVK:

   https://lava.sirena.org.uk/scheduler/job/887083

Bisect log for that, the others look similar (the long run of good/bad
tags at the start for random commits is my automation pulling test
results it already knows about in the affected range to try to speed up
the bisect):

# bad: [ceab669fdf7b7510b4e4997b33d6f66e433a96db] Add linux-next specific files for 20241023
# good: [ad023864550daf9a5062e68f7925320146404919] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
# good: [db7e59e6a39a4d3d54ca8197c796557e6d480b0d] ASoC: qcom: sc7280: Fix missing Soundwire runtime stream alloc
# good: [d0ccf760a405d243a49485be0a43bd5b66ed17e2] spi: geni-qcom: Fix boot warning related to pm_runtime and devres
# good: [1e5d0f106164d2089826c16bb521891d1d06d3ad] ASoC: fsl_xcvr: reset RX dpath after wrong preamble
# good: [602ff58ae4fe4289b0ca71cba9fb82f7de92cd64] regulator: core: remove machine init callback from config
# good: [e6d20a9b0f376fda3e3c3709a59cefa6c0021784] ASoC: dt-bindings: everest,es8328: Document audio graph port
# good: [6a646e6de58f4aedf5f6c7a4605a0393c4490ef1] ASoC: dt-bindings: qcom: Add SM8750 LPASS macro codecs
# good: [5337ff41d37d4171868bb7b34dade68e269743f0] ASoC: soc-utils: Remove PAGE_SIZE compile-time constant assumption
# good: [f45a4399c1b582c6ddc179cc940aed73907b9453] spi: dt-bindings: samsung: Add a compatible for samsung,exynos8895-spi
# good: [42d20a6a61b8fccbb57d80df1ccde7dd82d5bbd6] spi: spi-mem: Add Realtek SPI-NAND controller
# good: [36dbe4521a381fd4d2561a90200ae4a2a3efb222] spi: make class structs const
# good: [1b9971a4e01b80afbf061ad7cdf84ac6fbbbde8d] ASoC: nau8821: check regmap_raw_read/regmap_raw_write for failure
# good: [e92edcf8023d425c7abcf1d7abb5dcac53d106f5] ASoC: SOF: Intel: hda: use machine_check() for SoundWire
# good: [4de1cdb3c299bb98d70198c1fa20c71f0893835c] spi: dt-bindings: brcm,bcm2835-aux-spi: Convert to dtschema
# good: [83c062ae81e89f73e3ab85953111a8b3daaaf98e] ASoC: Intel: sof_sdw: Add quirks for some new Lenovo laptops
# good: [9cb86a9cf12504c8dd60b40a6a200856852c1813] ASoC: SOF: sof-of-dev: add parameter to override tplg/fw_filename
# good: [c6631ceea573ae364e4fe913045f2aad10a10784] ASoC: rt-sdw-common: Enhance switch case to prevent uninitialized variable
# good: [45b3605089b41b81ba36b231fbb97e3037a51beb] ASoC: loongson: Fix build warning when !CONFIG_PCI
# good: [f5a0ea8936a640d8229d5219515141fc496ec5d8] ASoC: mediatek: mt8188: Remove unnecessary variable assignments
# good: [e0941775e6bdcf45e6e20b7ff3bb87dbb7d92fbb] ASoC/SoundWire: Intel: lnl: enable interrupts after first power-up/before last power-down
# good: [c1789209701143b50cba3783fa800a23df30a088] ASoC: codecs: Fix error check in es8323_i2c_probe
# good: [a0aae96be5ffc5b456ca07bfe1385b721c20e184] ASoC: Intel: avs: Fix return status of avs_pcm_hw_constraints_init()
# good: [941584e2f3ddde26e4d71941ebc0836ece181594] spi: stm32: fix missing device mode capability in stm32mp25
# good: [b39eec95b84d5dc326c3d7c89e4e08b898dbc73c] ASoC: imx-card: Add CS42888 support
# good: [8658c4eb9d6b76311322c1b74b3d4e0dec3599d8] ASoC: rt721-sdca: Clean logically deadcode in rt721-sdca.c
# good: [fceffbfe57af7d9941d08e1a995cccf558d08451] regulator: max5970: Drop unused structs
# good: [b1258105f9ce5203f48a47fd2f2cec8c38c41841] spi: intel: Add protected and locked attributes
# good: [ba4c5fad598c07492844e514add3ccda467063b2] ASoC: loongson: Add I2S controller driver as platform device
# good: [4e9a2c91bff44336157eefd8d80b8ceb27918737] regulator: dt-bindings: vctrl-regulator: convert to YAML
# good: [47701a85af0c0d655e06dd23f6b8761848147450] ASoC: SOF: ipc4-topology: Add helper function to print the module's in/out audio format
# good: [f3a59ab98cfc18c7b2fb1d8164bedbb1569a7e76] spi: spi-imx: Fix casting warnings
# good: [e2fc05873905f2ee96b38a116ae86f45fe7d8e49] spi: rockchip: Use dev_{err,warn}_probe() in the probe path
# good: [5cd575a87f141e438b3e062533bf0c6cc9eba99a] ASoC: dt-bindings: rockchip,rk3036-codec: convert to yaml
# good: [e5553cb6612989d18229c2b03948d6b4ba5d45f2] ASoC: rt721-sdca: Fix issue of warning message
# good: [86ce355c1f9ab943bbe099ea7d0b8a3af2247f65] ASoC: rt721-sdca: Add RT721 SDCA driver
# good: [846a8d3cf3bace9f235c38caf1d8d853c323dbd4] ASoC: Intel: soc-acpi-intel-ptl-match: Add rt721 support
# good: [0372abfcd81a4db94070d235e1ae3ff928efcab9] ASoC: amd: acp: refactor sof_card_dai_links_create() function
# good: [c6e86e19e778553dbedab617aafb25b6bbaf4cd9] ASoC: fsl: fsl_qmc_audio: Remove the logging when parsing channels
# good: [eb6c65049a274c37f9b6fdf632843b609a0b8fa8] spi: Provide defer reason if getting irq during probe fails
# good: [56d3705e4b36bf454965e66d8264356a23135aa7] ASoC: Intel: sof_rt5682: Add support for ptl_max98360a_rt5682
# good: [e58b3914ab8303a2783ec1873c17b7a83dd515f7] ASoC: dt-bindings: Deprecate {hp,mic}-det-gpio
# good: [64207f8024899938f8e13c4649a060a19f18bff3] ASoC: sh: rz-ssi: Use SSIFCR_FIFO_RST macro
# good: [46854574fd76c711c890423f8ac60df4fb726559] spi: spi-ti-qspi: remove redundant assignment to variable ret
# good: [6061483d7141db3a805f8660eae23805af02d544] ASoC: codecs: wcd9335: remove unnecessary MODULE_ALIAS()
# good: [8cd4e1f087b6906bacbbf8b637cac4e479a9cb34] ASoC: amd: acp: drop bogus NULL check from i2s_irq_handler
# good: [667b5e803a94f1ce48ac85b3fef94891a8d40ccf] spi: spi-fsl-lpspi: support effective_speed_hz
# good: [a34b9d812d7ec95789b15ce84de5f03c6dd1137b] ASoC: rt1320: fix the range of patch code address
# good: [4649cbd97fdae5069e9a71cd7669b62b90e03669] ASoC: dt-bindings: mt6359: Update generic node name and dmic-mode
# good: [ac8775d7de5a8ccac225a398cbce9fb9fffdbb9f] ASoC: atmel: atmel_ssc_dai: Drop S24_LE support due to single channel limitation
# good: [9864c8af89eb14a2e5334f8e24bb82086182e894] ASoC: amd: acp: remove unused variable from acp platform driver
# good: [625de1881b5aee6a42a3130004e47dbd632429f8] spi: atmel-quadspi: Add cs_hold and cs_inactive setting support
# good: [3c44a715e389929b8243d6a0545992d78cff6cba] ASoC: atmel: mchp-spdifrx: Remove interface name from stream_name
# good: [8adff2ff73d8271c993549b106b26f301fa003cf] ASoC: constify snd_soc_component_driver struct
# good: [dc9004ea273a9141c16b90a687da70b77f5a640a] ASoC: codecs: Add NeoFidelity NTP8835 codec
# good: [1482c40b440fa58f956bc3e1ef3426e0cdbc09e0] spi: rockchip-sfc: Use dev_err_probe() in the probe path
# good: [cc3ae21f360bfa375fc3539e24e7adb0e643a9d4] ASoC: fsl_micfil: Enable micfil error interrupt
# good: [49a85851b14cf6630013d1b9bae2ac2345c9430b] regcache: Store values more directly in maple trees
# good: [36ec3f437227470568e5f460997f367f5446a34d] regulator: Add devres version of of_regulator_get_optional()
# good: [18be43aca2c0ec475037923a8086d0a29fcc9d16] regulator: qcom-smd: make smd_vreg_rpm static
# good: [04e800fc328e6eba9f4ec3df375f2b500802653a] ASoC: codecs: aw88399: Fix spelling mistake "unsupport" -> "unsupported"
# good: [6c30eee359127c31cd8c6b586c8c3ced9f50f74b] spi: spi_amd: Add HIDDMA basic read support
# good: [0809a9ccac4a2ffdfd1561bb551aec6099775545] spi: remove {devm_}spi_alloc_master/slave()
# good: [7a01e17e42fe944982acde1dd40bdea177372173] ASoC: stm: fix macro definition on STM_SAI_HAS_EXT_SYNC
git bisect start 'ceab669fdf7b7510b4e4997b33d6f66e433a96db' 'ad023864550daf9a5062e68f7925320146404919' 'db7e59e6a39a4d3d54ca8197c796557e6d480b0d' 'd0ccf760a405d243a49485be0a43bd5b66ed17e2' '1e5d0f106164d2089826c16bb521891d1d06d3ad' '602ff58ae4fe4289b0ca71cba9fb82f7de92cd64' 'e6d20a9b0f376fda3e3c3709a59cefa6c0021784' '6a646e6de58f4aedf5f6c7a4605a0393c4490ef1' '5337ff41d37d4171868bb7b34dade68e269743f0' 'f45a4399c1b582c6ddc179cc940aed73907b9453' '42d20a6a61b8fccbb57d80df1ccde7dd82d5bbd6' '36dbe4521a381fd4d2561a90200ae4a2a3efb222' '1b9971a4e01b80afbf061ad7cdf84ac6fbbbde8d' 'e92edcf8023d425c7abcf1d7abb5dcac53d106f5' '4de1cdb3c299bb98d70198c1fa20c71f0893835c' '83c062ae81e89f73e3ab85953111a8b3daaaf98e' '9cb86a9cf12504c8dd60b40a6a200856852c1813' 'c6631ceea573ae364e4fe913045f2aad10a10784' '45b3605089b41b81ba36b231fbb97e3037a51beb' 'f5a0ea8936a640d8229d5219515141fc496ec5d8' 'e0941775e6bdcf45e6e20b7ff3bb87dbb7d92fbb' 'c1789209701143b50cba3783fa800a23df30a088' 'a0aae96be5ffc5b456ca07bfe1385b721c20e184' '941584e2f3ddde26e4d71941ebc0836ece181594' 'b39eec95b84d5dc326c3d7c89e4e08b898dbc73c' '8658c4eb9d6b76311322c1b74b3d4e0dec3599d8' 'fceffbfe57af7d9941d08e1a995cccf558d08451' 'b1258105f9ce5203f48a47fd2f2cec8c38c41841' 'ba4c5fad598c07492844e514add3ccda467063b2' '4e9a2c91bff44336157eefd8d80b8ceb27918737' '47701a85af0c0d655e06dd23f6b8761848147450' 'f3a59ab98cfc18c7b2fb1d8164bedbb1569a7e76' 'e2fc05873905f2ee96b38a116ae86f45fe7d8e49' '5cd575a87f141e438b3e062533bf0c6cc9eba99a' 'e5553cb6612989d18229c2b03948d6b4ba5d45f2' '86ce355c1f9ab943bbe099ea7d0b8a3af2247f65' '846a8d3cf3bace9f235c38caf1d8d853c323dbd4' '0372abfcd81a4db94070d235e1ae3ff928efcab9' 'c6e86e19e778553dbedab617aafb25b6bbaf4cd9' 'eb6c65049a274c37f9b6fdf632843b609a0b8fa8' '56d3705e4b36bf454965e66d8264356a23135aa7' 'e58b3914ab8303a2783ec1873c17b7a83dd515f7' '64207f8024899938f8e13c4649a060a19f18bff3' '46854574fd76c711c890423f8ac60df4fb726559' '6061483d7141db3a805f8660eae23805af02d544' '8cd4e1f087b6906bacbbf8b637cac4e479a9cb34' '667b5e803a94f1ce48ac85b3fef94891a8d40ccf' 'a34b9d812d7ec95789b15ce84de5f03c6dd1137b' '4649cbd97fdae5069e9a71cd7669b62b90e03669' 'ac8775d7de5a8ccac225a398cbce9fb9fffdbb9f' '9864c8af89eb14a2e5334f8e24bb82086182e894' '625de1881b5aee6a42a3130004e47dbd632429f8' '3c44a715e389929b8243d6a0545992d78cff6cba' '8adff2ff73d8271c993549b106b26f301fa003cf' 'dc9004ea273a9141c16b90a687da70b77f5a640a' '1482c40b440fa58f956bc3e1ef3426e0cdbc09e0' 'cc3ae21f360bfa375fc3539e24e7adb0e643a9d4' '49a85851b14cf6630013d1b9bae2ac2345c9430b' '36ec3f437227470568e5f460997f367f5446a34d' '18be43aca2c0ec475037923a8086d0a29fcc9d16' '04e800fc328e6eba9f4ec3df375f2b500802653a' '6c30eee359127c31cd8c6b586c8c3ced9f50f74b' '0809a9ccac4a2ffdfd1561bb551aec6099775545' '7a01e17e42fe944982acde1dd40bdea177372173'
# bad: [ceab669fdf7b7510b4e4997b33d6f66e433a96db] Add linux-next specific files for 20241023
git bisect bad ceab669fdf7b7510b4e4997b33d6f66e433a96db
# good: [397886451c608b881fc990abac627839b5010e4c] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good 397886451c608b881fc990abac627839b5010e4c
# good: [9bbf38bc6804c62ec9b29548b739f0e5dbd11d6b] Merge branch 'for-mfd-next' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
git bisect good 9bbf38bc6804c62ec9b29548b739f0e5dbd11d6b
# good: [1b1952166f2b15f8d4665beeb8cc5443cda6f17d] Merge branch 'char-misc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git bisect good 1b1952166f2b15f8d4665beeb8cc5443cda6f17d
# good: [5a5a05d1e3cc8cb7127b5acac0fe647f4524567b] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
git bisect good 5a5a05d1e3cc8cb7127b5acac0fe647f4524567b
# bad: [5ee7b36c01a29f242bd4c29dc95406caa53d0f1a] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching
git bisect bad 5ee7b36c01a29f242bd4c29dc95406caa53d0f1a
# bad: [b0ec3aaca3890d7e345e3ca4d58fb1b93d56354c] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
git bisect bad b0ec3aaca3890d7e345e3ca4d58fb1b93d56354c
# bad: [8ddbd3e4039f2df83797cfda217f240d4a191bad] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git
git bisect bad 8ddbd3e4039f2df83797cfda217f240d4a191bad
# good: [2707a028c9b9c54a6dff22c9dcfebf3083ea095e] gpio: mpc8xxx: use a helper variable to store the address of pdev->dev
git bisect good 2707a028c9b9c54a6dff22c9dcfebf3083ea095e
# good: [8c44447bd76109e33a69fccda89c84715fbd5658] gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic
git bisect good 8c44447bd76109e33a69fccda89c84715fbd5658
# bad: [3aba8402910bfab998d5cf6c84744de5db466936] gpio: grgpio: remove remove()
git bisect bad 3aba8402910bfab998d5cf6c84744de5db466936
# bad: [07c61d4da43fa3b34c152b28010d20be115a96db] gpiolib: notify user-space about in-kernel line state changes
git bisect bad 07c61d4da43fa3b34c152b28010d20be115a96db
# good: [40b7c49950bd56c984b1f6722f865b922879260e] gpio: cdev: put emitting the line state events on a workqueue
git bisect good 40b7c49950bd56c984b1f6722f865b922879260e
# good: [fcc8b637c542d1a0c19e5797ad72f9258e10464c] gpiolib: switch the line state notifier to atomic
git bisect good fcc8b637c542d1a0c19e5797ad72f9258e10464c
# first bad commit: [07c61d4da43fa3b34c152b28010d20be115a96db] gpiolib: notify user-space about in-kernel line state changes

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Bartosz Golaszewski]

On Wed, Oct 23, 2024 at 11:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 18, 2024 at 11:10:16AM +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.
>
> Today's -next is not booting on several of my platforms, including
> beaglebone-black, i.MX8MP-EVK and pine64plus.  Bisects are pointing at
> this commit, and i.MX8MP-EVK is actually giving some console output:
>
> [    2.502208] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [    2.511036] Mem abort info:
>
> ...
>
> [    2.679934] Call trace:
> [    2.682379]  gpiod_direction_output+0x34/0x5c
> [    2.686745]  i2c_register_adapter+0x59c/0x670
> [    2.691111]  __i2c_add_numbered_adapter+0x58/0xa8
> [    2.695822]  i2c_add_adapter+0xa0/0xd0
> [    2.699578]  i2c_add_numbered_adapter+0x2c/0x38
> [    2.704117]  i2c_imx_probe+0x2d0/0x640
>
> which looks plausible given the change.
>
> Full boot log for i.MX8MP-EVK:
>
>    https://lava.sirena.org.uk/scheduler/job/887083
>
> Bisect log for that, the others look similar (the long run of good/bad
> tags at the start for random commits is my automation pulling test
> results it already knows about in the affected range to try to speed up
> the bisect):
>

Hi Mark!

Any chance you could post the output of

    scripts/faddr2line drivers/gpio/gpiolib.o gpiod_direction_output+0x34/0x5c

for that build?

Bart

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

[Bartosz Golaszewski]

On Thu, 24 Oct 2024 08:49:30 +0200, Bartosz Golaszewski <brgl@bgdev.pl> said:
> On Wed, Oct 23, 2024 at 11:05 PM Mark Brown <broonie@kernel.org> wrote:
>>
>> On Fri, Oct 18, 2024 at 11:10:16AM +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.
>>
>> Today's -next is not booting on several of my platforms, including
>> beaglebone-black, i.MX8MP-EVK and pine64plus.  Bisects are pointing at
>> this commit, and i.MX8MP-EVK is actually giving some console output:
>>
>> [    2.502208] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> [    2.511036] Mem abort info:
>>
>> ...
>>
>> [    2.679934] Call trace:
>> [    2.682379]  gpiod_direction_output+0x34/0x5c
>> [    2.686745]  i2c_register_adapter+0x59c/0x670
>> [    2.691111]  __i2c_add_numbered_adapter+0x58/0xa8
>> [    2.695822]  i2c_add_adapter+0xa0/0xd0
>> [    2.699578]  i2c_add_numbered_adapter+0x2c/0x38
>> [    2.704117]  i2c_imx_probe+0x2d0/0x640
>>
>> which looks plausible given the change.
>>
>> Full boot log for i.MX8MP-EVK:
>>
>>    https://lava.sirena.org.uk/scheduler/job/887083
>>
>> Bisect log for that, the others look similar (the long run of good/bad
>> tags at the start for random commits is my automation pulling test
>> results it already knows about in the affected range to try to speed up
>> the bisect):
>>
>
> Hi Mark!
>
> Any chance you could post the output of
>
>     scripts/faddr2line drivers/gpio/gpiolib.o gpiod_direction_output+0x34/0x5c
>
> for that build?
>
> Bart
>

While I can't really reproduce it, I've looked at what could be wrong and
figured that the NULL-pointer in question can possibly be the
line_state_notifier.

I realized that for some historical reasons we add the GPIO device to the
global list before it's fully set up - including initializing the notifier.
In some corner cases (devlinks borked?) this could lead to consumers requesting
GPIOs before their provider is ready.

Mark: could you try the following diff and let me know if it works?

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ae758ba6dc3d..4258acac0162 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -987,45 +987,6 @@ int gpiochip_add_data_with_key(struct gpio_chip
*gc, void *data,

 	gdev->ngpio = gc->ngpio;
 	gdev->can_sleep = gc->can_sleep;
-
-	scoped_guard(mutex, &gpio_devices_lock) {
-		/*
-		 * TODO: this allocates a Linux GPIO number base in the global
-		 * GPIO numberspace for this chip. In the long run we want to
-		 * get *rid* of this numberspace and use only descriptors, but
-		 * it may be a pipe dream. It will not happen before we get rid
-		 * of the sysfs interface anyways.
-		 */
-		base = gc->base;
-		if (base < 0) {
-			base = gpiochip_find_base_unlocked(gc->ngpio);
-			if (base < 0) {
-				ret = base;
-				base = 0;
-				goto err_free_label;
-			}
-
-			/*
-			 * TODO: it should not be necessary to reflect the
-			 * assigned base outside of the GPIO subsystem. Go over
-			 * drivers and see if anyone makes use of this, else
-			 * drop this and assign a poison instead.
-			 */
-			gc->base = base;
-		} else {
-			dev_warn(&gdev->dev,
-				 "Static allocation of GPIO base is deprecated, use dynamic
allocation.\n");
-		}
-
-		gdev->base = base;
-
-		ret = gpiodev_add_to_list_unlocked(gdev);
-		if (ret) {
-			chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
-			goto err_free_label;
-		}
-	}
-
 	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);

@@ -1103,6 +1064,45 @@ int gpiochip_add_data_with_key(struct gpio_chip
*gc, void *data,
 		if (ret)
 			goto err_remove_irqchip;
 	}
+
+	scoped_guard(mutex, &gpio_devices_lock) {
+		/*
+		 * TODO: this allocates a Linux GPIO number base in the global
+		 * GPIO numberspace for this chip. In the long run we want to
+		 * get *rid* of this numberspace and use only descriptors, but
+		 * it may be a pipe dream. It will not happen before we get rid
+		 * of the sysfs interface anyways.
+		 */
+		base = gc->base;
+		if (base < 0) {
+			base = gpiochip_find_base_unlocked(gc->ngpio);
+			if (base < 0) {
+				ret = base;
+				base = 0;
+				goto err_free_label;
+			}
+
+			/*
+			 * TODO: it should not be necessary to reflect the
+			 * assigned base outside of the GPIO subsystem. Go over
+			 * drivers and see if anyone makes use of this, else
+			 * drop this and assign a poison instead.
+			 */
+			gc->base = base;
+		} else {
+			dev_warn(&gdev->dev,
+				 "Static allocation of GPIO base is deprecated, use dynamic
allocation.\n");
+		}
+
+		gdev->base = base;
+
+		ret = gpiodev_add_to_list_unlocked(gdev);
+		if (ret) {
+			chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
+			goto err_free_label;
+		}
+	}
+
 	return 0;

 err_remove_irqchip:

Thanks
Bartosz

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

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 6768 bytes --]

Hi,

On 2024-10-24 10:15, Bartosz Golaszewski wrote:
> On Thu, 24 Oct 2024 08:49:30 +0200, Bartosz Golaszewski <brgl@bgdev.pl> said:
>> On Wed, Oct 23, 2024 at 11:05 PM Mark Brown <broonie@kernel.org> wrote:
>>>
>>> On Fri, Oct 18, 2024 at 11:10:16AM +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.
>>>
>>> Today's -next is not booting on several of my platforms, including
>>> beaglebone-black, i.MX8MP-EVK and pine64plus.  Bisects are pointing at
>>> this commit, and i.MX8MP-EVK is actually giving some console output:
>>>
>>> [    2.502208] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>>> [    2.511036] Mem abort info:
>>>
>>> ...
>>>
>>> [    2.679934] Call trace:
>>> [    2.682379]  gpiod_direction_output+0x34/0x5c
>>> [    2.686745]  i2c_register_adapter+0x59c/0x670
>>> [    2.691111]  __i2c_add_numbered_adapter+0x58/0xa8
>>> [    2.695822]  i2c_add_adapter+0xa0/0xd0
>>> [    2.699578]  i2c_add_numbered_adapter+0x2c/0x38
>>> [    2.704117]  i2c_imx_probe+0x2d0/0x640
>>>
>>> which looks plausible given the change.
>>>
>>> Full boot log for i.MX8MP-EVK:
>>>
>>>     https://lava.sirena.org.uk/scheduler/job/887083
>>>
>>> Bisect log for that, the others look similar (the long run of good/bad
>>> tags at the start for random commits is my automation pulling test
>>> results it already knows about in the affected range to try to speed up
>>> the bisect):
>>>
>>
>> Hi Mark!
>>
>> Any chance you could post the output of
>>
>>      scripts/faddr2line drivers/gpio/gpiolib.o gpiod_direction_output+0x34/0x5c
>>
>> for that build?
>>
>> Bart
>>
> 
> While I can't really reproduce it, I've looked at what could be wrong and
> figured that the NULL-pointer in question can possibly be the
> line_state_notifier.
> 
> I realized that for some historical reasons we add the GPIO device to the
> global list before it's fully set up - including initializing the notifier.
> In some corner cases (devlinks borked?) this could lead to consumers requesting
> GPIOs before their provider is ready.
> 
> Mark: could you try the following diff and let me know if it works?
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ae758ba6dc3d..4258acac0162 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -987,45 +987,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> 
>   	gdev->ngpio = gc->ngpio;
>   	gdev->can_sleep = gc->can_sleep;
> -
> -	scoped_guard(mutex, &gpio_devices_lock) {
> -		/*
> -		 * TODO: this allocates a Linux GPIO number base in the global
> -		 * GPIO numberspace for this chip. In the long run we want to
> -		 * get *rid* of this numberspace and use only descriptors, but
> -		 * it may be a pipe dream. It will not happen before we get rid
> -		 * of the sysfs interface anyways.
> -		 */
> -		base = gc->base;
> -		if (base < 0) {
> -			base = gpiochip_find_base_unlocked(gc->ngpio);
> -			if (base < 0) {
> -				ret = base;
> -				base = 0;
> -				goto err_free_label;
> -			}
> -
> -			/*
> -			 * TODO: it should not be necessary to reflect the
> -			 * assigned base outside of the GPIO subsystem. Go over
> -			 * drivers and see if anyone makes use of this, else
> -			 * drop this and assign a poison instead.
> -			 */
> -			gc->base = base;
> -		} else {
> -			dev_warn(&gdev->dev,
> -				 "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
> -		}
> -
> -		gdev->base = base;
> -
> -		ret = gpiodev_add_to_list_unlocked(gdev);
> -		if (ret) {
> -			chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
> -			goto err_free_label;
> -		}
> -	}
> -
>   	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
>   	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
> 
> @@ -1103,6 +1064,45 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   		if (ret)
>   			goto err_remove_irqchip;
>   	}
> +
> +	scoped_guard(mutex, &gpio_devices_lock) {
> +		/*
> +		 * TODO: this allocates a Linux GPIO number base in the global
> +		 * GPIO numberspace for this chip. In the long run we want to
> +		 * get *rid* of this numberspace and use only descriptors, but
> +		 * it may be a pipe dream. It will not happen before we get rid
> +		 * of the sysfs interface anyways.
> +		 */
> +		base = gc->base;
> +		if (base < 0) {
> +			base = gpiochip_find_base_unlocked(gc->ngpio);
> +			if (base < 0) {
> +				ret = base;
> +				base = 0;
> +				goto err_free_label;
> +			}
> +
> +			/*
> +			 * TODO: it should not be necessary to reflect the
> +			 * assigned base outside of the GPIO subsystem. Go over
> +			 * drivers and see if anyone makes use of this, else
> +			 * drop this and assign a poison instead.
> +			 */
> +			gc->base = base;
> +		} else {
> +			dev_warn(&gdev->dev,
> +				 "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
> +		}
> +
> +		gdev->base = base;
> +
> +		ret = gpiodev_add_to_list_unlocked(gdev);
> +		if (ret) {
> +			chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
> +			goto err_free_label;
> +		}
> +	}
> +
>   	return 0;
> 
>   err_remove_irqchip:
> 
> Thanks
> Bartosz
> 

I think I hit the same, or a similar bug, on my Edgerouter 6P (Cavium 
Octeon III):

CPU 3 Unable to handle kernel paging request at virtual address 
0000000000000000, epc == ffffffff817a40c8, ra == ffffffff817a40a4
Oops[#1]:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.12.0-rc4-next-20241023-00032-g01c8e35f8d89 #84
...
Call Trace:
gpiod_direction_output (drivers/gpio/gpiolib.c:4164 
drivers/gpio/gpiolib.c:2700 drivers/gpio/gpiolib.c:2694)
i2c_register_adapter (drivers/i2c/i2c-core-base.c:396 
drivers/i2c/i2c-core-base.c:422 drivers/i2c/i2c-core-base.c:434 
drivers/i2c/i2c-core-base.c:1574)
octeon_i2c_probe (drivers/i2c/busses/i2c-octeon-platdrv.c:247)

(the complete log is attached)

Unfortunately the oops remains after applying the diff and the call 
trace looks to be the same.

Please let me know if there's anything else you need.

Regards,
Klara Modin

[-- Attachment #2: gpiod_oops_decoded.gz --]
[-- Type: application/gzip, Size: 6180 bytes --]

[-- Attachment #3: gpio_oops_bisect --]
[-- Type: text/plain, Size: 2616 bytes --]

# bad: [30226ad165626fa1d00945758ecafcfbdb47aed0] sched/ext: fix fmt fmt__str inconsistency
git bisect start 'HEAD'
# status: waiting for good commit(s), bad commit known
# good: [c2ee9f594da826bea183ed14f2cc029c719bf4da] KVM: selftests: Fix build on on non-x86 architectures
git bisect good c2ee9f594da826bea183ed14f2cc029c719bf4da
# good: [1f64fb9c2040c018d0e045b785b3d11a3bc0bf61] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good 1f64fb9c2040c018d0e045b785b3d11a3bc0bf61
# good: [d72293fcdadf4ca9fe265c9057e2d4e4b8c3fa7f] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
git bisect good d72293fcdadf4ca9fe265c9057e2d4e4b8c3fa7f
# good: [d9e7d56ac23ba13e1255fa114aa1b90993386a40] Merge branch 'char-misc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git bisect good d9e7d56ac23ba13e1255fa114aa1b90993386a40
# bad: [a37d9a1b19767866febad59765806042bc49ad7c] Merge branch 'gpio/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect bad a37d9a1b19767866febad59765806042bc49ad7c
# good: [1096ccc17707eb50f677a6457bf65527bdf13d51] Merge branch 'staging-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
git bisect good 1096ccc17707eb50f677a6457bf65527bdf13d51
# good: [c6b673dd67833f12a52eedb2d7bb2429d7d95a8d] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
git bisect good c6b673dd67833f12a52eedb2d7bb2429d7d95a8d
# good: [3bd13ae04ccc20e3a312596f89a269b8b6416dca] gpio: menz127: simplify error path and remove remove()
git bisect good 3bd13ae04ccc20e3a312596f89a269b8b6416dca
# bad: [3aba8402910bfab998d5cf6c84744de5db466936] gpio: grgpio: remove remove()
git bisect bad 3aba8402910bfab998d5cf6c84744de5db466936
# good: [81625f362497509526a2f9ac53843ae30b4709cc] gpio: cdev: go back to storing debounce period in the GPIO descriptor
git bisect good 81625f362497509526a2f9ac53843ae30b4709cc
# good: [fcc8b637c542d1a0c19e5797ad72f9258e10464c] gpiolib: switch the line state notifier to atomic
git bisect good fcc8b637c542d1a0c19e5797ad72f9258e10464c
# bad: [bc40668def384256673bc18296865869c4a4187b] gpio: grgpio: drop Kconfig dependency on OF_GPIO
git bisect bad bc40668def384256673bc18296865869c4a4187b
# bad: [07c61d4da43fa3b34c152b28010d20be115a96db] gpiolib: notify user-space about in-kernel line state changes
git bisect bad 07c61d4da43fa3b34c152b28010d20be115a96db
# first bad commit: [07c61d4da43fa3b34c152b28010d20be115a96db] gpiolib: notify user-space about in-kernel line state changes

[-- Attachment #4: gpiod_oops_after_patch_decoded.gz --]
[-- Type: application/gzip, Size: 6203 bytes --]

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

[Bartosz Golaszewski]

On Thu, 24 Oct 2024 13:34:11 +0200, Klara Modin <klarasmodin@gmail.com> said:
>
> I think I hit the same, or a similar bug, on my Edgerouter 6P (Cavium
> Octeon III):
>
> CPU 3 Unable to handle kernel paging request at virtual address
> 0000000000000000, epc == ffffffff817a40c8, ra == ffffffff817a40a4
> Oops[#1]:
> CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.12.0-rc4-next-20241023-00032-g01c8e35f8d89 #84
> ...
> Call Trace:
> gpiod_direction_output (drivers/gpio/gpiolib.c:4164
> drivers/gpio/gpiolib.c:2700 drivers/gpio/gpiolib.c:2694)
> i2c_register_adapter (drivers/i2c/i2c-core-base.c:396
> drivers/i2c/i2c-core-base.c:422 drivers/i2c/i2c-core-base.c:434
> drivers/i2c/i2c-core-base.c:1574)
> octeon_i2c_probe (drivers/i2c/busses/i2c-octeon-platdrv.c:247)
>
> (the complete log is attached)
>
> Unfortunately the oops remains after applying the diff and the call
> trace looks to be the same.
>
> Please let me know if there's anything else you need.
>

Hmm so it's desc->gdev that is NULL... Could you try the following:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ae758ba6dc3d..c95c79ea2b49 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1026,6 +1026,9 @@ int gpiochip_add_data_with_key(struct gpio_chip
*gc, void *data,
 		}
 	}

+	for (desc_index = 0; desc_index < gc->ngpio; desc_index++)
+		gdev->descs[desc_index].gdev = gdev;
+
 	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);

@@ -1055,8 +1058,6 @@ int gpiochip_add_data_with_key(struct gpio_chip
*gc, void *data,
 	for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
 		struct gpio_desc *desc = &gdev->descs[desc_index];

-		desc->gdev = gdev;
-
 		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
 			assign_bit(FLAG_IS_OUT,
 				   &desc->flags, !gc->get_direction(gc, desc_index));

I'm wondering if this is not an earlier commit.

Bart

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

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

On 2024-10-24 13:45, Bartosz Golaszewski wrote:
> On Thu, 24 Oct 2024 13:34:11 +0200, Klara Modin <klarasmodin@gmail.com> said:
>>
>> I think I hit the same, or a similar bug, on my Edgerouter 6P (Cavium
>> Octeon III):
>>
>> CPU 3 Unable to handle kernel paging request at virtual address
>> 0000000000000000, epc == ffffffff817a40c8, ra == ffffffff817a40a4
>> Oops[#1]:
>> CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted
>> 6.12.0-rc4-next-20241023-00032-g01c8e35f8d89 #84
>> ...
>> Call Trace:
>> gpiod_direction_output (drivers/gpio/gpiolib.c:4164
>> drivers/gpio/gpiolib.c:2700 drivers/gpio/gpiolib.c:2694)
>> i2c_register_adapter (drivers/i2c/i2c-core-base.c:396
>> drivers/i2c/i2c-core-base.c:422 drivers/i2c/i2c-core-base.c:434
>> drivers/i2c/i2c-core-base.c:1574)
>> octeon_i2c_probe (drivers/i2c/busses/i2c-octeon-platdrv.c:247)
>>
>> (the complete log is attached)
>>
>> Unfortunately the oops remains after applying the diff and the call
>> trace looks to be the same.
>>
>> Please let me know if there's anything else you need.
>>
> 
> Hmm so it's desc->gdev that is NULL... Could you try the following:
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ae758ba6dc3d..c95c79ea2b49 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1026,6 +1026,9 @@ int gpiochip_add_data_with_key(struct gpio_chip
> *gc, void *data,
>   		}
>   	}
> 
> +	for (desc_index = 0; desc_index < gc->ngpio; desc_index++)
> +		gdev->descs[desc_index].gdev = gdev;
> +
>   	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
>   	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
> 
> @@ -1055,8 +1058,6 @@ int gpiochip_add_data_with_key(struct gpio_chip
> *gc, void *data,
>   	for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
>   		struct gpio_desc *desc = &gdev->descs[desc_index];
> 
> -		desc->gdev = gdev;
> -
>   		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
>   			assign_bit(FLAG_IS_OUT,
>   				   &desc->flags, !gc->get_direction(gc, desc_index));
> 
> I'm wondering if this is not an earlier commit.
> 
> Bart

That doesn't seem to change anything significantly:

Call Trace:
gpiod_direction_output (drivers/gpio/gpiolib.c:4165 
drivers/gpio/gpiolib.c:2701 drivers/gpio/gpiolib.c:2695)
i2c_register_adapter (drivers/i2c/i2c-core-base.c:396 
drivers/i2c/i2c-core-base.c:422 drivers/i2c/i2c-core-base.c:434 
drivers/i2c/i2c-core-base.c:1574)
octeon_i2c_probe (drivers/i2c/busses/i2c-octeon-platdrv.c:247)

[-- Attachment #2: gpiod_oops3_decoded.gz --]
[-- Type: application/gzip, Size: 6204 bytes --]

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

[Bartosz Golaszewski]

On Thu, Oct 24, 2024 at 2:00 PM Klara Modin <klarasmodin@gmail.com> wrote:
>
> On 2024-10-24 13:45, Bartosz Golaszewski wrote:
> > On Thu, 24 Oct 2024 13:34:11 +0200, Klara Modin <klarasmodin@gmail.com> said:
> >>
> >> I think I hit the same, or a similar bug, on my Edgerouter 6P (Cavium
> >> Octeon III):
> >>
> >> CPU 3 Unable to handle kernel paging request at virtual address
> >> 0000000000000000, epc == ffffffff817a40c8, ra == ffffffff817a40a4
> >> Oops[#1]:
> >> CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> >> 6.12.0-rc4-next-20241023-00032-g01c8e35f8d89 #84
> >> ...
> >> Call Trace:
> >> gpiod_direction_output (drivers/gpio/gpiolib.c:4164
> >> drivers/gpio/gpiolib.c:2700 drivers/gpio/gpiolib.c:2694)
> >> i2c_register_adapter (drivers/i2c/i2c-core-base.c:396
> >> drivers/i2c/i2c-core-base.c:422 drivers/i2c/i2c-core-base.c:434
> >> drivers/i2c/i2c-core-base.c:1574)
> >> octeon_i2c_probe (drivers/i2c/busses/i2c-octeon-platdrv.c:247)
> >>
> >> (the complete log is attached)
> >>
> >> Unfortunately the oops remains after applying the diff and the call
> >> trace looks to be the same.
> >>
> >> Please let me know if there's anything else you need.
> >>
> >
> > Hmm so it's desc->gdev that is NULL... Could you try the following:
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index ae758ba6dc3d..c95c79ea2b49 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1026,6 +1026,9 @@ int gpiochip_add_data_with_key(struct gpio_chip
> > *gc, void *data,
> >               }
> >       }
> >
> > +     for (desc_index = 0; desc_index < gc->ngpio; desc_index++)
> > +             gdev->descs[desc_index].gdev = gdev;
> > +
> >       ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
> >       BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
> >
> > @@ -1055,8 +1058,6 @@ int gpiochip_add_data_with_key(struct gpio_chip
> > *gc, void *data,
> >       for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
> >               struct gpio_desc *desc = &gdev->descs[desc_index];
> >
> > -             desc->gdev = gdev;
> > -
> >               if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> >                       assign_bit(FLAG_IS_OUT,
> >                                  &desc->flags, !gc->get_direction(gc, desc_index));
> >
> > I'm wondering if this is not an earlier commit.
> >
> > Bart
>
> That doesn't seem to change anything significantly:
>

I managed to reproduce it by using optional GPIOs. I sent out a fix
with both of you in Cc, please give it a try.

Bart

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Sverdlin, Alexander]

Hello Bartosz,

On Fri, 2024-10-18 at 11:10 +0200, Bartosz Golaszewski wrote:
> 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>

starting from this patch up to the current linux-next (v6.18-rcX)
I see the following refcnt warnings + KASAN UAF reports on either reboot
(when gpio-manager is being stopped) or
`systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
requested from (owned by) gpio-manager prior to kill):

------------[ cut here ]------------
struct file::f_count incremented from zero; use-after-free condition present!
WARNING: CPU: 1 PID: 560 at git/include/linux/fs.h:1082 lineinfo_changed_notify+0x238/0x260
CPU: 1 UID: 966 PID: 560 Comm: gpio-manager Tainted: G           O       6.12.0-rc3+git8340ac8106a4 #1
Tainted: [O]=OOT_MODULE
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : lineinfo_changed_notify+0x238/0x260
lr : lineinfo_changed_notify+0x238/0x260
Call trace:
 lineinfo_changed_notify+0x238/0x260
 notifier_call_chain+0xd4/0x248
 blocking_notifier_call_chain+0x5c/0xa8
 gpiod_free_commit+0x2c8/0x4a0
 gpiod_free+0x2c/0x90
 linereq_free+0xcc/0x190
 linereq_release+0x2c/0x48
 __fput+0x1b4/0x498
 ____fput+0x1c/0x30
 task_work_run+0x120/0x1d0
 do_exit+0x49c/0xff0
 do_group_exit+0x60/0xf8
 get_signal+0xdb8/0xea0
 do_signal+0x138/0x1b80
 do_notify_resume+0x118/0x1c8
 el0_svc+0xbc/0xc0
 el0t_64_sync_handler+0x120/0x130
 el0t_64_sync+0x190/0x198
irq event stamp: 212610
hardirqs last  enabled at (212609): [<ffff8000800d13fc>] finish_task_switch.isra.0+0xfc/0x3e8
hardirqs last disabled at (212610): [<ffff800081630928>] el1_dbg+0x28/0x90
softirqs last  enabled at (212600): [<ffff800080076e58>] handle_softirqs+0x6b0/0x6d8
softirqs last disabled at (212463): [<ffff800080010254>] __do_softirq+0x1c/0x28
---[ end trace 0000000000000000 ]---
==================================================================
BUG: KASAN: use-after-free in gpio_chrdev_release+0x30/0x98
Read of size 8 at addr ffff000006b94000 by task gpio-manager/560

CPU: 1 UID: 966 PID: 560 Comm: gpio-manager Tainted: G        W  O       6.12.0-rc3+git8340ac8106a4 #1
Tainted: [W]=WARN, [O]=OOT_MODULE
Call trace:
 dump_backtrace+0xa0/0x128
 show_stack+0x20/0x38
 dump_stack_lvl+0x8c/0xd0
 print_report+0xfc/0x5e0
 kasan_report+0xc0/0x100
 __asan_load8+0x9c/0xc0
 gpio_chrdev_release+0x30/0x98
 __fput+0x1b4/0x498
 ____fput+0x1c/0x30
 task_work_run+0x120/0x1d0
 do_exit+0x49c/0xff0
 do_group_exit+0x60/0xf8
 get_signal+0xdb8/0xea0
 do_signal+0x138/0x1b80
 do_notify_resume+0x118/0x1c8
 el0_svc+0xbc/0xc0
 el0t_64_sync_handler+0x120/0x130
 el0t_64_sync+0x190/0x198

The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff000006b974c0 pfn:0x86b94
flags: 0x0(zone=0)
page_type: f0(buddy)
raw: 0000000000000000 fffffdffc03eaf08 ffff800083356e90 0000000000000000
raw: ffff000006b974c0 0000000000000002 00000000f0000000 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff000006b93f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff000006b93f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff000006b94000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
                   ^
 ffff000006b94080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ffff000006b94100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================
------------[ cut here ]------------
WARNING: CPU: 1 PID: 30 at git/mm/slub.c:4689 free_large_kmalloc+0x100/0x140
CPU: 1 UID: 0 PID: 30 Comm: kworker/1:1 Tainted: G        W  O       6.12.0-rc3+git8340ac8106a4 #1
Disabling lock debugging due to kernel taint
Tainted: [W]=WARN, [O]=OOT_MODULE
object pointer: 0xffff000006b94000
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000028
Workqueue: events delayed_fput
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
Mem abort info:
pc : free_large_kmalloc+0x100/0x140
lr : kfree+0x2d4/0x418
Call trace:
 free_large_kmalloc+0x100/0x140
 kfree+0x2d4/0x418
 gpio_chrdev_release+0x78/0x98
Data abort info:
 __fput+0x1b4/0x498
  ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
 delayed_fput+0x68/0x88
  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
 process_one_work+0x454/0xa80
  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
 worker_thread+0x2c0/0x528
 kthread+0x1cc/0x1e0
 ret_from_fork+0x10/0x20
irq event stamp: 113358
hardirqs last  enabled at (113357): [<ffff800081645784>] _raw_spin_unlock_irqrestore+0x7c/0x88
hardirqs last disabled at (113358): [<ffff800081630928>] el1_dbg+0x28/0x90
user pgtable: 4k pages, 48-bit VAs, pgdp=0000000087eb6000
softirqs last  enabled at (111764): [<ffff800080076e58>] handle_softirqs+0x6b0/0x6d8
[0000000000000028] pgd=0000000000000000
softirqs last disabled at (111759): [<ffff800080010254>] __do_softirq+0x1c/0x28
, p4d=0000000000000000
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
WARNING: CPU: 1 PID: 30 at git/fs/dcache.c:774 dput.part.0+0x43c/0x458
CPU: 1 UID: 0 PID: 30 Comm: kworker/1:1 Tainted: G    B   W  O       6.12.0-rc3+git8340ac8106a4 #1
Tainted: [B]=BAD_PAGE, [W]=WARN, [O]=OOT_MODULE
Workqueue: events delayed_fput
pstate: a0000005 (NzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : dput.part.0+0x43c/0x458
lr : dput.part.0+0x36c/0x458
Call trace:
 dput.part.0+0x43c/0x458
 dput+0x1c/0x38
 __fput+0x218/0x498
 delayed_fput+0x68/0x88
 process_one_work+0x454/0xa80
 worker_thread+0x2c0/0x528
 kthread+0x1cc/0x1e0
 ret_from_fork+0x10/0x20
irq event stamp: 113358
hardirqs last  enabled at (113357): [<ffff800081645784>] _raw_spin_unlock_irqrestore+0x7c/0x88
hardirqs last disabled at (113358): [<ffff800081630928>] el1_dbg+0x28/0x90
Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
softirqs last  enabled at (111764): [<ffff800080076e58>] handle_softirqs+0x6b0/0x6d8
softirqs last disabled at (111759): [<ffff800080010254>] __do_softirq+0x1c/0x28
---[ end trace 0000000000000000 ]---


> ---
>  drivers/gpio/gpiolib-cdev.c | 82 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index b242fdb1ad28..7759dca92f8b 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2333,6 +2333,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)
> @@ -2502,28 +2503,86 @@ 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);
> +	/*
> +	 * 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);
> +	/* 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;
>  }
> @@ -2683,6 +2742,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)
> 

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Bartosz Golaszewski]

On Thu, Nov 13, 2025 at 10:11 PM Sverdlin, Alexander
<alexander.sverdlin@siemens.com> wrote:
>
> Hello Bartosz,
>
> On Fri, 2024-10-18 at 11:10 +0200, Bartosz Golaszewski wrote:
> > 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>
>
> starting from this patch up to the current linux-next (v6.18-rcX)
> I see the following refcnt warnings + KASAN UAF reports on either reboot
> (when gpio-manager is being stopped) or
> `systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
> requested from (owned by) gpio-manager prior to kill):
>

Hi!

Thanks for the bug report. I confirm it's reproducible on my side too.
It never occurred to me to try and SIGKILL gpio-manager. On normal
exit, nothing bad happens. Let me look into it.

Do you also go by ccpalex on github? I want to give you credit for
reporting the other bug in gpio-manager as well.

Bartosz

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Sverdlin, Alexander]

Hi Bartosz,

On Fri, 2025-11-14 at 09:21 +0100, Bartosz Golaszewski wrote:
> > > 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>
> > 
> > starting from this patch up to the current linux-next (v6.18-rcX)
> > I see the following refcnt warnings + KASAN UAF reports on either reboot
> > (when gpio-manager is being stopped) or
> > `systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
> > requested from (owned by) gpio-manager prior to kill):

[]

> Thanks for the bug report. I confirm it's reproducible on my side too.
> It never occurred to me to try and SIGKILL gpio-manager. On normal
> exit, nothing bad happens. Let me look into it.

In my case it happens also on every reboot/shutdown, however if I
`systemctl stop gpio-manager`, I don't see the issue... not sure yet,
what is the difference... and I'm not telling SIGKILL is how one
should stop gpio-manager, but I'm happy SIGKILL now allows you to
reproduce the issue in the kernel code!

> Do you also go by ccpalex on github? I want to give you credit for
> reporting the other bug in gpio-manager as well.

That's me! I didn't know where to report an issue with user-space libgpiod,
that's why I went with github ;-)

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Bartosz Golaszewski]

On Fri, 14 Nov 2025 10:04:11 +0100, "Sverdlin, Alexander"
<alexander.sverdlin@siemens.com> said:
> Hi Bartosz,
>
> On Fri, 2025-11-14 at 09:21 +0100, Bartosz Golaszewski wrote:
>> > > 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>
>> >
>> > starting from this patch up to the current linux-next (v6.18-rcX)
>> > I see the following refcnt warnings + KASAN UAF reports on either reboot
>> > (when gpio-manager is being stopped) or
>> > `systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
>> > requested from (owned by) gpio-manager prior to kill):
>
> []
>
>> Thanks for the bug report. I confirm it's reproducible on my side too.
>> It never occurred to me to try and SIGKILL gpio-manager. On normal
>> exit, nothing bad happens. Let me look into it.
>
> In my case it happens also on every reboot/shutdown, however if I
> `systemctl stop gpio-manager`, I don't see the issue... not sure yet,
> what is the difference... and I'm not telling SIGKILL is how one
> should stop gpio-manager, but I'm happy SIGKILL now allows you to
> reproduce the issue in the kernel code!
>

When the process is killed, it seems the character device's file struct is no
longer valid even though chrdev_release() was not yet called - it's called
after we try to send out this notification. I think it's best to just check
if the file is active and not do anything if it isn't.

Can you test the following change before I submit a formal patch?

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e6a289fa0f8f..0f1103a679b5 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2548,10 +2548,17 @@ static int lineinfo_changed_notify(struct
notifier_block *nb,
 		container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
 	struct lineinfo_changed_ctx *ctx;
 	struct gpio_desc *desc = data;
+	struct file *fp;

 	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
 		return NOTIFY_DONE;

+	/* Keep the file descriptor alive for the duration of the notification. */
+	fp = get_file_active(&cdev->fp);
+	if (!fp)
+		/* Chardev file descriptor was already released. */
+		return NOTIFY_DONE;
+
 	/*
 	 * If this is called from atomic context (for instance: with a spinlock
 	 * taken by the atomic notifier chain), any sleeping calls must be done
@@ -2575,8 +2582,6 @@ static int lineinfo_changed_notify(struct
notifier_block *nb,
 	/* 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);

>> Do you also go by ccpalex on github? I want to give you credit for
>> reporting the other bug in gpio-manager as well.
>
> That's me! I didn't know where to report an issue with user-space libgpiod,
> that's why I went with github ;-)
>

You can use this mailing list but github is fine too. Thanks.

Bartosz

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Sverdlin, Alexander]

Hi Bartosz,

On Fri, 2025-11-14 at 02:11 -0800, Bartosz Golaszewski wrote:
> > > > > 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>
> > > > 
> > > > starting from this patch up to the current linux-next (v6.18-rcX)
> > > > I see the following refcnt warnings + KASAN UAF reports on either reboot
> > > > (when gpio-manager is being stopped) or
> > > > `systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
> > > > requested from (owned by) gpio-manager prior to kill):
> > 
> > []
> > 
> > > Thanks for the bug report. I confirm it's reproducible on my side too.
> > > It never occurred to me to try and SIGKILL gpio-manager. On normal
> > > exit, nothing bad happens. Let me look into it.
> > 
> > In my case it happens also on every reboot/shutdown, however if I
> > `systemctl stop gpio-manager`, I don't see the issue... not sure yet,
> > what is the difference... and I'm not telling SIGKILL is how one
> > should stop gpio-manager, but I'm happy SIGKILL now allows you to
> > reproduce the issue in the kernel code!
> > 
> 
> When the process is killed, it seems the character device's file struct is no
> longer valid even though chrdev_release() was not yet called - it's called
> after we try to send out this notification. I think it's best to just check
> if the file is active and not do anything if it isn't.
> 
> Can you test the following change before I submit a formal patch?

thank you for the quick fix!
I've tested the below patch (on top of v6.18-rc4 with quite some debug options
and even KASAN) and I don't get any issues, neither killing gpio-manager,
nor during reboot.

Feel free to add my
Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e6a289fa0f8f..0f1103a679b5 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2548,10 +2548,17 @@ static int lineinfo_changed_notify(struct
> notifier_block *nb,
>  		container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
>  	struct lineinfo_changed_ctx *ctx;
>  	struct gpio_desc *desc = data;
> +	struct file *fp;
> 
>  	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
>  		return NOTIFY_DONE;
> 
> +	/* Keep the file descriptor alive for the duration of the notification. */
> +	fp = get_file_active(&cdev->fp);
> +	if (!fp)
> +		/* Chardev file descriptor was already released. */
> +		return NOTIFY_DONE;
> +
>  	/*
>  	 * If this is called from atomic context (for instance: with a spinlock
>  	 * taken by the atomic notifier chain), any sleeping calls must be done
> @@ -2575,8 +2582,6 @@ static int lineinfo_changed_notify(struct
> notifier_block *nb,
>  	/* 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);

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v5 6/8] gpio: cdev: put emitting the line state events on a workqueue

[Bartosz Golaszewski]

On Fri, Nov 14, 2025 at 12:57 PM Sverdlin, Alexander
<alexander.sverdlin@siemens.com> wrote:
>
> Hi Bartosz,
>
> On Fri, 2025-11-14 at 02:11 -0800, Bartosz Golaszewski wrote:
> > > > > > 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>
> > > > >
> > > > > starting from this patch up to the current linux-next (v6.18-rcX)
> > > > > I see the following refcnt warnings + KASAN UAF reports on either reboot
> > > > > (when gpio-manager is being stopped) or
> > > > > `systemctl kill --signal=SIGKILL gpio-manager` (if some GPIOs are being
> > > > > requested from (owned by) gpio-manager prior to kill):
> > >
> > > []
> > >
> > > > Thanks for the bug report. I confirm it's reproducible on my side too.
> > > > It never occurred to me to try and SIGKILL gpio-manager. On normal
> > > > exit, nothing bad happens. Let me look into it.
> > >
> > > In my case it happens also on every reboot/shutdown, however if I
> > > `systemctl stop gpio-manager`, I don't see the issue... not sure yet,
> > > what is the difference... and I'm not telling SIGKILL is how one
> > > should stop gpio-manager, but I'm happy SIGKILL now allows you to
> > > reproduce the issue in the kernel code!
> > >
> >
> > When the process is killed, it seems the character device's file struct is no
> > longer valid even though chrdev_release() was not yet called - it's called
> > after we try to send out this notification. I think it's best to just check
> > if the file is active and not do anything if it isn't.
> >
> > Can you test the following change before I submit a formal patch?
>
> thank you for the quick fix!
> I've tested the below patch (on top of v6.18-rc4 with quite some debug options
> and even KASAN) and I don't get any issues, neither killing gpio-manager,
> nor during reboot.
>
> Feel free to add my
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>

Thanks! I still want to investigate it a bit more as the same issue
appears even without SIGKILL if I run the following python script:

import gpiod
chip = gpiod.Chip("/dev/gpiochip0")
chip.watch_line_info(0)
req = chip.request_lines({0: None})

It basically happens if the same process is watching the line it
requests and then gets killed and the ordering of operations is right.
In gpio-manager when existing cleanly, we free resources in the right
order but this python script lets the OS handle cleanup and the same
issue occurs.

I want to get to the bottom of what's going on.

Bart