Message ID | 20230814112615.42448-1-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | [v1,1/1] gpiolib: sysfs: Do unexport GPIO when user asks for it | expand |
On Mon, Aug 14, 2023 at 1:19 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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 > Fixes: b0ce9ce408b6 ("gpiolib: Do not unexport GPIO on freeing") > Reported-by: Marek Behún <kabel@kernel.org> > Closes: https://lore.kernel.org/r/20230808102828.4a9eac09@dellmb > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Tested-by: Marek Behún <kabel@kernel.org> > --- > drivers/gpio/gpiolib-sysfs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c > index 530dfd19d7b5..50503a4525eb 100644 > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -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 >
On Mon, Aug 14, 2023 at 7:13 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Mon, Aug 14, 2023 at 1:19 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > 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. I'm fine with it, go ahead and apply with the change. Thank you!
On Mon, Aug 14, 2023 at 6:29 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Aug 14, 2023 at 7:13 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Mon, Aug 14, 2023 at 1:19 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > 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. > > I'm fine with it, go ahead and apply with the change. Thank you! > > -- > With Best Regards, > Andy Shevchenko Queued for fixes. Bart
On Mon, Aug 14, 2023 at 1:19 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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(). > > Fixes: b0ce9ce408b6 ("gpiolib: Do not unexport GPIO on freeing") > Reported-by: Marek Behún <kabel@kernel.org> > Closes: https://lore.kernel.org/r/20230808102828.4a9eac09@dellmb > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Tested-by: Marek Behún <kabel@kernel.org> Late to the show so patch already applied, but THANKS for drilling into it and smoking out this bug Andy. Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c index 530dfd19d7b5..50503a4525eb 100644 --- a/drivers/gpio/gpiolib-sysfs.c +++ b/drivers/gpio/gpiolib-sysfs.c @@ -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)