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
2022-08-31T21:04:43Z

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