[PATCH 1/3] Staging: iio/accel: Changed data type in adis16220_write_16bit to u16

[PATCH 1/3] Staging: iio/accel: Changed data type in adis16220_write_16bit to u16

[Andreas Ruprecht]

In the adis16220_write_16bit() function we used a long value to store
parsed data from the char* buffer buf.
The called function to actually write the data,
adis16220_spi_write_reg_16(), takes a u16 value as a parameter, so up
to now a value larger than u16 was silently ignored as it was only
truncated when passing the parameter.
Now this function will only accept values fitting into a u16.

Additionally the parsing function was changed to overcome the now
obsolete strict_strtol() function.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
---
 drivers/staging/iio/accel/adis16220_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c
index 6d4503d..bb72eb3 100644
--- a/drivers/staging/iio/accel/adis16220_core.c
+++ b/drivers/staging/iio/accel/adis16220_core.c
@@ -167,9 +167,9 @@ static ssize_t adis16220_write_16bit(struct device *dev,
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	int ret;
-	long val;
+	u16 val;
 
-	ret = strict_strtol(buf, 10, &val);
+	ret = kstrtou16(buf, 10, &val);
 	if (ret)
 		goto error_ret;
 	ret = adis16220_spi_write_reg_16(indio_dev, this_attr->address, val);
-- 
1.7.5.4

[PATCH 2/3] Staging: iio/accel: Changed data type for val to unsigned long in write_frequency

[Andreas Ruprecht]

In lis3102dq_write_frequency() we used a long variable to store the
value parsed from the char* buffer buf, as there only was a
strict_strtol() function to parse values.
Now we have got kstrto* which allows us to convert to the right data
type in most cases.

In this particular function we want to write a frequency value, and it
doesn't make sense to allow negative values here (as Dan Carpenter
pointed out in a previous email).
This means we can now parse the value into an unsigned long and get an
error for invalid (e.g. negative) values.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
---
 drivers/staging/iio/accel/lis3l02dq_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/accel/lis3l02dq_core.c b/drivers/staging/iio/accel/lis3l02dq_core.c
index 559545a..5566809 100644
--- a/drivers/staging/iio/accel/lis3l02dq_core.c
+++ b/drivers/staging/iio/accel/lis3l02dq_core.c
@@ -331,11 +331,11 @@ static ssize_t lis3l02dq_write_frequency(struct device *dev,
 					 size_t len)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	long val;
+	unsigned long val;
 	int ret;
 	u8 t;
 
-	ret = strict_strtol(buf, 10, &val);
+	ret = kstrtoul(buf, 10, &val);
 	if (ret)
 		return ret;
 
-- 
1.7.5.4

Re: [PATCH 2/3] Staging: iio/accel: Changed data type for val to unsigned long in write_frequency

[Jonathan Cameron]

On 11/27/2011 10:17 PM, Andreas Ruprecht wrote:
> In lis3102dq_write_frequency() we used a long variable to store the
> value parsed from the char* buffer buf, as there only was a
> strict_strtol() function to parse values.
> Now we have got kstrto* which allows us to convert to the right data
> type in most cases.
> 
> In this particular function we want to write a frequency value, and it
> doesn't make sense to allow negative values here (as Dan Carpenter
> pointed out in a previous email).
> This means we can now parse the value into an unsigned long and get an
> error for invalid (e.g. negative) values.
Not that it matters, but seeing as it goes into a switch statement we
would have gotten an error on any negatives anyway.  Nice enough
cleanup and indeed more logical though!
> 
> Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/accel/lis3l02dq_core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/lis3l02dq_core.c b/drivers/staging/iio/accel/lis3l02dq_core.c
> index 559545a..5566809 100644
> --- a/drivers/staging/iio/accel/lis3l02dq_core.c
> +++ b/drivers/staging/iio/accel/lis3l02dq_core.c
> @@ -331,11 +331,11 @@ static ssize_t lis3l02dq_write_frequency(struct device *dev,
>  					 size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	long val;
> +	unsigned long val;
>  	int ret;
>  	u8 t;
>  
> -	ret = strict_strtol(buf, 10, &val);
> +	ret = kstrtoul(buf, 10, &val);
>  	if (ret)
>  		return ret;
>  

[PATCH 3/3] Staging: iio/accel: Changed data type of val in store_measurement_mode to u8

[Andreas Ruprecht]

The code in sca3000_store_measurement_mode() uses the variable val to
do bitwise operations with an int mask and or-s it into st->rx[0] which
is an entry in a u8 array (see sca3000.h).

This means up to now values larger than a u8 were silently ignored and
just the lower 8 bits counted into the value that was written into
st->rx[0]. This code will return -ERANGE if the value in buf was too
large to fit into a u8.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
---
 drivers/staging/iio/accel/sca3000_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
index a44a705..c3f1f6a 100644
--- a/drivers/staging/iio/accel/sca3000_core.c
+++ b/drivers/staging/iio/accel/sca3000_core.c
@@ -382,10 +382,10 @@ sca3000_store_measurement_mode(struct device *dev,
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret;
 	int mask = 0x03;
-	long val;
+	u8 val;
 
 	mutex_lock(&st->lock);
-	ret = strict_strtol(buf, 10, &val);
+	ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		goto error_ret;
 	ret = sca3000_read_data_short(st, SCA3000_REG_ADDR_MODE, 1);
-- 
1.7.5.4

Re: [PATCH 3/3] Staging: iio/accel: Changed data type of val in store_measurement_mode to u8

[Jonathan Cameron]

On 11/27/2011 10:17 PM, Andreas Ruprecht wrote:
> The code in sca3000_store_measurement_mode() uses the variable val to
> do bitwise operations with an int mask and or-s it into st->rx[0] which
> is an entry in a u8 array (see sca3000.h).
> 
> This means up to now values larger than a u8 were silently ignored and
> just the lower 8 bits counted into the value that was written into
> st->rx[0]. This code will return -ERANGE if the value in buf was too
> large to fit into a u8.
Equal arguement holds for the mask being a u8 and for that matter we
should also verify the range is 0-3.

Do both of them and feel free to add Acked-by: Jonathan Cameron
<jic23@kernel.org>

When you get down to it this whole interface is horribly non standard
and will need cleaning up at some point.  Not that it is relevant to
this patch!

Thanks,

Jonathan

> 
> Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
> ---
>  drivers/staging/iio/accel/sca3000_core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
> index a44a705..c3f1f6a 100644
> --- a/drivers/staging/iio/accel/sca3000_core.c
> +++ b/drivers/staging/iio/accel/sca3000_core.c
> @@ -382,10 +382,10 @@ sca3000_store_measurement_mode(struct device *dev,
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  	int ret;
>  	int mask = 0x03;
> -	long val;
> +	u8 val;
>  
>  	mutex_lock(&st->lock);
> -	ret = strict_strtol(buf, 10, &val);
> +	ret = kstrtou8(buf, 10, &val);
>  	if (ret)
>  		goto error_ret;
>  	ret = sca3000_read_data_short(st, SCA3000_REG_ADDR_MODE, 1);

[PATCH 1/2] Staging: iio/accel: Changed data type of mask in store_measurement_mode() to u8

[Andreas Ruprecht]

In sca3000_store_measurement_mode() we parse a value from a string
buffer via kstrtou8, and store the parsed value into a u8 after
and-ing it with mask.

As we are only interested in the lowest two bytes here and mask is
initialized with a fixed value 0x03, mask may as well be a u8.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/accel/sca3000_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
index c3f1f6a..a33e742 100644
--- a/drivers/staging/iio/accel/sca3000_core.c
+++ b/drivers/staging/iio/accel/sca3000_core.c
@@ -381,7 +381,7 @@ sca3000_store_measurement_mode(struct device *dev,
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret;
-	int mask = 0x03;
+	u8 mask = 0x03;
 	u8 val;
 
 	mutex_lock(&st->lock);
-- 
1.7.5.4

[PATCH 2/2] Staging: iio/accel: Added a range check for val in store_measurement_mode()

[Andreas Ruprecht]

In sca3000_store_measurement_mode() we use val to and it with a mask.
This mask is only two bits long (as we are only interested in the
lowest two bits), so a value bigger than 3 was silently ignored so
far.

Now this function will return -EINVAL, if val is bigger than 3.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/accel/sca3000_core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
index a33e742..052005c 100644
--- a/drivers/staging/iio/accel/sca3000_core.c
+++ b/drivers/staging/iio/accel/sca3000_core.c
@@ -388,6 +388,10 @@ sca3000_store_measurement_mode(struct device *dev,
 	ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		goto error_ret;
+	if (val > 3) {
+		ret = -EINVAL;
+		goto error_ret;
+	}
 	ret = sca3000_read_data_short(st, SCA3000_REG_ADDR_MODE, 1);
 	if (ret)
 		goto error_ret;
-- 
1.7.5.4

Re: [PATCH 1/2] Staging: iio/accel: Changed data type of mask in store_measurement_mode() to u8

[Alessandro Rubini]

> As we are only interested in the lowest two bytes here and mask is
> initialized with a fixed value 0x03, mask may as well be a u8.

s/bytes/bits/

It's not a trivial typo, the reader can be confused (I've been)

/alessandro

[PATCH v2 1/2] Staging: iio/accel: Changed data type of mask in store_measurement_mode() to u8

[Andreas Ruprecht]

In sca3000_store_measurement_mode() we parse a value from a string
buffer via kstrtou8, and store the parsed value into a u8 after
and-ing it with mask.

As we are only interested in the lowest two bits here and mask is
initialized with a fixed value 0x03, mask may as well be a u8.

Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
[v2]: Fixed a confusing typo in the description (bytes/bits)
---
 drivers/staging/iio/accel/sca3000_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
index c3f1f6a..a33e742 100644
--- a/drivers/staging/iio/accel/sca3000_core.c
+++ b/drivers/staging/iio/accel/sca3000_core.c
@@ -381,7 +381,7 @@ sca3000_store_measurement_mode(struct device *dev,
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret;
-	int mask = 0x03;
+	u8 mask = 0x03;
 	u8 val;
 
 	mutex_lock(&st->lock);
-- 
1.7.5.4

Re: [PATCH 1/3] Staging: iio/accel: Changed data type in adis16220_write_16bit to u16

[Jonathan Cameron]

On 11/27/2011 10:17 PM, Andreas Ruprecht wrote:
> In the adis16220_write_16bit() function we used a long value to store
> parsed data from the char* buffer buf.
> The called function to actually write the data,
> adis16220_spi_write_reg_16(), takes a u16 value as a parameter, so up
> to now a value larger than u16 was silently ignored as it was only
> truncated when passing the parameter.
> Now this function will only accept values fitting into a u16.
> 
> Additionally the parsing function was changed to overcome the now
> obsolete strict_strtol() function.
> 
> Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/accel/adis16220_core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c
> index 6d4503d..bb72eb3 100644
> --- a/drivers/staging/iio/accel/adis16220_core.c
> +++ b/drivers/staging/iio/accel/adis16220_core.c
> @@ -167,9 +167,9 @@ static ssize_t adis16220_write_16bit(struct device *dev,
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  	int ret;
> -	long val;
> +	u16 val;
>  
> -	ret = strict_strtol(buf, 10, &val);
> +	ret = kstrtou16(buf, 10, &val);
>  	if (ret)
>  		goto error_ret;
>  	ret = adis16220_spi_write_reg_16(indio_dev, this_attr->address, val);