[PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings

[PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

A wrong use of one of these in
https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
included a reference from Nathan to a patch discouraging the use of
these strings in general:
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/

I did a quick grep and established we only have a few instances of these in
IIO anyway, so in the interests of avoiding those existing cases getting
cut and paste into new drivers, let's just clear them out now.

Note that patch from Arnd is now also part of this series, due to the
length specifier related issue Joe raised.

I have chosen to go with 0x%02x rather than %#04x as I find it more readable.

V2:
Use 0x%02x (Joe Perches)
Include Arnd's original patch, modified for the above.

Arnd Bergmann (1):
  iio: si1133: fix format string warnings

Jonathan Cameron (3):
  iio: light: si1133: Drop remaining uses of %hhx format string.
  iio: imu: inv_mpu6050: Drop use of %hhx format string.
  iio: light: si1145: Drop use of %hhx format specifier.

 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  5 ++---
 drivers/iio/light/si1133.c                 | 18 +++++++++---------
 drivers/iio/light/si1145.c                 | 10 +++++-----
 3 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] iio: si1133: fix format string warnings

[Jonathan Cameron]

From: Arnd Bergmann <arnd@arndb.de>

clang complains about multiple instances of printing an integer
using the %hhx format string:

drivers/iio/light/si1133.c:982:4: error: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Werror,-Wformat]
                 part_id, rev_id, mfr_id);
                 ^~~~~~~

Print them as a normal integer instead, leaving the "#02"
length modifier.

Use the 0x02x form as the length specifier when used with # includes
the 0x prefix and is very unlikely to be what was intended by the author.

Fixes: e01e7eaf37d8 ("iio: light: introduce si1133")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/si1133.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index c280b4195003..0accea7090ee 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -978,11 +978,11 @@ static int si1133_validate_ids(struct iio_dev *iio_dev)
 		return err;
 
 	dev_info(&iio_dev->dev,
-		 "Device ID part %#02hhx rev %#02hhx mfr %#02hhx\n",
+		 "Device ID part 0x%02x rev 0x%02x mfr 0x%02x\n",
 		 part_id, rev_id, mfr_id);
 	if (part_id != SI1133_PART_ID) {
 		dev_err(&iio_dev->dev,
-			"Part ID mismatch got %#02hhx, expected %#02x\n",
+			"Part ID mismatch got 0x%02x, expected 0x%02x\n",
 			part_id, SI1133_PART_ID);
 		return -ENODEV;
 	}
-- 
2.31.1

Re: [PATCH v2 1/4] iio: si1133: fix format string warnings

[Nathan Chancellor]

On Thu, Jun 03, 2021 at 07:06:09PM +0100, Jonathan Cameron wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang complains about multiple instances of printing an integer
> using the %hhx format string:
> 
> drivers/iio/light/si1133.c:982:4: error: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Werror,-Wformat]
>                  part_id, rev_id, mfr_id);
>                  ^~~~~~~
> 
> Print them as a normal integer instead, leaving the "#02"
> length modifier.
> 
> Use the 0x02x form as the length specifier when used with # includes
> the 0x prefix and is very unlikely to be what was intended by the author.
> 
> Fixes: e01e7eaf37d8 ("iio: light: introduce si1133")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/iio/light/si1133.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
> index c280b4195003..0accea7090ee 100644
> --- a/drivers/iio/light/si1133.c
> +++ b/drivers/iio/light/si1133.c
> @@ -978,11 +978,11 @@ static int si1133_validate_ids(struct iio_dev *iio_dev)
>  		return err;
>  
>  	dev_info(&iio_dev->dev,
> -		 "Device ID part %#02hhx rev %#02hhx mfr %#02hhx\n",
> +		 "Device ID part 0x%02x rev 0x%02x mfr 0x%02x\n",
>  		 part_id, rev_id, mfr_id);
>  	if (part_id != SI1133_PART_ID) {
>  		dev_err(&iio_dev->dev,
> -			"Part ID mismatch got %#02hhx, expected %#02x\n",
> +			"Part ID mismatch got 0x%02x, expected 0x%02x\n",
>  			part_id, SI1133_PART_ID);
>  		return -ENODEV;
>  	}
> -- 
> 2.31.1

[PATCH v2 2/4] iio: light: si1133: Drop remaining uses of %hhx format string.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Since:
commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
unnecessary %h[xudi] and %hh[xudi]")
use of these format strings has been discouraged.

Use the 0x02x form as the length specifier when used with # includes
the 0x prefix and is very unlikely to be what was intended by the author.

As there are not that many in IIO, this is part of an effort to clear
them out so we don't have any instances that might get copied into
new drivers.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com>
Cc: Nathan Chancellor <nathan@kernel.org>
---
 drivers/iio/light/si1133.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 0accea7090ee..f8c9b2cc322e 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -352,22 +352,22 @@ static int si1133_parse_response_err(struct device *dev, u32 resp, u8 cmd)
 
 	switch (resp) {
 	case SI1133_ERR_OUTPUT_BUFFER_OVERFLOW:
-		dev_warn(dev, "Output buffer overflow: %#02hhx\n", cmd);
+		dev_warn(dev, "Output buffer overflow: 0x%02x\n", cmd);
 		return -EOVERFLOW;
 	case SI1133_ERR_SATURATION_ADC_OR_OVERFLOW_ACCUMULATION:
-		dev_warn(dev, "Saturation of the ADC or overflow of accumulation: %#02hhx\n",
+		dev_warn(dev, "Saturation of the ADC or overflow of accumulation: 0x%02x\n",
 			 cmd);
 		return -EOVERFLOW;
 	case SI1133_ERR_INVALID_LOCATION_CMD:
 		dev_warn(dev,
-			 "Parameter access to an invalid location: %#02hhx\n",
+			 "Parameter access to an invalid location: 0x%02x\n",
 			 cmd);
 		return -EINVAL;
 	case SI1133_ERR_INVALID_CMD:
-		dev_warn(dev, "Invalid command %#02hhx\n", cmd);
+		dev_warn(dev, "Invalid command 0x%02x\n", cmd);
 		return -EINVAL;
 	default:
-		dev_warn(dev, "Unknown error %#02hhx\n", cmd);
+		dev_warn(dev, "Unknown error 0x%02x\n", cmd);
 		return -EINVAL;
 	}
 }
@@ -400,7 +400,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 
 	err = regmap_write(data->regmap, SI1133_REG_COMMAND, cmd);
 	if (err) {
-		dev_warn(dev, "Failed to write command %#02hhx, ret=%d\n", cmd,
+		dev_warn(dev, "Failed to write command 0x%02x, ret=%d\n", cmd,
 			 err);
 		goto out;
 	}
@@ -425,7 +425,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 					       SI1133_CMD_TIMEOUT_MS * 1000);
 		if (err) {
 			dev_warn(dev,
-				 "Failed to read command %#02hhx, ret=%d\n",
+				 "Failed to read command 0x%02x, ret=%d\n",
 				 cmd, err);
 			goto out;
 		}
-- 
2.31.1

Re: [PATCH v2 2/4] iio: light: si1133: Drop remaining uses of %hhx format string.

[Nathan Chancellor]

On Thu, Jun 03, 2021 at 07:06:10PM +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Since:
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
> use of these format strings has been discouraged.
> 
> Use the 0x02x form as the length specifier when used with # includes
> the 0x prefix and is very unlikely to be what was intended by the author.
> 
> As there are not that many in IIO, this is part of an effort to clear
> them out so we don't have any instances that might get copied into
> new drivers.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com>
> Cc: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/iio/light/si1133.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
> index 0accea7090ee..f8c9b2cc322e 100644
> --- a/drivers/iio/light/si1133.c
> +++ b/drivers/iio/light/si1133.c
> @@ -352,22 +352,22 @@ static int si1133_parse_response_err(struct device *dev, u32 resp, u8 cmd)
>  
>  	switch (resp) {
>  	case SI1133_ERR_OUTPUT_BUFFER_OVERFLOW:
> -		dev_warn(dev, "Output buffer overflow: %#02hhx\n", cmd);
> +		dev_warn(dev, "Output buffer overflow: 0x%02x\n", cmd);
>  		return -EOVERFLOW;
>  	case SI1133_ERR_SATURATION_ADC_OR_OVERFLOW_ACCUMULATION:
> -		dev_warn(dev, "Saturation of the ADC or overflow of accumulation: %#02hhx\n",
> +		dev_warn(dev, "Saturation of the ADC or overflow of accumulation: 0x%02x\n",
>  			 cmd);
>  		return -EOVERFLOW;
>  	case SI1133_ERR_INVALID_LOCATION_CMD:
>  		dev_warn(dev,
> -			 "Parameter access to an invalid location: %#02hhx\n",
> +			 "Parameter access to an invalid location: 0x%02x\n",
>  			 cmd);
>  		return -EINVAL;
>  	case SI1133_ERR_INVALID_CMD:
> -		dev_warn(dev, "Invalid command %#02hhx\n", cmd);
> +		dev_warn(dev, "Invalid command 0x%02x\n", cmd);
>  		return -EINVAL;
>  	default:
> -		dev_warn(dev, "Unknown error %#02hhx\n", cmd);
> +		dev_warn(dev, "Unknown error 0x%02x\n", cmd);
>  		return -EINVAL;
>  	}
>  }
> @@ -400,7 +400,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  
>  	err = regmap_write(data->regmap, SI1133_REG_COMMAND, cmd);
>  	if (err) {
> -		dev_warn(dev, "Failed to write command %#02hhx, ret=%d\n", cmd,
> +		dev_warn(dev, "Failed to write command 0x%02x, ret=%d\n", cmd,
>  			 err);
>  		goto out;
>  	}
> @@ -425,7 +425,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  					       SI1133_CMD_TIMEOUT_MS * 1000);
>  		if (err) {
>  			dev_warn(dev,
> -				 "Failed to read command %#02hhx, ret=%d\n",
> +				 "Failed to read command 0x%02x, ret=%d\n",
>  				 cmd, err);
>  			goto out;
>  		}
> -- 
> 2.31.1

[PATCH v2 3/4] iio: imu: inv_mpu6050: Drop use of %hhx format string.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Since:
commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
unnecessary %h[xudi] and %hh[xudi]")
use of these format strings has been discouraged.

Use the 0x02x form as the length specifier when used with # includes
the 0x prefix and is very unlikely to be what was intended by the author.

Part of a series removing all uses from IIO in the interestings of
avoiding providing bad examples for people to copy.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Nathan Chancellor <nathan@kernel.org>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 64704b55f6eb..b7254d9e0fe2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1314,8 +1314,7 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 		for (i = 0; i < INV_NUM_PARTS; ++i) {
 			if (regval == hw_info[i].whoami) {
 				dev_warn(regmap_get_device(st->map),
-					"whoami mismatch got %#02x (%s)"
-					"expected %#02hhx (%s)\n",
+					"whoami mismatch got 0x%02x (%s) expected 0x%02x (%s)\n",
 					regval, hw_info[i].name,
 					st->hw->whoami, st->hw->name);
 				break;
@@ -1323,7 +1322,7 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 		}
 		if (i >= INV_NUM_PARTS) {
 			dev_err(regmap_get_device(st->map),
-				"invalid whoami %#02x expected %#02hhx (%s)\n",
+				"invalid whoami 0x%02x expected 0x%02x (%s)\n",
 				regval, st->hw->whoami, st->hw->name);
 			return -ENODEV;
 		}
-- 
2.31.1

Re: [PATCH v2 3/4] iio: imu: inv_mpu6050: Drop use of %hhx format string.

[Nathan Chancellor]

On Thu, Jun 03, 2021 at 07:06:11PM +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Since:
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
> use of these format strings has been discouraged.
> 
> Use the 0x02x form as the length specifier when used with # includes
> the 0x prefix and is very unlikely to be what was intended by the author.
> 
> Part of a series removing all uses from IIO in the interestings of
> avoiding providing bad examples for people to copy.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 64704b55f6eb..b7254d9e0fe2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -1314,8 +1314,7 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  		for (i = 0; i < INV_NUM_PARTS; ++i) {
>  			if (regval == hw_info[i].whoami) {
>  				dev_warn(regmap_get_device(st->map),
> -					"whoami mismatch got %#02x (%s)"
> -					"expected %#02hhx (%s)\n",
> +					"whoami mismatch got 0x%02x (%s) expected 0x%02x (%s)\n",
>  					regval, hw_info[i].name,
>  					st->hw->whoami, st->hw->name);
>  				break;
> @@ -1323,7 +1322,7 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  		}
>  		if (i >= INV_NUM_PARTS) {
>  			dev_err(regmap_get_device(st->map),
> -				"invalid whoami %#02x expected %#02hhx (%s)\n",
> +				"invalid whoami 0x%02x expected 0x%02x (%s)\n",
>  				regval, st->hw->whoami, st->hw->name);
>  			return -ENODEV;
>  		}
> -- 
> 2.31.1

[PATCH v2 4/4] iio: light: si1145: Drop use of %hhx format specifier.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Since:
commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
unnecessary %h[xudi] and %hh[xudi]")
use of these format strings has been discouraged.

As there are only a few such instances in IIO, this is part of a
series clearing them out so they don't get copied into new drivers.

Use the 0x02x form as the length specifier when used with # includes
the 0x prefix and is very unlikely to be what was intended by the author.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Nathan Chancellor <nathan@kernel.org>
---
 drivers/iio/light/si1145.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
index 3fb52402fcc3..e2abad48b9f4 100644
--- a/drivers/iio/light/si1145.c
+++ b/drivers/iio/light/si1145.c
@@ -271,7 +271,7 @@ static int si1145_command(struct si1145_data *data, u8 cmd)
 		if ((ret & ~SI1145_RSP_COUNTER_MASK) == 0) {
 			if (ret == data->rsp_seq) {
 				if (time_after(jiffies, stop_jiffies)) {
-					dev_warn(dev, "timeout on command %#02hhx\n",
+					dev_warn(dev, "timeout on command 0x%02x\n",
 						 cmd);
 					ret = -ETIMEDOUT;
 					break;
@@ -291,12 +291,12 @@ static int si1145_command(struct si1145_data *data, u8 cmd)
 			ret = -EIO;
 		} else {
 			if (ret == SI1145_RSP_INVALID_SETTING) {
-				dev_warn(dev, "INVALID_SETTING error on command %#02hhx\n",
+				dev_warn(dev, "INVALID_SETTING error on command 0x%02x\n",
 					 cmd);
 				ret = -EINVAL;
 			} else {
 				/* All overflows are treated identically */
-				dev_dbg(dev, "overflow, ret=%d, cmd=%#02hhx\n",
+				dev_dbg(dev, "overflow, ret=%d, cmd=0x%02x\n",
 					ret, cmd);
 				ret = -EOVERFLOW;
 			}
@@ -1299,10 +1299,10 @@ static int si1145_probe(struct i2c_client *client,
 						SI1145_REG_SEQ_ID);
 	if (ret < 0)
 		return ret;
-	dev_info(&client->dev, "device ID part %#02hhx rev %#02hhx seq %#02hhx\n",
+	dev_info(&client->dev, "device ID part 0x%02x rev 0x%02x seq 0x%02x\n",
 			part_id, rev_id, seq_id);
 	if (part_id != data->part_info->part) {
-		dev_err(&client->dev, "part ID mismatch got %#02hhx, expected %#02x\n",
+		dev_err(&client->dev, "part ID mismatch got 0x%02x, expected 0x%02x\n",
 				part_id, data->part_info->part);
 		return -ENODEV;
 	}
-- 
2.31.1

Re: [PATCH v2 4/4] iio: light: si1145: Drop use of %hhx format specifier.

[Nathan Chancellor]

On Thu, Jun 03, 2021 at 07:06:12PM +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Since:
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
> use of these format strings has been discouraged.
> 
> As there are only a few such instances in IIO, this is part of a
> series clearing them out so they don't get copied into new drivers.
> 
> Use the 0x02x form as the length specifier when used with # includes
> the 0x prefix and is very unlikely to be what was intended by the author.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/iio/light/si1145.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
> index 3fb52402fcc3..e2abad48b9f4 100644
> --- a/drivers/iio/light/si1145.c
> +++ b/drivers/iio/light/si1145.c
> @@ -271,7 +271,7 @@ static int si1145_command(struct si1145_data *data, u8 cmd)
>  		if ((ret & ~SI1145_RSP_COUNTER_MASK) == 0) {
>  			if (ret == data->rsp_seq) {
>  				if (time_after(jiffies, stop_jiffies)) {
> -					dev_warn(dev, "timeout on command %#02hhx\n",
> +					dev_warn(dev, "timeout on command 0x%02x\n",
>  						 cmd);
>  					ret = -ETIMEDOUT;
>  					break;
> @@ -291,12 +291,12 @@ static int si1145_command(struct si1145_data *data, u8 cmd)
>  			ret = -EIO;
>  		} else {
>  			if (ret == SI1145_RSP_INVALID_SETTING) {
> -				dev_warn(dev, "INVALID_SETTING error on command %#02hhx\n",
> +				dev_warn(dev, "INVALID_SETTING error on command 0x%02x\n",
>  					 cmd);
>  				ret = -EINVAL;
>  			} else {
>  				/* All overflows are treated identically */
> -				dev_dbg(dev, "overflow, ret=%d, cmd=%#02hhx\n",
> +				dev_dbg(dev, "overflow, ret=%d, cmd=0x%02x\n",
>  					ret, cmd);
>  				ret = -EOVERFLOW;
>  			}
> @@ -1299,10 +1299,10 @@ static int si1145_probe(struct i2c_client *client,
>  						SI1145_REG_SEQ_ID);
>  	if (ret < 0)
>  		return ret;
> -	dev_info(&client->dev, "device ID part %#02hhx rev %#02hhx seq %#02hhx\n",
> +	dev_info(&client->dev, "device ID part 0x%02x rev 0x%02x seq 0x%02x\n",
>  			part_id, rev_id, seq_id);
>  	if (part_id != data->part_info->part) {
> -		dev_err(&client->dev, "part ID mismatch got %#02hhx, expected %#02x\n",
> +		dev_err(&client->dev, "part ID mismatch got 0x%02x, expected 0x%02x\n",
>  				part_id, data->part_info->part);
>  		return -ENODEV;
>  	}
> -- 
> 2.31.1

General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

[Joe Perches]

On Thu, 2021-06-03 at 19:06 +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> A wrong use of one of these in
> https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
> included a reference from Nathan to a patch discouraging the use of
> these strings in general:
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> 
> I did a quick grep and established we only have a few instances of these in
> IIO anyway, so in the interests of avoiding those existing cases getting
> cut and paste into new drivers, let's just clear them out now.
> 
> Note that patch from Arnd is now also part of this series, due to the
> length specifier related issue Joe raised.
> 
> I have chosen to go with 0x%02x rather than %#04x as I find it more readable.
> 
> V2:
> Use 0x%02x (Joe Perches)
> Include Arnd's original patch, modified for the above.

Hello again.

It looks to me as though %#<foo> is relatively commonly misused in the kernel.

Pehaps for the decimal portion of the format, checkpatch could have some
test for use of non-standard lengths.

Given the use is generally meant for a u8, u16, u32, or u64, perhaps
checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18.

(possible checkpatch patch below)

$ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn
    392 %#08x
    238 %#04x
    144 %#02x
    114 %#06x
     92 %#010x
     58 %#010Lx
     55 %#018llx
     47 %#010llx
     45 %#010lx
     38 %#016llx
     27 %#0x
     23 %#2x
     18 %#016lx
     17 %#3lx
     17 %#08lx
     17 %#018Lx
     15 %#3x
     14 %#03x
     10 %#06hx
      9 %#08zx
      8 %#10x
      7 %#16llx
      6 %#8x
      6 %#04X
      6 %#04llx
      6 %#012llx
      5 %#16
      4 %#08llx
      4 %#06llx
      4 %#05x
      4 %#02X
      4 %#016Lx
      3 %#04hx
      3 %#01x
      2 %#6x
      2 %#4x
      2 %#10
      2 %#09x
      2 %#05lx
      1 %#8lx
      1 %#5x
      1 %#5lx
      1 %#2Lx
      1 %#2llx
      1 %#16x
      1 %#16lx
      1 %#12x
      1 %#0x10000
      1 %#0lx
      1 %#08
      1 %#05llx
      1 %#04o
      1 %#04lx
      1 %#03X
      1 %#018lx
      1 %#010zx

---
 scripts/checkpatch.pl | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d65334588eb4c..5840f3f2aee6f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6695,6 +6695,31 @@ sub process {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
 
+				while ($fmt =~ /\%#([\d]+)/g) {
+					my $length = $1;
+					my $pref_len;
+					if ($length < 4) {
+						$pref_len = '04';
+					} elsif ($length == 5) {
+						$pref_len = '06';
+					} elsif ($length > 6 && $length < 10) {
+						$pref_len = '010';
+					} elsif ($length > 10 && $length < 18) {
+						$pref_len = '018';
+					} elsif ($length > 18) {
+						$pref_len = '<something else>';
+					}
+					if (defined($pref_len)) {
+						if (!defined($stat_real)) {
+							$stat_real = get_stat_real($linenr, $lc);
+						}
+						WARN("VSPRINTF_SPECIAL_LENGTH",
+						     "Unusual special length '%#$length' in 0x prefixed output, length is usually 2 more than the desired width - perhaps use '%#${pref_len}'\n" . "$here\n$stat_real");
+					}
+				}
+
+				pos($fmt) = 0;
+
 				while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) {
 					$specifier = $1;
 					$extension = $2;

Re: General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

[Jonathan Cameron]

On Thu, 03 Jun 2021 11:58:15 -0700
Joe Perches <joe@perches.com> wrote:

> On Thu, 2021-06-03 at 19:06 +0100, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > A wrong use of one of these in
> > https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
> > included a reference from Nathan to a patch discouraging the use of
> > these strings in general:
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> > 
> > I did a quick grep and established we only have a few instances of these in
> > IIO anyway, so in the interests of avoiding those existing cases getting
> > cut and paste into new drivers, let's just clear them out now.
> > 
> > Note that patch from Arnd is now also part of this series, due to the
> > length specifier related issue Joe raised.
> > 
> > I have chosen to go with 0x%02x rather than %#04x as I find it more readable.
> > 
> > V2:
> > Use 0x%02x (Joe Perches)
> > Include Arnd's original patch, modified for the above.  
> 
> Hello again.
> 
> It looks to me as though %#<foo> is relatively commonly misused in the kernel.
> 
> Pehaps for the decimal portion of the format, checkpatch could have some
> test for use of non-standard lengths.
> 
> Given the use is generally meant for a u8, u16, u32, or u64, perhaps
> checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18.

Would have saved me some trouble, so I'm definitely in favour of checkpatch
catching this.

I wonder if a better option is to match on 1, 2, 4, 8, 16 as likely to be
caused by people getting the usage wrong rather than a deliberate attempt
to pretty print something a little unusual?

Thanks.

Jonathan

> 
> (possible checkpatch patch below)
> 
> $ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn
>     392 %#08x
>     238 %#04x
>     144 %#02x
>     114 %#06x
>      92 %#010x
>      58 %#010Lx
>      55 %#018llx
>      47 %#010llx
>      45 %#010lx
>      38 %#016llx
>      27 %#0x
>      23 %#2x
>      18 %#016lx
>      17 %#3lx
>      17 %#08lx
>      17 %#018Lx
>      15 %#3x
>      14 %#03x
>      10 %#06hx
>       9 %#08zx
>       8 %#10x
>       7 %#16llx
>       6 %#8x
>       6 %#04X
>       6 %#04llx
>       6 %#012llx
>       5 %#16
>       4 %#08llx
>       4 %#06llx
>       4 %#05x
>       4 %#02X
>       4 %#016Lx
>       3 %#04hx
>       3 %#01x
>       2 %#6x
>       2 %#4x
>       2 %#10
>       2 %#09x
>       2 %#05lx
>       1 %#8lx
>       1 %#5x
>       1 %#5lx
>       1 %#2Lx
>       1 %#2llx
>       1 %#16x
>       1 %#16lx
>       1 %#12x
>       1 %#0x10000
>       1 %#0lx
>       1 %#08
>       1 %#05llx
>       1 %#04o
>       1 %#04lx
>       1 %#03X
>       1 %#018lx
>       1 %#010zx
> 
> ---
>  scripts/checkpatch.pl | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d65334588eb4c..5840f3f2aee6f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6695,6 +6695,31 @@ sub process {
>  				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
>  				$fmt =~ s/%%//g;
>  
> +				while ($fmt =~ /\%#([\d]+)/g) {
> +					my $length = $1;
> +					my $pref_len;
> +					if ($length < 4) {
> +						$pref_len = '04';
> +					} elsif ($length == 5) {
> +						$pref_len = '06';
> +					} elsif ($length > 6 && $length < 10) {
> +						$pref_len = '010';
> +					} elsif ($length > 10 && $length < 18) {
> +						$pref_len = '018';
> +					} elsif ($length > 18) {
> +						$pref_len = '<something else>';
> +					}
> +					if (defined($pref_len)) {
> +						if (!defined($stat_real)) {
> +							$stat_real = get_stat_real($linenr, $lc);
> +						}
> +						WARN("VSPRINTF_SPECIAL_LENGTH",
> +						     "Unusual special length '%#$length' in 0x prefixed output, length is usually 2 more than the desired width - perhaps use '%#${pref_len}'\n" . "$here\n$stat_real");
> +					}
> +				}
> +
> +				pos($fmt) = 0;
> +
>  				while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) {
>  					$specifier = $1;
>  					$extension = $2;
> 

Re: General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

[Joe Perches]

On Thu, 2021-06-03 at 20:25 +0100, Jonathan Cameron wrote:
> On Thu, 03 Jun 2021 11:58:15 -0700 Joe Perches <joe@perches.com> wrote:
> > It looks to me as though %#<foo> is relatively commonly misused in the kernel.
> > 
> > Pehaps for the decimal portion of the format, checkpatch could have some
> > test for use of non-standard lengths.
> > 
> > Given the use is generally meant for a u8, u16, u32, or u64, perhaps
> > checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18.
> 
> Would have saved me some trouble, so I'm definitely in favour of checkpatch
> catching this.
> 
> I wonder if a better option is to match on 1, 2, 4, 8, 16 as likely to be
> caused by people getting the usage wrong rather than a deliberate attempt
> to pretty print something a little unusual?

Dunno.  %#0x and %x[123] seems pretty silly as it'll always emit the number
of digits in the value.

There aren't too many other odd uses other than those.

> > $ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn

8 and 16 are perhaps commonly misused.
> >     392 %#08x
> >      17 %#08lx
> >       9 %#08zx
> >       6 %#8x
> >       4 %#08llx
> >       1 %#8lx
> >       1 %#08

> >       7 %#16llx
> >       5 %#16
> >       4 %#016Lx
> >       1 %#16x
> >       1 %#16lx

These are the odd ones:

> >     144 %#02x
> >      27 %#0x
> >      23 %#2x
> >      17 %#3lx
> >      15 %#3x
> >      14 %#03x
> >       6 %#012llx
> >       4 %#05x
> >       4 %#02X
> >       3 %#01x
> >       2 %#09x
> >       2 %#05lx
> >       1 %#5x
> >       1 %#5lx
> >       1 %#2Lx
> >       1 %#2llx
> >       1 %#12x
> >       1 %#0lx
> >       1 %#05llx
> >       1 %#03X

Re: General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

[Jonathan Cameron]

On Thu, 03 Jun 2021 12:34:28 -0700
Joe Perches <joe@perches.com> wrote:

> On Thu, 2021-06-03 at 20:25 +0100, Jonathan Cameron wrote:
> > On Thu, 03 Jun 2021 11:58:15 -0700 Joe Perches <joe@perches.com> wrote:  
> > > It looks to me as though %#<foo> is relatively commonly misused in the kernel.
> > > 
> > > Pehaps for the decimal portion of the format, checkpatch could have some
> > > test for use of non-standard lengths.
> > > 
> > > Given the use is generally meant for a u8, u16, u32, or u64, perhaps
> > > checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18.  
> > 
> > Would have saved me some trouble, so I'm definitely in favour of checkpatch
> > catching this.
> > 
> > I wonder if a better option is to match on 1, 2, 4, 8, 16 as likely to be
> > caused by people getting the usage wrong rather than a deliberate attempt
> > to pretty print something a little unusual?  
> 
> Dunno.  %#0x and %x[123] seems pretty silly as it'll always emit the number
> of digits in the value.

Good point for those two cases - definitely want to catch them.

> 
> There aren't too many other odd uses other than those.

Perhaps you are right - after all anyone who is deliberately doing something
unusual will know they are doing it and can ignore the checkpatch warning.
Not as though it's going to happen very often from your stats below - looks
like maybe 17 of those cases 'might' be deliberate.

Jonathan
> 
> > > $ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn  
> 
> 8 and 16 are perhaps commonly misused.
> > >     392 %#08x
> > >      17 %#08lx
> > >       9 %#08zx
> > >       6 %#8x
> > >       4 %#08llx
> > >       1 %#8lx
> > >       1 %#08  
> 
> > >       7 %#16llx
> > >       5 %#16
> > >       4 %#016Lx
> > >       1 %#16x
> > >       1 %#16lx  
> 
> These are the odd ones:
> 
> > >     144 %#02x
> > >      27 %#0x
> > >      23 %#2x
> > >      17 %#3lx
> > >      15 %#3x
> > >      14 %#03x
> > >       6 %#012llx
> > >       4 %#05x
> > >       4 %#02X
> > >       3 %#01x
> > >       2 %#09x
> > >       2 %#05lx
> > >       1 %#5x
> > >       1 %#5lx
> > >       1 %#2Lx
> > >       1 %#2llx
> > >       1 %#12x
> > >       1 %#0lx
> > >       1 %#05llx
> > >       1 %#03X  
> 
> 

Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings

[Jonathan Cameron]

Hi All,

Not quite sure how I managed to cc Nathan on the patches, but not the
cover letter.  Anyhow +Cc Nathan.

@Nathan. I dropped the tags you gave to v1 as described below, but
it's not a huge change. Very much appreciated if you could take a quick look.

Input from others also appreciated!

Thanks,

Jonathan


On Thu,  3 Jun 2021 19:06:08 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> A wrong use of one of these in
> https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
> included a reference from Nathan to a patch discouraging the use of
> these strings in general:
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> 
> I did a quick grep and established we only have a few instances of these in
> IIO anyway, so in the interests of avoiding those existing cases getting
> cut and paste into new drivers, let's just clear them out now.
> 
> Note that patch from Arnd is now also part of this series, due to the
> length specifier related issue Joe raised.
> 
> I have chosen to go with 0x%02x rather than %#04x as I find it more readable.
> 
> V2:
> Use 0x%02x (Joe Perches)
> Include Arnd's original patch, modified for the above.
> 
> Arnd Bergmann (1):
>   iio: si1133: fix format string warnings
> 
> Jonathan Cameron (3):
>   iio: light: si1133: Drop remaining uses of %hhx format string.
>   iio: imu: inv_mpu6050: Drop use of %hhx format string.
>   iio: light: si1145: Drop use of %hhx format specifier.
> 
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  5 ++---
>  drivers/iio/light/si1133.c                 | 18 +++++++++---------
>  drivers/iio/light/si1145.c                 | 10 +++++-----
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 

Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings

[Jonathan Cameron]

On Wed, 9 Jun 2021 14:09:34 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> Hi All,
> 
> Not quite sure how I managed to cc Nathan on the patches, but not the
> cover letter.  Anyhow +Cc Nathan.
> 
> @Nathan. I dropped the tags you gave to v1 as described below, but
> it's not a huge change. Very much appreciated if you could take a quick look.

Thanks for the quick turn around Nathan!

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at.

Note I won't push this out as non rebasing for a few days so
still time to take additional reviews into account etc.

Jonathan

> 
> Input from others also appreciated!
> 
> Thanks,
> 
> Jonathan
> 
> 
> On Thu,  3 Jun 2021 19:06:08 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > A wrong use of one of these in
> > https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
> > included a reference from Nathan to a patch discouraging the use of
> > these strings in general:
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> > 
> > I did a quick grep and established we only have a few instances of these in
> > IIO anyway, so in the interests of avoiding those existing cases getting
> > cut and paste into new drivers, let's just clear them out now.
> > 
> > Note that patch from Arnd is now also part of this series, due to the
> > length specifier related issue Joe raised.
> > 
> > I have chosen to go with 0x%02x rather than %#04x as I find it more readable.
> > 
> > V2:
> > Use 0x%02x (Joe Perches)
> > Include Arnd's original patch, modified for the above.
> > 
> > Arnd Bergmann (1):
> >   iio: si1133: fix format string warnings
> > 
> > Jonathan Cameron (3):
> >   iio: light: si1133: Drop remaining uses of %hhx format string.
> >   iio: imu: inv_mpu6050: Drop use of %hhx format string.
> >   iio: light: si1145: Drop use of %hhx format specifier.
> > 
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  5 ++---
> >  drivers/iio/light/si1133.c                 | 18 +++++++++---------
> >  drivers/iio/light/si1145.c                 | 10 +++++-----
> >  3 files changed, 16 insertions(+), 17 deletions(-)
> >   
>