| Message ID | 20201104115348.51930-1-damien.lemoal@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Do not trigger WARN() with sysfs gpio export/unexport | expand |
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
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
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
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
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
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
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
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.
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).
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
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
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
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);