[PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

If a user tries to export or unexport an invalid gpio (e.g. gpio number
>= ARCH_NR_GPIOS), gpio_to_desc() will trigger a register dump through a
WARN() call. Avoid this rather scary error message by first checking the
validity of the gpio number before calling gpio_to_desc() in
export_store() and unexport_store(). The user gets a normal error
message to signal his/her error without any possible confusion with a
kernel bug.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/gpio/gpiolib-sysfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 728f6c687182..b6fd0d82757a 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -3,6 +3,7 @@
 #include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/sysfs.h>
+#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
@@ -456,14 +457,15 @@ static ssize_t export_store(struct class *class,
 				const char *buf, size_t len)
 {
 	long			gpio;
-	struct gpio_desc	*desc;
+	struct gpio_desc	*desc = NULL;
 	int			status;
 
 	status = kstrtol(buf, 0, &gpio);
 	if (status < 0)
 		goto done;
 
-	desc = gpio_to_desc(gpio);
+	if (gpio_is_valid(gpio))
+		desc = gpio_to_desc(gpio);
 	/* reject invalid GPIOs */
 	if (!desc) {
 		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
@@ -503,14 +505,15 @@ static ssize_t unexport_store(struct class *class,
 				const char *buf, size_t len)
 {
 	long			gpio;
-	struct gpio_desc	*desc;
+	struct gpio_desc	*desc = NULL;
 	int			status;
 
 	status = kstrtol(buf, 0, &gpio);
 	if (status < 0)
 		goto done;
 
-	desc = gpio_to_desc(gpio);
+	if (gpio_is_valid(gpio))
+		desc = gpio_to_desc(gpio);
 	/* reject bogus commands (gpio_unexport ignores them) */
 	if (!desc) {
 		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
-- 
2.28.0

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Bartosz Golaszewski]

On Wed, Nov 4, 2020 at 12:53 PM Damien Le Moal <damien.lemoal@wdc.com> wrote:
>
> If a user tries to export or unexport an invalid gpio (e.g. gpio number
> >= ARCH_NR_GPIOS), gpio_to_desc() will trigger a register dump through a
> WARN() call. Avoid this rather scary error message by first checking the
> validity of the gpio number before calling gpio_to_desc() in
> export_store() and unexport_store(). The user gets a normal error
> message to signal his/her error without any possible confusion with a
> kernel bug.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 728f6c687182..b6fd0d82757a 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -3,6 +3,7 @@
>  #include <linux/mutex.h>
>  #include <linux/device.h>
>  #include <linux/sysfs.h>
> +#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/interrupt.h>
> @@ -456,14 +457,15 @@ static ssize_t export_store(struct class *class,
>                                 const char *buf, size_t len)
>  {
>         long                    gpio;
> -       struct gpio_desc        *desc;
> +       struct gpio_desc        *desc = NULL;
>         int                     status;
>
>         status = kstrtol(buf, 0, &gpio);
>         if (status < 0)
>                 goto done;
>
> -       desc = gpio_to_desc(gpio);
> +       if (gpio_is_valid(gpio))
> +               desc = gpio_to_desc(gpio);
>         /* reject invalid GPIOs */
>         if (!desc) {
>                 pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
> @@ -503,14 +505,15 @@ static ssize_t unexport_store(struct class *class,
>                                 const char *buf, size_t len)
>  {
>         long                    gpio;
> -       struct gpio_desc        *desc;
> +       struct gpio_desc        *desc = NULL;
>         int                     status;
>
>         status = kstrtol(buf, 0, &gpio);
>         if (status < 0)
>                 goto done;
>
> -       desc = gpio_to_desc(gpio);
> +       if (gpio_is_valid(gpio))
> +               desc = gpio_to_desc(gpio);
>         /* reject bogus commands (gpio_unexport ignores them) */
>         if (!desc) {
>                 pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
> --
> 2.28.0
>

How about we change that to an unconditional WARN() everytime the user
tries to export a GPIO over sysfs so that people switch over to the
character device?

I'm joking a bit but I think it's time to start discouraging people
from using sysfs and a warning may be a good start.

Bartosz

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

On Fri, 2020-11-06 at 10:27 +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 4, 2020 at 12:53 PM Damien Le Moal <damien.lemoal@wdc.com> wrote:
> > 
> > If a user tries to export or unexport an invalid gpio (e.g. gpio number
> > > = ARCH_NR_GPIOS), gpio_to_desc() will trigger a register dump through a
> > WARN() call. Avoid this rather scary error message by first checking the
> > validity of the gpio number before calling gpio_to_desc() in
> > export_store() and unexport_store(). The user gets a normal error
> > message to signal his/her error without any possible confusion with a
> > kernel bug.
> > 
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > ---
> >  drivers/gpio/gpiolib-sysfs.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index 728f6c687182..b6fd0d82757a 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -3,6 +3,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/device.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/gpio.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/interrupt.h>
> > @@ -456,14 +457,15 @@ static ssize_t export_store(struct class *class,
> >                                 const char *buf, size_t len)
> >  {
> >         long                    gpio;
> > -       struct gpio_desc        *desc;
> > +       struct gpio_desc        *desc = NULL;
> >         int                     status;
> > 
> >         status = kstrtol(buf, 0, &gpio);
> >         if (status < 0)
> >                 goto done;
> > 
> > -       desc = gpio_to_desc(gpio);
> > +       if (gpio_is_valid(gpio))
> > +               desc = gpio_to_desc(gpio);
> >         /* reject invalid GPIOs */
> >         if (!desc) {
> >                 pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
> > @@ -503,14 +505,15 @@ static ssize_t unexport_store(struct class *class,
> >                                 const char *buf, size_t len)
> >  {
> >         long                    gpio;
> > -       struct gpio_desc        *desc;
> > +       struct gpio_desc        *desc = NULL;
> >         int                     status;
> > 
> >         status = kstrtol(buf, 0, &gpio);
> >         if (status < 0)
> >                 goto done;
> > 
> > -       desc = gpio_to_desc(gpio);
> > +       if (gpio_is_valid(gpio))
> > +               desc = gpio_to_desc(gpio);
> >         /* reject bogus commands (gpio_unexport ignores them) */
> >         if (!desc) {
> >                 pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
> > --
> > 2.28.0
> > 
> 
> How about we change that to an unconditional WARN() everytime the user
> tries to export a GPIO over sysfs so that people switch over to the
> character device?
> 
> I'm joking a bit but I think it's time to start discouraging people
> from using sysfs and a warning may be a good start.

I am all for a good pr_warn() to tell the user "you screwed up". Adding another
warning along the lines of pr_warn("Please prefer using /dev/gpioXX instead of
sysfs\n") is probably an option too.

But a WARN_ON() with its register & stack dump is too much in my opinion. When
I hit that, I thought "Cr.p, another bug..." and 30s investigation made me
realize that I did an "echo 512 > /sys/class/gpio/export" to export gpio #7 of
my board controller which has a base of 504. The typical off-by-one brain bug
:)

Hence the patch I sent. It keeps the pr_warn for user mistakes like I did, AND
keeps the more serious WARN_ON() for internal bad usage of gpio numbers.

As for the sysfs interface, I would argue that it is already optional through
CONFIG_GPIO_SYSFS. It may not be the best interface for regular end users to
manipulate gpios, but it is definitely super useful for developers to do quick
tests of their setup/drivers (which is what I did for my work with the Kendryte
K210 RISC-V SoC support). We may want to enforce a policy of having arch
defconfigs not enabling this option by default, and recommend that distros
disable it if needed too. But such discussion is I think beyond the point of
this patch.

> 
> Bartosz

-- 
Damien Le Moal
Western Digital

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Linus Walleij]

On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:

> It may not be the best interface for regular end users to
> manipulate gpios, but it is definitely super useful for developers to do quick
> tests of their setup/drivers (which is what I did for my work with the Kendryte
> K210 RISC-V SoC support).

It is a bit discouraging that RISC-V, which was invented after we already
obsoleted the sysfs ABI, is deploying this for development and test.

We need to think about a similar facility for users which is less
damaging but fulfils the same needs. I think I saw something a while
back that looked promising and added some funky files in debugfs
in a hierarchical manner per-gpiochip instead. That is how debugfs
should be used.

Yours,
Linus Walleij

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 3:31 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>
> > It may not be the best interface for regular end users to
> > manipulate gpios, but it is definitely super useful for developers to do quick
> > tests of their setup/drivers (which is what I did for my work with the Kendryte
> > K210 RISC-V SoC support).
>
> It is a bit discouraging that RISC-V, which was invented after we already
> obsoleted the sysfs ABI, is deploying this for development and test.
>
> We need to think about a similar facility for users which is less
> damaging but fulfils the same needs. I think I saw something a while
> back that looked promising and added some funky files in debugfs
> in a hierarchical manner per-gpiochip instead. That is how debugfs
> should be used.
>

Basically something like what gpio-mockup does for events? Was it
something out-of-tree or was it on the mailing list?

Also: quick tests have the tendency to become long-term solutions. :)

Is gpioget/gpioset duo difficult/cumbersome to use? It's a serious
question - I wrote it in a way that was as user-friendly as possible
but maybe I'm missing something about sysfs that makes users prefer it
over a command-line tool. To me sysfs was always a PITA with the
global numbers etc. but it still seems to stick with others.

Bartosz

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Michael Walle]

Am 2020-11-10 15:40, schrieb Bartosz Golaszewski:
> On Tue, Nov 10, 2020 at 3:31 PM Linus Walleij 
> <linus.walleij@linaro.org> wrote:
>> 
>> On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> 
>> wrote:
>> 
>> > It may not be the best interface for regular end users to
>> > manipulate gpios, but it is definitely super useful for developers to do quick
>> > tests of their setup/drivers (which is what I did for my work with the Kendryte
>> > K210 RISC-V SoC support).
>> 
>> It is a bit discouraging that RISC-V, which was invented after we 
>> already
>> obsoleted the sysfs ABI, is deploying this for development and test.
>> 
>> We need to think about a similar facility for users which is less
>> damaging but fulfils the same needs. I think I saw something a while
>> back that looked promising and added some funky files in debugfs
>> in a hierarchical manner per-gpiochip instead. That is how debugfs
>> should be used.
>> 
> 
> Basically something like what gpio-mockup does for events? Was it
> something out-of-tree or was it on the mailing list?
> 
> Also: quick tests have the tendency to become long-term solutions. :)
> 
> Is gpioget/gpioset duo difficult/cumbersome to use?

No, but
  (1) you have to know that it actually exists. This might be obvious for
      you, but I don't know whether every embedded developer is aware 
that
      there is actually a tool to control GPIOs from userspace. So a 
simple
      find /sys -name "*gpio*" and figure out how to use it might be his
      first choice.
  (2) you have to have it installed. If the reference board doesn't come
      with it preinstalled, the sysfs is usually easier to get going
      because its just there.


> It's a serious
> question - I wrote it in a way that was as user-friendly as possible
> but maybe I'm missing something about sysfs that makes users prefer it
> over a command-line tool. To me sysfs was always a PITA with the
> global numbers etc. but it still seems to stick with others.

That is correct, and I actually find it a lot easier to use than 
figuring
out the sysfs numbering, esp. if your DT contains gpio line names. But
there are still old habits (at least in our company).

-michael

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

On Tue, 2020-11-10 at 16:09 +0100, Michael Walle wrote:
> Am 2020-11-10 15:40, schrieb Bartosz Golaszewski:
> > On Tue, Nov 10, 2020 at 3:31 PM Linus Walleij 
> > <linus.walleij@linaro.org> wrote:
> > > 
> > > On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> 
> > > wrote:
> > > 
> > > > It may not be the best interface for regular end users to
> > > > manipulate gpios, but it is definitely super useful for developers to do quick
> > > > tests of their setup/drivers (which is what I did for my work with the Kendryte
> > > > K210 RISC-V SoC support).
> > > 
> > > It is a bit discouraging that RISC-V, which was invented after we 
> > > already
> > > obsoleted the sysfs ABI, is deploying this for development and test.
> > > 
> > > We need to think about a similar facility for users which is less
> > > damaging but fulfils the same needs. I think I saw something a while
> > > back that looked promising and added some funky files in debugfs
> > > in a hierarchical manner per-gpiochip instead. That is how debugfs
> > > should be used.
> > > 
> > 
> > Basically something like what gpio-mockup does for events? Was it
> > something out-of-tree or was it on the mailing list?
> > 
> > Also: quick tests have the tendency to become long-term solutions. :)
> > 
> > Is gpioget/gpioset duo difficult/cumbersome to use?
> 
> No, but
>   (1) you have to know that it actually exists. This might be obvious for
>       you, but I don't know whether every embedded developer is aware 
> that
>       there is actually a tool to control GPIOs from userspace. So a 
> simple
>       find /sys -name "*gpio*" and figure out how to use it might be his
>       first choice.
>   (2) you have to have it installed. If the reference board doesn't come
>       with it preinstalled, the sysfs is usually easier to get going
>       because its just there.

You perfectly described what happened to me :)

> > It's a serious
> > question - I wrote it in a way that was as user-friendly as possible
> > but maybe I'm missing something about sysfs that makes users prefer it
> > over a command-line tool. To me sysfs was always a PITA with the
> > global numbers etc. but it still seems to stick with others.
> 
> That is correct, and I actually find it a lot easier to use than 
> figuring
> out the sysfs numbering, esp. if your DT contains gpio line names. But
> there are still old habits (at least in our company).

-- 
Damien Le Moal
Western Digital

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

On Tue, 2020-11-10 at 16:09 +0100, Michael Walle wrote:
> Am 2020-11-10 15:40, schrieb Bartosz Golaszewski:
> > On Tue, Nov 10, 2020 at 3:31 PM Linus Walleij 
> > <linus.walleij@linaro.org> wrote:
> > > 
> > > On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> 
> > > wrote:
> > > 
> > > > It may not be the best interface for regular end users to
> > > > manipulate gpios, but it is definitely super useful for developers to do quick
> > > > tests of their setup/drivers (which is what I did for my work with the Kendryte
> > > > K210 RISC-V SoC support).
> > > 
> > > It is a bit discouraging that RISC-V, which was invented after we 
> > > already
> > > obsoleted the sysfs ABI, is deploying this for development and test.
> > > 
> > > We need to think about a similar facility for users which is less
> > > damaging but fulfils the same needs. I think I saw something a while
> > > back that looked promising and added some funky files in debugfs
> > > in a hierarchical manner per-gpiochip instead. That is how debugfs
> > > should be used.
> > > 
> > 
> > Basically something like what gpio-mockup does for events? Was it
> > something out-of-tree or was it on the mailing list?
> > 
> > Also: quick tests have the tendency to become long-term solutions. :)
> > 
> > Is gpioget/gpioset duo difficult/cumbersome to use?
> 
> No, but
>   (1) you have to know that it actually exists. This might be obvious for
>       you, but I don't know whether every embedded developer is aware 
> that
>       there is actually a tool to control GPIOs from userspace. So a 
> simple
>       find /sys -name "*gpio*" and figure out how to use it might be his
>       first choice.
>   (2) you have to have it installed. If the reference board doesn't come
>       with it preinstalled, the sysfs is usually easier to get going
>       because its just there.

Forgot to add: I was knew to using gpio in Linux so I started googling "how to"
about it. And all the top hits I got only mentioned sysfs. I actually never saw
any reference to gpioset/gpioget. That includes the references to the kernel
Documentation/admin-guide/gpio/sysfs.rst, which do mention that sysfs is
deprecated (I see that now), but still has the explanation text. May be the
explanation text should be moved to something like documentation/admin-
guide/obsolete/... and keep only the reference to the new tools and char dev ?


> > It's a serious
> > question - I wrote it in a way that was as user-friendly as possible
> > but maybe I'm missing something about sysfs that makes users prefer it
> > over a command-line tool. To me sysfs was always a PITA with the
> > global numbers etc. but it still seems to stick with others.
> 
> That is correct, and I actually find it a lot easier to use than 
> figuring
> out the sysfs numbering, esp. if your DT contains gpio line names. But
> there are still old habits (at least in our company).
> 
> -michael

-- 
Damien Le Moal
Western Digital

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

On Tue, 2020-11-10 at 15:40 +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 3:31 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > 
> > On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> > 
> > > It may not be the best interface for regular end users to
> > > manipulate gpios, but it is definitely super useful for developers to do quick
> > > tests of their setup/drivers (which is what I did for my work with the Kendryte
> > > K210 RISC-V SoC support).
> > 
> > It is a bit discouraging that RISC-V, which was invented after we already
> > obsoleted the sysfs ABI, is deploying this for development and test.
> > 
> > We need to think about a similar facility for users which is less
> > damaging but fulfils the same needs. I think I saw something a while
> > back that looked promising and added some funky files in debugfs
> > in a hierarchical manner per-gpiochip instead. That is how debugfs
> > should be used.
> > 
> 
> Basically something like what gpio-mockup does for events? Was it
> something out-of-tree or was it on the mailing list?
> 
> Also: quick tests have the tendency to become long-term solutions. :)
> 
> Is gpioget/gpioset duo difficult/cumbersome to use? It's a serious
> question - I wrote it in a way that was as user-friendly as possible
> but maybe I'm missing something about sysfs that makes users prefer it
> over a command-line tool. To me sysfs was always a PITA with the
> global numbers etc. but it still seems to stick with others.

In my particular case, I am using a simple busybox userspace that has nothing
else but a shell, so I did not have the gpioget/gpioset tools. And to be frank,
I did not even know what tool to use to control GPIOs. It is the first time I
am touching GPIOs with Linux and did not spent time to study what should be
used once I saw that sysfs allowed controlling the pins for tests.

Removing sysfs entirely would likely force people to look for these tools to do
tests, and I really am an example here :)

I am using buildroot to generate the toolchain and busybox userspace for the
board.  I now see that libgpiod and gpio tools packages are in there but for
whatever reasons, I cannot select them for my RISC-V build. Will need to
explore why.


-- 
Damien Le Moal
Western Digital

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Damien Le Moal]

On Tue, 2020-11-10 at 15:31 +0100, Linus Walleij wrote:
> On Fri, Nov 6, 2020 at 12:27 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> 
> > It may not be the best interface for regular end users to
> > manipulate gpios, but it is definitely super useful for developers to do quick
> > tests of their setup/drivers (which is what I did for my work with the Kendryte
> > K210 RISC-V SoC support).
> 
> It is a bit discouraging that RISC-V, which was invented after we already
> obsoleted the sysfs ABI, is deploying this for development and test.

This is not a RISC-V thing. It is just me who found sysfs very convenient for
testing my work on the K210 board. I was not aware that the sysfs GPIO
interface is being deprecated. I have added it to the K210 default kernel
config file. I will remove it. 

> We need to think about a similar facility for users which is less
> damaging but fulfils the same needs. I think I saw something a while
> back that looked promising and added some funky files in debugfs
> in a hierarchical manner per-gpiochip instead. That is how debugfs
> should be used.

I like this idea too. The point is (my opinion only), anything that allows
quick testing using only a shell without any extra tooling needed is fine.
Extra tooling is not really an issue when using a full distro, but it can be a
problem when working with things like buildroot (or busybox directly). And
indeed, as its name implies, debugfs seems like a good alternative to sysfs.

> 
> Yours,
> Linus Walleij

-- 
Damien Le Moal
Western Digital

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Linus Walleij]

On Wed, Nov 11, 2020 at 7:54 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:

> > We need to think about a similar facility for users which is less
> > damaging but fulfils the same needs. I think I saw something a while
> > back that looked promising and added some funky files in debugfs
> > in a hierarchical manner per-gpiochip instead. That is how debugfs
> > should be used.
>
> I like this idea too. The point is (my opinion only), anything that allows
> quick testing using only a shell without any extra tooling needed is fine.
> Extra tooling is not really an issue when using a full distro, but it can be a
> problem when working with things like buildroot (or busybox directly). And
> indeed, as its name implies, debugfs seems like a good alternative to sysfs.

I would say the problem is something like, I want to test some simple
GPIO access like turning a LED on/off and recompiling the rootfs
is a pain, so some simple debugfs facility would be nice to have to test
it and get on with development.

OK I'll think of some TODO item.

I am slightly worried that people will start abusing debugfs to do products
"because it is so simple" if we add this but wel...

Yours,
Linus Walleij

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Geert Uytterhoeven]

Hi Linus,

On Wed, Nov 11, 2020 at 4:16 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 11, 2020 at 7:54 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> > > We need to think about a similar facility for users which is less
> > > damaging but fulfils the same needs. I think I saw something a while
> > > back that looked promising and added some funky files in debugfs
> > > in a hierarchical manner per-gpiochip instead. That is how debugfs
> > > should be used.
> >
> > I like this idea too. The point is (my opinion only), anything that allows
> > quick testing using only a shell without any extra tooling needed is fine.
> > Extra tooling is not really an issue when using a full distro, but it can be a
> > problem when working with things like buildroot (or busybox directly). And
> > indeed, as its name implies, debugfs seems like a good alternative to sysfs.
>
> I would say the problem is something like, I want to test some simple
> GPIO access like turning a LED on/off and recompiling the rootfs
> is a pain, so some simple debugfs facility would be nice to have to test
> it and get on with development.
>
> OK I'll think of some TODO item.

I'm fully aware of the existence of libgpiod, and I'm still using sysfs
GPIO for testing (and board farm control ;-)

One reason is that sysfs GPIO just needs echo and cat, which are
available on all my file systems (some predating even sysfs GPIO itself),
while libgpiod is one extra barrier^Wstep to take...

Something simple in debugfs (in/high/low) would be great!

> I am slightly worried that people will start abusing debugfs to do products
> "because it is so simple" if we add this but wel...

Yeah, a while ago, there was some fuzz about distros enabling debugfs,
and this being a security issue.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: Do not trigger WARN() with sysfs gpio export/unexport

[Linus Walleij]

On Fri, Nov 6, 2020 at 10:27 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> How about we change that to an unconditional WARN() everytime the user
> tries to export a GPIO over sysfs so that people switch over to the
> character device?
>
> I'm joking a bit but I think it's time to start discouraging people
> from using sysfs and a warning may be a good start.

I think maybe we should hide the Kconfig for sysfs under
EXPERT as well.

I would also seriously don't mind to make CDEV compulsory
for GPIO_SYSFS.

GPIO_SYSFS
   depends on EXPERT
   select GPIO_CDEV

If people to bad only want the deprecated sysfs they can patch
their kernel to get rid of it :/

I will send a patch for this.

Yours,
Linus Walleij