Staging: iio/accel: Changed data type in adis16220_write_16bit to u16

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.

---
 drivers/staging/iio/accel/adis16220_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 6d4503d..bb72eb3 100644
@@ -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

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.

---
 drivers/staging/iio/accel/lis3l02dq_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 559545a..5566809 100644
@@ -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

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!

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.

---
 drivers/staging/iio/accel/sca3000_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index a44a705..c3f1f6a 100644
@@ -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

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 <[email protected]>

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

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.

---
 drivers/staging/iio/accel/sca3000_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index c3f1f6a..a33e742 100644
@@ -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

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.

---
 drivers/staging/iio/accel/sca3000_core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index a33e742..052005c 100644
@@ -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

s/bytes/bits/

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

/alessandro

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.

---
[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(-)

index c3f1f6a..a33e742 100644
@@ -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