* [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-28 22:25 kernel test robot
2022-08-30 10:03 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: kernel test robot @ 2022-08-28 22:25 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: llvm, kbuild-all, linux-kernel, Jonathan Cameron, Andy Shevchenko
tree: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
head: fc32c348a52157665cd8f3f060669ef4e8a03cd4
commit: 1a622d75985c5950a470edc50c7ad7c10e79a1d3 [124/129] iio: add MEMSensing MSA311 3-axis accelerometer driver
config: powerpc-randconfig-r024-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290618.wU7mHfOp-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project a2100daf12fb980a29fd1a9c85ccf8eaaaf79730)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=1a622d75985c5950a470edc50c7ad7c10e79a1d3
git remote add jic23-iio https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
git fetch --no-tags jic23-iio testing
git checkout 1a622d75985c5950a470edc50c7ad7c10e79a1d3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
"msa311-%hhx", partid);
~~~~ ^~~~~~
%x
1 warning generated.
vim +993 drivers/iio/accel/msa311.c
977
978 static int msa311_check_partid(struct msa311_priv *msa311)
979 {
980 struct device *dev = msa311->dev;
981 unsigned int partid;
982 int err;
983
984 err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
985 if (err)
986 return dev_err_probe(dev, err, "failed to read partid\n");
987
988 if (partid != MSA311_WHO_AM_I)
989 dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
990 partid, MSA311_WHO_AM_I);
991
992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> 993 "msa311-%hhx", partid);
994 if (!msa311->chip_name)
995 return dev_err_probe(dev, -ENOMEM, "can't alloc chip name\n");
996
997 return 0;
998 }
999
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-28 22:25 [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' kernel test robot
@ 2022-08-30 10:03 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2022-08-30 10:03 UTC (permalink / raw)
To: kernel test robot
Cc: Dmitry Rokosov, llvm, kbuild-all, linux-kernel, Andy Shevchenko
On Mon, 29 Aug 2022 06:25:53 +0800
kernel test robot <lkp@intel.com> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
> head: fc32c348a52157665cd8f3f060669ef4e8a03cd4
> commit: 1a622d75985c5950a470edc50c7ad7c10e79a1d3 [124/129] iio: add MEMSensing MSA311 3-axis accelerometer driver
> config: powerpc-randconfig-r024-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290618.wU7mHfOp-lkp@intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project a2100daf12fb980a29fd1a9c85ccf8eaaaf79730)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc cross compiling tool for clang build
> # apt-get install binutils-powerpc-linux-gnu
> # https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=1a622d75985c5950a470edc50c7ad7c10e79a1d3
> git remote add jic23-iio https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> git fetch --no-tags jic23-iio testing
> git checkout 1a622d75985c5950a470edc50c7ad7c10e79a1d3
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/iio/accel/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> "msa311-%hhx", partid);
> ~~~~ ^~~~~~
> %x
> 1 warning generated.
>
>
> vim +993 drivers/iio/accel/msa311.c
>
> 977
> 978 static int msa311_check_partid(struct msa311_priv *msa311)
> 979 {
> 980 struct device *dev = msa311->dev;
> 981 unsigned int partid;
> 982 int err;
> 983
> 984 err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> 985 if (err)
> 986 return dev_err_probe(dev, err, "failed to read partid\n");
> 987
> 988 if (partid != MSA311_WHO_AM_I)
> 989 dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> 990 partid, MSA311_WHO_AM_I);
> 991
> 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > 993 "msa311-%hhx", partid);
Dmitry,
I'm thinking intent here was to limit range of what was printed. Maybe better to use
local u8 variable or cast?
I can fix it up if that's fine with you - or even better send me a patch that fixes
it however you prefer!
Jonathan
> 994 if (!msa311->chip_name)
> 995 return dev_err_probe(dev, -ENOMEM, "can't alloc chip name\n");
> 996
> 997 return 0;
> 998 }
> 999
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-30 10:03 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2022-08-30 10:03 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]
On Mon, 29 Aug 2022 06:25:53 +0800
kernel test robot <lkp@intel.com> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
> head: fc32c348a52157665cd8f3f060669ef4e8a03cd4
> commit: 1a622d75985c5950a470edc50c7ad7c10e79a1d3 [124/129] iio: add MEMSensing MSA311 3-axis accelerometer driver
> config: powerpc-randconfig-r024-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290618.wU7mHfOp-lkp(a)intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project a2100daf12fb980a29fd1a9c85ccf8eaaaf79730)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc cross compiling tool for clang build
> # apt-get install binutils-powerpc-linux-gnu
> # https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=1a622d75985c5950a470edc50c7ad7c10e79a1d3
> git remote add jic23-iio https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> git fetch --no-tags jic23-iio testing
> git checkout 1a622d75985c5950a470edc50c7ad7c10e79a1d3
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/iio/accel/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> "msa311-%hhx", partid);
> ~~~~ ^~~~~~
> %x
> 1 warning generated.
>
>
> vim +993 drivers/iio/accel/msa311.c
>
> 977
> 978 static int msa311_check_partid(struct msa311_priv *msa311)
> 979 {
> 980 struct device *dev = msa311->dev;
> 981 unsigned int partid;
> 982 int err;
> 983
> 984 err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> 985 if (err)
> 986 return dev_err_probe(dev, err, "failed to read partid\n");
> 987
> 988 if (partid != MSA311_WHO_AM_I)
> 989 dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> 990 partid, MSA311_WHO_AM_I);
> 991
> 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > 993 "msa311-%hhx", partid);
Dmitry,
I'm thinking intent here was to limit range of what was printed. Maybe better to use
local u8 variable or cast?
I can fix it up if that's fine with you - or even better send me a patch that fixes
it however you prefer!
Jonathan
> 994 if (!msa311->chip_name)
> 995 return dev_err_probe(dev, -ENOMEM, "can't alloc chip name\n");
> 996
> 997 return 0;
> 998 }
> 999
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-30 10:03 ` Jonathan Cameron
@ 2022-08-30 12:45 ` Andy Shevchenko
-1 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-08-30 12:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kernel test robot, Dmitry Rokosov, llvm, kbuild-all,
Linux Kernel Mailing List
On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Mon, 29 Aug 2022 06:25:53 +0800
> kernel test robot <lkp@intel.com> wrote:
...
> > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > "msa311-%hhx", partid);
> > ~~~~ ^~~~~~
> > %x
> > 1 warning generated.
> > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > 993 "msa311-%hhx", partid);
> I'm thinking intent here was to limit range of what was printed. Maybe better to use
> local u8 variable or cast?
>
> I can fix it up if that's fine with you - or even better send me a patch that fixes
> it however you prefer!
Looking back at what Linus said about those specifiers, I would rather
go with simple %x or %02x.
P.S. Surprisingly many C developers don't know the difference between
%hhx and %02x, which is easy to check by
char a = -1;
printf("%hhx <==> %02x\n", a, a);
a = 217;
printf("%hhx <==> %02x\n", a, a);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-30 12:45 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-08-30 12:45 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]
On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Mon, 29 Aug 2022 06:25:53 +0800
> kernel test robot <lkp@intel.com> wrote:
...
> > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > "msa311-%hhx", partid);
> > ~~~~ ^~~~~~
> > %x
> > 1 warning generated.
> > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > 993 "msa311-%hhx", partid);
> I'm thinking intent here was to limit range of what was printed. Maybe better to use
> local u8 variable or cast?
>
> I can fix it up if that's fine with you - or even better send me a patch that fixes
> it however you prefer!
Looking back at what Linus said about those specifiers, I would rather
go with simple %x or %02x.
P.S. Surprisingly many C developers don't know the difference between
%hhx and %02x, which is easy to check by
char a = -1;
printf("%hhx <==> %02x\n", a, a);
a = 217;
printf("%hhx <==> %02x\n", a, a);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-30 12:45 ` Andy Shevchenko
@ 2022-08-30 12:51 ` Andy Shevchenko
-1 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-08-30 12:51 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kernel test robot, Dmitry Rokosov, llvm, kbuild-all,
Linux Kernel Mailing List
On Tue, Aug 30, 2022 at 3:45 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Mon, 29 Aug 2022 06:25:53 +0800
> > kernel test robot <lkp@intel.com> wrote:
>
> ...
>
> > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > "msa311-%hhx", partid);
> > > ~~~~ ^~~~~~
> > > %x
> > > 1 warning generated.
>
> > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > 993 "msa311-%hhx", partid);
>
> > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > local u8 variable or cast?
> >
> > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > it however you prefer!
>
> Looking back at what Linus said about those specifiers, I would rather
> go with simple %x or %02x.
>
> P.S. Surprisingly many C developers don't know the difference between
> %hhx and %02x, which is easy to check by
>
> char a = -1;
> printf("%hhx <==> %02x\n", a, a);
> a = 217;
> printf("%hhx <==> %02x\n", a, a);
And additional part
unsigned int b = 7, c = 1027;
printf("%02x(b) %02x(c)\n", b, c);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-30 12:51 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-08-30 12:51 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
On Tue, Aug 30, 2022 at 3:45 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Mon, 29 Aug 2022 06:25:53 +0800
> > kernel test robot <lkp@intel.com> wrote:
>
> ...
>
> > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > "msa311-%hhx", partid);
> > > ~~~~ ^~~~~~
> > > %x
> > > 1 warning generated.
>
> > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > 993 "msa311-%hhx", partid);
>
> > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > local u8 variable or cast?
> >
> > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > it however you prefer!
>
> Looking back at what Linus said about those specifiers, I would rather
> go with simple %x or %02x.
>
> P.S. Surprisingly many C developers don't know the difference between
> %hhx and %02x, which is easy to check by
>
> char a = -1;
> printf("%hhx <==> %02x\n", a, a);
> a = 217;
> printf("%hhx <==> %02x\n", a, a);
And additional part
unsigned int b = 7, c = 1027;
printf("%02x(b) %02x(c)\n", b, c);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-30 12:45 ` Andy Shevchenko
@ 2022-08-31 0:24 ` Dmitry Rokosov
-1 siblings, 0 replies; 13+ messages in thread
From: Dmitry Rokosov @ 2022-08-31 0:24 UTC (permalink / raw)
To: Andy Shevchenko, Jonathan Cameron
Cc: kernel test robot, llvm, kbuild-all, Linux Kernel Mailing List
Hello Jonathan and Andy,
Sorry for such a late response, a couple of days ago my daughter was born.
So I couldn't reach my laptop :)
Please find my thoughts below.
> > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > "msa311-%hhx", partid);
> > > ~~~~ ^~~~~~
> > > %x
> > > 1 warning generated.
>
> > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > 993 "msa311-%hhx", partid);
>
> > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > local u8 variable or cast?
> >
> > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > it however you prefer!
>
> Looking back at what Linus said about those specifiers, I would rather
> go with simple %x or %02x.
>
> P.S. Surprisingly many C developers don't know the difference between
> %hhx and %02x, which is easy to check by
>
> char a = -1;
> printf("%hhx <==> %02x\n", a, a);
> a = 217;
> printf("%hhx <==> %02x\n", a, a);
Thank you for pointing to Linus answer. I have explored it at the link:
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
Actually, Linus described one exception to this rule, which I have
in my patch. I have an integer which I want to print as a char.
I see that Linus mentions it's a bad idea. I agree with that. But
currently %hhx => %02x replacement breaks the requested behavior, %02x
will not shrink integer value to char. I want to say, maybe it's better
just cast the value to u8 type and print as %x. What do you think? I can
prepare such a patch.
P.S. Andy's example to show the difference between %hhx and %02x makes
more clear why such a replacement is not acceptable here.
Output:
ff <==> ffffffff
d9 <==> ffffffd9
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-31 0:24 ` Dmitry Rokosov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Rokosov @ 2022-08-31 0:24 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]
Hello Jonathan and Andy,
Sorry for such a late response, a couple of days ago my daughter was born.
So I couldn't reach my laptop :)
Please find my thoughts below.
> > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > "msa311-%hhx", partid);
> > > ~~~~ ^~~~~~
> > > %x
> > > 1 warning generated.
>
> > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > 993 "msa311-%hhx", partid);
>
> > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > local u8 variable or cast?
> >
> > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > it however you prefer!
>
> Looking back at what Linus said about those specifiers, I would rather
> go with simple %x or %02x.
>
> P.S. Surprisingly many C developers don't know the difference between
> %hhx and %02x, which is easy to check by
>
> char a = -1;
> printf("%hhx <==> %02x\n", a, a);
> a = 217;
> printf("%hhx <==> %02x\n", a, a);
Thank you for pointing to Linus answer. I have explored it at the link:
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q(a)mail.gmail.com/
Actually, Linus described one exception to this rule, which I have
in my patch. I have an integer which I want to print as a char.
I see that Linus mentions it's a bad idea. I agree with that. But
currently %hhx => %02x replacement breaks the requested behavior, %02x
will not shrink integer value to char. I want to say, maybe it's better
just cast the value to u8 type and print as %x. What do you think? I can
prepare such a patch.
P.S. Andy's example to show the difference between %hhx and %02x makes
more clear why such a replacement is not acceptable here.
Output:
ff <==> ffffffff
d9 <==> ffffffd9
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-31 0:24 ` Dmitry Rokosov
@ 2022-08-31 8:38 ` Jonathan Cameron
-1 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2022-08-31 8:38 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Andy Shevchenko, kernel test robot, llvm, kbuild-all,
Linux Kernel Mailing List
On Wed, 31 Aug 2022 03:24:05 +0300
Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
> Hello Jonathan and Andy,
>
> Sorry for such a late response, a couple of days ago my daughter was born.
> So I couldn't reach my laptop :)
Congratulations and good luck! :)
>
> Please find my thoughts below.
>
> > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > > "msa311-%hhx", partid);
> > > > ~~~~ ^~~~~~
> > > > %x
> > > > 1 warning generated.
> >
> > > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > 993 "msa311-%hhx", partid);
> >
> > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > local u8 variable or cast?
> > >
> > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > it however you prefer!
> >
> > Looking back at what Linus said about those specifiers, I would rather
> > go with simple %x or %02x.
> >
> > P.S. Surprisingly many C developers don't know the difference between
> > %hhx and %02x, which is easy to check by
> >
> > char a = -1;
> > printf("%hhx <==> %02x\n", a, a);
> > a = 217;
> > printf("%hhx <==> %02x\n", a, a);
>
> Thank you for pointing to Linus answer. I have explored it at the link:
>
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
>
> Actually, Linus described one exception to this rule, which I have
> in my patch. I have an integer which I want to print as a char.
> I see that Linus mentions it's a bad idea. I agree with that. But
> currently %hhx => %02x replacement breaks the requested behavior, %02x
> will not shrink integer value to char. I want to say, maybe it's better
> just cast the value to u8 type and print as %x. What do you think? I can
> prepare such a patch.
>
> P.S. Andy's example to show the difference between %hhx and %02x makes
> more clear why such a replacement is not acceptable here.
>
> Output:
> ff <==> ffffffff
> d9 <==> ffffffd9
>
In this case the storage is an unsigned int, not an unsigned char.
Hence the value will be small and positive. So I'm fairly sure you
won't hit the above because it's
0x000000ff --> ff
0x000000d9 --> d9
The range is limited to 8 bits because that's all the underlying register
holds.
Jonathan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-31 8:38 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2022-08-31 8:38 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]
On Wed, 31 Aug 2022 03:24:05 +0300
Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
> Hello Jonathan and Andy,
>
> Sorry for such a late response, a couple of days ago my daughter was born.
> So I couldn't reach my laptop :)
Congratulations and good luck! :)
>
> Please find my thoughts below.
>
> > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > > "msa311-%hhx", partid);
> > > > ~~~~ ^~~~~~
> > > > %x
> > > > 1 warning generated.
> >
> > > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > 993 "msa311-%hhx", partid);
> >
> > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > local u8 variable or cast?
> > >
> > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > it however you prefer!
> >
> > Looking back at what Linus said about those specifiers, I would rather
> > go with simple %x or %02x.
> >
> > P.S. Surprisingly many C developers don't know the difference between
> > %hhx and %02x, which is easy to check by
> >
> > char a = -1;
> > printf("%hhx <==> %02x\n", a, a);
> > a = 217;
> > printf("%hhx <==> %02x\n", a, a);
>
> Thank you for pointing to Linus answer. I have explored it at the link:
>
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q(a)mail.gmail.com/
>
> Actually, Linus described one exception to this rule, which I have
> in my patch. I have an integer which I want to print as a char.
> I see that Linus mentions it's a bad idea. I agree with that. But
> currently %hhx => %02x replacement breaks the requested behavior, %02x
> will not shrink integer value to char. I want to say, maybe it's better
> just cast the value to u8 type and print as %x. What do you think? I can
> prepare such a patch.
>
> P.S. Andy's example to show the difference between %hhx and %02x makes
> more clear why such a replacement is not acceptable here.
>
> Output:
> ff <==> ffffffff
> d9 <==> ffffffd9
>
In this case the storage is an unsigned int, not an unsigned char.
Hence the value will be small and positive. So I'm fairly sure you
won't hit the above because it's
0x000000ff --> ff
0x000000d9 --> d9
The range is limited to 8 bits because that's all the underlying register
holds.
Jonathan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
2022-08-31 8:38 ` Jonathan Cameron
@ 2022-08-31 21:04 ` Dmitry Rokosov
-1 siblings, 0 replies; 13+ messages in thread
From: Dmitry Rokosov @ 2022-08-31 21:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, kernel test robot, llvm, kbuild-all, kernel,
Linux Kernel Mailing List
On Wed, Aug 31, 2022 at 09:38:10AM +0100, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 03:24:05 +0300
> Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
>
> > Hello Jonathan and Andy,
> >
> > Sorry for such a late response, a couple of days ago my daughter was born.
> > So I couldn't reach my laptop :)
>
> Congratulations and good luck! :)
Thank you! :)
> > > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > > > "msa311-%hhx", partid);
> > > > > ~~~~ ^~~~~~
> > > > > %x
> > > > > 1 warning generated.
> > >
> > > > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > 993 "msa311-%hhx", partid);
> > >
> > > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > > local u8 variable or cast?
> > > >
> > > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > > it however you prefer!
> > >
> > > Looking back at what Linus said about those specifiers, I would rather
> > > go with simple %x or %02x.
> > >
> > > P.S. Surprisingly many C developers don't know the difference between
> > > %hhx and %02x, which is easy to check by
> > >
> > > char a = -1;
> > > printf("%hhx <==> %02x\n", a, a);
> > > a = 217;
> > > printf("%hhx <==> %02x\n", a, a);
> >
> > Thank you for pointing to Linus answer. I have explored it at the link:
> >
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> >
> > Actually, Linus described one exception to this rule, which I have
> > in my patch. I have an integer which I want to print as a char.
> > I see that Linus mentions it's a bad idea. I agree with that. But
> > currently %hhx => %02x replacement breaks the requested behavior, %02x
> > will not shrink integer value to char. I want to say, maybe it's better
> > just cast the value to u8 type and print as %x. What do you think? I can
> > prepare such a patch.
> >
> > P.S. Andy's example to show the difference between %hhx and %02x makes
> > more clear why such a replacement is not acceptable here.
> >
> > Output:
> > ff <==> ffffffff
> > d9 <==> ffffffd9
> >
> In this case the storage is an unsigned int, not an unsigned char.
> Hence the value will be small and positive. So I'm fairly sure you
> won't hit the above because it's
>
> 0x000000ff --> ff
> 0x000000d9 --> d9
>
> The range is limited to 8 bits because that's all the underlying register
> holds.
From "data" format point of view you are right. We have regmap over I2C
and register values will be limited to 8 bits only. But in general
unsigned int value bigger than 0xff formatted by %02x will not be
limited by two positions only. In other words, we can use a simple %x
with the same success.
I want to say if our goal is shrinking the unsigned int value to first
byte in hex format w/o %hhx using, we need to cast unsigned int value to
unsigned char and printout it using simple %x or %02x.
For example, in my opinion, in the below code snippet, only first and
third printout formatting are correct. Currently, we are using the
second in the merged patchset.
>>>
unsigned int a = 0xDEADBEEF;
printf("%hhx <==> %02x (uint8_t:%02x)\n", a, a, (unsigned char)a);
<<<
Output:
ef <==> deadbeef (uint8_t:ef)
===
Anyway, regmap over I2C abstraction limits our value to the 8-bit range,
so functionally %02x is working well here.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'
@ 2022-08-31 21:04 ` Dmitry Rokosov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Rokosov @ 2022-08-31 21:04 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]
On Wed, Aug 31, 2022 at 09:38:10AM +0100, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 03:24:05 +0300
> Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
>
> > Hello Jonathan and Andy,
> >
> > Sorry for such a late response, a couple of days ago my daughter was born.
> > So I couldn't reach my laptop :)
>
> Congratulations and good luck! :)
Thank you! :)
> > > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]
> > > > > "msa311-%hhx", partid);
> > > > > ~~~~ ^~~~~~
> > > > > %x
> > > > > 1 warning generated.
> > >
> > > > > 992 msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > 993 "msa311-%hhx", partid);
> > >
> > > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > > local u8 variable or cast?
> > > >
> > > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > > it however you prefer!
> > >
> > > Looking back at what Linus said about those specifiers, I would rather
> > > go with simple %x or %02x.
> > >
> > > P.S. Surprisingly many C developers don't know the difference between
> > > %hhx and %02x, which is easy to check by
> > >
> > > char a = -1;
> > > printf("%hhx <==> %02x\n", a, a);
> > > a = 217;
> > > printf("%hhx <==> %02x\n", a, a);
> >
> > Thank you for pointing to Linus answer. I have explored it at the link:
> >
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q(a)mail.gmail.com/
> >
> > Actually, Linus described one exception to this rule, which I have
> > in my patch. I have an integer which I want to print as a char.
> > I see that Linus mentions it's a bad idea. I agree with that. But
> > currently %hhx => %02x replacement breaks the requested behavior, %02x
> > will not shrink integer value to char. I want to say, maybe it's better
> > just cast the value to u8 type and print as %x. What do you think? I can
> > prepare such a patch.
> >
> > P.S. Andy's example to show the difference between %hhx and %02x makes
> > more clear why such a replacement is not acceptable here.
> >
> > Output:
> > ff <==> ffffffff
> > d9 <==> ffffffd9
> >
> In this case the storage is an unsigned int, not an unsigned char.
> Hence the value will be small and positive. So I'm fairly sure you
> won't hit the above because it's
>
> 0x000000ff --> ff
> 0x000000d9 --> d9
>
> The range is limited to 8 bits because that's all the underlying register
> holds.
>From "data" format point of view you are right. We have regmap over I2C
and register values will be limited to 8 bits only. But in general
unsigned int value bigger than 0xff formatted by %02x will not be
limited by two positions only. In other words, we can use a simple %x
with the same success.
I want to say if our goal is shrinking the unsigned int value to first
byte in hex format w/o %hhx using, we need to cast unsigned int value to
unsigned char and printout it using simple %x or %02x.
For example, in my opinion, in the below code snippet, only first and
third printout formatting are correct. Currently, we are using the
second in the merged patchset.
>>>
unsigned int a = 0xDEADBEEF;
printf("%hhx <==> %02x (uint8_t:%02x)\n", a, a, (unsigned char)a);
<<<
Output:
ef <==> deadbeef (uint8_t:ef)
===
Anyway, regmap over I2C abstraction limits our value to the 8-bit range,
so functionally %02x is working well here.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-08-31 21:04 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-28 22:25 [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' kernel test robot
2022-08-30 10:03 ` Jonathan Cameron
2022-08-30 10:03 ` Jonathan Cameron
2022-08-30 12:45 ` Andy Shevchenko
2022-08-30 12:45 ` Andy Shevchenko
2022-08-30 12:51 ` Andy Shevchenko
2022-08-30 12:51 ` Andy Shevchenko
2022-08-31 0:24 ` Dmitry Rokosov
2022-08-31 0:24 ` Dmitry Rokosov
2022-08-31 8:38 ` Jonathan Cameron
2022-08-31 8:38 ` Jonathan Cameron
2022-08-31 21:04 ` Dmitry Rokosov
2022-08-31 21:04 ` Dmitry Rokosov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.