drivers: iio: chemical: Fix sparse endianness warnings cast to restricted __be16

PATCH In review Sandhya Bankar 3 participants 4 messages Sep 24, 2016 – Sep 27, 2016 IIO

lore

Share & tools

lore

Sandhya Bankar

author

2016-09-24 19:16 UTC #0 lore raw

PATCH 1/1 drivers: iio: chemical: Fix sparse endianness warnings cast to restricted __be16

Fix the following sparse endianness warnings:

drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16

Signed-off-by Sandhya Bankar <[email protected]>

---
 drivers/iio/chemical/atlas-ph-sensor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iio/chemical/atlas-ph-sensor.c +4 -3

index 407f141..a3fbdb7 100644
@@ -207,13 +207,14 @@ static int atlas_check_ec_calibration(struct atlas_data *data)
 	struct device *dev = &data->client->dev;
 	int ret;
 	unsigned int val;
+	__be16	rval;
 
-	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &val, 2);
+	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &rval, 2);
 	if (ret)
 		return ret;
 
-	dev_info(dev, "probe set to K = %d.%.2d", be16_to_cpu(val) / 100,
-						 be16_to_cpu(val) % 100);
+	val = be16_to_cpu(rval);
+	dev_info(dev, "probe set to K = %d.%.2d", val / 100, val % 100);
 
 	ret = regmap_read(data->regmap, ATLAS_REG_EC_CALIB_STATUS, &val);
 	if (ret)

Signature · 1 line

1.8.3.1

Jonathan Cameron

maintainer

2016-09-25 08:50 UTC #1 lore raw

Quoted 11 lines · Sandhya Bankar

Fix the following sparse endianness warnings:
drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16

Quoted 1 line

Signed-off-by Sandhya Bankar <[email protected]>

Hi Sandhya,

Please also CC the author of a driver if possible. Here it's a little trickier as Matt's email address changed recently.

Patch is fine though so applied to the togreg branch of iio.git. Initially pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 27 lines

---
 drivers/iio/chemical/atlas-ph-sensor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iio/chemical/atlas-ph-sensor.c +4 -3

index 407f141..a3fbdb7 100644
@@ -207,13 +207,14 @@ static int atlas_check_ec_calibration(struct atlas_data *data)
 	struct device *dev = &data->client->dev;
 	int ret;
 	unsigned int val;
+	__be16	rval;
 
-	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &val, 2);
+	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &rval, 2);
 	if (ret)
 		return ret;
 
-	dev_info(dev, "probe set to K = %d.%.2d", be16_to_cpu(val) / 100,
-						 be16_to_cpu(val) % 100);
+	val = be16_to_cpu(rval);
+	dev_info(dev, "probe set to K = %d.%.2d", val / 100, val % 100);
 
 	ret = regmap_read(data->regmap, ATLAS_REG_EC_CALIB_STATUS, &val);
 	if (ret)

Lars-Peter Clausen

2016-09-25 15:04 UTC #2 lore raw

In reply to Sandhya Bankar

Quoted 10 lines · Sandhya Bankar

Fix the following sparse endianness warnings:
drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16

Hi,

Thanks for patches, this is all good work, thanks for doing it.

When it comes to patches like these that address issue found by code checkers it is useful to distinguish between whether the patch is

a) cosmetic. Meaning the code is currently correct and the behavior of the code will not change with the patch applied. But having the correct annotations will give us additional confidence that the code is correct and also help catch future mistakes.

b) a bug fix. Meaning the current code is not correct and the patch alters the behavior of the code to address it.

The distinction between the two cases should be noted in the commit message as it allows maintainers to decide how the patch should be handled. E.g. if the patch is cosmetic it will take the normal route of new patch, while if the patch is a bug fix it will get priority and also should be applied to stable trees.

E.g. this patch is a bug fix. val is a unsigned int which is a 32-bit variable, of which the pointer is taken and then treated as a 16-bit storage location by regmap_bulk_read(). This means that result is that the data is stored in the upper two bytes of the variable.

Now val is passed to be16_to_cpu() which expects a 16-bit variable as its parameter, so the 32-bit val variable is cast to a 16-bit value. Now here is where the behavior is different on a big-endian and a little-endian architecture. On a little-endian architecture the upper two bytes represent the 16-bit value, so all is good since that's where the data was stored. On a big-endian architecture the lower two bytes represent the 16-bit value. The lower two bytes of val have never been initialized though, so the result is that random data being passed to be16_to_cpu() and not the data that was read by regmap_bulk_read().

This means this code does not work correctly on a big-endian architecture, the patch message should ideally mention this and that the patch addresses the issue.

Some of the other patches you send were cosmetic only since the variable already had the correct storage size and the behavior of the code was the same on BE and LE systems. Ideally the commit message for those kind of patches would also mention this.

- Lars

Quoted 29 lines

Signed-off-by Sandhya Bankar <[email protected]>

---
 drivers/iio/chemical/atlas-ph-sensor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iio/chemical/atlas-ph-sensor.c +4 -3

index 407f141..a3fbdb7 100644
@@ -207,13 +207,14 @@ static int atlas_check_ec_calibration(struct atlas_data *data)
 	struct device *dev = &data->client->dev;
 	int ret;
 	unsigned int val;
+	__be16	rval;
 
-	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &val, 2);
+	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &rval, 2);
 	if (ret)
 		return ret;
 
-	dev_info(dev, "probe set to K = %d.%.2d", be16_to_cpu(val) / 100,
-						 be16_to_cpu(val) % 100);
+	val = be16_to_cpu(rval);
+	dev_info(dev, "probe set to K = %d.%.2d", val / 100, val % 100);
 
 	ret = regmap_read(data->regmap, ATLAS_REG_EC_CALIB_STATUS, &val);
 	if (ret)

Jonathan Cameron

maintainer

2016-09-27 20:18 UTC #3 lore raw

Quoted 52 lines · Lars-Peter Clausen

Quoted 10 lines · Sandhya Bankar
Fix the following sparse endianness warnings:
drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16 drivers/iio/chemical/atlas-ph-sensor.c:215:9: warning: cast to restricted __be16
Hi,
Thanks for patches, this is all good work, thanks for doing it.
When it comes to patches like these that address issue found by code checkers it is useful to distinguish between whether the patch is
a) cosmetic. Meaning the code is currently correct and the behavior of the code will not change with the patch applied. But having the correct annotations will give us additional confidence that the code is correct and also help catch future mistakes.
b) a bug fix. Meaning the current code is not correct and the patch alters the behavior of the code to address it.
The distinction between the two cases should be noted in the commit message as it allows maintainers to decide how the patch should be handled. E.g. if the patch is cosmetic it will take the normal route of new patch, while if the patch is a bug fix it will get priority and also should be applied to stable trees.
E.g. this patch is a bug fix. val is a unsigned int which is a 32-bit variable, of which the pointer is taken and then treated as a 16-bit storage location by regmap_bulk_read(). This means that result is that the data is stored in the upper two bytes of the variable.
Now val is passed to be16_to_cpu() which expects a 16-bit variable as its parameter, so the 32-bit val variable is cast to a 16-bit value. Now here is where the behavior is different on a big-endian and a little-endian architecture. On a little-endian architecture the upper two bytes represent the 16-bit value, so all is good since that's where the data was stored. On a big-endian architecture the lower two bytes represent the 16-bit value. The lower two bytes of val have never been initialized though, so the result is that random data being passed to be16_to_cpu() and not the data that was read by regmap_bulk_read().
This means this code does not work correctly on a big-endian architecture, the patch message should ideally mention this and that the patch addresses the issue.

Quoted 6 lines

Some of the other patches you send were cosmetic only since the variable already had the correct storage size and the behavior of the code was the same on BE and LE systems. Ideally the commit message for those kind of patches would also mention this.
- Lars

Thanks Lars, I'd missed this entirely.

Now dropped from togreg and applied to fixes-togreg + marked for stable. I've supplemented the commit message with a very short form of Lars' analysis of the bug and a fixes tag for the patch that introduced this.

Marked for stable.

Thanks,

Jonathan

Quoted 31 lines

Quoted 30 lines

Signed-off-by Sandhya Bankar <[email protected]>

---
 drivers/iio/chemical/atlas-ph-sensor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iio/chemical/atlas-ph-sensor.c +4 -3

index 407f141..a3fbdb7 100644
@@ -207,13 +207,14 @@ static int atlas_check_ec_calibration(struct atlas_data *data)
 	struct device *dev = &data->client->dev;
 	int ret;
 	unsigned int val;
+	__be16	rval;
 
-	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &val, 2);
+	ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &rval, 2);
 	if (ret)
 		return ret;
 
-	dev_info(dev, "probe set to K = %d.%.2d", be16_to_cpu(val) / 100,
-						 be16_to_cpu(val) % 100);
+	val = be16_to_cpu(rval);
+	dev_info(dev, "probe set to K = %d.%.2d", val / 100, val % 100);
 
 	ret = regmap_read(data->regmap, ATLAS_REG_EC_CALIB_STATUS, &val);
 	if (ret)