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 <Jonathan.Cameron@huawei.com>]

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