[PATCH] asus-laptop: get rid of parse_arg()

[PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

parse_arg() duplicates the funcionality of kstrtoint() so use the latter
function instead. There is no funcionality change except that in the
case of input being too big -ERANGE will be returned instead of -EINVAL
which is not bad because -ERANGE makes more sense here. The check for
!count is already done by the sysfs core so no need to duplicate it
again. Also, add some minor corrections to error handling to accommodate
the change in return values (parse_arg returned count if everything
succeeded whereas kstrtoint returns 0 in the same situation)

As a result of this patch asus-laptop.ko size is reduced by almost 1%:
add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
function                                     old     new   delta
__UNIQUE_ID_vermagic0                         69      70      +1
ls_switch_store                              133     117     -16
ledd_store                                   175     159     -16
display_store                                157     141     -16
ls_level_store                               193     176     -17
gps_store                                    200     178     -22
sysfs_acpi_set.isra                          148     125     -23
parse_arg.part                                39       -     -39
Total: Before=19160, After=19012, chg -0.77%

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index 15f1311..28551f5 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(infos);
 
-static int parse_arg(const char *buf, unsigned long count, int *val)
-{
-	if (!count)
-		return 0;
-	if (count > 31)
-		return -EINVAL;
-	if (sscanf(buf, "%i", val) != 1)
-		return -EINVAL;
-	return count;
-}
-
 static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
 			      const char *buf, size_t count,
 			      const char *method)
 {
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv <= 0)
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
 		return rv;
 
 	if (write_acpi_int(asus->handle, method, value))
 		return -ENODEV;
-	return rv;
+	return count;
 }
 
 /*
@@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0) {
-		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
-			pr_warn("LED display write failed\n");
-			return -ENODEV;
-		}
-		asus->ledd_status = (u32) value;
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
+		return rv;
+
+	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
+		pr_warn("LED display write failed\n");
+		return -ENODEV;
 	}
-	return rv;
+
+	asus->ledd_status = (u32) value;
+	return count;
 }
 static DEVICE_ATTR_RW(ledd);
 
@@ -1148,10 +1139,12 @@ static ssize_t display_store(struct device *dev, struct device_attribute *attr,
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		asus_set_display(asus, value);
-	return rv;
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
+		return rv;
+
+	asus_set_display(asus, value);
+	return count;
 }
 static DEVICE_ATTR_WO(display);
 
@@ -1190,11 +1183,12 @@ static ssize_t ls_switch_store(struct device *dev,
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0)
-		asus_als_switch(asus, value ? 1 : 0);
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
+		return rv;
 
-	return rv;
+	asus_als_switch(asus, value ? 1 : 0);
+	return count;
 }
 static DEVICE_ATTR_RW(ls_switch);
 
@@ -1219,14 +1213,15 @@ static ssize_t ls_level_store(struct device *dev, struct device_attribute *attr,
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv > 0) {
-		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
-		/* 0 <= value <= 15 */
-		asus_als_level(asus, value);
-	}
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
+		return rv;
+
+	value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
+	/* 0 <= value <= 15 */
+	asus_als_level(asus, value);
 
-	return rv;
+	return count;
 }
 static DEVICE_ATTR_RW(ls_level);
 
@@ -1301,14 +1296,14 @@ static ssize_t gps_store(struct device *dev, struct device_attribute *attr,
 	int rv, value;
 	int ret;
 
-	rv = parse_arg(buf, count, &value);
-	if (rv <= 0)
-		return -EINVAL;
+	rv = kstrtoint(buf, 0, &value);
+	if (rv < 0)
+		return rv;
 	ret = asus_gps_switch(asus, !!value);
 	if (ret)
 		return ret;
 	rfkill_set_sw_state(asus->gps.rfkill, !value);
-	return rv;
+	return count;
 }
 static DEVICE_ATTR_RW(gps);
 
-- 
2.9.0

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Darren Hart]

On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> function instead. There is no funcionality change except that in the
> case of input being too big -ERANGE will be returned instead of -EINVAL
> which is not bad because -ERANGE makes more sense here. The check for
> !count is already done by the sysfs core so no need to duplicate it
> again. Also, add some minor corrections to error handling to accommodate
> the change in return values (parse_arg returned count if everything
> succeeded whereas kstrtoint returns 0 in the same situation)
> 
> As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> function                                     old     new   delta
> __UNIQUE_ID_vermagic0                         69      70      +1
> ls_switch_store                              133     117     -16
> ledd_store                                   175     159     -16
> display_store                                157     141     -16
> ls_level_store                               193     176     -17
> gps_store                                    200     178     -22
> sysfs_acpi_set.isra                          148     125     -23
> parse_arg.part                                39       -     -39
> Total: Before=19160, After=19012, chg -0.77%
> 
> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> ---
>  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> index 15f1311..28551f5 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(infos);
>  
> -static int parse_arg(const char *buf, unsigned long count, int *val)
> -{
> -	if (!count)
> -		return 0;
> -	if (count > 31)
> -		return -EINVAL;
> -	if (sscanf(buf, "%i", val) != 1)
> -		return -EINVAL;
> -	return count;
> -}
> -
>  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
>  			      const char *buf, size_t count,
>  			      const char *method)
>  {
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv <= 0)
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
>  		return rv;
>  
>  	if (write_acpi_int(asus->handle, method, value))
>  		return -ENODEV;
> -	return rv;
> +	return count;

This makes explicit what was hidden before - count is merely a range check, it
isn't used in parsing the string... I'm not sure if this is a problem, but it
caught my interest. If count is passed as 12, but buf only contains 3 character,
it may succeed and return 12. I suppose this is a failure in the caller, and
doesn't impact this function - unless the caller isn't expected to properly
terminate the string... but if that were the case, it would have failed
previously as we didn't check for that in parse_arg either.... this is fine as
is I suppose - can be addressed separately if need be.

>  }
>  
>  /*
> @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv > 0) {
> -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> -			pr_warn("LED display write failed\n");
> -			return -ENODEV;
> -		}
> -		asus->ledd_status = (u32) value;
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
> +		return rv;
>

This inverts the check to check for failure (this is preferred), but it does
change the successful path to include the value of 0, which was skipped over in
the original above.

> +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {

What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
above. Please consider, and apply decision to all similar instances below.

> +		pr_warn("LED display write failed\n");
> +		return -ENODEV;
>  	}
> -	return rv;
> +
> +	asus->ledd_status = (u32) value;
> +	return count;
>  }
>  static DEVICE_ATTR_RW(ledd);
>  
> @@ -1148,10 +1139,12 @@ static ssize_t display_store(struct device *dev, struct device_attribute *attr,
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv > 0)
> -		asus_set_display(asus, value);
> -	return rv;
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
> +		return rv;
> +
> +	asus_set_display(asus, value);
> +	return count;
>  }
>  static DEVICE_ATTR_WO(display);
>  
> @@ -1190,11 +1183,12 @@ static ssize_t ls_switch_store(struct device *dev,
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv > 0)
> -		asus_als_switch(asus, value ? 1 : 0);
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
> +		return rv;
>  
> -	return rv;
> +	asus_als_switch(asus, value ? 1 : 0);
> +	return count;
>  }
>  static DEVICE_ATTR_RW(ls_switch);
>  
> @@ -1219,14 +1213,15 @@ static ssize_t ls_level_store(struct device *dev, struct device_attribute *attr,
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  	int rv, value;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv > 0) {
> -		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
> -		/* 0 <= value <= 15 */
> -		asus_als_level(asus, value);
> -	}
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
> +		return rv;
> +
> +	value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
> +	/* 0 <= value <= 15 */
> +	asus_als_level(asus, value);
>  
> -	return rv;
> +	return count;
>  }
>  static DEVICE_ATTR_RW(ls_level);
>  
> @@ -1301,14 +1296,14 @@ static ssize_t gps_store(struct device *dev, struct device_attribute *attr,
>  	int rv, value;
>  	int ret;
>  
> -	rv = parse_arg(buf, count, &value);
> -	if (rv <= 0)
> -		return -EINVAL;
> +	rv = kstrtoint(buf, 0, &value);
> +	if (rv < 0)
> +		return rv;
>  	ret = asus_gps_switch(asus, !!value);
>  	if (ret)
>  		return ret;
>  	rfkill_set_sw_state(asus->gps.rfkill, !value);
> -	return rv;
> +	return count;
>  }
>  static DEVICE_ATTR_RW(gps);
>  
> -- 
> 2.9.0
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > function instead. There is no funcionality change except that in the
> > case of input being too big -ERANGE will be returned instead of -EINVAL
> > which is not bad because -ERANGE makes more sense here. The check for
> > !count is already done by the sysfs core so no need to duplicate it
> > again. Also, add some minor corrections to error handling to accommodate
> > the change in return values (parse_arg returned count if everything
> > succeeded whereas kstrtoint returns 0 in the same situation)
> > 
> > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > function                                     old     new   delta
> > __UNIQUE_ID_vermagic0                         69      70      +1
> > ls_switch_store                              133     117     -16
> > ledd_store                                   175     159     -16
> > display_store                                157     141     -16
> > ls_level_store                               193     176     -17
> > gps_store                                    200     178     -22
> > sysfs_acpi_set.isra                          148     125     -23
> > parse_arg.part                                39       -     -39
> > Total: Before=19160, After=19012, chg -0.77%
> > 
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > ---
> >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> >  1 file changed, 36 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > index 15f1311..28551f5 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(infos);
> >  
> > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > -{
> > -	if (!count)
> > -		return 0;
> > -	if (count > 31)
> > -		return -EINVAL;
> > -	if (sscanf(buf, "%i", val) != 1)
> > -		return -EINVAL;
> > -	return count;
> > -}
> > -
> >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> >  			      const char *buf, size_t count,
> >  			      const char *method)
> >  {
> >  	int rv, value;
> >  
> > -	rv = parse_arg(buf, count, &value);
> > -	if (rv <= 0)
> > +	rv = kstrtoint(buf, 0, &value);
> > +	if (rv < 0)
> >  		return rv;
> >  
> >  	if (write_acpi_int(asus->handle, method, value))
> >  		return -ENODEV;
> > -	return rv;
> > +	return count;
> 
> This makes explicit what was hidden before - count is merely a range check, it
> isn't used in parsing the string... I'm not sure if this is a problem, but it
> caught my interest. If count is passed as 12, but buf only contains 3 character,
> it may succeed and return 12. I suppose this is a failure in the caller, and
> doesn't impact this function - unless the caller isn't expected to properly
> terminate the string... but if that were the case, it would have failed
> previously as we didn't check for that in parse_arg either.... this is fine as
> is I suppose - can be addressed separately if need be.
> 
According to Documentation/filesystems/sysfs.txt:
"On write(2), ... A terminating null is added after the data on stores. This
makes functions like sysfs_streq() safe to use."
So it should be guaranteed that the buffer is a proper C string. Also, we could
say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
(as it says in the documentation) as it was before this patch (parse_int
returned count if everything succeeded).

> >  }
> >  
> >  /*
> > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> >  	int rv, value;
> >  
> > -	rv = parse_arg(buf, count, &value);
> > -	if (rv > 0) {
> > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > -			pr_warn("LED display write failed\n");
> > -			return -ENODEV;
> > -		}
> > -		asus->ledd_status = (u32) value;
> > +	rv = kstrtoint(buf, 0, &value);
> > +	if (rv < 0)
> > +		return rv;
> >
> 
> This inverts the check to check for failure (this is preferred), but it does
> change the successful path to include the value of 0, which was skipped over in
> the original above.
> 
> > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> 
> What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> above. Please consider, and apply decision to all similar instances below.
> 
Yes but in this case 0 indicates success so it doesn't make sense to test for <=
0 as it would be triggered on success. To be honest I didn't get the idea of
what you wanted to say is wrong with this patch. Could you elaborate more?

-- 
        Giedrius

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Darren Hart]

On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > function instead. There is no funcionality change except that in the
> > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > which is not bad because -ERANGE makes more sense here. The check for
> > > !count is already done by the sysfs core so no need to duplicate it
> > > again. Also, add some minor corrections to error handling to accommodate
> > > the change in return values (parse_arg returned count if everything
> > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > 
> > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > function                                     old     new   delta
> > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > ls_switch_store                              133     117     -16
> > > ledd_store                                   175     159     -16
> > > display_store                                157     141     -16
> > > ls_level_store                               193     176     -17
> > > gps_store                                    200     178     -22
> > > sysfs_acpi_set.isra                          148     125     -23
> > > parse_arg.part                                39       -     -39
> > > Total: Before=19160, After=19012, chg -0.77%
> > > 
> > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > ---
> > >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > index 15f1311..28551f5 100644
> > > --- a/drivers/platform/x86/asus-laptop.c
> > > +++ b/drivers/platform/x86/asus-laptop.c
> > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > >  }
> > >  static DEVICE_ATTR_RO(infos);
> > >  
> > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > -{
> > > -	if (!count)
> > > -		return 0;
> > > -	if (count > 31)
> > > -		return -EINVAL;
> > > -	if (sscanf(buf, "%i", val) != 1)
> > > -		return -EINVAL;
> > > -	return count;
> > > -}
> > > -
> > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > >  			      const char *buf, size_t count,
> > >  			      const char *method)
> > >  {
> > >  	int rv, value;
> > >  
> > > -	rv = parse_arg(buf, count, &value);
> > > -	if (rv <= 0)
> > > +	rv = kstrtoint(buf, 0, &value);
> > > +	if (rv < 0)
> > >  		return rv;
> > >  
> > >  	if (write_acpi_int(asus->handle, method, value))
> > >  		return -ENODEV;
> > > -	return rv;
> > > +	return count;
> > 
> > This makes explicit what was hidden before - count is merely a range check, it
> > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > it may succeed and return 12. I suppose this is a failure in the caller, and
> > doesn't impact this function - unless the caller isn't expected to properly
> > terminate the string... but if that were the case, it would have failed
> > previously as we didn't check for that in parse_arg either.... this is fine as
> > is I suppose - can be addressed separately if need be.
> > 

OK, good - thanks for the context.

> According to Documentation/filesystems/sysfs.txt:
> "On write(2), ... A terminating null is added after the data on stores. This
> makes functions like sysfs_streq() safe to use."
> So it should be guaranteed that the buffer is a proper C string. Also, we could
> say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> (as it says in the documentation) as it was before this patch (parse_int
> returned count if everything succeeded).
> 
> > >  }
> > >  
> > >  /*
> > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > >  	int rv, value;
> > >  
> > > -	rv = parse_arg(buf, count, &value);
> > > -	if (rv > 0) {
> > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > -			pr_warn("LED display write failed\n");
> > > -			return -ENODEV;
> > > -		}
> > > -		asus->ledd_status = (u32) value;
> > > +	rv = kstrtoint(buf, 0, &value);
> > > +	if (rv < 0)
> > > +		return rv;
> > >
> > 
> > This inverts the check to check for failure (this is preferred), but it does
> > change the successful path to include the value of 0, which was skipped over in
> > the original above.
> > 
> > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > 
> > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > above. Please consider, and apply decision to all similar instances below.
> > 
> Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> 0 as it would be triggered on success. To be honest I didn't get the idea of
> what you wanted to say is wrong with this patch. Could you elaborate more?
> 

Your commit message states there is no functional change, but this changes the
behavior of this function (and others) for the 0 edge case.

Previously if rv == 0, it would not call write_acpi_int(), now it will and set
the ledd_status.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > function instead. There is no funcionality change except that in the
> > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > !count is already done by the sysfs core so no need to duplicate it
> > > > again. Also, add some minor corrections to error handling to accommodate
> > > > the change in return values (parse_arg returned count if everything
> > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > 
> > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > function                                     old     new   delta
> > > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > > ls_switch_store                              133     117     -16
> > > > ledd_store                                   175     159     -16
> > > > display_store                                157     141     -16
> > > > ls_level_store                               193     176     -17
> > > > gps_store                                    200     178     -22
> > > > sysfs_acpi_set.isra                          148     125     -23
> > > > parse_arg.part                                39       -     -39
> > > > Total: Before=19160, After=19012, chg -0.77%
> > > > 
> > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > ---
> > > >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > index 15f1311..28551f5 100644
> > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > >  }
> > > >  static DEVICE_ATTR_RO(infos);
> > > >  
> > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > -{
> > > > -	if (!count)
> > > > -		return 0;
> > > > -	if (count > 31)
> > > > -		return -EINVAL;
> > > > -	if (sscanf(buf, "%i", val) != 1)
> > > > -		return -EINVAL;
> > > > -	return count;
> > > > -}
> > > > -
> > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > >  			      const char *buf, size_t count,
> > > >  			      const char *method)
> > > >  {
> > > >  	int rv, value;
> > > >  
> > > > -	rv = parse_arg(buf, count, &value);
> > > > -	if (rv <= 0)
> > > > +	rv = kstrtoint(buf, 0, &value);
> > > > +	if (rv < 0)
> > > >  		return rv;
> > > >  
> > > >  	if (write_acpi_int(asus->handle, method, value))
> > > >  		return -ENODEV;
> > > > -	return rv;
> > > > +	return count;
> > > 
> > > This makes explicit what was hidden before - count is merely a range check, it
> > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > doesn't impact this function - unless the caller isn't expected to properly
> > > terminate the string... but if that were the case, it would have failed
> > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > is I suppose - can be addressed separately if need be.
> > > 
> 
> OK, good - thanks for the context.
> 
> > According to Documentation/filesystems/sysfs.txt:
> > "On write(2), ... A terminating null is added after the data on stores. This
> > makes functions like sysfs_streq() safe to use."
> > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > (as it says in the documentation) as it was before this patch (parse_int
> > returned count if everything succeeded).
> > 
> > > >  }
> > > >  
> > > >  /*
> > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > > >  	int rv, value;
> > > >  
> > > > -	rv = parse_arg(buf, count, &value);
> > > > -	if (rv > 0) {
> > > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > -			pr_warn("LED display write failed\n");
> > > > -			return -ENODEV;
> > > > -		}
> > > > -		asus->ledd_status = (u32) value;
> > > > +	rv = kstrtoint(buf, 0, &value);
> > > > +	if (rv < 0)
> > > > +		return rv;
> > > >
> > > 
> > > This inverts the check to check for failure (this is preferred), but it does
> > > change the successful path to include the value of 0, which was skipped over in
> > > the original above.
> > > 
> > > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > 
> > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > above. Please consider, and apply decision to all similar instances below.
> > > 
> > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > what you wanted to say is wrong with this patch. Could you elaborate more?
> > 
> 
> Your commit message states there is no functional change, but this changes the
> behavior of this function (and others) for the 0 edge case.
> 
> Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> the ledd_status.
>

sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
0 but in fs/sysfs/file.c sysfs_kf_write() there already is:

if (!count)
    return 0;

-- 
        Giedrius

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Darren Hart]

On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > > function instead. There is no funcionality change except that in the
> > > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > again. Also, add some minor corrections to error handling to accommodate
> > > > > the change in return values (parse_arg returned count if everything
> > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > 
> > > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > function                                     old     new   delta
> > > > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > > > ls_switch_store                              133     117     -16
> > > > > ledd_store                                   175     159     -16
> > > > > display_store                                157     141     -16
> > > > > ls_level_store                               193     176     -17
> > > > > gps_store                                    200     178     -22
> > > > > sysfs_acpi_set.isra                          148     125     -23
> > > > > parse_arg.part                                39       -     -39
> > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > 
> > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > > ---
> > > > >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > > index 15f1311..28551f5 100644
> > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > > >  }
> > > > >  static DEVICE_ATTR_RO(infos);
> > > > >  
> > > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > > -{
> > > > > -	if (!count)
> > > > > -		return 0;
> > > > > -	if (count > 31)
> > > > > -		return -EINVAL;
> > > > > -	if (sscanf(buf, "%i", val) != 1)
> > > > > -		return -EINVAL;
> > > > > -	return count;
> > > > > -}
> > > > > -
> > > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > >  			      const char *buf, size_t count,
> > > > >  			      const char *method)
> > > > >  {
> > > > >  	int rv, value;
> > > > >  
> > > > > -	rv = parse_arg(buf, count, &value);
> > > > > -	if (rv <= 0)
> > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > +	if (rv < 0)
> > > > >  		return rv;
> > > > >  
> > > > >  	if (write_acpi_int(asus->handle, method, value))
> > > > >  		return -ENODEV;
> > > > > -	return rv;
> > > > > +	return count;
> > > > 
> > > > This makes explicit what was hidden before - count is merely a range check, it
> > > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > > doesn't impact this function - unless the caller isn't expected to properly
> > > > terminate the string... but if that were the case, it would have failed
> > > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > > is I suppose - can be addressed separately if need be.
> > > > 
> > 
> > OK, good - thanks for the context.
> > 
> > > According to Documentation/filesystems/sysfs.txt:
> > > "On write(2), ... A terminating null is added after the data on stores. This
> > > makes functions like sysfs_streq() safe to use."
> > > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > > (as it says in the documentation) as it was before this patch (parse_int
> > > returned count if everything succeeded).
> > > 
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > >  	int rv, value;
> > > > >  
> > > > > -	rv = parse_arg(buf, count, &value);
> > > > > -	if (rv > 0) {
> > > > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > -			pr_warn("LED display write failed\n");
> > > > > -			return -ENODEV;
> > > > > -		}
> > > > > -		asus->ledd_status = (u32) value;
> > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > +	if (rv < 0)
> > > > > +		return rv;
> > > > >
> > > > 
> > > > This inverts the check to check for failure (this is preferred), but it does
> > > > change the successful path to include the value of 0, which was skipped over in
> > > > the original above.
> > > > 
> > > > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > 
> > > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > > above. Please consider, and apply decision to all similar instances below.
> > > > 
> > > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > > what you wanted to say is wrong with this patch. Could you elaborate more?
> > > 
> > 
> > Your commit message states there is no functional change, but this changes the
> > behavior of this function (and others) for the 0 edge case.
> > 
> > Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> > the ledd_status.
> >
> 
> sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
> 0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
> 
> if (!count)
>     return 0;

>From a defensive programming context, we should be explicit in what we pass on
to other functions and not rely on the caller to filter for us, especially when
it's a simple matter of <= versus < in our comparison.

Please update the changed comparison sites in the patch to have the same end
result as before with respect to what it would do if a 0 were received.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Darren Hart]

On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > > function instead. There is no funcionality change except that in the
> > > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > again. Also, add some minor corrections to error handling to accommodate
> > > > > the change in return values (parse_arg returned count if everything
> > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > 
> > > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > function                                     old     new   delta
> > > > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > > > ls_switch_store                              133     117     -16
> > > > > ledd_store                                   175     159     -16
> > > > > display_store                                157     141     -16
> > > > > ls_level_store                               193     176     -17
> > > > > gps_store                                    200     178     -22
> > > > > sysfs_acpi_set.isra                          148     125     -23
> > > > > parse_arg.part                                39       -     -39
> > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > 
> > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > > ---
> > > > >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > > index 15f1311..28551f5 100644
> > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > > >  }
> > > > >  static DEVICE_ATTR_RO(infos);
> > > > >  
> > > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > > -{
> > > > > -	if (!count)
> > > > > -		return 0;
> > > > > -	if (count > 31)
> > > > > -		return -EINVAL;
> > > > > -	if (sscanf(buf, "%i", val) != 1)
> > > > > -		return -EINVAL;
> > > > > -	return count;
> > > > > -}
> > > > > -
> > > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > >  			      const char *buf, size_t count,
> > > > >  			      const char *method)
> > > > >  {
> > > > >  	int rv, value;
> > > > >  
> > > > > -	rv = parse_arg(buf, count, &value);
> > > > > -	if (rv <= 0)
> > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > +	if (rv < 0)
> > > > >  		return rv;
> > > > >  
> > > > >  	if (write_acpi_int(asus->handle, method, value))
> > > > >  		return -ENODEV;
> > > > > -	return rv;
> > > > > +	return count;
> > > > 
> > > > This makes explicit what was hidden before - count is merely a range check, it
> > > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > > doesn't impact this function - unless the caller isn't expected to properly
> > > > terminate the string... but if that were the case, it would have failed
> > > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > > is I suppose - can be addressed separately if need be.
> > > > 
> > 
> > OK, good - thanks for the context.
> > 
> > > According to Documentation/filesystems/sysfs.txt:
> > > "On write(2), ... A terminating null is added after the data on stores. This
> > > makes functions like sysfs_streq() safe to use."
> > > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > > (as it says in the documentation) as it was before this patch (parse_int
> > > returned count if everything succeeded).
> > > 
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > >  	int rv, value;
> > > > >  
> > > > > -	rv = parse_arg(buf, count, &value);
> > > > > -	if (rv > 0) {
> > > > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > -			pr_warn("LED display write failed\n");
> > > > > -			return -ENODEV;
> > > > > -		}
> > > > > -		asus->ledd_status = (u32) value;
> > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > +	if (rv < 0)
> > > > > +		return rv;
> > > > >
> > > > 
> > > > This inverts the check to check for failure (this is preferred), but it does
> > > > change the successful path to include the value of 0, which was skipped over in
> > > > the original above.
> > > > 
> > > > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > 
> > > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > > above. Please consider, and apply decision to all similar instances below.
> > > > 
> > > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > > what you wanted to say is wrong with this patch. Could you elaborate more?
> > > 
> > 
> > Your commit message states there is no functional change, but this changes the
> > behavior of this function (and others) for the 0 edge case.
> > 
> > Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> > the ledd_status.
> >
> 
> sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
> 0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
> 
> if (!count)
>     return 0;

From a defensive programming context, we should be explicit in what we pass on
to other functions and not rely on the caller to filter for us, especially when
it's a simple matter of <= versus < in our comparison.

Please update the changed comparison sites in the patch to have the same end
result as before with respect to what it would do if a 0 were received.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Wed, Aug 17, 2016 at 11:23:15AM -0700, Darren Hart wrote:
> On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > > > function instead. There is no funcionality change except that in the
> > > > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > > again. Also, add some minor corrections to error handling to accommodate
> > > > > > the change in return values (parse_arg returned count if everything
> > > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > > 
> > > > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > > function                                     old     new   delta
> > > > > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > > > > ls_switch_store                              133     117     -16
> > > > > > ledd_store                                   175     159     -16
> > > > > > display_store                                157     141     -16
> > > > > > ls_level_store                               193     176     -17
> > > > > > gps_store                                    200     178     -22
> > > > > > sysfs_acpi_set.isra                          148     125     -23
> > > > > > parse_arg.part                                39       -     -39
> > > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > > 
> > > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > > > ---
> > > > > >  drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > > > index 15f1311..28551f5 100644
> > > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > > > >  }
> > > > > >  static DEVICE_ATTR_RO(infos);
> > > > > >  
> > > > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > > > -{
> > > > > > -	if (!count)
> > > > > > -		return 0;
> > > > > > -	if (count > 31)
> > > > > > -		return -EINVAL;
> > > > > > -	if (sscanf(buf, "%i", val) != 1)
> > > > > > -		return -EINVAL;
> > > > > > -	return count;
> > > > > > -}
> > > > > > -
> > > > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > > >  			      const char *buf, size_t count,
> > > > > >  			      const char *method)
> > > > > >  {
> > > > > >  	int rv, value;
> > > > > >  
> > > > > > -	rv = parse_arg(buf, count, &value);
> > > > > > -	if (rv <= 0)
> > > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > > +	if (rv < 0)
> > > > > >  		return rv;
> > > > > >  
> > > > > >  	if (write_acpi_int(asus->handle, method, value))
> > > > > >  		return -ENODEV;
> > > > > > -	return rv;
> > > > > > +	return count;
> > > > > 
> > > > > This makes explicit what was hidden before - count is merely a range check, it
> > > > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > > > doesn't impact this function - unless the caller isn't expected to properly
> > > > > terminate the string... but if that were the case, it would have failed
> > > > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > > > is I suppose - can be addressed separately if need be.
> > > > > 
> > > 
> > > OK, good - thanks for the context.
> > > 
> > > > According to Documentation/filesystems/sysfs.txt:
> > > > "On write(2), ... A terminating null is added after the data on stores. This
> > > > makes functions like sysfs_streq() safe to use."
> > > > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > > > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > > > (as it says in the documentation) as it was before this patch (parse_int
> > > > returned count if everything succeeded).
> > > > 
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > > >  	int rv, value;
> > > > > >  
> > > > > > -	rv = parse_arg(buf, count, &value);
> > > > > > -	if (rv > 0) {
> > > > > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > > -			pr_warn("LED display write failed\n");
> > > > > > -			return -ENODEV;
> > > > > > -		}
> > > > > > -		asus->ledd_status = (u32) value;
> > > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > > +	if (rv < 0)
> > > > > > +		return rv;
> > > > > >
> > > > > 
> > > > > This inverts the check to check for failure (this is preferred), but it does
> > > > > change the successful path to include the value of 0, which was skipped over in
> > > > > the original above.
> > > > > 
> > > > > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > 
> > > > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > > > above. Please consider, and apply decision to all similar instances below.
> > > > > 
> > > > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > > > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > > > what you wanted to say is wrong with this patch. Could you elaborate more?
> > > > 
> > >
> > > Your commit message states there is no functional change, but this changes the
> > > behavior of this function (and others) for the 0 edge case.
> > >
> > > Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> > > the ledd_status.
> > >
> >
> > sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
> > 0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
> >
> > if (!count)
> >     return 0;
>
> From a defensive programming context, we should be explicit in what we pass on
> to other functions and not rely on the caller to filter for us, especially when
> it's a simple matter of <= versus < in our comparison.

Which comparison? kstrtoint returns 0 on success so we can't use <=.

>
> Please update the changed comparison sites in the patch to have the same end
> result as before with respect to what it would do if a 0 were received.
>

So you want me to add:

if (!count)
    return 0;

Before each kstrtoint()? Sorry but I don't see a point in adding duplicated code
that is not functional because these functions aren't and will never be used
outside of sysfs. Just take a look at how many drivers in your own tree use
kstrtoint() in sysfs methods and there isn't such check in place because it's
pointless. Only samsung-laptop.c is an exception but it could be cleaned up
because !count is a pointless check. Not only sysfs can't call us when count is
0 but kstrtoint() also returns -EINVAL if the string is empty (in case of the
code in samsung-laptop).

thanks,
Giedrius

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Darren Hart]

On Sat, Sep 03, 2016 at 08:51:26AM +0300, Giedrius Statkevičius wrote:
> On Wed, Aug 17, 2016 at 11:23:15AM -0700, Darren Hart wrote:
> > On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> > > On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > > > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > > > > function instead. There is no funcionality change except that in the
> > > > > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > > > again. Also, add some minor corrections to error handling to accommodate
> > > > > > > the change in return values (parse_arg returned count if everything
> > > > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > > > 
> > > > > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > > > function                                     old     new   delta
> > > > > > > __UNIQUE_ID_vermagic0                         69      70      +1
> > > > > > > ls_switch_store                              133     117     -16
> > > > > > > ledd_store                                   175     159     -16
> > > > > > > display_store                                157     141     -16
> > > > > > > ls_level_store                               193     176     -17
> > > > > > > gps_store                                    200     178     -22
> > > > > > > sysfs_acpi_set.isra                          148     125     -23
> > > > > > > parse_arg.part                                39       -     -39
> > > > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > > > 
> > > > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > > > >  }

...

> > > > > > >  
> > > > > > >  /*
> > > > > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > > > >  	struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > > > >  	int rv, value;
> > > > > > >  
> > > > > > > -	rv = parse_arg(buf, count, &value);
> > > > > > > -	if (rv > 0) {
> > > > > > > -		if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > > > -			pr_warn("LED display write failed\n");
> > > > > > > -			return -ENODEV;
> > > > > > > -		}
> > > > > > > -		asus->ledd_status = (u32) value;
> > > > > > > +	rv = kstrtoint(buf, 0, &value);
> > > > > > > +	if (rv < 0)

I was pointing out that for functional equivalence, this should be:

		if (rv <= 0)

But as you point out below, this can't be achieved directly as kstrtoint returns
0 on success. I missed that in my first review (I guess), apologies.

> > > > > > > +		return rv;
> > > > > > >
> > > > > > 
> > > > > > This inverts the check to check for failure (this is preferred), but it does
> > > > > > change the successful path to include the value of 0, which was skipped over in
> > > > > > the original above.
> > > > > > 
> > > > > > > +	if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > > 
> > > > > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > > > > above. Please consider, and apply decision to all similar instances below.
> > > > > > 
> > > > > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > > > > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > > > > what you wanted to say is wrong with this patch. Could you elaborate more?
> > > > > 
> > > >
> > > > Your commit message states there is no functional change, but this changes the
> > > > behavior of this function (and others) for the 0 edge case.
> > > >
> > > > Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> > > > the ledd_status.
> > > >
> > >
> > > sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
> > > 0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
> > >
> > > if (!count)
> > >     return 0;
> >
> > From a defensive programming context, we should be explicit in what we pass on
> > to other functions and not rely on the caller to filter for us, especially when
> > it's a simple matter of <= versus < in our comparison.
> 
> Which comparison? kstrtoint returns 0 on success so we can't use <=.
> 
> >
> > Please update the changed comparison sites in the patch to have the same end
> > result as before with respect to what it would do if a 0 were received.
> >
> 
> So you want me to add:
> 
> if (!count)
>     return 0;
> 
> Before each kstrtoint()? Sorry but I don't see a point in adding duplicated code
> that is not functional because these functions aren't and will never be used
> outside of sysfs. Just take a look at how many drivers in your own tree use
> kstrtoint() in sysfs methods and there isn't such check in place because it's
> pointless. Only samsung-laptop.c is an exception but it could be cleaned up
> because !count is a pointless check. Not only sysfs can't call us when count is
> 0 but kstrtoint() also returns -EINVAL if the string is empty (in case of the
> code in samsung-laptop).

You're correct.

Queued to testing. Thanks for taking the time to follow up. Sorry for the
hassle.


-- 
Darren Hart
Intel Open Source Technology Center