Re: [PATCH 2/2] staging: iio: light: tsl2583.c: obsolete use of strict_strtoul

Re: [PATCH 2/2] staging: iio: light: tsl2583.c: obsolete use of strict_strtoul

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]

These patches need to be CC'd to the iio mailing list, and Jonathan
Cameron as well.  Use the ./scripts/get_maintainer.pl script for
hints who should be CC'd.

On Mon, Dec 12, 2011 at 03:35:03PM +0100, Johannes Tenschert wrote:
> @@ -621,7 +621,7 @@ static ssize_t taos_als_trim_store(struct device *dev,
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
>  	unsigned long value;
>  
> -	if (strict_strtoul(buf, 0, &value))
> +	if (kstrtoul(buf, 0, &value))
>  		return -EINVAL;
>  
>  	if (value)

We save value to
	chip->taos_settings.als_gain_trim = value;
als_gain_trim is an int, so on a 64bit system this would truncate
the high bits away and we could get a zero where we don't expect
one.

I don't think it's a problem, but looking at how als_gain_trim is
used, I noticed this potential bug:

   362          lux_val = taos_get_lux(indio_dev);
                ^^^^^^^
lux_val can be zero under certain circumstances.

   363          if (lux_val < 0) {
   364                  dev_err(&chip->client->dev, "taos_als_calibrate failed to get lux\n");
   365                  return lux_val;
   366          }
   367          gain_trim_val = (unsigned int) (((chip->taos_settings.als_cal_target)
   368                          * chip->taos_settings.als_gain_trim) / lux_val);
                                                                     ^^^^^^^^^
leading to a divide by zero bug.

   369  
   370          if ((gain_trim_val < 250) || (gain_trim_val > 4000)) {
   371                  dev_err(&chip->client->dev,
   372                          "taos_als_calibrate failed: trim_val of %d is out of range\n",
   373                          gain_trim_val);
   374                  return -ENODATA;
   375          }
   376          chip->taos_settings.als_gain_trim = (int) gain_trim_val;


Also I don't understand why als_gain_trim isn't unsigned.  So
basically I think we should make it unsigned and I think we should
use kstrtouint() here.  Also instead of returning -EINVAL, we
should preserve the return code from kstrtoul() and return that.
But I don't know the code that well so I may have missed something.

The other divide by zero bug should be fixed in a different patch.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]