Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config

[Bartosz Golaszewski]

On Tue, Mar 30, 2021 at 6:32 AM Matti Vaittinen
<matti.vaittinen@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> 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
>
>