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

[Julia Lawall]

On Thu, 22 Oct 2026, Alex Elder wrote:

> On 10/22/26 8:35 AM, Julia Lawall wrote:
> > 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...
>
> Bryan was on the Greybus kernel team.  Remove that address from
> the list.  I'll let him know that e-mail is bouncing so he can
> add himself back.

Actually, the email comes from the MAINTAINERS file.

julia

>
> 					-Alex
>
> >
> > 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;
> > > >    	}
> > >
> > > . . .
> > >
> > >
>
>