| Message ID | d2c8b7f9a3b420c2764f645da531a57db16905f3.1617017060.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: sysfs: Obey valid_mask | expand |
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.
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?
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/; + } + } +
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?
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"?
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
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
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 > >
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) {