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

[Alex Elder <elder@ieee.org>]

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@nexus-software.ie>
> 
> 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.

					-Alex

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