| Message ID | 20230821153339.26305-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod,v1.6.x] tests: mockup: unbind mockup devices before unloading the module | expand |
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.
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
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
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.
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
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
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
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
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;