[v2] gpiolib: acpi: use correct format characters

When compiling with -Wformat, clang emits the following warning:

drivers/gpio/gpiolib-acpi.c:393:4: warning: format specifies type
'unsigned char' but the argument has type 'int' [-Wformat]
                        pin);
                        ^~~

The types of these arguments are unconditionally defined, so this patch
updates the format character to the correct ones casts to unsigned to
retain the behavior or the "hh" modifier..

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Bill Wendling <morbo@google.com>
---
v2 - Cast "pin" to retain the same width as the original.
---
 drivers/gpio/gpiolib-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

So I think that clang warning is only annoying, not helpful, but:

On Sat, Mar 19, 2022 at 3:22 PM Bill Wendling <morbo@google.com> wrote:
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index a5495ad31c9c..92dd9b8784f2 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -388,9 +388,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>
>         if (pin <= 255) {
>                 char ev_name[5];
> -               sprintf(ev_name, "_%c%02hhX",
> +               sprintf(ev_name, "_%c%02X",

This part I approve of.

>                         agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> -                       pin);
> +                       (unsigned char)pin);

But this cast seems pointless and wrong.

Casts in general are bad, and should be avoided unless there's a real
reason for them. And that reason doesn't seem to exist. We don't
actually want to truncate the value of 'pin', and just a few lines
earlier actually checked that it is in range.

And if 'pin' can't be negative - it comes from a 'u16' table
dereference - but even if it could have been that would have been a
different bug here anyway (and should have been fixed by tightening
the check).

So the cast doesn't add anything - not for humans, and not for a
compiler that could just optimize it away because it saw the range
check.

End result: just fix the pointless 'hh' in the print specifier. It
doesn't add anything, and only causes problems. Anybody who uses '%02X
to print a byte should only use it for byte values - and the code
already does.

Of course, the _reason_ for this all was a warning that was pointless
to begin with, and should never have existed. Clang was not smart
enough to take the range knowledge that it _could_ have taken into
account, and instead wrote out a completely bogus warning.

It's completely bogus not just because clang didn't do a sufficiently
good job of range analysis - it's completely bogus because a 'varargs'
function DOES NOT TAKE arguments of type 'char'.

So the *only* reason to use '%hhX' in the first place is that you
*want* the sprintf() to actually limit the value to a byte for you
(possibly because you have a signed char, know it will sign-extend to
'int', and want to limit it back to 8 bits).

If you *actually* had a 'unsigned char' to begin with, you'd be
completely insane to use %hhX. It's just pointless.

So warning that '%hhX' is paired with an 'int' is all just completely
mindless and wrong.

              Linus

On Sat, Mar 19, 2022 at 3:55 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I think that clang warning is only annoying, not helpful, but:
>
> On Sat, Mar 19, 2022 at 3:22 PM Bill Wendling <morbo@google.com> wrote:
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index a5495ad31c9c..92dd9b8784f2 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -388,9 +388,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
> >
> >         if (pin <= 255) {
> >                 char ev_name[5];
> > -               sprintf(ev_name, "_%c%02hhX",
> > +               sprintf(ev_name, "_%c%02X",
>
> This part I approve of.
>
> >                         agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> > -                       pin);
> > +                       (unsigned char)pin);
>
> But this cast seems pointless and wrong.
>
You're right. I was trying to ensure that the patch didn't change
behavior. But the cast achieves nothing. Thanks!

-bw

> Casts in general are bad, and should be avoided unless there's a real
> reason for them. And that reason doesn't seem to exist. We don't
> actually want to truncate the value of 'pin', and just a few lines
> earlier actually checked that it is in range.
>
> And if 'pin' can't be negative - it comes from a 'u16' table
> dereference - but even if it could have been that would have been a
> different bug here anyway (and should have been fixed by tightening
> the check).
>
> So the cast doesn't add anything - not for humans, and not for a
> compiler that could just optimize it away because it saw the range
> check.
>
> End result: just fix the pointless 'hh' in the print specifier. It
> doesn't add anything, and only causes problems. Anybody who uses '%02X
> to print a byte should only use it for byte values - and the code
> already does.
>
> Of course, the _reason_ for this all was a warning that was pointless
> to begin with, and should never have existed. Clang was not smart
> enough to take the range knowledge that it _could_ have taken into
> account, and instead wrote out a completely bogus warning.
>
> It's completely bogus not just because clang didn't do a sufficiently
> good job of range analysis - it's completely bogus because a 'varargs'
> function DOES NOT TAKE arguments of type 'char'.
>
> So the *only* reason to use '%hhX' in the first place is that you
> *want* the sprintf() to actually limit the value to a byte for you
> (possibly because you have a signed char, know it will sign-extend to
> 'int', and want to limit it back to 8 bits).
>
> If you *actually* had a 'unsigned char' to begin with, you'd be
> completely insane to use %hhX. It's just pointless.
>
> So warning that '%hhX' is paired with an 'int' is all just completely
> mindless and wrong.
>
>               Linus