Re: [PATCH] staging: greybus: Replace format specifiers

[Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>]

On Thu, Oct 22, 2026 at 09:30:26AM -0400, Julia Lawall wrote:
> 
> 
> On Thu, 22 Oct 2026, Alex Elder wrote:
> 
> > On 10/21/26 11:34 AM, pavankumaryalagada@gmail.com wrote:
> > > From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > >
> > > Unsigned variables are printed using %d.
> > > Replace them with %u to match their types.
> > >
> > > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> >
> > I have reviewed every one of these and they look good.  Quite a few
> > of them--because the size of the value being formatted is less than
> > 32 bits--would print correctly no matter what their value.  But I
> > think it at least does no harm to convert the format specifier to
> > unsigned in all of these cases.
> 
> I was suprised the compiler doesn't complain about this.  It seems to
> complain a lot about type-related format specifiers.  Yalagada maybe it
> could be worth adding something clearly invalid in one of the argument
> lists to see if the code is actually getting compiled, or if you need to
> set some config option to make that happen.

The files compiled sucessfully with the normal build settings. I also
enabled -Wformat-signedness to check for format mismatchse, as this check
is not enabled by default. It revealed a few more issues, which i'll look
into as well.

Pavan

> 
> julia
> 
> >
> > I have highlighted below an existing bug that should be fixed.  It
> > gives an example of something to look for when you do this sort of
> > activity.  Please arrange to get that bug fixed.
> >
> > I also pointed out one other observation for you to think about.
> >
> > Thanks for sending this.  I didn't check to find any other instances
> > you might have missed, but you seem to have found quite a few.
> >
> > Reviewed-by: Alex Elder <elder@riscstar.com>
> >
> > > ---
> > >   drivers/staging/greybus/audio_codec.c    | 20 ++++++++--------
> > >   drivers/staging/greybus/audio_topology.c | 30 ++++++++++++------------
> > >   drivers/staging/greybus/bootrom.c        |  2 +-
> > >   drivers/staging/greybus/fw-download.c    |  2 +-
> > >   drivers/staging/greybus/fw-management.c  |  2 +-
> > >   drivers/staging/greybus/gpio.c           |  2 +-
> > >   drivers/staging/greybus/light.c          |  2 +-
> > >   drivers/staging/greybus/log.c            |  4 ++--
> > >   drivers/staging/greybus/loopback.c       |  4 ++--
> > >   drivers/staging/greybus/power_supply.c   |  2 +-
> > >   drivers/staging/greybus/raw.c            |  2 +-
> > >   drivers/staging/greybus/sdio.c           |  4 ++--
> > >   drivers/staging/greybus/spilib.c         |  2 +-
> > >   drivers/staging/greybus/uart.c           |  4 ++--
> > >   14 files changed, 41 insertions(+), 41 deletions(-)
> > >
> >
> > . . .
> >
> > > diff --git a/drivers/staging/greybus/bootrom.c
> > > b/drivers/staging/greybus/bootrom.c
> > > index 83921d90c322..fb1fbd3f7b99 100644
> > > --- a/drivers/staging/greybus/bootrom.c
> > > +++ b/drivers/staging/greybus/bootrom.c
> > > @@ -222,7 +222,7 @@ static int gb_bootrom_firmware_size_request(struct
> > > gb_operation *op)
> > >   	size_response = op->response->payload;
> > >   	size_response->size = cpu_to_le32(bootrom->fw->size);
> > >   -	dev_dbg(dev, "%s: firmware size %d bytes\n",
> > > +	dev_dbg(dev, "%s: firmware size %u bytes\n",
> > >   		__func__, size_response->size);
> >
> > This line actually contains an error (and it was there before).
> >
> > The type of gb_bootrom_firmware_size_response->size is __le32.
> > So the value being formatted needs to be converted into CPU
> > byte order before being used.  That could be done this way:
> > 	le32_to_cpu(size_response->size)
> > or just use this, which is known from a few lines above to
> > have the proper value and byte order:
> > 	bootrom->fw->size
> >
> > Please add this to your list of things to fix, and submit it
> > as a separate patch.  This would be a bug fix, which would
> > also give you a little bit of a new experience.
> >
> > >     unlock:
> >
> > . . .
> >
> > > diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
> > > index 4f1b3f4db404..2314496a858a 100644
> > > --- a/drivers/staging/greybus/raw.c
> > > +++ b/drivers/staging/greybus/raw.c
> > > @@ -112,7 +112,7 @@ static int gb_raw_request_handler(struct gb_operation
> > > *op)
> > >   	receive = op->request->payload;
> > >   	len = le32_to_cpu(receive->len);
> > >   	if (len != (int)(op->request->payload_size - sizeof(__le32))) {
> > > -		dev_err(dev, "raw receive request wrong size %d vs %d\n", len,
> > > +		dev_err(dev, "raw receive request wrong size %u vs %d\n", len,
> > >   			(int)(op->request->payload_size - sizeof(__le32)));
> >
> > The above size calculation was done somewhere else without the
> > (int) cast, and the format specifier was %zu in that case.
> > I don't think this needs to be changed necessarily, but it's
> > something inconsistent across the Greybus code.  Consistency
> > helps readability and maintainability.
> >  >   		return -EINVAL;
> > >   	}
> >
> > . . .
> >
> >