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

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

[Bartosz Golaszewski]

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

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

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

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

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

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

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

[Bartosz Golaszewski]

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

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

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0d1e10654d4c..839036b116a2 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 v2 2/6] gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic

[Bartosz Golaszewski]

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

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

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

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

-- 
2.43.0

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

[Kent Gibson]

On Thu, Oct 10, 2024 at 11:10:23AM +0200, Bartosz Golaszewski wrote:
> 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 | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index b0050250ac3a..8c48a53b4fa8 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2361,7 +2361,7 @@ static void gpio_v2_line_info_changed_to_v1(
>  #endif /* CONFIG_GPIO_CDEV_V1 */
>
>  static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> -				  struct gpio_v2_line_info *info)
> +				  struct gpio_v2_line_info *info, bool atomic)
>  {
>  	unsigned long dflags;
>  	const char *label;
> @@ -2402,9 +2402,13 @@ 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;
> +
> +	if (!atomic) {
> +		if (!pinctrl_gpio_can_use_line(guard.gc, info->offset))
> +			info->flags |= GPIO_V2_LINE_FLAG_USED;
> +	}
>

Should be else if.

Cheers,
Kent.

>  	if (test_bit(FLAG_IS_OUT, &dflags))
>  		info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
> @@ -2502,7 +2506,7 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
>  			return -EBUSY;
>  	}
>
> -	gpio_desc_to_lineinfo(desc, &lineinfo_v2);
> +	gpio_desc_to_lineinfo(desc, &lineinfo_v2, false);
>  	gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
>
>  	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
> @@ -2539,7 +2543,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
>  		if (test_and_set_bit(lineinfo.offset, cdev->watched_lines))
>  			return -EBUSY;
>  	}
> -	gpio_desc_to_lineinfo(desc, &lineinfo);
> +	gpio_desc_to_lineinfo(desc, &lineinfo, false);
>  	supinfo_to_lineinfo(desc, &lineinfo);
>
>  	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
> @@ -2632,7 +2636,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
>  	memset(&chg, 0, sizeof(chg));
>  	chg.event_type = action;
>  	chg.timestamp_ns = ktime_get_ns();
> -	gpio_desc_to_lineinfo(desc, &chg.info);
> +	gpio_desc_to_lineinfo(desc, &chg.info, false);
>  	supinfo_to_lineinfo(desc, &chg.info);
>
>  	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
>
> --
> 2.43.0
>

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 3:58 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Oct 10, 2024 at 11:10:23AM +0200, Bartosz Golaszewski wrote:
> > 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>
> > ---
> >       unsigned long dflags;
> >       const char *label;
> > @@ -2402,9 +2402,13 @@ 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;
> > +
> > +     if (!atomic) {
> > +             if (!pinctrl_gpio_can_use_line(guard.gc, info->offset))
> > +                     info->flags |= GPIO_V2_LINE_FLAG_USED;
> > +     }
> >
>
> Should be else if.
>

If we're not atomic, let's call pinctrl_gpio_can_use_line() and update
the flag accordingly. If we're in atomic, just don't do it. In any
case do the rest. Looks good to me, am I missing something?

Bart

>
> >       if (test_bit(FLAG_IS_OUT, &dflags))
> >               info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
> > @@ -2502,7 +2506,7 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
> >                       return -EBUSY;
> >       }
> >

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

[Kent Gibson]

On Mon, Oct 14, 2024 at 09:45:19AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 14, 2024 at 3:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 11:10:23AM +0200, Bartosz Golaszewski wrote:
> > > 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>
> > > ---
> > >       unsigned long dflags;
> > >       const char *label;
> > > @@ -2402,9 +2402,13 @@ 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;
> > > +
> > > +     if (!atomic) {
> > > +             if (!pinctrl_gpio_can_use_line(guard.gc, info->offset))
> > > +                     info->flags |= GPIO_V2_LINE_FLAG_USED;
> > > +     }
> > >
> >
> > Should be else if.
> >
>
> If we're not atomic, let's call pinctrl_gpio_can_use_line() and update
> the flag accordingly. If we're in atomic, just don't do it. In any
> case do the rest. Looks good to me, am I missing something?
>

Previously the preceding if short circuits and doesn't perform the
pinctl check if ANY of the preceding checks are true.
The pinctrl check should be in an else-if to get the same behaviour.

I am refering to the if (!atomic), btw, not the if in its body.
(that is why my comment is placed after the closing bracket)

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 10:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Oct 14, 2024 at 09:45:19AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 14, 2024 at 3:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 11:10:23AM +0200, Bartosz Golaszewski wrote:
> > > > 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>
> > > > ---
> > > >       unsigned long dflags;
> > > >       const char *label;
> > > > @@ -2402,9 +2402,13 @@ 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;
> > > > +
> > > > +     if (!atomic) {
> > > > +             if (!pinctrl_gpio_can_use_line(guard.gc, info->offset))
> > > > +                     info->flags |= GPIO_V2_LINE_FLAG_USED;
> > > > +     }
> > > >
> > >
> > > Should be else if.
> > >
> >
> > If we're not atomic, let's call pinctrl_gpio_can_use_line() and update
> > the flag accordingly. If we're in atomic, just don't do it. In any
> > case do the rest. Looks good to me, am I missing something?
> >
>
> Previously the preceding if short circuits and doesn't perform the
> pinctl check if ANY of the preceding checks are true.
> The pinctrl check should be in an else-if to get the same behaviour.
>
> I am refering to the if (!atomic), btw, not the if in its body.
> (that is why my comment is placed after the closing bracket)
>

Ok, got it.

Bartosz

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

[Bartosz Golaszewski]

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

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

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8c48a53b4fa8..dec6c9a6005f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2869,6 +2869,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;
@@ -2885,6 +2890,7 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 
 void gpiolib_cdev_unregister(struct gpio_device *gdev)
 {
+	destroy_workqueue(gdev->line_state_wq);
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
 	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 067197d61d57..d24cd9e8b17c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/srcu.h>
+#include <linux/workqueue.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
 
@@ -44,6 +45,8 @@
  * @list: links gpio_device:s together for traversal
  * @line_state_notifier: used to notify subscribers about lines being
  *                       requested, released or reconfigured
+ * @line_state_wq: used to emit line state events from a separate thread in
+ *                 process context
  * @device_notifier: used to notify character device wait queues about the GPIO
  *                   device being unregistered
  * @srcu: protects the pointer to the underlying GPIO chip
@@ -70,6 +73,7 @@ struct gpio_device {
 	void			*data;
 	struct list_head        list;
 	struct blocking_notifier_head line_state_notifier;
+	struct workqueue_struct	*line_state_wq;
 	struct blocking_notifier_head device_notifier;
 	struct srcu_struct	srcu;
 

-- 
2.43.0

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

[Bartosz Golaszewski]

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

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

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

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

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index dec6c9a6005f..2677134b52cd 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2451,6 +2451,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)
@@ -2621,29 +2622,75 @@ 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;
+
+	scoped_guard(srcu, &ctx->gdev->srcu) {
+		gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu);
+		if (!gc)
+			return;
+
+		/*
+		 * 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.
+		 */
+		if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset))
+			ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED;
+	}
+
+	ret = kfifo_in_spinlocked(&ctx->cdev->events, &ctx->chg, 1,
+				  &ctx->cdev->wait.lock);
+	if (ret)
+		wake_up_poll(&ctx->cdev->wait, EPOLLIN);
+	else
+		pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+
+	gpio_device_put(ctx->gdev);
+	fput(ctx->cdev->fp);
+	kfree(ctx);
+}
+
 static int lineinfo_changed_notify(struct notifier_block *nb,
 				   unsigned long action, void *data)
 {
 	struct gpio_chardev_data *cdev =
 		container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
-	struct gpio_v2_line_info_changed chg;
+	struct lineinfo_changed_ctx *ctx;
 	struct gpio_desc *desc = data;
-	int ret;
 
 	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
 		return NOTIFY_DONE;
 
-	memset(&chg, 0, sizeof(chg));
-	chg.event_type = action;
-	chg.timestamp_ns = ktime_get_ns();
-	gpio_desc_to_lineinfo(desc, &chg.info, false);
-	supinfo_to_lineinfo(desc, &chg.info);
+	ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC);
+	if (!ctx) {
+		pr_err("Failed to allocate memory for line info notification\n");
+		return NOTIFY_DONE;
+	}
 
-	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->chg.event_type = action;
+	ctx->chg.timestamp_ns = ktime_get_ns();
+	gpio_desc_to_lineinfo(desc, &ctx->chg.info, true);
+	supinfo_to_lineinfo(desc, &ctx->chg.info);
+	/* Keep the GPIO device alive until we emit the event. */
+	ctx->gdev = gpio_device_get(desc->gdev);
+	ctx->cdev = cdev;
+	/* Keep the file descriptor alive too. */
+	get_file(ctx->cdev->fp);
+
+	INIT_WORK(&ctx->work, lineinfo_changed_func);
+	queue_work(ctx->gdev->line_state_wq, &ctx->work);
 
 	return NOTIFY_OK;
 }
@@ -2803,6 +2850,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

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

[Kent Gibson]

On Thu, Oct 10, 2024 at 11:10:25AM +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>
> ---
>  drivers/gpio/gpiolib-cdev.c | 72 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 60 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index dec6c9a6005f..2677134b52cd 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2451,6 +2451,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)
> @@ -2621,29 +2622,75 @@ 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;
> +
> +	scoped_guard(srcu, &ctx->gdev->srcu) {
> +		gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu);
> +		if (!gc)
> +			return;
> +
> +		/*
> +		 * 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.
> +		 */
> +		if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset))
> +			ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED;
> +	}
> +

This block should be conditional on GPIO_V2_LINE_FLAG_USED not already
being set - most of the time it will be and then this is pointless work.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 4:09 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> > +
> > +     scoped_guard(srcu, &ctx->gdev->srcu) {
> > +             gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu);
> > +             if (!gc)
> > +                     return;
> > +
> > +             /*
> > +              * 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.
> > +              */
> > +             if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset))
> > +                     ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED;
> > +     }
> > +
>
> This block should be conditional on GPIO_V2_LINE_FLAG_USED not already
> being set - most of the time it will be and then this is pointless work.
>

Good point, thanks!

Bart

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

[Bartosz Golaszewski]

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

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

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 2677134b52cd..7eae0b17a1d6 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2673,6 +2673,16 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
 		return NOTIFY_DONE;
 
+	/*
+	 * This is called from atomic context (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.
+	 */
+
 	ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC);
 	if (!ctx) {
 		pr_err("Failed to allocate memory for line info notification\n");
@@ -2837,8 +2847,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;
 
@@ -2862,8 +2872,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);
@@ -2887,8 +2897,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 839036b116a2..9b10f47832d5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1026,7 +1026,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		}
 	}
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
+	ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 
 	ret = init_srcu_struct(&gdev->srcu);
@@ -4089,8 +4089,8 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep);
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action)
 {
-	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
-				     action, desc);
+	atomic_notifier_call_chain(&desc->gdev->line_state_notifier,
+				   action, desc);
 }
 
 /**
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d24cd9e8b17c..2799157a1f6b 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -72,7 +72,7 @@ struct gpio_device {
 	const char		*label;
 	void			*data;
 	struct list_head        list;
-	struct blocking_notifier_head line_state_notifier;
+	struct atomic_notifier_head line_state_notifier;
 	struct workqueue_struct	*line_state_wq;
 	struct blocking_notifier_head device_notifier;
 	struct srcu_struct	srcu;

-- 
2.43.0

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

[Kent Gibson]

On Thu, Oct 10, 2024 at 11:10:26AM +0200, Bartosz Golaszewski wrote:
> 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 | 22 ++++++++++++++++------
>  drivers/gpio/gpiolib.c      |  6 +++---
>  drivers/gpio/gpiolib.h      |  2 +-
>  3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 2677134b52cd..7eae0b17a1d6 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2673,6 +2673,16 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
>  	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
>  		return NOTIFY_DONE;
>
> +	/*
> +	 * This is called from atomic context (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.
> +	 */
> +

Should be in patch 4?  You aren't otherwise changing that function here.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > +     /*
> > +      * This is called from atomic context (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.
> > +      */
> > +
>
> Should be in patch 4?  You aren't otherwise changing that function here.
>

Until this patch, the comment isn't really true, so I figured it makes
more sense here.

Bart

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

[Kent Gibson]

On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > +     /*
> > > +      * This is called from atomic context (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.
> > > +      */
> > > +
> >
> > Should be in patch 4?  You aren't otherwise changing that function here.
> >
>
> Until this patch, the comment isn't really true, so I figured it makes
> more sense here.
>

So the validity of the comment depends on how the function is being called?
Then perhaps you should reword it as well.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > +     /*
> > > > +      * This is called from atomic context (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.
> > > > +      */
> > > > +
> > >
> > > Should be in patch 4?  You aren't otherwise changing that function here.
> > >
> >
> > Until this patch, the comment isn't really true, so I figured it makes
> > more sense here.
> >
>
> So the validity of the comment depends on how the function is being called?
> Then perhaps you should reword it as well.
>

The validity of the comment depends on the type of the notifier used.
As long as it's a blocking notifier, it's called with a mutex taken -
it's process context. When we switch to the atomic notifier, this
function is now called with a spinlock taken, so it's considered
atomic.

Bart

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

[Kent Gibson]

On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > +     /*
> > > > > +      * This is called from atomic context (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.
> > > > > +      */
> > > > > +
> > > >
> > > > Should be in patch 4?  You aren't otherwise changing that function here.
> > > >
> > >
> > > Until this patch, the comment isn't really true, so I figured it makes
> > > more sense here.
> > >
> >
> > So the validity of the comment depends on how the function is being called?
> > Then perhaps you should reword it as well.
> >
>
> The validity of the comment depends on the type of the notifier used.
> As long as it's a blocking notifier, it's called with a mutex taken -
> it's process context. When we switch to the atomic notifier, this
> function is now called with a spinlock taken, so it's considered
> atomic.
>

Indeed - so the comment is brittle.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > +     /*
> > > > > > +      * This is called from atomic context (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.
> > > > > > +      */
> > > > > > +
> > > > >
> > > > > Should be in patch 4?  You aren't otherwise changing that function here.
> > > > >
> > > >
> > > > Until this patch, the comment isn't really true, so I figured it makes
> > > > more sense here.
> > > >
> > >
> > > So the validity of the comment depends on how the function is being called?
> > > Then perhaps you should reword it as well.
> > >
> >
> > The validity of the comment depends on the type of the notifier used.
> > As long as it's a blocking notifier, it's called with a mutex taken -
> > it's process context. When we switch to the atomic notifier, this
> > function is now called with a spinlock taken, so it's considered
> > atomic.
> >
>
> Indeed - so the comment is brittle.
>

I'm not sure what you're saying. We know it's an atomic notifier, we
assign this callback to the block and register by calling
atomic_notifier_chain_register(). I fail to see why you consider it
"brittle".

Bart

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

[Kent Gibson]

On Mon, Oct 14, 2024 at 11:32:24AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> > > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > > >
> > > > > > > +     /*
> > > > > > > +      * This is called from atomic context (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.
> > > > > > > +      */
> > > > > > > +
> > > > > >
> > > > > > Should be in patch 4?  You aren't otherwise changing that function here.
> > > > > >
> > > > >
> > > > > Until this patch, the comment isn't really true, so I figured it makes
> > > > > more sense here.
> > > > >
> > > >
> > > > So the validity of the comment depends on how the function is being called?
> > > > Then perhaps you should reword it as well.
> > > >
> > >
> > > The validity of the comment depends on the type of the notifier used.
> > > As long as it's a blocking notifier, it's called with a mutex taken -
> > > it's process context. When we switch to the atomic notifier, this
> > > function is now called with a spinlock taken, so it's considered
> > > atomic.
> > >
> >
> > Indeed - so the comment is brittle.
> >
>
> I'm not sure what you're saying. We know it's an atomic notifier, we
> assign this callback to the block and register by calling
> atomic_notifier_chain_register(). I fail to see why you consider it
> "brittle".
>


I realise that - I'm not sure how to rephrase.
The comment is describing changes in behaviour that were added in a previous
patch.  The comment should describe the change in behaviour there and in a
generic way that is independent of the notifier chain type.  Tying it to the
notifier chain type is what makes it brittle - if that is changed in the
future then the comment becomes confusing or invalid.

I'm not sure that adds anything to what I've already said.
It isn't a deal breaker - just seems like poor form to me.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 11:55 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Oct 14, 2024 at 11:32:24AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote:
> > > > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > > > >
> > > > > > > > +     /*
> > > > > > > > +      * This is called from atomic context (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.
> > > > > > > > +      */
> > > > > > > > +
> > > > > > >
> > > > > > > Should be in patch 4?  You aren't otherwise changing that function here.
> > > > > > >
> > > > > >
> > > > > > Until this patch, the comment isn't really true, so I figured it makes
> > > > > > more sense here.
> > > > > >
> > > > >
> > > > > So the validity of the comment depends on how the function is being called?
> > > > > Then perhaps you should reword it as well.
> > > > >
> > > >
> > > > The validity of the comment depends on the type of the notifier used.
> > > > As long as it's a blocking notifier, it's called with a mutex taken -
> > > > it's process context. When we switch to the atomic notifier, this
> > > > function is now called with a spinlock taken, so it's considered
> > > > atomic.
> > > >
> > >
> > > Indeed - so the comment is brittle.
> > >
> >
> > I'm not sure what you're saying. We know it's an atomic notifier, we
> > assign this callback to the block and register by calling
> > atomic_notifier_chain_register(). I fail to see why you consider it
> > "brittle".
> >
>
>
> I realise that - I'm not sure how to rephrase.
> The comment is describing changes in behaviour that were added in a previous
> patch.  The comment should describe the change in behaviour there and in a
> generic way that is independent of the notifier chain type.  Tying it to the
> notifier chain type is what makes it brittle - if that is changed in the
> future then the comment becomes confusing or invalid.
>
> I'm not sure that adds anything to what I've already said.
> It isn't a deal breaker - just seems like poor form to me.
>

Ok, let me see what I can do for v3.

Bart

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

[Bartosz Golaszewski]

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

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

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

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

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

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

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

-- 
2.43.0

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

[Kent Gibson]

On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We currently only notify user-space about line config changes that are
> made from user-space. Any kernel config changes are not signalled.
>
> Let's improve the situation by emitting the events closer to the source.
> To that end let's call the relevant notifier chain from the functions
> setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
> gpiod_toggle_active_low(). This covers all the options that we can
> inform the user-space about. We ignore events which don't have
> corresponding flags exported to user-space on purpose - otherwise the
> user would see a config-changed event but the associated line-info would
> remain unchanged.
>
> gpiod_direction_output/input() can be called from any context.
> Fortunately, we now emit line state events using an atomic notifier
> chain, so it's no longer an issue.
>
> Let's also add non-notifying wrappers around the direction setters in
> order to not emit superfluous reconfigure events when requesting the
> lines as the initial config should be part of the request notification.
>

So lines requested from kernel space will result in a LINE_REQUESTED and
then a series of LINE_CHANGED_CONFIG?  Whereas for lines requested from
userspace those will be collapsed into the one LINE_REQUESTED event?
That's not ideal, but I realise making the rest of the kernel behave as
per cdev would be non-trivial, so ok - it all comes out in the wash.
And it is clearly better than nothing.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, Oct 14, 2024 at 4:24 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We currently only notify user-space about line config changes that are
> > made from user-space. Any kernel config changes are not signalled.
> >
> > Let's improve the situation by emitting the events closer to the source.
> > To that end let's call the relevant notifier chain from the functions
> > setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
> > gpiod_toggle_active_low(). This covers all the options that we can
> > inform the user-space about. We ignore events which don't have
> > corresponding flags exported to user-space on purpose - otherwise the
> > user would see a config-changed event but the associated line-info would
> > remain unchanged.
> >
> > gpiod_direction_output/input() can be called from any context.
> > Fortunately, we now emit line state events using an atomic notifier
> > chain, so it's no longer an issue.
> >
> > Let's also add non-notifying wrappers around the direction setters in
> > order to not emit superfluous reconfigure events when requesting the
> > lines as the initial config should be part of the request notification.
> >
>
> So lines requested from kernel space will result in a LINE_REQUESTED and
> then a series of LINE_CHANGED_CONFIG?  Whereas for lines requested from
> userspace those will be collapsed into the one LINE_REQUESTED event?

No, why? I added the notification about the request to
gpiod_find_and_request() which is called by all the kernel getters and
it already configures all the flags without emitting events and calls
the non-notify variant of the direction setter. When a kernel driver
requests a GPIO, I only see a single event UNLESS after the
gpiod_get() call returns, it sets direction or changes config - just
like user-space.

Bart

> That's not ideal, but I realise making the rest of the kernel behave as
> per cdev would be non-trivial, so ok - it all comes out in the wash.
> And it is clearly better than nothing.
>
> Cheers,
> Kent.
>

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

[Kent Gibson]

On Mon, Oct 14, 2024 at 10:13:59AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 14, 2024 at 4:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We currently only notify user-space about line config changes that are
> > > made from user-space. Any kernel config changes are not signalled.
> > >
> > > Let's improve the situation by emitting the events closer to the source.
> > > To that end let's call the relevant notifier chain from the functions
> > > setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
> > > gpiod_toggle_active_low(). This covers all the options that we can
> > > inform the user-space about. We ignore events which don't have
> > > corresponding flags exported to user-space on purpose - otherwise the
> > > user would see a config-changed event but the associated line-info would
> > > remain unchanged.
> > >
> > > gpiod_direction_output/input() can be called from any context.
> > > Fortunately, we now emit line state events using an atomic notifier
> > > chain, so it's no longer an issue.
> > >
> > > Let's also add non-notifying wrappers around the direction setters in
> > > order to not emit superfluous reconfigure events when requesting the
> > > lines as the initial config should be part of the request notification.
> > >
> >
> > So lines requested from kernel space will result in a LINE_REQUESTED and
> > then a series of LINE_CHANGED_CONFIG?  Whereas for lines requested from
> > userspace those will be collapsed into the one LINE_REQUESTED event?
>
> No, why? I added the notification about the request to
> gpiod_find_and_request() which is called by all the kernel getters and
> it already configures all the flags without emitting events and calls
> the non-notify variant of the direction setter. When a kernel driver
> requests a GPIO, I only see a single event UNLESS after the
> gpiod_get() call returns, it sets direction or changes config - just
> like user-space.
>

Oh, ok, I was assuming there could be others using gpiolib the same way
cdev does.  So cdev is the only one that takes the gpiod_request(),
gpiod_direction_output() etc path?  All good then.

Cheers,
Kent.