Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> Right before this value is checked for errors, it is and'ed with 0xff.
> This means c->cur can never be less than zero. Besides that, the actual
> error value is ignored.
> 
> c->num is also and'ed with 0xff, which means we can ignore values below
> zero.
> 
> Check the result of get_acpi() right away. While at it, propagate the
> error if we got one.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 47488d3..828db56 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -332,9 +332,12 @@ struct eeepc_cpufv {
>  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
>  {
>  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> +	if (c->cur < 0)
> +		return c->cur;
> +
>  	c->num = (c->cur >> 8) & 0xff;
>  	c->cur &= 0xff;
> -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> +	if (c->num == 0 || c->num > 12)
>  		return -ENODEV;
>  	return 0;

This patch is fine as is. However, Greg has supported propogating the error code
through to the sysfs interface (if I understand him correctly on an earlier post
to this list). This would require an addition change to this patch would
propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
error should probably return -ENXIO as I understand it.

However, there was a rather famous change in error code handling in which pulse
audio broke and Linus was very upset with one of his maintainers.

How do we know when it is acceptable to change which error code is returned?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Greg Kroah-Hartman]

On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> > In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> > Right before this value is checked for errors, it is and'ed with 0xff.
> > This means c->cur can never be less than zero. Besides that, the actual
> > error value is ignored.
> > 
> > c->num is also and'ed with 0xff, which means we can ignore values below
> > zero.
> > 
> > Check the result of get_acpi() right away. While at it, propagate the
> > error if we got one.
> > 
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index 47488d3..828db56 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -332,9 +332,12 @@ struct eeepc_cpufv {
> >  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
> >  {
> >  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> > +	if (c->cur < 0)
> > +		return c->cur;
> > +
> >  	c->num = (c->cur >> 8) & 0xff;
> >  	c->cur &= 0xff;
> > -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> > +	if (c->num == 0 || c->num > 12)
> >  		return -ENODEV;
> >  	return 0;
> 
> This patch is fine as is. However, Greg has supported propogating the error code
> through to the sysfs interface (if I understand him correctly on an earlier post
> to this list). This would require an addition change to this patch would
> propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> error should probably return -ENXIO as I understand it.

I really have no idea at this point in time what to recommend.  How
about just stick with what is happening today so that:

> However, there was a rather famous change in error code handling in which pulse
> audio broke and Linus was very upset with one of his maintainers.

That doesn't happen :)

thanks,

greg k-h

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> > > In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> > > Right before this value is checked for errors, it is and'ed with 0xff.
> > > This means c->cur can never be less than zero. Besides that, the actual
> > > error value is ignored.
> > > 
> > > c->num is also and'ed with 0xff, which means we can ignore values below
> > > zero.
> > > 
> > > Check the result of get_acpi() right away. While at it, propagate the
> > > error if we got one.
> > > 
> > > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > > ---
> > >  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > > index 47488d3..828db56 100644
> > > --- a/drivers/platform/x86/eeepc-laptop.c
> > > +++ b/drivers/platform/x86/eeepc-laptop.c
> > > @@ -332,9 +332,12 @@ struct eeepc_cpufv {
> > >  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
> > >  {
> > >  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> > > +	if (c->cur < 0)
> > > +		return c->cur;
> > > +
> > >  	c->num = (c->cur >> 8) & 0xff;
> > >  	c->cur &= 0xff;
> > > -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> > > +	if (c->num == 0 || c->num > 12)
> > >  		return -ENODEV;
> > >  	return 0;
> > 
> > This patch is fine as is. However, Greg has supported propogating the error code
> > through to the sysfs interface (if I understand him correctly on an earlier post
> > to this list). This would require an addition change to this patch would
> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > error should probably return -ENXIO as I understand it.
> 
> I really have no idea at this point in time what to recommend.  How
> about just stick with what is happening today so that:
> 
> > However, there was a rather famous change in error code handling in which pulse
> > audio broke and Linus was very upset with one of his maintainers.
> 
> That doesn't happen :)

So if I interpret that correctly, we're dropping the last patch
(ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
earlier, I already saw something else break because I returned ENXIO
instead of ENODEV.

Maybe it's a good idea to try and document the expected behavior
somewhere, if even Greg isn't sure what to do.

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
>> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
>> >
>> > This patch is fine as is. However, Greg has supported propogating the error code
>> > through to the sysfs interface (if I understand him correctly on an earlier post
>> > to this list). This would require an addition change to this patch would
>> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
>> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
>> > error should probably return -ENXIO as I understand it.
>>
>> I really have no idea at this point in time what to recommend.  How
>> about just stick with what is happening today so that:
>>
>> > However, there was a rather famous change in error code handling in which pulse
>> > audio broke and Linus was very upset with one of his maintainers.
>>
>> That doesn't happen :)
>
> So if I interpret that correctly, we're dropping the last patch
> (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> earlier, I already saw something else break because I returned ENXIO
> instead of ENODEV.
>
> Maybe it's a good idea to try and document the expected behavior
> somewhere, if even Greg isn't sure what to do.

For good measure:

v2 will not change the return values at the sysfs interface, meaning
we will always return -ENODEV on error. I am going to try to keep as
much internal functions propagating errors as possible though, unless
someone strongly disagrees.

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> >> >
> >> > This patch is fine as is. However, Greg has supported propogating the error code
> >> > through to the sysfs interface (if I understand him correctly on an earlier post
> >> > to this list). This would require an addition change to this patch would
> >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> >> > error should probably return -ENXIO as I understand it.
> >>
> >> I really have no idea at this point in time what to recommend.  How
> >> about just stick with what is happening today so that:
> >>
> >> > However, there was a rather famous change in error code handling in which pulse
> >> > audio broke and Linus was very upset with one of his maintainers.
> >>
> >> That doesn't happen :)
> >
> > So if I interpret that correctly, we're dropping the last patch
> > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > earlier, I already saw something else break because I returned ENXIO
> > instead of ENODEV.
> >
> > Maybe it's a good idea to try and document the expected behavior
> > somewhere, if even Greg isn't sure what to do.
> 
> For good measure:
> 
> v2 will not change the return values at the sysfs interface, meaning
> we will always return -ENODEV on error. I am going to try to keep as
> much internal functions propagating errors as possible though, unless
> someone strongly disagrees.
> 
> Thanks,
> Frans

I cornered Linus today and asked about this specifically. The policy is this:

Don't change the sysfs return codes without good reason. A good reason could be
a real bug or problem with the return codes. It could also be to consolidate
error handling which makes things more uniform, etc.

If this results in broken userspace, the maintainer will revert the change.

This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > >> >
> > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > >> > to this list). This would require an addition change to this patch would
> > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > >> > error should probably return -ENXIO as I understand it.
> > >>
> > >> I really have no idea at this point in time what to recommend.  How
> > >> about just stick with what is happening today so that:
> > >>
> > >> > However, there was a rather famous change in error code handling in which pulse
> > >> > audio broke and Linus was very upset with one of his maintainers.
> > >>
> > >> That doesn't happen :)
> > >
> > > So if I interpret that correctly, we're dropping the last patch
> > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > earlier, I already saw something else break because I returned ENXIO
> > > instead of ENODEV.
> > >
> > > Maybe it's a good idea to try and document the expected behavior
> > > somewhere, if even Greg isn't sure what to do.
> > 
> > For good measure:
> > 
> > v2 will not change the return values at the sysfs interface, meaning
> > we will always return -ENODEV on error. I am going to try to keep as
> > much internal functions propagating errors as possible though, unless
> > someone strongly disagrees.
> > 
> > Thanks,
> > Frans
> 
> I cornered Linus today and asked about this specifically. The policy is this:
> 
> Don't change the sysfs return codes without good reason. A good reason could be
> a real bug or problem with the return codes. It could also be to consolidate
> error handling which makes things more uniform, etc.
> 
> If this results in broken userspace, the maintainer will revert the change.

Alright, that is basically what I was expecting it to be. As it happens,
this also means that we'll have to decide what to do about returning
-EIO/-ENODEV/rv in show_sys_acpi and store_sys_acpi. The latter was
changed by Paul Bolle's "eeepc-laptop: simplify parse_arg()". The return
value of these functions is propagated to the sysfs interface.


> This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.

Agreed.

While I was removing the patches changing the sysfs error behavior, I
also noticed that eeepc-laptop.c doesn't really seem to handle errors
uniformly. In some cases the callers of get_acpi and set_acpi don't even
seem to consider that these functions may return an error code (e.g.
eeepc_hotk_thaw()). I'd fix those in a new series though.

Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > >> >
> > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > >> > to this list). This would require an addition change to this patch would
> > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > >> > error should probably return -ENXIO as I understand it.
> > >>
> > >> I really have no idea at this point in time what to recommend.  How
> > >> about just stick with what is happening today so that:
> > >>
> > >> > However, there was a rather famous change in error code handling in which pulse
> > >> > audio broke and Linus was very upset with one of his maintainers.
> > >>
> > >> That doesn't happen :)
> > >
> > > So if I interpret that correctly, we're dropping the last patch
> > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > earlier, I already saw something else break because I returned ENXIO
> > > instead of ENODEV.
> > >
> > > Maybe it's a good idea to try and document the expected behavior
> > > somewhere, if even Greg isn't sure what to do.
> > 
> > For good measure:
> > 
> > v2 will not change the return values at the sysfs interface, meaning
> > we will always return -ENODEV on error. I am going to try to keep as
> > much internal functions propagating errors as possible though, unless
> > someone strongly disagrees.
> > 
> > Thanks,
> > Frans
> 
> I cornered Linus today and asked about this specifically. The policy is this:
> 
> Don't change the sysfs return codes without good reason. A good reason could be
> a real bug or problem with the return codes. It could also be to consolidate
> error handling which makes things more uniform, etc.
> 
> If this results in broken userspace, the maintainer will revert the change.
> 
> This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.
> 

What do people think of appending this to sysfs-rules.txt?

- When reading and writing sysfs device attribute files, avoid dependency
  on specific error codes wherever possible. This minimizes coupling to
  the error handling implemementation within the kernel.

  In general, failures to read or write sysfs device attributes shall
  propogate errors wherever possible. Common errors include, but are not
  limited to:

  -EIO: The read or store operation is not supported, typically returned by
        the sysfs system itself if the read or store pointer is NULL.

  -ENXIO: The read or store operation failed

  Error codes will not be changed without good reason, and should a change
  to error codes result in user-space breakage, it will be fixed, or the
  the offending change will be reverted.

  Userspace applications can, however, expect the format and contents of
  the attribute files to remain consistent in the absence of a version
  attribute change in the context of a given attributes.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Greg Kroah-Hartman]

On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> > On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > > >> >
> > > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > > >> > to this list). This would require an addition change to this patch would
> > > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > > >> > error should probably return -ENXIO as I understand it.
> > > >>
> > > >> I really have no idea at this point in time what to recommend.  How
> > > >> about just stick with what is happening today so that:
> > > >>
> > > >> > However, there was a rather famous change in error code handling in which pulse
> > > >> > audio broke and Linus was very upset with one of his maintainers.
> > > >>
> > > >> That doesn't happen :)
> > > >
> > > > So if I interpret that correctly, we're dropping the last patch
> > > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > > earlier, I already saw something else break because I returned ENXIO
> > > > instead of ENODEV.
> > > >
> > > > Maybe it's a good idea to try and document the expected behavior
> > > > somewhere, if even Greg isn't sure what to do.
> > > 
> > > For good measure:
> > > 
> > > v2 will not change the return values at the sysfs interface, meaning
> > > we will always return -ENODEV on error. I am going to try to keep as
> > > much internal functions propagating errors as possible though, unless
> > > someone strongly disagrees.
> > > 
> > > Thanks,
> > > Frans
> > 
> > I cornered Linus today and asked about this specifically. The policy is this:
> > 
> > Don't change the sysfs return codes without good reason. A good reason could be
> > a real bug or problem with the return codes. It could also be to consolidate
> > error handling which makes things more uniform, etc.
> > 
> > If this results in broken userspace, the maintainer will revert the change.
> > 
> > This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.
> > 
> 
> What do people think of appending this to sysfs-rules.txt?
> 
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed
> 
>   Error codes will not be changed without good reason, and should a change
>   to error codes result in user-space breakage, it will be fixed, or the
>   the offending change will be reverted.
> 
>   Userspace applications can, however, expect the format and contents of
>   the attribute files to remain consistent in the absence of a version
>   attribute change in the context of a given attributes.

Looks reasonable, thanks.  Care to turn it into a patch that I can
apply?

thanks,

greg k-h

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> 
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed
> 
>   Error codes will not be changed without good reason, and should a change
>   to error codes result in user-space breakage, it will be fixed, or the
>   the offending change will be reverted.

sysfs-rules.txt is written for user space? In that case, reverting the
change is as much a fix as patching it.

> 
>   Userspace applications can, however, expect the format and contents of
>   the attribute files to remain consistent in the absence of a version
>   attribute change in the context of a given attributes.

...attribute.

That's it for the nit-picking.

Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 11:40:37PM +0200, Frans Klaver wrote:
> On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> > 
> > - When reading and writing sysfs device attribute files, avoid dependency
> >   on specific error codes wherever possible. This minimizes coupling to
> >   the error handling implemementation within the kernel.
> > 
> >   In general, failures to read or write sysfs device attributes shall
> >   propogate errors wherever possible. Common errors include, but are not
> >   limited to:
> > 
> >   -EIO: The read or store operation is not supported, typically returned by
> >         the sysfs system itself if the read or store pointer is NULL.
> > 
> >   -ENXIO: The read or store operation failed
> > 
> >   Error codes will not be changed without good reason, and should a change
> >   to error codes result in user-space breakage, it will be fixed, or the
> >   the offending change will be reverted.
> 
> sysfs-rules.txt is written for user space? In that case, reverting the
> change is as much a fix as patching it.
> 
> > 
> >   Userspace applications can, however, expect the format and contents of
> >   the attribute files to remain consistent in the absence of a version
> >   attribute change in the context of a given attributes.

Agreed, but I wanted to make this clear to kernel devs reading this as well,
rather than duplicating this blurb elsewhere.

> 
> ...attribute.
> 
> That's it for the nit-picking.

Thanks for the plural catch.

I'll send to Greg as a patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 11:10:01PM +0200, Frans Klaver wrote:
> On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> > On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > > >> >
> > > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > > >> > to this list). This would require an addition change to this patch would
> > > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > > >> > error should probably return -ENXIO as I understand it.
> > > >>
> > > >> I really have no idea at this point in time what to recommend.  How
> > > >> about just stick with what is happening today so that:
> > > >>
> > > >> > However, there was a rather famous change in error code handling in which pulse
> > > >> > audio broke and Linus was very upset with one of his maintainers.
> > > >>
> > > >> That doesn't happen :)
> > > >
> > > > So if I interpret that correctly, we're dropping the last patch
> > > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > > earlier, I already saw something else break because I returned ENXIO
> > > > instead of ENODEV.
> > > >
> > > > Maybe it's a good idea to try and document the expected behavior
> > > > somewhere, if even Greg isn't sure what to do.
> > > 
> > > For good measure:
> > > 
> > > v2 will not change the return values at the sysfs interface, meaning
> > > we will always return -ENODEV on error. I am going to try to keep as
> > > much internal functions propagating errors as possible though, unless
> > > someone strongly disagrees.
> > > 
> > > Thanks,
> > > Frans
> > 
> > I cornered Linus today and asked about this specifically. The policy is this:
> > 
> > Don't change the sysfs return codes without good reason. A good reason could be
> > a real bug or problem with the return codes. It could also be to consolidate
> > error handling which makes things more uniform, etc.
> > 
> > If this results in broken userspace, the maintainer will revert the change.
> 
> Alright, that is basically what I was expecting it to be. As it happens,
> this also means that we'll have to decide what to do about returning
> -EIO/-ENODEV/rv in show_sys_acpi and store_sys_acpi. The latter was
> changed by Paul Bolle's "eeepc-laptop: simplify parse_arg()". The return
> value of these functions is propagated to the sysfs interface.

Yes, that change to store_sys_acpi needs to be reverted. I don't think
show_sys_acpi is actually changed in that patch.

Paul, can you resend that patch with the store_sys_acpi() -EIO return restored?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Henrique de Moraes Holschuh]

On Tue, 16 Sep 2014, Darren Hart wrote:
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed

from errno(3):
       EIO             Input/output error (POSIX.1)
       ENXIO           No such device or address (POSIX.1)

It makes sense to retry EIO.  ENXIO means there's nobody listening at the
time, and isn't usually retried.

The device-based interfaces get it right.  A typical example is the
cpu-based devices, where ENXIO means "no such processor", while EIO means
"whatever you're trying to do failed",  so a MSR read would return ENXIO if
the processor core is offline/doesn't exist, and EIO if the processor core
is there, but raised a #GP when the MSR read was attempted.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Wed, Sep 17, 2014 at 12:34 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 16 Sep 2014, Darren Hart wrote:
>> - When reading and writing sysfs device attribute files, avoid dependency
>>   on specific error codes wherever possible. This minimizes coupling to
>>   the error handling implemementation within the kernel.
>>
>>   In general, failures to read or write sysfs device attributes shall
>>   propogate errors wherever possible. Common errors include, but are not
>>   limited to:
>>
>>   -EIO: The read or store operation is not supported, typically returned by
>>         the sysfs system itself if the read or store pointer is NULL.
>>
>>   -ENXIO: The read or store operation failed
>
> from errno(3):
>        EIO             Input/output error (POSIX.1)
>        ENXIO           No such device or address (POSIX.1)
>
> It makes sense to retry EIO.  ENXIO means there's nobody listening at the
> time, and isn't usually retried.
>
> The device-based interfaces get it right.  A typical example is the
> cpu-based devices, where ENXIO means "no such processor", while EIO means
> "whatever you're trying to do failed",  so a MSR read would return ENXIO if
> the processor core is offline/doesn't exist, and EIO if the processor core
> is there, but raised a #GP when the MSR read was attempted.

Here's something I don't quite understand. How should one then
distinguish between sysfs's use of EIO "can't (read from|write to)
this file", and this example's EIO "something went wrong, you might
want to try again"? Why not use EAGAIN "Resource temporarily
unavailable" in the case where trying again makes sense? I wouldn't
normally retry the last operation if I was just told something
actually went wrong.

I've only been at it for a couple of weeks, but I get the impression
that sysfs has never really been guided regarding error codes, or has
gone to live its own life now kept in check with "don't change the
errors, it may break userspace". Does that make sense?

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Wed, Sep 17, 2014 at 01:57:33PM +0200, Frans Klaver wrote:
> On Wed, Sep 17, 2014 at 12:34 PM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > On Tue, 16 Sep 2014, Darren Hart wrote:
> >> - When reading and writing sysfs device attribute files, avoid dependency
> >>   on specific error codes wherever possible. This minimizes coupling to
> >>   the error handling implemementation within the kernel.
> >>
> >>   In general, failures to read or write sysfs device attributes shall
> >>   propogate errors wherever possible. Common errors include, but are not
> >>   limited to:
> >>
> >>   -EIO: The read or store operation is not supported, typically returned by
> >>         the sysfs system itself if the read or store pointer is NULL.
> >>
> >>   -ENXIO: The read or store operation failed
> >
> > from errno(3):
> >        EIO             Input/output error (POSIX.1)
> >        ENXIO           No such device or address (POSIX.1)
> >
> > It makes sense to retry EIO.  ENXIO means there's nobody listening at the
> > time, and isn't usually retried.
> >
> > The device-based interfaces get it right.  A typical example is the
> > cpu-based devices, where ENXIO means "no such processor", while EIO means
> > "whatever you're trying to do failed",  so a MSR read would return ENXIO if
> > the processor core is offline/doesn't exist, and EIO if the processor core
> > is there, but raised a #GP when the MSR read was attempted.
> 
> Here's something I don't quite understand. How should one then
> distinguish between sysfs's use of EIO "can't (read from|write to)
> this file", and this example's EIO "something went wrong, you might
> want to try again"? Why not use EAGAIN "Resource temporarily
> unavailable" in the case where trying again makes sense? I wouldn't
> normally retry the last operation if I was just told something
> actually went wrong.
> 
> I've only been at it for a couple of weeks, but I get the impression
> that sysfs has never really been guided regarding error codes, or has
> gone to live its own life now kept in check with "don't change the
> errors, it may break userspace". Does that make sense?

Right, this was the distinction I was trying to make with the above description.
Henrique's points are valid, but based on the sysfs subsystem already using EIO
in the way that it does, I felt the above made sense.

That said, I'm not personally tied to them, it's just what I have derived from
recent discussions on the subject and what I observed in existing usage.

-- 
Darren Hart
Intel Open Source Technology Center