[PATCH] eeepc-laptop: remove possible use of uninitialized value

[PATCH] eeepc-laptop: remove possible use of uninitialized value

[Frans Klaver]

In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
fails, 'value' remains possibly uninitialized. In that case 'value'
shouldn't be used to produce the store_sys_acpi()s return value.

Only test the return value of set_acpi() if we can actually call it.
Return rv otherwise.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bd533c2..41f12ba 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
 	int rv, value;
 
 	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		value = set_acpi(eeepc, cm, value);
-	if (value < 0)
-		return -EIO;
+	if (rv > 0) {
+		if (set_acpi(eeepc, cm, value) < 0)
+			return -EIO;
+	}
 	return rv;
 }
 
-- 
2.1.0

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Darren Hart]

On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
> In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> fails, 'value' remains possibly uninitialized. In that case 'value'
> shouldn't be used to produce the store_sys_acpi()s return value.
> 
> Only test the return value of set_acpi() if we can actually call it.
> Return rv otherwise.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c2..41f12ba 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>  	int rv, value;
>  
>  	rv = parse_arg(buf, count, &value);
> -	if (rv > 0)
> -		value = set_acpi(eeepc, cm, value);

That was rather horrible wasn't it? :-)

> -	if (value < 0)
> -		return -EIO;
> +	if (rv > 0) {
> +		if (set_acpi(eeepc, cm, value) < 0)
> +			return -EIO;

Is there a compelling reason not to propogate the return code of set_acpi?
(ENODEV specifically). I see -EIO in Documentation/filesystems/sysfs.txt, but
it's used by default if the show() pointer is NULL (for example), but otherwise
propogates the error.

Specifically it states:

- show() or store() can always return errors. If a bad value comes
  through, be sure to return an error.

Greg, does this need to be -EIO? or is returning someting like ENODEV preferable
if it more accurately reflects the error?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Greg Kroah-Hartman]

On Wed, Sep 03, 2014 at 05:49:47PM -0700, Darren Hart wrote:
> On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> > fails, 'value' remains possibly uninitialized. In that case 'value'
> > shouldn't be used to produce the store_sys_acpi()s return value.
> > 
> > Only test the return value of set_acpi() if we can actually call it.
> > Return rv otherwise.
> > 
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index bd533c2..41f12ba 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> >  	int rv, value;
> >  
> >  	rv = parse_arg(buf, count, &value);
> > -	if (rv > 0)
> > -		value = set_acpi(eeepc, cm, value);
> 
> That was rather horrible wasn't it? :-)
> 
> > -	if (value < 0)
> > -		return -EIO;
> > +	if (rv > 0) {
> > +		if (set_acpi(eeepc, cm, value) < 0)
> > +			return -EIO;
> 
> Is there a compelling reason not to propogate the return code of set_acpi?
> (ENODEV specifically). I see -EIO in Documentation/filesystems/sysfs.txt, but
> it's used by default if the show() pointer is NULL (for example), but otherwise
> propogates the error.
> 
> Specifically it states:
> 
> - show() or store() can always return errors. If a bad value comes
>   through, be sure to return an error.
> 
> Greg, does this need to be -EIO? or is returning someting like ENODEV preferable
> if it more accurately reflects the error?

Just return the value of set_acpi() and you should be fine.

thanks,

greg k-h

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Frans Klaver]

On Thu, Sep 4, 2014 at 3:14 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Sep 03, 2014 at 05:49:47PM -0700, Darren Hart wrote:
>> On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
>> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
>> > fails, 'value' remains possibly uninitialized. In that case 'value'
>> > shouldn't be used to produce the store_sys_acpi()s return value.

Here I should probably remove either 'the' or the 's' after store_sys_acpi().


>> > Only test the return value of set_acpi() if we can actually call it.
>> > Return rv otherwise.
>> >
>> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> > ---
>> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
>> > index bd533c2..41f12ba 100644
>> > --- a/drivers/platform/x86/eeepc-laptop.c
>> > +++ b/drivers/platform/x86/eeepc-laptop.c
>> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>> >     int rv, value;
>> >
>> >     rv = parse_arg(buf, count, &value);
>> > -   if (rv > 0)
>> > -           value = set_acpi(eeepc, cm, value);
>>
>> That was rather horrible wasn't it? :-)
>>
>> > -   if (value < 0)
>> > -           return -EIO;
>> > +   if (rv > 0) {
>> > +           if (set_acpi(eeepc, cm, value) < 0)
>> > +                   return -EIO;
>>
>> Is there a compelling reason not to propogate the return code of set_acpi?
>> (ENODEV specifically). I see -EIO in Documentation/filesystems/sysfs.txt, but
>> it's used by default if the show() pointer is NULL (for example), but otherwise
>> propogates the error.
>>
>> Specifically it states:
>>
>> - show() or store() can always return errors. If a bad value comes
>>   through, be sure to return an error.
>>
>> Greg, does this need to be -EIO? or is returning someting like ENODEV preferable
>> if it more accurately reflects the error?
>
> Just return the value of set_acpi() and you should be fine.

According to 6dff29b63a5bf2eaf3313cb8a84f0b7520c43401 "eeepc-laptop:
disp attribute should be write-only" it should be -EIO. -ENODEV would
be misleading.

Thanks,
Frans

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Paul Bolle]

On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
> In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> fails, 'value' remains possibly uninitialized. In that case 'value'
> shouldn't be used to produce the store_sys_acpi()s return value.
> 
> Only test the return value of set_acpi() if we can actually call it.
> Return rv otherwise.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c2..41f12ba 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>  	int rv, value;
>  
>  	rv = parse_arg(buf, count, &value);
> -	if (rv > 0)
> -		value = set_acpi(eeepc, cm, value);
> -	if (value < 0)
> -		return -EIO;
> +	if (rv > 0) {
> +		if (set_acpi(eeepc, cm, value) < 0)
> +			return -EIO;
> +	}
>  	return rv;
>  }
>  

The warning that this code (currently) generated triggered me to submit
https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
get rid of it. I received no reactions so far. Here's that patch again:

------------>8------------
From: Paul Bolle <pebolle@tiscali.nl>
Subject: [PATCH] eeepc-laptop: simplify parse_arg()

parse_arg() has three possible return values:
    -EINVAL if sscanf(), in short, fails;
    zero if "count" is zero; and
    "count" in all other cases

But "count" will never be zero. See, parse_arg() is called by the
various store functions. And the callchain of these functions starts
with sysfs_kf_write(). And that function checks for a zero "count". So
we can stop checking for a zero "count", drop the "count" argument
entirely, and transform parse_arg() into a function that returns zero on
success or a negative error. That, in turn, allows to make those store
functions just return "count" on success. The net effect is that the
code becomes a bit easier to understand.

A nice side effect is that this GCC warning is silenced too:
    drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
    drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      int rv, value;

Which is, of course, the reason to have a look at parse_arg().

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bd533c22be57..78515b850165 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
 /*
  * Sys helpers
  */
-static int parse_arg(const char *buf, unsigned long count, int *val)
+static int parse_arg(const char *buf, int *val)
 {
-	if (!count)
-		return 0;
 	if (sscanf(buf, "%i", val) != 1)
 		return -EINVAL;
-	return count;
+	return 0;
 }
 
 static ssize_t store_sys_acpi(struct device *dev, int cm,
@@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		value = set_acpi(eeepc, cm, value);
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	value = set_acpi(eeepc, cm, value);
 	if (value < 0)
 		return -EIO;
-	return rv;
+	return count;
 }
 
 static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
@@ -377,13 +376,13 @@ static ssize_t store_cpufv(struct device *dev,
 		return -EPERM;
 	if (get_cpufv(eeepc, &c))
 		return -ENODEV;
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
-	if (!rv || value < 0 || value >= c.num)
+	if (value < 0 || value >= c.num)
 		return -EINVAL;
 	set_acpi(eeepc, CM_ASL_CPUFV, value);
-	return rv;
+	return count;
 }
 
 static ssize_t show_cpufv_disabled(struct device *dev,
@@ -402,7 +401,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
 
@@ -412,7 +411,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 			pr_warn("cpufv enabled (not officially supported "
 				"on this model)\n");
 		eeepc->cpufv_disabled = false;
-		return rv;
+		return count;
 	case 1:
 		return -EPERM;
 	default:
@@ -1042,10 +1041,11 @@ static ssize_t store_sys_hwmon(void (*set)(int), const char *buf, size_t count)
 {
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		set(value);
-	return rv;
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	set(value);
+	return count;
 }
 
 static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
-- 


Paul Bolle

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Frans Klaver]

On Thu, Sep 4, 2014 at 9:08 AM, Paul Bolle <pebolle@tiscali.nl> wrote:
> On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
>> In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
>> fails, 'value' remains possibly uninitialized. In that case 'value'
>> shouldn't be used to produce the store_sys_acpi()s return value.
>>
>> Only test the return value of set_acpi() if we can actually call it.
>> Return rv otherwise.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>>  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
>> index bd533c2..41f12ba 100644
>> --- a/drivers/platform/x86/eeepc-laptop.c
>> +++ b/drivers/platform/x86/eeepc-laptop.c
>> @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>>       int rv, value;
>>
>>       rv = parse_arg(buf, count, &value);
>> -     if (rv > 0)
>> -             value = set_acpi(eeepc, cm, value);
>> -     if (value < 0)
>> -             return -EIO;
>> +     if (rv > 0) {
>> +             if (set_acpi(eeepc, cm, value) < 0)
>> +                     return -EIO;
>> +     }
>>       return rv;
>>  }
>>
>
> The warning that this code (currently) generated triggered me to submit
> https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
> get rid of it. I received no reactions so far. Here's that patch again:
>
> ------------>8------------
> From: Paul Bolle <pebolle@tiscali.nl>
> Subject: [PATCH] eeepc-laptop: simplify parse_arg()
>
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
>
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
>
> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
>
> Which is, of course, the reason to have a look at parse_arg().
>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

Curious. I hadn't found this one when searching for earlier patches.

Thanks,
Frans


> ---
>  drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c22be57..78515b850165 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
>  /*
>   * Sys helpers
>   */
> -static int parse_arg(const char *buf, unsigned long count, int *val)
> +static int parse_arg(const char *buf, int *val)
>  {
> -       if (!count)
> -               return 0;
>         if (sscanf(buf, "%i", val) != 1)
>                 return -EINVAL;
> -       return count;
> +       return 0;
>  }
>
>  static ssize_t store_sys_acpi(struct device *dev, int cm,
> @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> -       if (rv > 0)
> -               value = set_acpi(eeepc, cm, value);
> +       rv = parse_arg(buf, &value);
> +       if (rv < 0)
> +               return rv;
> +       value = set_acpi(eeepc, cm, value);
>         if (value < 0)
>                 return -EIO;
> -       return rv;
> +       return count;
>  }
>
>  static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
> @@ -377,13 +376,13 @@ static ssize_t store_cpufv(struct device *dev,
>                 return -EPERM;
>         if (get_cpufv(eeepc, &c))
>                 return -ENODEV;
> -       rv = parse_arg(buf, count, &value);
> +       rv = parse_arg(buf, &value);
>         if (rv < 0)
>                 return rv;
> -       if (!rv || value < 0 || value >= c.num)
> +       if (value < 0 || value >= c.num)
>                 return -EINVAL;
>         set_acpi(eeepc, CM_ASL_CPUFV, value);
> -       return rv;
> +       return count;
>  }
>
>  static ssize_t show_cpufv_disabled(struct device *dev,
> @@ -402,7 +401,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
>         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> +       rv = parse_arg(buf, &value);
>         if (rv < 0)
>                 return rv;
>
> @@ -412,7 +411,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
>                         pr_warn("cpufv enabled (not officially supported "
>                                 "on this model)\n");
>                 eeepc->cpufv_disabled = false;
> -               return rv;
> +               return count;
>         case 1:
>                 return -EPERM;
>         default:
> @@ -1042,10 +1041,11 @@ static ssize_t store_sys_hwmon(void (*set)(int), const char *buf, size_t count)
>  {
>         int rv, value;
>
> -       rv = parse_arg(buf, count, &value);
> -       if (rv > 0)
> -               set(value);
> -       return rv;
> +       rv = parse_arg(buf, &value);
> +       if (rv < 0)
> +               return rv;
> +       set(value);
> +       return count;
>  }
>
>  static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
> --
>
>
> Paul Bolle
>

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Greg Kroah-Hartman]

On Thu, Sep 04, 2014 at 08:46:40AM +0200, Frans Klaver wrote:
> On Thu, Sep 4, 2014 at 3:14 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Sep 03, 2014 at 05:49:47PM -0700, Darren Hart wrote:
> >> On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
> >> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> >> > fails, 'value' remains possibly uninitialized. In that case 'value'
> >> > shouldn't be used to produce the store_sys_acpi()s return value.
> 
> Here I should probably remove either 'the' or the 's' after store_sys_acpi().
> 
> 
> >> > Only test the return value of set_acpi() if we can actually call it.
> >> > Return rv otherwise.
> >> >
> >> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> >> > ---
> >> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
> >> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> >> > index bd533c2..41f12ba 100644
> >> > --- a/drivers/platform/x86/eeepc-laptop.c
> >> > +++ b/drivers/platform/x86/eeepc-laptop.c
> >> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> >> >     int rv, value;
> >> >
> >> >     rv = parse_arg(buf, count, &value);
> >> > -   if (rv > 0)
> >> > -           value = set_acpi(eeepc, cm, value);
> >>
> >> That was rather horrible wasn't it? :-)
> >>
> >> > -   if (value < 0)
> >> > -           return -EIO;
> >> > +   if (rv > 0) {
> >> > +           if (set_acpi(eeepc, cm, value) < 0)
> >> > +                   return -EIO;
> >>
> >> Is there a compelling reason not to propogate the return code of set_acpi?
> >> (ENODEV specifically). I see -EIO in Documentation/filesystems/sysfs.txt, but
> >> it's used by default if the show() pointer is NULL (for example), but otherwise
> >> propogates the error.
> >>
> >> Specifically it states:
> >>
> >> - show() or store() can always return errors. If a bad value comes
> >>   through, be sure to return an error.
> >>
> >> Greg, does this need to be -EIO? or is returning someting like ENODEV preferable
> >> if it more accurately reflects the error?
> >
> > Just return the value of set_acpi() and you should be fine.
> 
> According to 6dff29b63a5bf2eaf3313cb8a84f0b7520c43401 "eeepc-laptop:
> disp attribute should be write-only" it should be -EIO. -ENODEV would
> be misleading.

If something is "write only" then there should not be a store function
for it at all, the file should not be marked as writable, to prevent
anything from ever being written to it at the higher-level filesystem
layer, and never get down to the driver layer...

thanks,

greg k-h

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Frans Klaver]

On Thu, Sep 4, 2014 at 4:10 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 04, 2014 at 08:46:40AM +0200, Frans Klaver wrote:
>> On Thu, Sep 4, 2014 at 3:14 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Sep 03, 2014 at 05:49:47PM -0700, Darren Hart wrote:
>> >> On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
>> >> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
>> >> > fails, 'value' remains possibly uninitialized. In that case 'value'
>> >> > shouldn't be used to produce the store_sys_acpi()s return value.
>>
>> Here I should probably remove either 'the' or the 's' after store_sys_acpi().
>>
>>
>> >> > Only test the return value of set_acpi() if we can actually call it.
>> >> > Return rv otherwise.
>> >> >
>> >> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> >> > ---
>> >> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
>> >> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
>> >> > index bd533c2..41f12ba 100644
>> >> > --- a/drivers/platform/x86/eeepc-laptop.c
>> >> > +++ b/drivers/platform/x86/eeepc-laptop.c
>> >> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>> >> >     int rv, value;
>> >> >
>> >> >     rv = parse_arg(buf, count, &value);
>> >> > -   if (rv > 0)
>> >> > -           value = set_acpi(eeepc, cm, value);
>> >>
>> >> That was rather horrible wasn't it? :-)
>> >>
>> >> > -   if (value < 0)
>> >> > -           return -EIO;
>> >> > +   if (rv > 0) {
>> >> > +           if (set_acpi(eeepc, cm, value) < 0)
>> >> > +                   return -EIO;
>> >>
>> >> Is there a compelling reason not to propogate the return code of set_acpi?
>> >> (ENODEV specifically). I see -EIO in Documentation/filesystems/sysfs.txt, but
>> >> it's used by default if the show() pointer is NULL (for example), but otherwise
>> >> propogates the error.
>> >>
>> >> Specifically it states:
>> >>
>> >> - show() or store() can always return errors. If a bad value comes
>> >>   through, be sure to return an error.
>> >>
>> >> Greg, does this need to be -EIO? or is returning someting like ENODEV preferable
>> >> if it more accurately reflects the error?
>> >
>> > Just return the value of set_acpi() and you should be fine.
>>
>> According to 6dff29b63a5bf2eaf3313cb8a84f0b7520c43401 "eeepc-laptop:
>> disp attribute should be write-only" it should be -EIO. -ENODEV would
>> be misleading.
>
> If something is "write only" then there should not be a store function
> for it at all, the file should not be marked as writable, to prevent
> anything from ever being written to it at the higher-level filesystem
> layer, and never get down to the driver layer...

If something is "write only" it should not have a show function.
Removing the show function for the disp attribute would probably also
remove the necessity for the file permissions.

I'll I fire up my eeepc and see what I can figure out. Should I take
Pauls patch and see how it fits into this?

Thanks,
Frans

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Paul Bolle]

On Thu, 2014-09-04 at 16:40 +0200, Frans Klaver wrote:
> I'll I fire up my eeepc and see what I can figure out. Should I take
> Pauls patch and see how it fits into this?

I haven't followed the conversation you're having with Greg, but I would
be grateful if you'd test _just_ my patch. Unless someone jumps in and
points to flaws in my patch, that is.

I suppose testing would mean writing some values to the related files
under /sys. I have no idea whatsoever what those files actually do, so
I'm afraid I can't tell you what values to write and what effects to
expect, sorry. Perhaps someone else can help you there.


Paul Bolle

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Darren Hart]

On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> > fails, 'value' remains possibly uninitialized. In that case 'value'
> > shouldn't be used to produce the store_sys_acpi()s return value.
> > 
> > Only test the return value of set_acpi() if we can actually call it.
> > Return rv otherwise.
> > 
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index bd533c2..41f12ba 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> >  	int rv, value;
> >  
> >  	rv = parse_arg(buf, count, &value);
> > -	if (rv > 0)
> > -		value = set_acpi(eeepc, cm, value);
> > -	if (value < 0)
> > -		return -EIO;
> > +	if (rv > 0) {
> > +		if (set_acpi(eeepc, cm, value) < 0)
> > +			return -EIO;
> > +	}
> >  	return rv;
> >  }
> >  
> 
> The warning that this code (currently) generated triggered me to submit
> https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
> get rid of it. I received no reactions so far. Here's that patch again:

Thanks for resending.

> 
> ------------>8------------
> From: Paul Bolle <pebolle@tiscali.nl>
> Subject: [PATCH] eeepc-laptop: simplify parse_arg()
> 
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
> 
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
> 

Seems reasonable.

> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
> 
> Which is, of course, the reason to have a look at parse_arg().
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c22be57..78515b850165 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
>  /*
>   * Sys helpers
>   */
> -static int parse_arg(const char *buf, unsigned long count, int *val)
> +static int parse_arg(const char *buf, int *val)
>  {
> -	if (!count)
> -		return 0;
>  	if (sscanf(buf, "%i", val) != 1)
>  		return -EINVAL;
> -	return count;
> +	return 0;
>  }
>  
>  static ssize_t store_sys_acpi(struct device *dev, int cm,
> @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>  	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv > 0)
> -		value = set_acpi(eeepc, cm, value);
> +	rv = parse_arg(buf, &value);
> +	if (rv < 0)
> +		return rv;
> +	value = set_acpi(eeepc, cm, value);
>  	if (value < 0)

I suppose it's harmless, but it would be more explicit to reuse rv here instead
of value.

>  		return -EIO;

And as with Frans' version, I suggest propogating the error. We're talking about
a missing/invalid ACPI control method name here, ENODEV seems approprirate.

Rafael, do you have a strong preference about what to return in such an event?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Rafael J. Wysocki]

On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
> On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> > On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
> > > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> > > fails, 'value' remains possibly uninitialized. In that case 'value'
> > > shouldn't be used to produce the store_sys_acpi()s return value.
> > > 
> > > Only test the return value of set_acpi() if we can actually call it.
> > > Return rv otherwise.
> > > 
> > > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > > ---
> > >  drivers/platform/x86/eeepc-laptop.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > > index bd533c2..41f12ba 100644
> > > --- a/drivers/platform/x86/eeepc-laptop.c
> > > +++ b/drivers/platform/x86/eeepc-laptop.c
> > > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> > >  	int rv, value;
> > >  
> > >  	rv = parse_arg(buf, count, &value);
> > > -	if (rv > 0)
> > > -		value = set_acpi(eeepc, cm, value);
> > > -	if (value < 0)
> > > -		return -EIO;
> > > +	if (rv > 0) {
> > > +		if (set_acpi(eeepc, cm, value) < 0)
> > > +			return -EIO;
> > > +	}
> > >  	return rv;
> > >  }
> > >  
> > 
> > The warning that this code (currently) generated triggered me to submit
> > https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
> > get rid of it. I received no reactions so far. Here's that patch again:
> 
> Thanks for resending.
> 
> > 
> > ------------>8------------
> > From: Paul Bolle <pebolle@tiscali.nl>
> > Subject: [PATCH] eeepc-laptop: simplify parse_arg()
> > 
> > parse_arg() has three possible return values:
> >     -EINVAL if sscanf(), in short, fails;
> >     zero if "count" is zero; and
> >     "count" in all other cases
> > 
> > But "count" will never be zero. See, parse_arg() is called by the
> > various store functions. And the callchain of these functions starts
> > with sysfs_kf_write(). And that function checks for a zero "count". So
> > we can stop checking for a zero "count", drop the "count" argument
> > entirely, and transform parse_arg() into a function that returns zero on
> > success or a negative error. That, in turn, allows to make those store
> > functions just return "count" on success. The net effect is that the
> > code becomes a bit easier to understand.
> > 
> 
> Seems reasonable.
> 
> > A nice side effect is that this GCC warning is silenced too:
> >     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
> >     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >       int rv, value;
> > 
> > Which is, of course, the reason to have a look at parse_arg().
> > 
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
> >  1 file changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index bd533c22be57..78515b850165 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
> >  /*
> >   * Sys helpers
> >   */
> > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > +static int parse_arg(const char *buf, int *val)
> >  {
> > -	if (!count)
> > -		return 0;
> >  	if (sscanf(buf, "%i", val) != 1)
> >  		return -EINVAL;
> > -	return count;
> > +	return 0;
> >  }
> >  
> >  static ssize_t store_sys_acpi(struct device *dev, int cm,
> > @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> >  	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
> >  	int rv, value;
> >  
> > -	rv = parse_arg(buf, count, &value);
> > -	if (rv > 0)
> > -		value = set_acpi(eeepc, cm, value);
> > +	rv = parse_arg(buf, &value);
> > +	if (rv < 0)
> > +		return rv;
> > +	value = set_acpi(eeepc, cm, value);
> >  	if (value < 0)
> 
> I suppose it's harmless, but it would be more explicit to reuse rv here instead
> of value.
> 
> >  		return -EIO;
> 
> And as with Frans' version, I suggest propogating the error. We're talking about
> a missing/invalid ACPI control method name here, ENODEV seems approprirate.
> 
> Rafael, do you have a strong preference about what to return in such an event?

No, I don't, although -ENXIO could be used here too.

Rafael

[PATCH] eeepc-laptop: remove disp attribute show function

[Frans Klaver]

The disp attribute is write-only. This is enforced by mimicking sysfs
behavior store_sys_acpi() and show_sys_acpi(), but this is not ideal.
Behaving like sysfs is better left to sysfs.

Remove the show function from the disp attribute. This ensures userspace
can only write to disp at all times. While at it, propagate any errors
produced in store_sys_acpi() and show_sys_acpi(), instead of overriding
them with -EIO.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---

This patch applies on top of Paul's "eeepc-laptop: simplify parse_arg()".
Tested both patches separately, as well as combined. As far as I can see the
system behaves exactly like before.

 drivers/platform/x86/eeepc-laptop.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 78515b8..03bcbed 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -281,7 +281,7 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
 		return rv;
 	value = set_acpi(eeepc, cm, value);
 	if (value < 0)
-		return -EIO;
+		return value;
 	return count;
 }
 
@@ -291,7 +291,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	int value = get_acpi(eeepc, cm);
 
 	if (value < 0)
-		return -EIO;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -318,7 +318,20 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 
 EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
 EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
-EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
+
+static ssize_t store_disp(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	return store_sys_acpi(dev, CM_ASL_DISPLAYSWITCH, buf, count);
+}
+
+static struct device_attribute dev_attr_disp = {
+	.attr = {
+		.name = "disp",
+		.mode = 0200 },
+	.store  = store_disp,
+};
 
 struct eeepc_cpufv {
 	int num;
-- 
2.1.0

Re: [PATCH] eeepc-laptop: remove disp attribute show function

[Greg Kroah-Hartman]

On Mon, Sep 08, 2014 at 11:12:42PM +0200, Frans Klaver wrote:
> The disp attribute is write-only. This is enforced by mimicking sysfs
> behavior store_sys_acpi() and show_sys_acpi(), but this is not ideal.
> Behaving like sysfs is better left to sysfs.
> 
> Remove the show function from the disp attribute. This ensures userspace
> can only write to disp at all times. While at it, propagate any errors
> produced in store_sys_acpi() and show_sys_acpi(), instead of overriding
> them with -EIO.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
> 
> This patch applies on top of Paul's "eeepc-laptop: simplify parse_arg()".
> Tested both patches separately, as well as combined. As far as I can see the
> system behaves exactly like before.
> 
>  drivers/platform/x86/eeepc-laptop.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 78515b8..03bcbed 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -281,7 +281,7 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>  		return rv;
>  	value = set_acpi(eeepc, cm, value);
>  	if (value < 0)
> -		return -EIO;
> +		return value;
>  	return count;
>  }
>  
> @@ -291,7 +291,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  	int value = get_acpi(eeepc, cm);
>  
>  	if (value < 0)
> -		return -EIO;
> +		return value;
>  	return sprintf(buf, "%d\n", value);
>  }
>  
> @@ -318,7 +318,20 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  
>  EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
>  EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
> -EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
> +
> +static ssize_t store_disp(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	return store_sys_acpi(dev, CM_ASL_DISPLAYSWITCH, buf, count);
> +}
> +
> +static struct device_attribute dev_attr_disp = {
> +	.attr = {
> +		.name = "disp",
> +		.mode = 0200 },
> +	.store  = store_disp,
> +};

DEVICE_ATTR()?

Re: [PATCH] eeepc-laptop: remove disp attribute show function

[Greg Kroah-Hartman]

On Mon, Sep 08, 2014 at 11:44:38PM +0200, Frans Klaver wrote:
> On Mon, Sep 08, 2014 at 11:23:06PM +0200, Frans Klaver wrote:
> > On Mon, Sep 08, 2014 at 02:16:27PM -0700, Greg Kroah-Hartman wrote:
> > > On Mon, Sep 08, 2014 at 11:12:42PM +0200, Frans Klaver wrote:
> > > >
> > > >  EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
> > > >  EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
> > > > -EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
> > > > +
> > > > +static ssize_t store_disp(struct device *dev,
> > > > +			  struct device_attribute *attr,
> > > > +			  const char *buf, size_t count)
> > > > +{
> > > > +	return store_sys_acpi(dev, CM_ASL_DISPLAYSWITCH, buf, count);
> > > > +}
> > > > +
> > > > +static struct device_attribute dev_attr_disp = {
> > > > +	.attr = {
> > > > +		.name = "disp",
> > > > +		.mode = 0200 },
> > > > +	.store  = store_disp,
> > > > +};
> > > 
> > > DEVICE_ATTR()?
> > 
> > That would make sense, wouldn't it? The only defense I have is that it
> > isn't used in EEEPC_CREATE_DEVICE_ATTR() either.
> 
> I might as well fix that as well, and the fact that the octal
> permissions are used instead of S_IRUGO and friends. Would you prefer
> that in a separate patch?

I'm not taking these patches, so I can't answer that...

Re: [PATCH] eeepc-laptop: remove disp attribute show function

[Darren Hart]

On Mon, Sep 08, 2014 at 02:57:23PM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 08, 2014 at 11:44:38PM +0200, Frans Klaver wrote:
> > On Mon, Sep 08, 2014 at 11:23:06PM +0200, Frans Klaver wrote:
> > > On Mon, Sep 08, 2014 at 02:16:27PM -0700, Greg Kroah-Hartman wrote:
> > > > On Mon, Sep 08, 2014 at 11:12:42PM +0200, Frans Klaver wrote:
> > > > >
> > > > >  EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
> > > > >  EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
> > > > > -EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
> > > > > +
> > > > > +static ssize_t store_disp(struct device *dev,
> > > > > +			  struct device_attribute *attr,
> > > > > +			  const char *buf, size_t count)
> > > > > +{
> > > > > +	return store_sys_acpi(dev, CM_ASL_DISPLAYSWITCH, buf, count);
> > > > > +}
> > > > > +
> > > > > +static struct device_attribute dev_attr_disp = {
> > > > > +	.attr = {
> > > > > +		.name = "disp",
> > > > > +		.mode = 0200 },
> > > > > +	.store  = store_disp,
> > > > > +};
> > > > 
> > > > DEVICE_ATTR()?
> > > 
> > > That would make sense, wouldn't it? The only defense I have is that it
> > > isn't used in EEEPC_CREATE_DEVICE_ATTR() either.
> > 
> > I might as well fix that as well, and the fact that the octal
> > permissions are used instead of S_IRUGO and friends. Would you prefer
> > that in a separate patch?

If you're offering to fix it (thanks!), please do any necessary cleanup first
(like switching to S_IRUGO and friends), and then follow up with a DEVICE_ATTR
solution.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Darren Hart]

On Thu, Sep 04, 2014 at 12:53:25AM +0200, Frans Klaver wrote:
> In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> fails, 'value' remains possibly uninitialized. In that case 'value'
> shouldn't be used to produce the store_sys_acpi()s return value.
> 
> Only test the return value of set_acpi() if we can actually call it.
> Return rv otherwise.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>


FYI, based on recent discussion with Paul, Greg, and Frans, I am not picking
this up, but waiting for a next rev from Frans, possibly including Paul's
original patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Paul Bolle]

Hi Darren,

On Sat, 2014-09-06 at 23:17 +0200, Rafael J. Wysocki wrote:
> On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
> > On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> > [...]
> > >  static ssize_t store_sys_acpi(struct device *dev, int cm,
> > > @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> > >  	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
> > >  	int rv, value;
> > >  
> > > -	rv = parse_arg(buf, count, &value);
> > > -	if (rv > 0)
> > > -		value = set_acpi(eeepc, cm, value);
> > > +	rv = parse_arg(buf, &value);
> > > +	if (rv < 0)
> > > +		return rv;
> > > +	value = set_acpi(eeepc, cm, value);
> > >  	if (value < 0)
> > 
> > I suppose it's harmless, but it would be more explicit to reuse rv here instead
> > of value.

Fine with me.
 
> > >  		return -EIO;
> > 
> > And as with Frans' version, I suggest propogating the error. We're talking about
> > a missing/invalid ACPI control method name here, ENODEV seems approprirate.
> > 
> > Rafael, do you have a strong preference about what to return in such an event?
> 
> No, I don't, although -ENXIO could be used here too.

If you could say what value you'd like best I'll resend using that
value. (I don't know what the effect is of using a specific error here,
so I guess I'll have to bluff about it in the commit explanation.)

Thanks,


Paul Bolle

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Darren Hart]

On Tue, Sep 09, 2014 at 10:50:08AM +0200, Paul Bolle wrote:
> Hi Darren,
> 
> On Sat, 2014-09-06 at 23:17 +0200, Rafael J. Wysocki wrote:
> > On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
> > > On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> > > [...]
> > > >  static ssize_t store_sys_acpi(struct device *dev, int cm,
> > > > @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> > > >  	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
> > > >  	int rv, value;
> > > >  
> > > > -	rv = parse_arg(buf, count, &value);
> > > > -	if (rv > 0)
> > > > -		value = set_acpi(eeepc, cm, value);
> > > > +	rv = parse_arg(buf, &value);
> > > > +	if (rv < 0)
> > > > +		return rv;
> > > > +	value = set_acpi(eeepc, cm, value);
> > > >  	if (value < 0)
> > > 
> > > I suppose it's harmless, but it would be more explicit to reuse rv here instead
> > > of value.
> 
> Fine with me.
>  
> > > >  		return -EIO;
> > > 
> > > And as with Frans' version, I suggest propogating the error. We're talking about
> > > a missing/invalid ACPI control method name here, ENODEV seems approprirate.
> > > 
> > > Rafael, do you have a strong preference about what to return in such an event?
> > 
> > No, I don't, although -ENXIO could be used here too.
> 
> If you could say what value you'd like best I'll resend using that
> value. (I don't know what the effect is of using a specific error here,
> so I guess I'll have to bluff about it in the commit explanation.)

First, I would prefer we propogate the error code rather than remap it.

We could consider changing what the callee returns...

#define EIO              5      /* I/O error */
#define ENXIO            6      /* No such device or address */
#define ENODEV          19      /* No such device */

Of those, ENXIO seems like the most appropriate in this case.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Frans Klaver]

On Wed, Sep 10, 2014 at 5:33 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Tue, Sep 09, 2014 at 10:50:08AM +0200, Paul Bolle wrote:
>> Hi Darren,
>>
>> On Sat, 2014-09-06 at 23:17 +0200, Rafael J. Wysocki wrote:
>> > On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
>> > > On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
>> > > [...]
>> > > >  static ssize_t store_sys_acpi(struct device *dev, int cm,
>> > > > @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
>> > > >         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>> > > >         int rv, value;
>> > > >
>> > > > -       rv = parse_arg(buf, count, &value);
>> > > > -       if (rv > 0)
>> > > > -               value = set_acpi(eeepc, cm, value);
>> > > > +       rv = parse_arg(buf, &value);
>> > > > +       if (rv < 0)
>> > > > +               return rv;
>> > > > +       value = set_acpi(eeepc, cm, value);
>> > > >         if (value < 0)
>> > >
>> > > I suppose it's harmless, but it would be more explicit to reuse rv here instead
>> > > of value.
>>
>> Fine with me.
>>
>> > > >                 return -EIO;
>> > >
>> > > And as with Frans' version, I suggest propogating the error. We're talking about
>> > > a missing/invalid ACPI control method name here, ENODEV seems approprirate.
>> > >
>> > > Rafael, do you have a strong preference about what to return in such an event?
>> >
>> > No, I don't, although -ENXIO could be used here too.
>>
>> If you could say what value you'd like best I'll resend using that
>> value. (I don't know what the effect is of using a specific error here,
>> so I guess I'll have to bluff about it in the commit explanation.)
>
> First, I would prefer we propogate the error code rather than remap it.
>
> We could consider changing what the callee returns...
>
> #define EIO              5      /* I/O error */
> #define ENXIO            6      /* No such device or address */
> #define ENODEV          19      /* No such device */
>
> Of those, ENXIO seems like the most appropriate in this case.

Would it be fair to say that for consistency we should then also
change the return values of acpi_setter_handle()? It has the same
basic layout and checks as set_acpi() and get_acpi() have.

Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value

[Darren Hart]

On Wed, Sep 10, 2014 at 04:42:49PM +0200, Frans Klaver wrote:
> On Wed, Sep 10, 2014 at 5:33 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Tue, Sep 09, 2014 at 10:50:08AM +0200, Paul Bolle wrote:
> >> Hi Darren,
> >>
> >> On Sat, 2014-09-06 at 23:17 +0200, Rafael J. Wysocki wrote:
> >> > On Friday, September 05, 2014 07:17:57 PM Darren Hart wrote:
> >> > > On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> >> > > [...]
> >> > > >  static ssize_t store_sys_acpi(struct device *dev, int cm,
> >> > > > @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> >> > > >         struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
> >> > > >         int rv, value;
> >> > > >
> >> > > > -       rv = parse_arg(buf, count, &value);
> >> > > > -       if (rv > 0)
> >> > > > -               value = set_acpi(eeepc, cm, value);
> >> > > > +       rv = parse_arg(buf, &value);
> >> > > > +       if (rv < 0)
> >> > > > +               return rv;
> >> > > > +       value = set_acpi(eeepc, cm, value);
> >> > > >         if (value < 0)
> >> > >
> >> > > I suppose it's harmless, but it would be more explicit to reuse rv here instead
> >> > > of value.
> >>
> >> Fine with me.
> >>
> >> > > >                 return -EIO;
> >> > >
> >> > > And as with Frans' version, I suggest propogating the error. We're talking about
> >> > > a missing/invalid ACPI control method name here, ENODEV seems approprirate.
> >> > >
> >> > > Rafael, do you have a strong preference about what to return in such an event?
> >> >
> >> > No, I don't, although -ENXIO could be used here too.
> >>
> >> If you could say what value you'd like best I'll resend using that
> >> value. (I don't know what the effect is of using a specific error here,
> >> so I guess I'll have to bluff about it in the commit explanation.)
> >
> > First, I would prefer we propogate the error code rather than remap it.
> >
> > We could consider changing what the callee returns...
> >
> > #define EIO              5      /* I/O error */
> > #define ENXIO            6      /* No such device or address */
> > #define ENODEV          19      /* No such device */
> >
> > Of those, ENXIO seems like the most appropriate in this case.
> 
> Would it be fair to say that for consistency we should then also
> change the return values of acpi_setter_handle()? It has the same
> basic layout and checks as set_acpi() and get_acpi() have.

Yes, that would be appropriate as well.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v2] eeepc-laptop: simplify parse_arg()

[Paul Bolle]

parse_arg() has three possible return values:
    -EINVAL if sscanf(), in short, fails;
    zero if "count" is zero; and
    "count" in all other cases

But "count" will never be zero. See, parse_arg() is called by the
various store functions. And the callchain of these functions starts
with sysfs_kf_write(). And that function checks for a zero "count". So
we can stop checking for a zero "count", drop the "count" argument
entirely, and transform parse_arg() into a function that returns zero on
success or a negative error. That, in turn, allows to make those store
functions just return "count" on success. The net effect is that the
code becomes a bit easier to understand.

While we're at it, let store_sys_acpi() return whatever error set_acpi()
returns instead of remapping it to EIO.

A nice side effect is that this GCC warning is silenced too:
    drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
    drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      int rv, value;

Which is, of course, the reason to have a look at parse_arg().

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
v2: let store_sys_acpi() return whatever error set_acpi()
returns instead of remapping it to EIO. The new line about that in the
commit explanation is silly, but I couldn't come up with a better
explanation.

Still build tested only on top of v3.17-rc4 (I think Frans actually
tested v1, which is almost identical). 

There was a lot of discussion on the errors returned by this driver. I
think using new error codes is actually out of scope for this patch and
should be done in follow up patches.

 drivers/platform/x86/eeepc-laptop.c | 38 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bd533c22be57..90be993b140e 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
 /*
  * Sys helpers
  */
-static int parse_arg(const char *buf, unsigned long count, int *val)
+static int parse_arg(const char *buf, int *val)
 {
-	if (!count)
-		return 0;
 	if (sscanf(buf, "%i", val) != 1)
 		return -EINVAL;
-	return count;
+	return 0;
 }
 
 static ssize_t store_sys_acpi(struct device *dev, int cm,
@@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		value = set_acpi(eeepc, cm, value);
-	if (value < 0)
-		return -EIO;
-	return rv;
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	rv = set_acpi(eeepc, cm, value);
+	if (rv < 0)
+		return rv;
+	return count;
 }
 
 static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
@@ -377,13 +376,13 @@ static ssize_t store_cpufv(struct device *dev,
 		return -EPERM;
 	if (get_cpufv(eeepc, &c))
 		return -ENODEV;
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
-	if (!rv || value < 0 || value >= c.num)
+	if (value < 0 || value >= c.num)
 		return -EINVAL;
 	set_acpi(eeepc, CM_ASL_CPUFV, value);
-	return rv;
+	return count;
 }
 
 static ssize_t show_cpufv_disabled(struct device *dev,
@@ -402,7 +401,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
 
@@ -412,7 +411,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 			pr_warn("cpufv enabled (not officially supported "
 				"on this model)\n");
 		eeepc->cpufv_disabled = false;
-		return rv;
+		return count;
 	case 1:
 		return -EPERM;
 	default:
@@ -1042,10 +1041,11 @@ static ssize_t store_sys_hwmon(void (*set)(int), const char *buf, size_t count)
 {
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		set(value);
-	return rv;
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	set(value);
+	return count;
 }
 
 static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
-- 
1.9.3

Re: [PATCH v2] eeepc-laptop: simplify parse_arg()

[Darren Hart]

On Wed, Sep 10, 2014 at 10:05:20PM +0200, Paul Bolle wrote:
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
> 
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
> 
> While we're at it, let store_sys_acpi() return whatever error set_acpi()
> returns instead of remapping it to EIO.
> 
> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
> 
> Which is, of course, the reason to have a look at parse_arg().
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

Queued, thanks Paul.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] eeepc-laptop: simplify parse_arg()

[Darren Hart]

On Thu, Sep 11, 2014 at 03:37:39PM -0700, Darren Hart wrote:
> On Wed, Sep 10, 2014 at 10:05:20PM +0200, Paul Bolle wrote:
> > parse_arg() has three possible return values:
> >     -EINVAL if sscanf(), in short, fails;
> >     zero if "count" is zero; and
> >     "count" in all other cases
> > 
> > But "count" will never be zero. See, parse_arg() is called by the
> > various store functions. And the callchain of these functions starts
> > with sysfs_kf_write(). And that function checks for a zero "count". So
> > we can stop checking for a zero "count", drop the "count" argument
> > entirely, and transform parse_arg() into a function that returns zero on
> > success or a negative error. That, in turn, allows to make those store
> > functions just return "count" on success. The net effect is that the
> > code becomes a bit easier to understand.
> > 
> > While we're at it, let store_sys_acpi() return whatever error set_acpi()
> > returns instead of remapping it to EIO.
> > 
> > A nice side effect is that this GCC warning is silenced too:
> >     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
> >     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >       int rv, value;
> > 
> > Which is, of course, the reason to have a look at parse_arg().
> > 
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> 
> Queued, thanks Paul.

After discussion with Linus, I have had to drop this patch from the queue. We
need to restore the -EIO error code mapping to store_sys_acpi().

Apologies for the run around here, we had to define some policy around this.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v3] eeepc-laptop: simplify parse_arg()

[Paul Bolle]

parse_arg() has three possible return values:
    -EINVAL if sscanf(), in short, fails;
    zero if "count" is zero; and
    "count" in all other cases

But "count" will never be zero. See, parse_arg() is called by the
various store functions. And the callchain of these functions starts
with sysfs_kf_write(). And that function checks for a zero "count". So
we can stop checking for a zero "count", drop the "count" argument
entirely, and transform parse_arg() into a function that returns zero on
success or a negative error. That, in turn, allows to make those store
functions just return "count" on success. The net effect is that the
code becomes a bit easier to understand.

A nice side effect is that this GCC warning is silenced too:
    drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
    drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      int rv, value;

Which is, of course, the reason to have a look at parse_arg().

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
Still build tested only, but now on top of v3.17-rc5. Has Frans tested
writing zero length values to these sysfs files?

v3: store_sys_acpi() again returns -EIO if set_acpi() fails.

v2: let store_sys_acpi() return whatever error set_acpi() returns
instead of remapping it to EIO. The new line about that in the commit
explanation is silly, but I couldn't come up with a better explanation.

 drivers/platform/x86/eeepc-laptop.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bd533c22be57..3095d386c7f4 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
 /*
  * Sys helpers
  */
-static int parse_arg(const char *buf, unsigned long count, int *val)
+static int parse_arg(const char *buf, int *val)
 {
-	if (!count)
-		return 0;
 	if (sscanf(buf, "%i", val) != 1)
 		return -EINVAL;
-	return count;
+	return 0;
 }
 
 static ssize_t store_sys_acpi(struct device *dev, int cm,
@@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		value = set_acpi(eeepc, cm, value);
-	if (value < 0)
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	rv = set_acpi(eeepc, cm, value);
+	if (rv < 0)
 		return -EIO;
-	return rv;
+	return count;
 }
 
 static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
@@ -377,13 +376,13 @@ static ssize_t store_cpufv(struct device *dev,
 		return -EPERM;
 	if (get_cpufv(eeepc, &c))
 		return -ENODEV;
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
-	if (!rv || value < 0 || value >= c.num)
+	if (value < 0 || value >= c.num)
 		return -EINVAL;
 	set_acpi(eeepc, CM_ASL_CPUFV, value);
-	return rv;
+	return count;
 }
 
 static ssize_t show_cpufv_disabled(struct device *dev,
@@ -402,7 +401,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
+	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
 
@@ -412,7 +411,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 			pr_warn("cpufv enabled (not officially supported "
 				"on this model)\n");
 		eeepc->cpufv_disabled = false;
-		return rv;
+		return count;
 	case 1:
 		return -EPERM;
 	default:
@@ -1042,10 +1041,11 @@ static ssize_t store_sys_hwmon(void (*set)(int), const char *buf, size_t count)
 {
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		set(value);
-	return rv;
+	rv = parse_arg(buf, &value);
+	if (rv < 0)
+		return rv;
+	set(value);
+	return count;
 }
 
 static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
-- 
1.9.3

Re: [PATCH v3] eeepc-laptop: simplify parse_arg()

[Darren Hart]

On Wed, Sep 17, 2014 at 09:02:51PM +0200, Paul Bolle wrote:
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
> 
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
> 
> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
> 
> Which is, of course, the reason to have a look at parse_arg().
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> Still build tested only, but now on top of v3.17-rc5. Has Frans tested
> writing zero length values to these sysfs files?
> 
> v3: store_sys_acpi() again returns -EIO if set_acpi() fails.
> 
> v2: let store_sys_acpi() return whatever error set_acpi() returns
> instead of remapping it to EIO. The new line about that in the commit
> explanation is silly, but I couldn't come up with a better explanation.

Queued, thanks.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3] eeepc-laptop: simplify parse_arg()

[Darren Hart]

On Wed, Sep 17, 2014 at 09:02:51PM +0200, Paul Bolle wrote:
> parse_arg() has three possible return values:
>     -EINVAL if sscanf(), in short, fails;
>     zero if "count" is zero; and
>     "count" in all other cases
> 
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
> 
> A nice side effect is that this GCC warning is silenced too:
>     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
>     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       int rv, value;
> 
> Which is, of course, the reason to have a look at parse_arg().
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> Still build tested only, but now on top of v3.17-rc5. Has Frans tested
> writing zero length values to these sysfs files?

Frans, can you confirm testing when you get a chance please?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3] eeepc-laptop: simplify parse_arg()

[Frans Klaver]

On Wed, Sep 17, 2014 at 01:35:10PM -0700, Darren Hart wrote:
> On Wed, Sep 17, 2014 at 09:02:51PM +0200, Paul Bolle wrote:
> > parse_arg() has three possible return values:
> >     -EINVAL if sscanf(), in short, fails;
> >     zero if "count" is zero; and
> >     "count" in all other cases
> > 
> > But "count" will never be zero. See, parse_arg() is called by the
> > various store functions. And the callchain of these functions starts
> > with sysfs_kf_write(). And that function checks for a zero "count". So
> > we can stop checking for a zero "count", drop the "count" argument
> > entirely, and transform parse_arg() into a function that returns zero on
> > success or a negative error. That, in turn, allows to make those store
> > functions just return "count" on success. The net effect is that the
> > code becomes a bit easier to understand.
> > 
> > A nice side effect is that this GCC warning is silenced too:
> >     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
> >     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >       int rv, value;
> > 
> > Which is, of course, the reason to have a look at parse_arg().
> > 
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> > ---
> > Still build tested only, but now on top of v3.17-rc5. Has Frans tested
> > writing zero length values to these sysfs files?
> 
> Frans, can you confirm testing when you get a chance please?

Will do.

Frans

Re: [PATCH v3] eeepc-laptop: simplify parse_arg()

[Frans Klaver]

On Wed, Sep 17, 2014 at 01:35:10PM -0700, Darren Hart wrote:
> On Wed, Sep 17, 2014 at 09:02:51PM +0200, Paul Bolle wrote:
> > parse_arg() has three possible return values:
> >     -EINVAL if sscanf(), in short, fails;
> >     zero if "count" is zero; and
> >     "count" in all other cases
> > 
> > But "count" will never be zero. See, parse_arg() is called by the
> > various store functions. And the callchain of these functions starts
> > with sysfs_kf_write(). And that function checks for a zero "count". So
> > we can stop checking for a zero "count", drop the "count" argument
> > entirely, and transform parse_arg() into a function that returns zero on
> > success or a negative error. That, in turn, allows to make those store
> > functions just return "count" on success. The net effect is that the
> > code becomes a bit easier to understand.
> > 
> > A nice side effect is that this GCC warning is silenced too:
> >     drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
> >     drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >       int rv, value;
> > 
> > Which is, of course, the reason to have a look at parse_arg().
> > 
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> > ---
> > Still build tested only, but now on top of v3.17-rc5. Has Frans tested
> > writing zero length values to these sysfs files?
> 
> Frans, can you confirm testing when you get a chance please?

Tested. Looks OK to me.

Frans