diff mbox series

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

Message ID 20241004-gpio-notify-in-kernel-events-v1-4-8ac29e1df4fe@linaro.org
State New
Headers show
Series gpio: notify user-space about config changes in the kernel | expand

Commit Message

Bartosz Golaszewski Oct. 4, 2024, 2:43 p.m. UTC 
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(-)

Comments

Kent Gibson Oct. 5, 2024, 3:46 a.m. UTC | #1 
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.
Bartosz Golaszewski Oct. 5, 2024, 9:34 a.m. UTC | #2 
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
Kent Gibson Oct. 5, 2024, 9:49 a.m. UTC | #3 
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.
diff mbox series

Patch

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;
 }