Skip to contentSkip to conversation

gpiolib: sysfs: Do unexport GPIO when user asks for it

PATCH In review Andy Shevchenko 4 participants 5 messages Aug 14, 2023 – Aug 16, 2023 GPIO

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 gpiolib: sysfs: Do unexport GPIO when user asks for it

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is not good and prone to regressions. Fix it by explicitly calling gpiod_unexport().

---
 drivers/gpio/gpiolib-sysfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib-sysfs.c +5 -2
index 530dfd19d7b5..50503a4525eb 100644
@@ -515,8 +515,9 @@ static ssize_t unexport_store(const struct class *class,
 	 * they may be undone on its behalf too.
 	 */
 	if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) {
+		gpiod_unexport(desc);
+		gpiod_free(desc);
 		status = 0;
-		gpiod_free(desc);
 	}
 done:
 	if (status)
@@ -781,8 +782,10 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 	mutex_unlock(&sysfs_lock);
 
 	/* unregister gpiod class devices owned by sysfs */
-	for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS)
+	for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS) {
+		gpiod_unexport(desc);
 		gpiod_free(desc);
+	}
 }
 
 static int __init gpiolib_sysfs_init(void)
Signature · 1 line
2.40.0.1.gaa8946217a0b
#1 lore raw
Quoted 5 lines · Andy Shevchenko

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is not good and prone to regressions. Fix it by explicitly calling gpiod_unexport().

I wouldn't say it's prone to regressions, it's literally just that gpiod_free() should not deal with sysfs.

How about that for commit message (I can change it when applying):

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is logically incorrect as core gpiolib should not deal with sysfs so instead of restoring it, let's call gpiod_unexport() from sysfs code.

Bart

Quoted 39 lines
---
 drivers/gpio/gpiolib-sysfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib-sysfs.c +5 -3
index 530dfd19d7b5..50503a4525eb 100644
@@ -515,8 +515,9 @@ static ssize_t unexport_store(const struct class *class,
         * they may be undone on its behalf too.
         */
        if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) {
+               gpiod_unexport(desc);
+               gpiod_free(desc);
                status = 0;
-               gpiod_free(desc);
        }
 done:
        if (status)
@@ -781,8 +782,10 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
        mutex_unlock(&sysfs_lock);
 
        /* unregister gpiod class devices owned by sysfs */
-       for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS)
+       for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS) {
+               gpiod_unexport(desc);
                gpiod_free(desc);
+       }
 }
 
 static int __init gpiolib_sysfs_init(void)
--

2.40.0.1.gaa8946217a0b

#2 lore raw
Quoted 11 lines · Bartosz Golaszewski
Quoted 5 lines · Andy Shevchenko

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is not good and prone to regressions. Fix it by explicitly calling gpiod_unexport().

I wouldn't say it's prone to regressions, it's literally just that gpiod_free() should not deal with sysfs.

Quoted 6 lines

How about that for commit message (I can change it when applying):

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is logically incorrect as core gpiolib should not deal with sysfs so instead of restoring it, let's call gpiod_unexport() from sysfs code.

I'm fine with it, go ahead and apply with the change. Thank you!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#3 lore raw
Quoted 20 lines · Andy Shevchenko
Quoted 11 lines · Bartosz Golaszewski
Quoted 5 lines · Andy Shevchenko

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is not good and prone to regressions. Fix it by explicitly calling gpiod_unexport().

I wouldn't say it's prone to regressions, it's literally just that gpiod_free() should not deal with sysfs.

Quoted 6 lines

How about that for commit message (I can change it when applying):

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is logically incorrect as core gpiolib should not deal with sysfs so instead of restoring it, let's call gpiod_unexport() from sysfs code.

Quoted 5 lines

I'm fine with it, go ahead and apply with the change. Thank you!

--
With Best Regards,
Andy Shevchenko

Queued for fixes.

Bart

#4 lore raw
In reply to Andy Shevchenko
Quoted 9 lines · Andy Shevchenko

It seems that sysfs interface implicitly relied on the gpiod_free() to unexport the line. This is not good and prone to regressions. Fix it by explicitly calling gpiod_unexport().

Late to the show so patch already applied, but THANKS for drilling into it and smoking out this bug Andy.

Yours,
Linus Walleij