linux-kernel - 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'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 1 Sep 2022 00:04:35 +0300
From:   Dmitry Rokosov <ddrokosov@...rdevices.ru>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        kernel test robot <lkp@...el.com>, <llvm@...ts.linux.dev>,
        <kbuild-all@...ts.01.org>, <kernel@...rdevices.ru>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: 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'

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@...rdevices.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