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

[Julia Lawall]

Does anyone on the greybus list know anything about this person:

Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx>

The email address is not valid, which is somewhat tiresome...

julia



On Thu, 22 Oct 2026, Alex Elder wrote:

> On 10/21/26 11:34 AM, pavankumaryalagada@xxxxxxxxx wrote:
> > From: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
> >
> > Unsigned variables are printed using %d.
> > Replace them with %u to match their types.
> >
> > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
>
> 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 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@xxxxxxxxxxxx>
>
> > ---
> >   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;
> >   	}
>
> . . .
>
>