* [PATCH 0/2] gpiolib: misc fixups @ 2021-03-29 11:40 Matti Vaittinen 2021-03-29 11:41 ` [PATCH 1/2] gpio: sysfs: Obey valid_mask Matti Vaittinen 2021-03-29 11:41 ` [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config Matti Vaittinen 0 siblings, 2 replies; 17+ messages in thread From: Matti Vaittinen @ 2021-03-29 11:40 UTC (permalink / raw) To: matti.vaittinen, mazziesaccount Cc: Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, linux-gpio, linux-kernel Patch 1/2: The deprecated and obsoleted - but still used (especially in older releases) /sys/class/gpio interface allows modifying GPIOs which are excluded by the "valid_mask". This makes the valid_mask not suitable for cases where toggling GPIO can cause damage. Patch adds validity check to export. I assume many people are still using the /sys/class/gpio at least for testing purposes - especially with older releases. Thus it might be worth the hassle to exclude invalid GPIOs - they're probably set invalid for a good reason. (Actually, I noticed this when trying to use the valid_mask to invalidate undocumented GPO on BD71815 - which may be connected to GND. The people using BD71815 are likely to be using some stable-release, which probably supports the (deprected, obsolete) sysfs until the end of the days => valid_mask is not safe way unless fix to this is backported to stable). Patch 2/2: checkpatch nowadays reminds us that ENOTSUPP is not valid error and should be replaced by EOPNOTSUPP. It'd be nice if GPIO drivers could return also the EOPNOTSUPP from config and avoid being nagged by checkpatch. Matti Vaittinen (2): gpio: sysfs: Obey valid_mask gpiolib: Allow drivers to return EOPNOTSUPP from config drivers/gpio/gpiolib-sysfs.c | 8 ++++++++ drivers/gpio/gpiolib.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-29 11:40 [PATCH 0/2] gpiolib: misc fixups Matti Vaittinen @ 2021-03-29 11:41 ` Matti Vaittinen 2021-03-29 11:56 ` Andy Shevchenko 2021-03-31 7:56 ` Bartosz Golaszewski 2021-03-29 11:41 ` [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config Matti Vaittinen 1 sibling, 2 replies; 17+ messages in thread From: Matti Vaittinen @ 2021-03-29 11:41 UTC (permalink / raw) To: matti.vaittinen, mazziesaccount Cc: Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, linux-gpio, linux-kernel Do not allow exporting GPIOs which are set invalid by the driver's valid mask. Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/gpio/gpiolib-sysfs.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c index 26c5466b8179..ae49bb23c6ed 100644 --- a/drivers/gpio/gpiolib-sysfs.c +++ b/drivers/gpio/gpiolib-sysfs.c @@ -458,6 +458,8 @@ static ssize_t export_store(struct class *class, long gpio; struct gpio_desc *desc; int status; + struct gpio_chip *gc; + int offset; status = kstrtol(buf, 0, &gpio); if (status < 0) @@ -469,6 +471,12 @@ static ssize_t export_store(struct class *class, pr_warn("%s: invalid GPIO %ld\n", __func__, gpio); return -EINVAL; } + gc = desc->gdev->chip; + offset = gpio_chip_hwgpio(desc); + if (!gpiochip_line_is_valid(gc, offset)) { + pr_warn("%s: GPIO %ld masked\n", __func__, gpio); + return -EINVAL; + } /* No extra locking here; FLAG_SYSFS just signifies that the * request and export were done by on behalf of userspace, so -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-29 11:41 ` [PATCH 1/2] gpio: sysfs: Obey valid_mask Matti Vaittinen @ 2021-03-29 11:56 ` Andy Shevchenko 2021-03-31 7:56 ` Bartosz Golaszewski 1 sibling, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2021-03-29 11:56 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, Mar 29, 2021 at 2:42 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Do not allow exporting GPIOs which are set invalid > by the driver's valid mask. > Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Should not be blank lines in the tag block. Dunno if you can convert those pr_*() to dev_*(), but it's definitely out of the scope here. I like the idea, FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/gpio/gpiolib-sysfs.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c > index 26c5466b8179..ae49bb23c6ed 100644 > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -458,6 +458,8 @@ static ssize_t export_store(struct class *class, > long gpio; > struct gpio_desc *desc; > int status; > + struct gpio_chip *gc; > + int offset; > > status = kstrtol(buf, 0, &gpio); > if (status < 0) > @@ -469,6 +471,12 @@ static ssize_t export_store(struct class *class, > pr_warn("%s: invalid GPIO %ld\n", __func__, gpio); > return -EINVAL; > } > + gc = desc->gdev->chip; > + offset = gpio_chip_hwgpio(desc); > + if (!gpiochip_line_is_valid(gc, offset)) { > + pr_warn("%s: GPIO %ld masked\n", __func__, gpio); > + return -EINVAL; > + } > > /* No extra locking here; FLAG_SYSFS just signifies that the > * request and export were done by on behalf of userspace, so > -- > 2.25.4 > > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ > Thanks to Simon Glass for the translation =] -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-29 11:41 ` [PATCH 1/2] gpio: sysfs: Obey valid_mask Matti Vaittinen 2021-03-29 11:56 ` Andy Shevchenko @ 2021-03-31 7:56 ` Bartosz Golaszewski 2021-03-31 12:25 ` Andy Shevchenko 1 sibling, 1 reply; 17+ messages in thread From: Bartosz Golaszewski @ 2021-03-31 7:56 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Stephen Boyd, Andy Shevchenko, linux-gpio, LKML On Mon, Mar 29, 2021 at 1:41 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Do not allow exporting GPIOs which are set invalid > by the driver's valid mask. > > Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- Applied, thanks! Bart ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-31 7:56 ` Bartosz Golaszewski @ 2021-03-31 12:25 ` Andy Shevchenko 2021-03-31 18:29 ` Bartosz Golaszewski 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-03-31 12:25 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Matti Vaittinen, Matti Vaittinen, Linus Walleij, Stephen Boyd, Andy Shevchenko, linux-gpio, LKML On Wed, Mar 31, 2021 at 10:58 AM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Mon, Mar 29, 2021 at 1:41 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > Do not allow exporting GPIOs which are set invalid > > by the driver's valid mask. > > > > Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 I have just noticed that this is invalid format for the Fixes tag (luckily, haha, due to a blank line it's not recognized as a tag!). Matti, I highly recommend to add in your .gitconfig file an alias: one = show -s --pretty='format:%h (\"%s\")' Bart, there are real Fixes tag issues from another series. I will comment there as well to let an author know. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-31 12:25 ` Andy Shevchenko @ 2021-03-31 18:29 ` Bartosz Golaszewski 2021-04-01 3:56 ` Matti Vaittinen 0 siblings, 1 reply; 17+ messages in thread From: Bartosz Golaszewski @ 2021-03-31 18:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Matti Vaittinen, Matti Vaittinen, Linus Walleij, Stephen Boyd, Andy Shevchenko, linux-gpio, LKML On Wed, Mar 31, 2021 at 2:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Mar 31, 2021 at 10:58 AM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > > > On Mon, Mar 29, 2021 at 1:41 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > > Do not allow exporting GPIOs which are set invalid > > > by the driver's valid mask. > > > > > > Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 > > I have just noticed that this is invalid format for the Fixes tag > (luckily, haha, due to a blank line it's not recognized as a tag!). > > Matti, I highly recommend to add in your .gitconfig file an alias: > one = show -s --pretty='format:%h (\"%s\")' > > Bart, there are real Fixes tag issues from another series. I will > comment there as well to let an author know. > > -- Eek, sorry I should have looked more carefully. I'll fix it in my tree. Bartosz ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gpio: sysfs: Obey valid_mask 2021-03-31 18:29 ` Bartosz Golaszewski @ 2021-04-01 3:56 ` Matti Vaittinen 0 siblings, 0 replies; 17+ messages in thread From: Matti Vaittinen @ 2021-04-01 3:56 UTC (permalink / raw) To: Bartosz Golaszewski, Andy Shevchenko Cc: Linus Walleij, Stephen Boyd, Andy Shevchenko, linux-gpio, LKML On Wed, 2021-03-31 at 20:29 +0200, Bartosz Golaszewski wrote: > On Wed, Mar 31, 2021 at 2:25 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Mar 31, 2021 at 10:58 AM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > > > On Mon, Mar 29, 2021 at 1:41 PM Matti Vaittinen > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > Do not allow exporting GPIOs which are set invalid > > > > by the driver's valid mask. > > > > > > > > Fixes: 726cb3ba49692bdae6caff457755e7cdb432efa4 > > > > I have just noticed that this is invalid format for the Fixes tag > > (luckily, haha, due to a blank line it's not recognized as a tag!). > > > > Matti, I highly recommend to add in your .gitconfig file an alias: > > one = show -s --pretty='format:%h (\"%s\")' > > > > Bart, there are real Fixes tag issues from another series. I will > > comment there as well to let an author know. > > > > -- > > Eek, sorry I should have looked more carefully. I'll fix it in my > tree. Thanks for fixing this Bartosz. Andy - well spotted. And the alias you pointed is something I've missed :) Sorry for the trouble! I should have used the correct tag format. Thanks again! Best Regards Matti Vaittinen ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 11:40 [PATCH 0/2] gpiolib: misc fixups Matti Vaittinen 2021-03-29 11:41 ` [PATCH 1/2] gpio: sysfs: Obey valid_mask Matti Vaittinen @ 2021-03-29 11:41 ` Matti Vaittinen 2021-03-29 11:59 ` Andy Shevchenko 1 sibling, 1 reply; 17+ messages in thread From: Matti Vaittinen @ 2021-03-29 11:41 UTC (permalink / raw) To: matti.vaittinen, mazziesaccount Cc: Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, linux-gpio, linux-kernel The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP Make the gpiolib allow drivers to return both so driver developers can avoid one of the checkpatch complaints. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6367646dce83..60d61a6314b0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2134,7 +2134,7 @@ static int gpio_set_config_with_argument_optional(struct gpio_desc *desc, int ret; ret = gpio_set_config_with_argument(desc, mode, argument); - if (ret != -ENOTSUPP) + if (ret != -ENOTSUPP && ret != -EOPNOTSUPP) return ret; switch (mode) { -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 11:41 ` [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config Matti Vaittinen @ 2021-03-29 11:59 ` Andy Shevchenko 2021-03-29 12:20 ` Matti Vaittinen 2021-03-29 15:08 ` Joe Perches 0 siblings, 2 replies; 17+ messages in thread From: Andy Shevchenko @ 2021-03-29 11:59 UTC (permalink / raw) To: Matti Vaittinen, Joe Perches Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > Make the gpiolib allow drivers to return both so driver developers > can avoid one of the checkpatch complaints. Internally we are fine to use the ENOTSUPP. Checkpatch false positives there. I doubt we need this change. Rather checkpatch should rephrase this to point out that this is only applicable to _user-visible_ error path. Cc'ed Joe. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 11:59 ` Andy Shevchenko @ 2021-03-29 12:20 ` Matti Vaittinen 2021-03-29 13:30 ` Andy Shevchenko 2021-03-29 15:08 ` Joe Perches 1 sibling, 1 reply; 17+ messages in thread From: Matti Vaittinen @ 2021-03-29 12:20 UTC (permalink / raw) To: Andy Shevchenko, Joe Perches Cc: Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > > > Make the gpiolib allow drivers to return both so driver developers > > can avoid one of the checkpatch complaints. > > Internally we are fine to use the ENOTSUPP. > Checkpatch false positives there. I agree. OTOH, the checkpatch check makes sense to user-visible stuff. Yet, the checkpatch has hard time guessing what is user-visible - so it probably is easiest to nag about all ENOTSUPP uses as it does now. > I doubt we need this change. Rather checkpatch should rephrase this > to > point out that this is only applicable to _user-visible_ error path. > Cc'ed Joe. Yes, thanks for pulling Joe in. Anyways, no matter what the warning says, all false positives are annoying. I don't see why we should stay with ENOTSUPP in gpiolib? (other than the burden of changing it). But I have no strong opinion on this. All options I see have downsides. Accepting both ENOTSUPP and EOPNOTSUPP is the easy way to avoid allowing checkpatch warnings - but I admit it isn't stylish. Converting all ENOTSUPP cases inside gpiolib to EOPNOTSUPP is teodious although end result might be nicer. Leaving it as is gives annoying false-positives to driver developers. My personal preference was this patch - others can have other view like Andy does. I'll leave this to community/maintainers to evaluate :) Best Regards Matti Vaittinen ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 12:20 ` Matti Vaittinen @ 2021-03-29 13:30 ` Andy Shevchenko 2021-03-30 4:32 ` Matti Vaittinen 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-03-29 13:30 UTC (permalink / raw) To: Matti Vaittinen Cc: Joe Perches, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, Mar 29, 2021 at 03:20:07PM +0300, Matti Vaittinen wrote: > On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > > > > > Make the gpiolib allow drivers to return both so driver developers > > > can avoid one of the checkpatch complaints. > > > > Internally we are fine to use the ENOTSUPP. > > Checkpatch false positives there. > > I agree. OTOH, the checkpatch check makes sense to user-visible stuff. > Yet, the checkpatch has hard time guessing what is user-visible - so it > probably is easiest to nag about all ENOTSUPP uses as it does now. > > > I doubt we need this change. Rather checkpatch should rephrase this > > to > > point out that this is only applicable to _user-visible_ error path. > > Cc'ed Joe. > > Yes, thanks for pulling Joe in. > > Anyways, no matter what the warning says, all false positives are > annoying. I don't see why we should stay with ENOTSUPP in gpiolib? > (other than the burden of changing it). For sake of the changing we are not changing the code. > But I have no strong opinion on this. All options I see have downsides. > > Accepting both ENOTSUPP and EOPNOTSUPP is the easy way to avoid > allowing checkpatch warnings - but I admit it isn't stylish. I think the error code which is Linux kernel internal is for a reason. > Converting all ENOTSUPP cases inside gpiolib to EOPNOTSUPP is teodious > although end result might be nicer. Why? You still missed the justification except satisfying some tool that gives you false positives. We don't do that. It's the tool that has to be fixed / amended. > Leaving it as is gives annoying false-positives to driver developers. > > My personal preference was this patch - others can have other view like > Andy does. I'll leave this to community/maintainers to evaluate :) This patch misses documentation fixes, for example. Also, do you suggest that we have to do the same in entire pin control subsystem? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 13:30 ` Andy Shevchenko @ 2021-03-30 4:32 ` Matti Vaittinen 2021-03-30 10:19 ` Andy Shevchenko 2021-03-31 8:10 ` Bartosz Golaszewski 0 siblings, 2 replies; 17+ messages in thread From: Matti Vaittinen @ 2021-03-30 4:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Joe Perches, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List Morning Folks, On Mon, 2021-03-29 at 16:30 +0300, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 03:20:07PM +0300, Matti Vaittinen wrote: > > On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > > > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer > > > > > EOPNOTSUPP > > > > > > > > Make the gpiolib allow drivers to return both so driver > > > > developers > > > > can avoid one of the checkpatch complaints. > > > > > > Internally we are fine to use the ENOTSUPP. > > > Checkpatch false positives there. > > > > I agree. OTOH, the checkpatch check makes sense to user-visible > > stuff. > > Yet, the checkpatch has hard time guessing what is user-visible - > > so it > > probably is easiest to nag about all ENOTSUPP uses as it does now. > > > > > I doubt we need this change. Rather checkpatch should rephrase > > > this > > > to > > > point out that this is only applicable to _user-visible_ error > > > path. > > > Cc'ed Joe. > > > > Yes, thanks for pulling Joe in. > > > > Anyways, no matter what the warning says, all false positives are > > annoying. I don't see why we should stay with ENOTSUPP in gpiolib? > > (other than the burden of changing it). > > For sake of the changing we are not changing the code. No. But for the sake of making it better / more consistent :) Anyway - after giving this second thought (thanks Andy for provoking me to thinking this further) - I do agree with Andy that this particular change is bad. More I think of this, less I like the idea of having two separate return values to indicate the same thing. So we should support only one which makes my patch terrible. For the sake of consistency it would be cleaner to use same, single value, for same error both inside the gpiolib and at user-interface. That would be EOPNOTSUPP. As I said, having two separate error codes to indicate same thing is confusing. Now the confusion is at the boundary of gpiolib and user-land. Please educate me - is there difference in the meaning of ENOTSUPP and EOPNOTSUPP or are they really indicating the same thing? If yes, then yes - correct fix would be to use only one and ditch the other. Whether the amount of work is such it is practically worth is another topic - but that would be the right thing to do (tm). > > > But I have no strong opinion on this. All options I see have > > downsides. > > > > Accepting both ENOTSUPP and EOPNOTSUPP is the easy way to avoid > > allowing checkpatch warnings - but I admit it isn't stylish. > > I think the error code which is Linux kernel internal is for a > reason. If so, then the current checkpatch warning is even more questionable. > > > Converting all ENOTSUPP cases inside gpiolib to EOPNOTSUPP is > > teodious > > although end result might be nicer. > > Why? You still missed the justification except satisfying some tool > that gives > you false positives. We don't do that. It's the tool that has to be > fixed / > amended. > > > Leaving it as is gives annoying false-positives to driver > > developers. > > > > My personal preference was this patch - others can have other view > > like > > Andy does. I'll leave this to community/maintainers to evaluate :) > > This patch misses documentation fixes, for example. Valid point. > Also, do you suggest that we have to do the same in entire pin > control > subsystem? After reading/writing this, I am unsure. This is why the discussion is good :) I don't see why we should have two separate error codes for same thing - but as you put it: > I think the error code which is Linux kernel internal is for a > reason. not all of us thinks the same. So maybe I just don't get it? :) Best Regards Matti Vaittinen ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-30 4:32 ` Matti Vaittinen @ 2021-03-30 10:19 ` Andy Shevchenko 2021-03-31 8:10 ` Bartosz Golaszewski 1 sibling, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2021-03-30 10:19 UTC (permalink / raw) To: Matti Vaittinen Cc: Joe Perches, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Tue, Mar 30, 2021 at 7:32 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > On Mon, 2021-03-29 at 16:30 +0300, Andy Shevchenko wrote: > > On Mon, Mar 29, 2021 at 03:20:07PM +0300, Matti Vaittinen wrote: ... > > I think the error code which is Linux kernel internal is for a > > reason. > > not all of us thinks the same. So maybe I just don't get it? :) Thanks for following me now, appreciate. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-30 4:32 ` Matti Vaittinen 2021-03-30 10:19 ` Andy Shevchenko @ 2021-03-31 8:10 ` Bartosz Golaszewski 1 sibling, 0 replies; 17+ messages in thread From: Bartosz Golaszewski @ 2021-03-31 8:10 UTC (permalink / raw) To: Matti Vaittinen Cc: Andy Shevchenko, Joe Perches, Linus Walleij, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Tue, Mar 30, 2021 at 6:32 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Morning Folks, > > On Mon, 2021-03-29 at 16:30 +0300, Andy Shevchenko wrote: > > On Mon, Mar 29, 2021 at 03:20:07PM +0300, Matti Vaittinen wrote: > > > On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > > > > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer > > > > > > EOPNOTSUPP > > > > > > > > > > Make the gpiolib allow drivers to return both so driver > > > > > developers > > > > > can avoid one of the checkpatch complaints. > > > > > > > > Internally we are fine to use the ENOTSUPP. > > > > Checkpatch false positives there. > > > > > > I agree. OTOH, the checkpatch check makes sense to user-visible > > > stuff. > > > Yet, the checkpatch has hard time guessing what is user-visible - > > > so it > > > probably is easiest to nag about all ENOTSUPP uses as it does now. > > > > > > > I doubt we need this change. Rather checkpatch should rephrase > > > > this > > > > to > > > > point out that this is only applicable to _user-visible_ error > > > > path. > > > > Cc'ed Joe. > > > > > > Yes, thanks for pulling Joe in. > > > > > > Anyways, no matter what the warning says, all false positives are > > > annoying. I don't see why we should stay with ENOTSUPP in gpiolib? > > > (other than the burden of changing it). > > > > For sake of the changing we are not changing the code. > No. But for the sake of making it better / more consistent :) > > Anyway - after giving this second thought (thanks Andy for provoking me > to thinking this further) - I do agree with Andy that this particular > change is bad. More I think of this, less I like the idea of having two > separate return values to indicate the same thing. So we should support > only one which makes my patch terrible. > > For the sake of consistency it would be cleaner to use same, single > value, for same error both inside the gpiolib and at user-interface. > That would be EOPNOTSUPP. As I said, having two separate error codes to > indicate same thing is confusing. Now the confusion is at the boundary > of gpiolib and user-land. Please educate me - is there difference in > the meaning of ENOTSUPP and EOPNOTSUPP or are they really indicating > the same thing? If yes, then yes - correct fix would be to use only one > and ditch the other. Whether the amount of work is such it is > practically worth is another topic - but that would be the right thing > to do (tm). > In user-space there's no ENOTSUPP but there's ENOTSUP which is defined in most sane toolchains as: #define ENOTSUP EOPNOTSUPP While ENOTSUPP is not the same number as EOPNOTSUPP. So to answer the question: they mean the same thing in the kernel but not to user-space. We must never return ENOTSUPP to user-space because it doesn't know it. Bartosz > > > > > But I have no strong opinion on this. All options I see have > > > downsides. > > > > > > Accepting both ENOTSUPP and EOPNOTSUPP is the easy way to avoid > > > allowing checkpatch warnings - but I admit it isn't stylish. > > > > I think the error code which is Linux kernel internal is for a > > reason. > > If so, then the current checkpatch warning is even more questionable. > > > > > > Converting all ENOTSUPP cases inside gpiolib to EOPNOTSUPP is > > > teodious > > > although end result might be nicer. > > > > Why? You still missed the justification except satisfying some tool > > that gives > > you false positives. We don't do that. It's the tool that has to be > > fixed / > > amended. > > > > > Leaving it as is gives annoying false-positives to driver > > > developers. > > > > > > My personal preference was this patch - others can have other view > > > like > > > Andy does. I'll leave this to community/maintainers to evaluate :) > > > > This patch misses documentation fixes, for example. > > Valid point. > > > Also, do you suggest that we have to do the same in entire pin > > control > > subsystem? > > After reading/writing this, I am unsure. This is why the discussion is > good :) I don't see why we should have two separate error codes for > same thing - but as you put it: > > > I think the error code which is Linux kernel internal is for a > > reason. > > not all of us thinks the same. So maybe I just don't get it? :) > > Best Regards > Matti Vaittinen > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 11:59 ` Andy Shevchenko 2021-03-29 12:20 ` Matti Vaittinen @ 2021-03-29 15:08 ` Joe Perches 2021-03-29 15:25 ` Andy Shevchenko 1 sibling, 1 reply; 17+ messages in thread From: Joe Perches @ 2021-03-29 15:08 UTC (permalink / raw) To: Andy Shevchenko, Matti Vaittinen, Jakub Kicinski Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, Andy Shevchenko, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > > > Make the gpiolib allow drivers to return both so driver developers > > can avoid one of the checkpatch complaints. > > Internally we are fine to use the ENOTSUPP. > Checkpatch false positives there. > > I doubt we need this change. Rather checkpatch should rephrase this to > point out that this is only applicable to _user-visible_ error path. > Cc'ed Joe. Adding CC for Jakub Kicinski who added that particular rule/test. And the output message report of the rule is merely a suggestion indicating a preference. It's always up to an individual to accept/reject. At best, perhaps wordsmithing the checkpatch message might be an OK option. +# ENOTSUPP is not a standard error code and should be avoided in new patches. +# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. +# Similarly to ENOSYS warning a small number of false positives is expected. + if (!$file && $line =~ /\bENOTSUPP\b/) { + if (WARN("ENOTSUPP", + "ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\bENOTSUPP\b/EOPNOTSUPP/; + } + } + ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 15:08 ` Joe Perches @ 2021-03-29 15:25 ` Andy Shevchenko 2021-03-29 17:46 ` Jakub Kicinski 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-03-29 15:25 UTC (permalink / raw) To: Joe Perches Cc: Matti Vaittinen, Jakub Kicinski, Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, Mar 29, 2021 at 08:08:52AM -0700, Joe Perches wrote: > On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > > > > > Make the gpiolib allow drivers to return both so driver developers > > > can avoid one of the checkpatch complaints. > > > > Internally we are fine to use the ENOTSUPP. > > Checkpatch false positives there. > > > > I doubt we need this change. Rather checkpatch should rephrase this to > > point out that this is only applicable to _user-visible_ error path. > > Cc'ed Joe. > > Adding CC for Jakub Kicinski who added that particular rule/test. > > And the output message report of the rule is merely a suggestion indicating > a preference. It's always up to an individual to accept/reject. > > At best, perhaps wordsmithing the checkpatch message might be an OK option. Thanks, Joe! Jakub, what do you think? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config 2021-03-29 15:25 ` Andy Shevchenko @ 2021-03-29 17:46 ` Jakub Kicinski 0 siblings, 0 replies; 17+ messages in thread From: Jakub Kicinski @ 2021-03-29 17:46 UTC (permalink / raw) To: Andy Shevchenko Cc: Joe Perches, Matti Vaittinen, Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Stephen Boyd, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List On Mon, 29 Mar 2021 18:25:46 +0300 Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 08:08:52AM -0700, Joe Perches wrote: > > On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote: > > > On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > > > > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP. > > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > > > > > > > Make the gpiolib allow drivers to return both so driver developers > > > > can avoid one of the checkpatch complaints. > > > > > > Internally we are fine to use the ENOTSUPP. > > > Checkpatch false positives there. > > > > > > I doubt we need this change. Rather checkpatch should rephrase this to > > > point out that this is only applicable to _user-visible_ error path. > > > Cc'ed Joe. > > > > Adding CC for Jakub Kicinski who added that particular rule/test. > > > > And the output message report of the rule is merely a suggestion indicating > > a preference. It's always up to an individual to accept/reject. > > > > At best, perhaps wordsmithing the checkpatch message might be an OK option. > > Thanks, Joe! > > Jakub, what do you think? Agreed, weaving into the message that ENOTSUPP is okay internally sounds good. Perhaps we should append "if error may be returned to user space"? ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-04-01 3:58 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-29 11:40 [PATCH 0/2] gpiolib: misc fixups Matti Vaittinen 2021-03-29 11:41 ` [PATCH 1/2] gpio: sysfs: Obey valid_mask Matti Vaittinen 2021-03-29 11:56 ` Andy Shevchenko 2021-03-31 7:56 ` Bartosz Golaszewski 2021-03-31 12:25 ` Andy Shevchenko 2021-03-31 18:29 ` Bartosz Golaszewski 2021-04-01 3:56 ` Matti Vaittinen 2021-03-29 11:41 ` [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config Matti Vaittinen 2021-03-29 11:59 ` Andy Shevchenko 2021-03-29 12:20 ` Matti Vaittinen 2021-03-29 13:30 ` Andy Shevchenko 2021-03-30 4:32 ` Matti Vaittinen 2021-03-30 10:19 ` Andy Shevchenko 2021-03-31 8:10 ` Bartosz Golaszewski 2021-03-29 15:08 ` Joe Perches 2021-03-29 15:25 ` Andy Shevchenko 2021-03-29 17:46 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).