[libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Bartosz Golaszewski]

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

gpio-mockup relies on the GPIO devices being registered in module's __init
function and them being unregistered in __exit. This works with the GPIO
subsystem as it only takes a reference to the underlying owner module when
a GPIO descriptor is requested and not when the GPIO device is
instantiated.

This behavior may change in the future in the kernel so make the behavior
of libgpiomockup more correct and have it unbind all mockup devices over
sysfs before unloading the module.

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

diff --git a/tests/mockup/gpio-mockup.c b/tests/mockup/gpio-mockup.c
index fa27bd7..387e449 100644
--- a/tests/mockup/gpio-mockup.c
+++ b/tests/mockup/gpio-mockup.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2019 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <dirent.h>
 #include <errno.h>
 #include <libkmod.h>
 #include <libudev.h>
@@ -357,6 +358,51 @@ err_out:
 	return -1;
 }
 
+static int dir_filter(const struct dirent *entry)
+{
+	return !strncmp(entry->d_name, "gpio-mockup.", 12);
+}
+
+static void free_dirs(struct dirent **entries, size_t count)
+{
+	size_t i;
+
+	for (i = 0; i < count; i++)
+		free(entries[i]);
+	free(entries);
+}
+
+static int unbind_devices(void)
+{
+	struct dirent **entries;
+	int i, count, fd;
+	ssize_t ret;
+
+	count = scandir("/sys/bus/platform/drivers/gpio-mockup", &entries,
+			dir_filter, alphasort);
+	if (count < 0)
+		return -1;
+
+	fd = open("/sys/bus/platform/drivers/gpio-mockup/unbind", O_WRONLY);
+	if (fd < 0) {
+		free_dirs(entries, count);
+		return -1;
+	}
+
+	for (i = 0; i < count; i++) {
+		ret = write(fd, entries[i]->d_name, strlen(entries[i]->d_name));
+		if (ret < 0) {
+			close(fd);
+			free_dirs(entries, count);
+			return -1;
+		}
+	}
+
+	close(fd);
+	free_dirs(entries, count);
+	return 0;
+}
+
 EXPORT int gpio_mockup_remove(struct gpio_mockup *ctx)
 {
 	unsigned int i;
@@ -367,6 +413,10 @@ EXPORT int gpio_mockup_remove(struct gpio_mockup *ctx)
 		return -1;
 	}
 
+	rv = unbind_devices();
+	if (rv)
+		return -1;
+
 	rv = kmod_module_remove_module(ctx->module, 0);
 	if (rv)
 		return -1;
-- 
2.39.2

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Kent Gibson]

On Mon, Aug 21, 2023 at 05:33:39PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpio-mockup relies on the GPIO devices being registered in module's __init
> function and them being unregistered in __exit. This works with the GPIO
> subsystem as it only takes a reference to the underlying owner module when
> a GPIO descriptor is requested and not when the GPIO device is
> instantiated.
> 
> This behavior may change in the future in the kernel so make the behavior
> of libgpiomockup more correct and have it unbind all mockup devices over
> sysfs before unloading the module.
> 

Never knew that unbinding was even an option.
Maybe update gpio-mockup's documentation?

Just clarifying what the potential impact of the existing libgpiomockup
behaviour and future kernel behaviour is - the kernel may log errors but
otherwise correctly handle userspace unloading behaving badly?
So this patch is pre-emptory noise reduction?

Cheers,
Kent.

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Bartosz Golaszewski]

On Tue, Aug 22, 2023 at 2:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Aug 21, 2023 at 05:33:39PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpio-mockup relies on the GPIO devices being registered in module's __init
> > function and them being unregistered in __exit. This works with the GPIO
> > subsystem as it only takes a reference to the underlying owner module when
> > a GPIO descriptor is requested and not when the GPIO device is
> > instantiated.
> >
> > This behavior may change in the future in the kernel so make the behavior
> > of libgpiomockup more correct and have it unbind all mockup devices over
> > sysfs before unloading the module.
> >
>
> Never knew that unbinding was even an option.
> Maybe update gpio-mockup's documentation?
>

Yeah, I might once we agree on that reference counting patch.

> Just clarifying what the potential impact of the existing libgpiomockup
> behaviour and future kernel behaviour is - the kernel may log errors but
> otherwise correctly handle userspace unloading behaving badly?
> So this patch is pre-emptory noise reduction?
>

No, it's a bug-fix-in-advance. gpio-mockup will fail to unload (until
we unbind all devices anyway) if we couple the module's reference with
struct gpio_device. So will every driver that registers devices from
its module_init() function and tears them down in module_exit(). But
these drivers are wrong to do so in the first place and unloading them
sound like a rare thing to do anyway, so I'm willing to give it a try.

Bartosz

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Bartosz Golaszewski]

On Tue, Aug 22, 2023 at 9:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Aug 22, 2023 at 2:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, Aug 21, 2023 at 05:33:39PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > gpio-mockup relies on the GPIO devices being registered in module's __init
> > > function and them being unregistered in __exit. This works with the GPIO
> > > subsystem as it only takes a reference to the underlying owner module when
> > > a GPIO descriptor is requested and not when the GPIO device is
> > > instantiated.
> > >
> > > This behavior may change in the future in the kernel so make the behavior
> > > of libgpiomockup more correct and have it unbind all mockup devices over
> > > sysfs before unloading the module.
> > >
> >
> > Never knew that unbinding was even an option.
> > Maybe update gpio-mockup's documentation?
> >
>
> Yeah, I might once we agree on that reference counting patch.
>
> > Just clarifying what the potential impact of the existing libgpiomockup
> > behaviour and future kernel behaviour is - the kernel may log errors but
> > otherwise correctly handle userspace unloading behaving badly?
> > So this patch is pre-emptory noise reduction?
> >
>
> No, it's a bug-fix-in-advance. gpio-mockup will fail to unload (until
> we unbind all devices anyway) if we couple the module's reference with
> struct gpio_device. So will every driver that registers devices from
> its module_init() function and tears them down in module_exit(). But
> these drivers are wrong to do so in the first place and unloading them
> sound like a rare thing to do anyway, so I'm willing to give it a try.
>
> Bartosz

So what do you think Kent? Does it make sense to have it in v1.6? I
would need to make a new bugfix release but I have something else
queued anyway.

Bart

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Kent Gibson]

On Thu, Aug 24, 2023 at 09:32:06AM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 22, 2023 at 9:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Tue, Aug 22, 2023 at 2:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Mon, Aug 21, 2023 at 05:33:39PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > gpio-mockup relies on the GPIO devices being registered in module's __init
> > > > function and them being unregistered in __exit. This works with the GPIO
> > > > subsystem as it only takes a reference to the underlying owner module when
> > > > a GPIO descriptor is requested and not when the GPIO device is
> > > > instantiated.
> > > >
> > > > This behavior may change in the future in the kernel so make the behavior
> > > > of libgpiomockup more correct and have it unbind all mockup devices over
> > > > sysfs before unloading the module.
> > > >
> > >
> > > Never knew that unbinding was even an option.
> > > Maybe update gpio-mockup's documentation?
> > >
> >
> > Yeah, I might once we agree on that reference counting patch.
> >
> > > Just clarifying what the potential impact of the existing libgpiomockup
> > > behaviour and future kernel behaviour is - the kernel may log errors but
> > > otherwise correctly handle userspace unloading behaving badly?
> > > So this patch is pre-emptory noise reduction?
> > >
> >
> > No, it's a bug-fix-in-advance. gpio-mockup will fail to unload (until
> > we unbind all devices anyway) if we couple the module's reference with
> > struct gpio_device. So will every driver that registers devices from
> > its module_init() function and tears them down in module_exit(). But
> > these drivers are wrong to do so in the first place and unloading them
> > sound like a rare thing to do anyway, so I'm willing to give it a try.
> >
> > Bartosz
> 
> So what do you think Kent? Does it make sense to have it in v1.6? I
> would need to make a new bugfix release but I have something else
> queued anyway.
> 

If the plan is to change the kernel such that it will no longer unload
modules with bound devices then the patch totally makes sense.

Cheers,
Kent.

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Bartosz Golaszewski]

On Thu, Aug 24, 2023 at 9:36 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 09:32:06AM +0200, Bartosz Golaszewski wrote:
> > On Tue, Aug 22, 2023 at 9:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 2:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 21, 2023 at 05:33:39PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > gpio-mockup relies on the GPIO devices being registered in module's __init
> > > > > function and them being unregistered in __exit. This works with the GPIO
> > > > > subsystem as it only takes a reference to the underlying owner module when
> > > > > a GPIO descriptor is requested and not when the GPIO device is
> > > > > instantiated.
> > > > >
> > > > > This behavior may change in the future in the kernel so make the behavior
> > > > > of libgpiomockup more correct and have it unbind all mockup devices over
> > > > > sysfs before unloading the module.
> > > > >
> > > >
> > > > Never knew that unbinding was even an option.
> > > > Maybe update gpio-mockup's documentation?
> > > >
> > >
> > > Yeah, I might once we agree on that reference counting patch.
> > >
> > > > Just clarifying what the potential impact of the existing libgpiomockup
> > > > behaviour and future kernel behaviour is - the kernel may log errors but
> > > > otherwise correctly handle userspace unloading behaving badly?
> > > > So this patch is pre-emptory noise reduction?
> > > >
> > >
> > > No, it's a bug-fix-in-advance. gpio-mockup will fail to unload (until
> > > we unbind all devices anyway) if we couple the module's reference with
> > > struct gpio_device. So will every driver that registers devices from
> > > its module_init() function and tears them down in module_exit(). But
> > > these drivers are wrong to do so in the first place and unloading them
> > > sound like a rare thing to do anyway, so I'm willing to give it a try.
> > >
> > > Bartosz
> >
> > So what do you think Kent? Does it make sense to have it in v1.6? I
> > would need to make a new bugfix release but I have something else
> > queued anyway.
> >
>
> If the plan is to change the kernel such that it will no longer unload
> modules with bound devices then the patch totally makes sense.
>
> Cheers,
> Kent.

Linus has not commented on that yet and there's a thing I need to
check first (hopefully today) but I think it will make more sense.
This patch on the other hand is not incorrect even if the behavior
doesn't change. I will queue it.

Bart

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Linus Walleij]

On Thu, Aug 24, 2023 at 9:40 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Kent:
> > If the plan is to change the kernel such that it will no longer unload
> > modules with bound devices then the patch totally makes sense.
>
> Linus has not commented on that yet

[Fear Of Missing Out intensifies]

Is this some mail thread I should look at but didn't?

Or do you refer to Torvalds?

> and there's a thing I need to
> check first (hopefully today) but I think it will make more sense.
> This patch on the other hand is not incorrect even if the behavior
> doesn't change. I will queue it.

This patch is doing the right thing from a module management point
of view for sure.

Yours,
Linus Walleij

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Linus Walleij]

On Mon, Aug 21, 2023 at 5:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpio-mockup relies on the GPIO devices being registered in module's __init
> function and them being unregistered in __exit. This works with the GPIO
> subsystem as it only takes a reference to the underlying owner module when
> a GPIO descriptor is requested and not when the GPIO device is
> instantiated.
>
> This behavior may change in the future in the kernel so make the behavior
> of libgpiomockup more correct and have it unbind all mockup devices over
> sysfs before unloading the module.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [libgpiod v1.6.x][PATCH] tests: mockup: unbind mockup devices before unloading the module

[Bartosz Golaszewski]

On Thu, Aug 24, 2023 at 11:05 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Aug 24, 2023 at 9:40 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > Kent:
> > > If the plan is to change the kernel such that it will no longer unload
> > > modules with bound devices then the patch totally makes sense.
> >
> > Linus has not commented on that yet
>
> [Fear Of Missing Out intensifies]
>
> Is this some mail thread I should look at but didn't?
>

I was talking about this one:
https://lore.kernel.org/lkml/20230818190108.22031-1-brgl@bgdev.pl/T/

Bart

> Or do you refer to Torvalds?
>
> > and there's a thing I need to
> > check first (hopefully today) but I think it will make more sense.
> > This patch on the other hand is not incorrect even if the behavior
> > doesn't change. I will queue it.
>
> This patch is doing the right thing from a module management point
> of view for sure.
>
> Yours,
> Linus Walleij