tests: mockup: unbind mockup devices before unloading the module

PATCH In review Bartosz Golaszewski 3 participants 9 messages Aug 21, 2023 – Aug 24, 2023 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-08-21 15:33 UTC #0 lore raw

PATCH 1/1 tests: mockup: unbind mockup devices before unloading the module

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 tests/mockup/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

tests/mockup/gpio-mockup.c +50 -0

index fa27bd7..387e449 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2019 Bartosz Golaszewski <[email protected]>
  */
 
+#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;

Signature · 1 line

2.39.2

Kent Gibson

2023-08-22 00:22 UTC #1 lore raw

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 4 lines

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.

Bartosz Golaszewski

author

2023-08-22 07:08 UTC #2 lore raw

Quoted 10 lines · Kent Gibson

Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.

Quoted 8 lines

Quoted 4 lines
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.

Quoted 5 lines

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

Bartosz Golaszewski

author

2023-08-24 07:33 UTC #3 lore raw

Quoted 29 lines · Bartosz Golaszewski

Quoted 10 lines · Kent Gibson
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 5 lines
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?

Quoted 8 lines

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

Kent Gibson

2023-08-24 07:37 UTC #4 lore raw

Quoted 37 lines · Bartosz Golaszewski

Quoted 36 lines · Bartosz Golaszewski
Quoted 10 lines · Kent Gibson
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 5 lines
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.

Quoted 6 lines

Quoted 1 line
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.

Bartosz Golaszewski

author

2023-08-24 07:42 UTC #5 lore raw

Quoted 46 lines · Kent Gibson

Quoted 37 lines · Bartosz Golaszewski
Quoted 36 lines · Bartosz Golaszewski
Quoted 10 lines · Kent Gibson
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 1 line
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.

Quoted 5 lines

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

Linus Walleij

2023-08-24 09:06 UTC #6 lore raw

Quoted 5 lines · Bartosz Golaszewski

Kent:
Quoted 2 lines
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?

Quoted 4 lines

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

Bartosz Golaszewski

author

2023-08-24 09:42 UTC #7 lore raw

Quoted 12 lines · Linus Walleij

Quoted 5 lines · Bartosz Golaszewski
Kent:
Quoted 2 lines
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/[email protected]/T/

Bart

Quoted 12 lines

Or do you refer to Torvalds?
Quoted 4 lines
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

Linus Walleij

Reviewed-by on v1 1/1

2023-08-24 09:06 UTC #8 lore raw

In reply to Bartosz Golaszewski

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 5 lines

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 <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij