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

[PATCH 0/5] 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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (5):
      gpiolib: use v2 defines for line state change events
      gpiolib: unify two loops initializing GPIO descriptors
      gpio: cdev: update flags at once when reconfiguring from user-space
      gpiolib: simplify notifying user-space about line requests
      gpiolib: notify user-space about in-kernel line state changes

 drivers/gpio/gpiolib-cdev.c |  88 +++++++++++++++++------------------
 drivers/gpio/gpiolib.c      | 110 ++++++++++++++++++++++++++++++++++++--------
 drivers/gpio/gpiolib.h      |   9 +++-
 3 files changed, 141 insertions(+), 66 deletions(-)
---
base-commit: 58ca61c1a866bfdaa5e19fb19a2416764f847d75
change-id: 20240924-gpio-notify-in-kernel-events-07cd912153e8

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

[PATCH 1/5] gpiolib: use v2 defines for line state change events

[Bartosz Golaszewski]

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

We should only use v2 defines for line state change events. They will get
tranlated to v1 if needed by gpio_v2_line_info_changed_to_v1().

This isn't really a functional change as they have the same values but
let's do it for consistency.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6a5c28babf35..c1051d77fb88 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2434,7 +2434,7 @@ static void gpiod_free_commit(struct gpio_desc *desc)
 		desc_set_label(desc, NULL);
 		WRITE_ONCE(desc->flags, flags);
 
-		gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_RELEASED);
 	}
 }
 
@@ -4366,7 +4366,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 		return ERR_PTR(ret);
 	}
 
-	gpiod_line_state_notify(desc, GPIOLINE_CHANGED_REQUESTED);
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
 
 	return desc;
 }

-- 
2.43.0

[PATCH 2/5] gpiolib: unify two loops initializing GPIO descriptors

[Bartosz Golaszewski]

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

We currently iterate over the descriptors owned by the GPIO device we're
adding twice with the first loop just setting the gdev pointer. It's not
used anywhere between this and the second loop so just drop the first
one and move the assignment to the second.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c1051d77fb88..97346b746ef5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1026,9 +1026,6 @@ 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;
-
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 
@@ -1058,6 +1055,8 @@ 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));

-- 
2.43.0

Re: [PATCH 2/5] gpiolib: unify two loops initializing GPIO descriptors

[Linus Walleij]

On Fri, Oct 4, 2024 at 4:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We currently iterate over the descriptors owned by the GPIO device we're
> adding twice with the first loop just setting the gdev pointer. It's not
> used anywhere between this and the second loop so just drop the first
> one and move the assignment to the second.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH 3/5] gpio: cdev: update flags at once when reconfiguring from user-space

[Bartosz Golaszewski]

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

Make updating the descriptor flags when reconfiguring from user-space
consistent with the rest of the codebase: read the current state
atomically, update it according to user's instructions and write it back
atomically as well.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 6113a283c34a..b0050250ac3a 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -143,18 +143,22 @@ static int linehandle_validate_flags(u32 flags)
 
 static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp)
 {
-	assign_bit(FLAG_ACTIVE_LOW, flagsp,
+	unsigned long flags = READ_ONCE(*flagsp);
+
+	assign_bit(FLAG_ACTIVE_LOW, &flags,
 		   lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
-	assign_bit(FLAG_OPEN_DRAIN, flagsp,
+	assign_bit(FLAG_OPEN_DRAIN, &flags,
 		   lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
-	assign_bit(FLAG_OPEN_SOURCE, flagsp,
+	assign_bit(FLAG_OPEN_SOURCE, &flags,
 		   lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
-	assign_bit(FLAG_PULL_UP, flagsp,
+	assign_bit(FLAG_PULL_UP, &flags,
 		   lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
-	assign_bit(FLAG_PULL_DOWN, flagsp,
+	assign_bit(FLAG_PULL_DOWN, &flags,
 		   lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
-	assign_bit(FLAG_BIAS_DISABLE, flagsp,
+	assign_bit(FLAG_BIAS_DISABLE, &flags,
 		   lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
+
+	WRITE_ONCE(*flagsp, flags);
 }
 
 static long linehandle_set_config(struct linehandle_state *lh,
@@ -1348,38 +1352,42 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
 	return 0;
 }
 
-static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
+static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
 						    unsigned long *flagsp)
 {
-	assign_bit(FLAG_ACTIVE_LOW, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
+	unsigned long flags = READ_ONCE(*flagsp);
 
-	if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
-		set_bit(FLAG_IS_OUT, flagsp);
-	else if (flags & GPIO_V2_LINE_FLAG_INPUT)
-		clear_bit(FLAG_IS_OUT, flagsp);
+	assign_bit(FLAG_ACTIVE_LOW, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
 
-	assign_bit(FLAG_EDGE_RISING, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_EDGE_RISING);
-	assign_bit(FLAG_EDGE_FALLING, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_EDGE_FALLING);
+	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
+		set_bit(FLAG_IS_OUT, &flags);
+	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
+		clear_bit(FLAG_IS_OUT, &flags);
 
-	assign_bit(FLAG_OPEN_DRAIN, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
-	assign_bit(FLAG_OPEN_SOURCE, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
+	assign_bit(FLAG_EDGE_RISING, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_EDGE_RISING);
+	assign_bit(FLAG_EDGE_FALLING, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_EDGE_FALLING);
 
-	assign_bit(FLAG_PULL_UP, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
-	assign_bit(FLAG_PULL_DOWN, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
-	assign_bit(FLAG_BIAS_DISABLE, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
+	assign_bit(FLAG_OPEN_DRAIN, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
+	assign_bit(FLAG_OPEN_SOURCE, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
 
-	assign_bit(FLAG_EVENT_CLOCK_REALTIME, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME);
-	assign_bit(FLAG_EVENT_CLOCK_HTE, flagsp,
-		   flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE);
+	assign_bit(FLAG_PULL_UP, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
+	assign_bit(FLAG_PULL_DOWN, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
+	assign_bit(FLAG_BIAS_DISABLE, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
+
+	assign_bit(FLAG_EVENT_CLOCK_REALTIME, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME);
+	assign_bit(FLAG_EVENT_CLOCK_HTE, &flags,
+		   lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE);
+
+	WRITE_ONCE(*flagsp, flags);
 }
 
 static long linereq_get_values(struct linereq *lr, void __user *ip)

-- 
2.43.0

Re: [PATCH 3/5] gpio: cdev: update flags at once when reconfiguring from user-space

[Linus Walleij]

On Fri, Oct 4, 2024 at 4:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Make updating the descriptor flags when reconfiguring from user-space
> consistent with the rest of the codebase: read the current state
> atomically, update it according to user's instructions and write it back
> atomically as well.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH 4/5] gpiolib: simplify notifying user-space about line requests

[Bartosz Golaszewski]

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

Instead of emitting the line state change event on request in three
different places, just do it once, closer to the source: in
gpiod_request_commit().

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b0050250ac3a..f614a981253d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 				goto out_free_lh;
 		}
 
-		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
 		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
 			offset);
 	}
@@ -1842,8 +1840,6 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 
 		lr->lines[i].edflags = edflags;
 
-		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
 		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
 			offset);
 	}
@@ -2234,8 +2230,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		goto out_free_le;
 
-	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
 	irq = gpiod_to_irq(desc);
 	if (irq <= 0) {
 		ret = -ENODEV;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 97346b746ef5..190ece4d6850 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2345,6 +2345,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (ret)
 		goto out_clear_bit;
 
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+
 	return 0;
 
 out_clear_bit:
@@ -4365,8 +4367,6 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 		return ERR_PTR(ret);
 	}
 
-	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
 	return desc;
 }
 

-- 
2.43.0

Re: [PATCH 4/5] gpiolib: simplify notifying user-space about line requests

[Kent Gibson]

On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Instead of emitting the line state change event on request in three
> different places, just do it once, closer to the source: in
> gpiod_request_commit().
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib-cdev.c | 6 ------
>  drivers/gpio/gpiolib.c      | 4 ++--
>  2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index b0050250ac3a..f614a981253d 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>  				goto out_free_lh;
>  		}
>
> -		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
> -
>  		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
>  			offset);

This moves the notify to before the desc->flags have been set.
So the notified will now see the flags as previously set, not what they
have been requested as.

That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG
when the flags are set, but that is not done here and you explicitly don't
notify from here in patch 5 when you add notifying to gpiod_direction_output()
etc.

Cheers,
Kent.

Re: [PATCH 4/5] gpiolib: simplify notifying user-space about line requests

[Bartosz Golaszewski]

On Sat, Oct 5, 2024 at 5:46 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Instead of emitting the line state change event on request in three
> > different places, just do it once, closer to the source: in
> > gpiod_request_commit().
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 6 ------
> >  drivers/gpio/gpiolib.c      | 4 ++--
> >  2 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index b0050250ac3a..f614a981253d 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> >                               goto out_free_lh;
> >               }
> >
> > -             gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
> > -
> >               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> >                       offset);
>
> This moves the notify to before the desc->flags have been set.
> So the notified will now see the flags as previously set, not what they
> have been requested as.
>

Ah, I got fooled by the libgpiod tests passing. I guess we should
cover that first in tests-kernel-uapi.c.

> That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG
> when the flags are set, but that is not done here and you explicitly don't
> notify from here in patch 5 when you add notifying to gpiod_direction_output()
> etc.
>

IMO it doesn't make sense to always emit REQUESTED and CONFIG_CHANGED
events together. The initial config should be part of the request
event. I'll get back to the drawing board.

Bart

Re: [PATCH 4/5] gpiolib: simplify notifying user-space about line requests

[Kent Gibson]

On Sat, Oct 05, 2024 at 11:34:26AM +0200, Bartosz Golaszewski wrote:
> On Sat, Oct 5, 2024 at 5:46 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Instead of emitting the line state change event on request in three
> > > different places, just do it once, closer to the source: in
> > > gpiod_request_commit().
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 6 ------
> > >  drivers/gpio/gpiolib.c      | 4 ++--
> > >  2 files changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index b0050250ac3a..f614a981253d 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > >                               goto out_free_lh;
> > >               }
> > >
> > > -             gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
> > > -
> > >               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > >                       offset);
> >
> > This moves the notify to before the desc->flags have been set.
> > So the notified will now see the flags as previously set, not what they
> > have been requested as.
> >
>
> Ah, I got fooled by the libgpiod tests passing. I guess we should
> cover that first in tests-kernel-uapi.c.
>
> > That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG
> > when the flags are set, but that is not done here and you explicitly don't
> > notify from here in patch 5 when you add notifying to gpiod_direction_output()
> > etc.
> >
>
> IMO it doesn't make sense to always emit REQUESTED and CONFIG_CHANGED
> events together. The initial config should be part of the request
> event. I'll get back to the drawing board.
>

Oh, I agree - that "might" is doing a lot of heavy lifting - there should
only be the one event.

Cheers,
Kent.

[PATCH 5/5] 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.

There is a problem with gpiod_direction_output/input(), namely the fact
that they can be called both from sleeping as well as atomic context. We
cannot call the blocking notifier from atomic and we cannot switch to
atomic notifier because the pinctrl functions we call higher up the stack
take a mutex. Let's instead use a workqueue and schedule a task to emit
the event from process context on the unbound system queue for minimal
latencies.

In tests, I typically get around 5 microseconds between scheduling the
work and it being executed. That could of course differ during heavy
system load but in general it should be enough to not miss direction
change events which typically are not in hot paths.

Let's also add non-notifying wrappers around the direction setters in
order to not emit superfluous reconfigure events when requesting the
lines.

We can also now make gpiod_line_state_notify() static.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f614a981253d..bb00c9c29613 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;
 		}
@@ -1566,8 +1564,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;
 }
@@ -1824,11 +1820,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 190ece4d6850..281502923482 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -870,6 +870,26 @@ static void gpiochip_set_data(struct gpio_chip *gc, void *data)
 	gc->gpiodev->data = data;
 }
 
+static void gpiod_line_state_notify(struct gpio_desc *desc,
+				    unsigned long action)
+{
+	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
+				     action, desc);
+}
+
+static void gpiod_config_change_func(struct work_struct *work)
+{
+	struct gpio_desc *desc = container_of(work, struct gpio_desc,
+					      line_state_work);
+
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+}
+
+static void gpiod_line_config_change_notify(struct gpio_desc *desc)
+{
+	queue_work(system_unbound_wq, &desc->line_state_work);
+}
+
 /**
  * gpiochip_get_data() - get per-subdriver data for the chip
  * @gc: GPIO chip
@@ -1064,6 +1084,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			assign_bit(FLAG_IS_OUT,
 				   &desc->flags, !gc->direction_input);
 		}
+
+		INIT_WORK(&desc->line_state_work, gpiod_config_change_func);
 	}
 
 	ret = of_gpiochip_add(gc);
@@ -2673,6 +2695,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_config_change_notify(desc);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_input);
+
+int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 {
 	int ret = 0;
 
@@ -2720,7 +2754,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 +2815,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_config_change_notify(desc);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
 
@@ -2801,6 +2841,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_config_change_notify(desc);
+
+	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 +2915,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 +2993,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 +3087,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 +3732,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);
 
@@ -4087,12 +4166,6 @@ int gpiod_set_array_value_cansleep(unsigned int array_size,
 }
 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);
-}
-
 /**
  * gpiod_add_lookup_table() - register GPIO device consumers
  * @table: table of consumers to register
@@ -4537,10 +4610,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 067197d61d57..e07e053c7860 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"
 
@@ -137,6 +138,9 @@ struct gpio_array {
 	for_each_gpio_desc(gc, desc)					\
 		if (!test_bit(flag, &desc->flags)) {} else
 
+int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value);
+int gpiod_direction_input_nonotify(struct gpio_desc *desc);
+
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
@@ -150,8 +154,6 @@ 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);
-
 struct gpio_desc_label {
 	struct rcu_head rh;
 	char str[];
@@ -165,6 +167,8 @@ struct gpio_desc_label {
  * @label:		Name of the consumer
  * @name:		Line name
  * @hog:		Pointer to the device node that hogs this line (if any)
+ * @line_state_work:	Used to schedule line state updates to user-space from
+ *			atomic context.
  *
  * These are obtained using gpiod_get() and are preferable to the old
  * integer-based handles.
@@ -202,6 +206,7 @@ struct gpio_desc {
 #ifdef CONFIG_OF_DYNAMIC
 	struct device_node	*hog;
 #endif
+	struct work_struct	line_state_work;
 };
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)

-- 
2.43.0

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

[Kent Gibson]

On Fri, Oct 04, 2024 at 04:43:26PM +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.
>
> There is a problem with gpiod_direction_output/input(), namely the fact
> that they can be called both from sleeping as well as atomic context. We
> cannot call the blocking notifier from atomic and we cannot switch to
> atomic notifier because the pinctrl functions we call higher up the stack
> take a mutex. Let's instead use a workqueue and schedule a task to emit
> the event from process context on the unbound system queue for minimal
> latencies.
>

So now there is a race between the state of the desc changing and the
notified reading it?

Cheers,
Kent.

> In tests, I typically get around 5 microseconds between scheduling the
> work and it being executed. That could of course differ during heavy
> system load but in general it should be enough to not miss direction
> change events which typically are not in hot paths.
>
> Let's also add non-notifying wrappers around the direction setters in
> order to not emit superfluous reconfigure events when requesting the
> lines.
>
> We can also now make gpiod_line_state_notify() static.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

[Bartosz Golaszewski]

On Sat, Oct 5, 2024 at 9:46 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Oct 04, 2024 at 04:43:26PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > There is a problem with gpiod_direction_output/input(), namely the fact
> > that they can be called both from sleeping as well as atomic context. We
> > cannot call the blocking notifier from atomic and we cannot switch to
> > atomic notifier because the pinctrl functions we call higher up the stack
> > take a mutex. Let's instead use a workqueue and schedule a task to emit
> > the event from process context on the unbound system queue for minimal
> > latencies.
> >
>
> So now there is a race between the state of the desc changing and the
> notified reading it?
>

Theoretically? Well, yes... In practice I don't think this would
matter. But I understand the concern and won't insist if it's a
deal-breaker for you.

Ideally we'd switch to an atomic notifier but I don't have a good idea
on how to handle pinctrl_gpio_can_use_line(). It digs deep into the
pinctrl code and it's all synchronized with a mutex. Unlike GPIO, it
doesn't make any sense to spend days converting pinctrl to SRCU for a
single corner-case.

I wanted to use in_atomic() to determine whether we can emit the event
immediately or (if we're in interrupt or with a spinlock taken) we
should use a workqueue as a fallback but checkpatch.pl is very adamant
about it being an error (in capital reds).

Bart

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

[Kent Gibson]

On Sat, Oct 05, 2024 at 11:42:34AM +0200, Bartosz Golaszewski wrote:
> On Sat, Oct 5, 2024 at 9:46 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Oct 04, 2024 at 04:43:26PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > There is a problem with gpiod_direction_output/input(), namely the fact
> > > that they can be called both from sleeping as well as atomic context. We
> > > cannot call the blocking notifier from atomic and we cannot switch to
> > > atomic notifier because the pinctrl functions we call higher up the stack
> > > take a mutex. Let's instead use a workqueue and schedule a task to emit
> > > the event from process context on the unbound system queue for minimal
> > > latencies.
> > >
> >
> > So now there is a race between the state of the desc changing and the
> > notified reading it?
> >
>
> Theoretically? Well, yes... In practice I don't think this would
> matter. But I understand the concern and won't insist if it's a
> deal-breaker for you.
>

I don't like that correctness depends on timing, so this is a deal
breaker for me as it stands.  I would like to see the relevant state passed
via the notifier chain, rather than assuming it can be pulled from the desc
when the notifier is eventually called.

Cheers,
Kent.

> Ideally we'd switch to an atomic notifier but I don't have a good idea
> on how to handle pinctrl_gpio_can_use_line(). It digs deep into the
> pinctrl code and it's all synchronized with a mutex. Unlike GPIO, it
> doesn't make any sense to spend days converting pinctrl to SRCU for a
> single corner-case.
>
> I wanted to use in_atomic() to determine whether we can emit the event
> immediately or (if we're in interrupt or with a spinlock taken) we
> should use a workqueue as a fallback but checkpatch.pl is very adamant
> about it being an error (in capital reds).
>
> Bart

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

[Bartosz Golaszewski]

On Sat, Oct 5, 2024 at 11:54 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sat, Oct 05, 2024 at 11:42:34AM +0200, Bartosz Golaszewski wrote:
> > On Sat, Oct 5, 2024 at 9:46 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, Oct 04, 2024 at 04:43:26PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > There is a problem with gpiod_direction_output/input(), namely the fact
> > > > that they can be called both from sleeping as well as atomic context. We
> > > > cannot call the blocking notifier from atomic and we cannot switch to
> > > > atomic notifier because the pinctrl functions we call higher up the stack
> > > > take a mutex. Let's instead use a workqueue and schedule a task to emit
> > > > the event from process context on the unbound system queue for minimal
> > > > latencies.
> > > >
> > >
> > > So now there is a race between the state of the desc changing and the
> > > notified reading it?
> > >
> >
> > Theoretically? Well, yes... In practice I don't think this would
> > matter. But I understand the concern and won't insist if it's a
> > deal-breaker for you.
> >
>
> I don't like that correctness depends on timing, so this is a deal
> breaker for me as it stands.  I would like to see the relevant state passed
> via the notifier chain, rather than assuming it can be pulled from the desc
> when the notifier is eventually called.
>

We could potentially still use the workqueue but atomically allocate
the work_struct in any context, store the descriptor data, timestamp
etc. (except the info from pinctrl which is rarely modified and would
be retrieved just before emitting the event in process context) in it
and pass it to the workqueue which would then put the data into the
kfifo and free the work_struct. We can enforce ordering of work
execution so we wouldn't mangle them, userspace would still see the
events with correct timestamps and in the right order. Does this sound
like something viable?

Bart

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

[Kent Gibson]

On Sat, Oct 05, 2024 at 08:45:17PM +0200, Bartosz Golaszewski wrote:
> On Sat, Oct 5, 2024 at 11:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Sat, Oct 05, 2024 at 11:42:34AM +0200, Bartosz Golaszewski wrote:
> > > On Sat, Oct 5, 2024 at 9:46 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 04, 2024 at 04:43:26PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > There is a problem with gpiod_direction_output/input(), namely the fact
> > > > > that they can be called both from sleeping as well as atomic context. We
> > > > > cannot call the blocking notifier from atomic and we cannot switch to
> > > > > atomic notifier because the pinctrl functions we call higher up the stack
> > > > > take a mutex. Let's instead use a workqueue and schedule a task to emit
> > > > > the event from process context on the unbound system queue for minimal
> > > > > latencies.
> > > > >
> > > >
> > > > So now there is a race between the state of the desc changing and the
> > > > notified reading it?
> > > >
> > >
> > > Theoretically? Well, yes... In practice I don't think this would
> > > matter. But I understand the concern and won't insist if it's a
> > > deal-breaker for you.
> > >
> >
> > I don't like that correctness depends on timing, so this is a deal
> > breaker for me as it stands.  I would like to see the relevant state passed
> > via the notifier chain, rather than assuming it can be pulled from the desc
> > when the notifier is eventually called.
> >
>
> We could potentially still use the workqueue but atomically allocate
> the work_struct in any context, store the descriptor data, timestamp
> etc. (except the info from pinctrl which is rarely modified and would
> be retrieved just before emitting the event in process context) in it
> and pass it to the workqueue which would then put the data into the
> kfifo and free the work_struct. We can enforce ordering of work
> execution so we wouldn't mangle them, userspace would still see the
> events with correct timestamps and in the right order. Does this sound
> like something viable?
>

That is what I had in mind.  The passed/queued state only has to be the
fields subject to change, which isn't all that much.  You have to keep any
queues finite and deal with potential overflows, though that should be
unlikely if they are reasonably sized - and as you noted earlier this is not
a hot path so even "reasonably sized" is probably going to be small.

Cheers,
Kent.

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

[Kent Gibson]

On Fri, Oct 04, 2024 at 04:43:21PM +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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Patches 1-3 look good to me.

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

for those three.

> ---
> Bartosz Golaszewski (5):
>       gpiolib: use v2 defines for line state change events
>       gpiolib: unify two loops initializing GPIO descriptors
>       gpio: cdev: update flags at once when reconfiguring from user-space
>       gpiolib: simplify notifying user-space about line requests
>       gpiolib: notify user-space about in-kernel line state changes
>
>  drivers/gpio/gpiolib-cdev.c |  88 +++++++++++++++++------------------
>  drivers/gpio/gpiolib.c      | 110 ++++++++++++++++++++++++++++++++++++--------
>  drivers/gpio/gpiolib.h      |   9 +++-
>  3 files changed, 141 insertions(+), 66 deletions(-)
> ---
> base-commit: 58ca61c1a866bfdaa5e19fb19a2416764f847d75
> change-id: 20240924-gpio-notify-in-kernel-events-07cd912153e8
>
> Best regards,
> --
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>

Re: (subset) [PATCH 0/5] gpio: notify user-space about config changes in the kernel

[Bartosz Golaszewski]

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


On Fri, 04 Oct 2024 16:43:21 +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.
> 
> 

Applied, thanks!

[1/5] gpiolib: use v2 defines for line state change events
      commit: ee194b12bf9a2f2ed57b9c5bc7a5f221f7f4a06f
[2/5] gpiolib: unify two loops initializing GPIO descriptors
      commit: fa17f749ee5bc6afdaa9e0ddbe6a816b490dad7d
[3/5] gpio: cdev: update flags at once when reconfiguring from user-space
      commit: b7adfb6076ff0c1ebbde56d1903daa3d07db92c5

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