[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'

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

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

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'

[Jonathan Cameron]

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	
> 

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'

[Andy Shevchenko]

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

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'

[Andy Shevchenko]

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

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'

[Dmitry Rokosov]

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

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'

[Jonathan Cameron]

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

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'

[Dmitry Rokosov]

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