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/[email protected]/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 <[email protected]>
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
On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
<[email protected]> wrote:
> On Mon, 29 Aug 2022 06:25:53 +0800
> kernel test robot <[email protected]> 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
On Tue, Aug 30, 2022 at 3:45 PM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Aug 30, 2022 at 1:03 PM Jonathan Cameron
> <[email protected]> wrote:
> > On Mon, 29 Aug 2022 06:25:53 +0800
> > kernel test robot <[email protected]> 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
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
On Wed, Aug 31, 2022 at 09:38:10AM +0100, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 03:24:05 +0300
> Dmitry Rokosov <[email protected]> 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