Re: [PATCH] staging: greybus: fw-download: Fix find firmware req

[Ayush Singh <ayush@beagleboard.org>]

On 10/22/25 7:40 PM, Dan Carpenter wrote:

> On Wed, Oct 22, 2025 at 07:22:49PM +0530, Ayush Singh wrote:
>> On 10/22/25 5:33 PM, Dan Carpenter wrote:
>>> On Wed, Oct 22, 2025 at 12:57:57PM +0530, Ayush Singh wrote:
>>>> diff --git a/drivers/staging/greybus/fw-download.c b/drivers/staging/greybus/fw-download.c
>>>> index 9a09bd3af79ba0dcf7efa683f4e86246bcd473a5..06f1be8f3121e29551ea8416d5ee2666339b2fe3 100644
>>>> --- a/drivers/staging/greybus/fw-download.c
>>>> +++ b/drivers/staging/greybus/fw-download.c
>>>> @@ -159,7 +159,7 @@ static int exceeds_release_timeout(struct fw_request *fw_req)
>>>>    /* This returns path of the firmware blob on the disk */
>>>>    static struct fw_request *find_firmware(struct fw_download *fw_download,
>>>> -					const char *tag)
>>>> +					const char *tag, const char *format)
>>>>    {
>>>>    	struct gb_interface *intf = fw_download->connection->bundle->intf;
>>>>    	struct fw_request *fw_req;
>>>> @@ -178,10 +178,17 @@ static struct fw_request *find_firmware(struct fw_download *fw_download,
>>>>    	}
>>>>    	fw_req->firmware_id = ret;
>>>> -	snprintf(fw_req->name, sizeof(fw_req->name),
>>>> -		 FW_NAME_PREFIX "%08x_%08x_%08x_%08x_%s.tftf",
>>>> -		 intf->ddbl1_manufacturer_id, intf->ddbl1_product_id,
>>>> -		 intf->vendor_id, intf->product_id, tag);
>>>> +	if (strnlen(format, GB_FIRMWARE_FORMAT_MAX_SIZE) == 0) {
>>> Change this to:
>>>
>>> 	if (format[0] == '\0') {
>>>
>>> In the caller, the assumption that format is at least
>>> GB_FIRMWARE_FORMAT_MAX_SIZE makes sense but in this function it
>>> doesn't make sense.
>> Ok, will do in the next version.
>>
>>>> +		snprintf(fw_req->name, sizeof(fw_req->name),
>>>> +			 FW_NAME_PREFIX "%08x_%08x_%08x_%08x_%s",
>>>> +			 intf->ddbl1_manufacturer_id, intf->ddbl1_product_id,
>>>> +			 intf->vendor_id, intf->product_id, tag);
>>>> +	} else {
>>>> +		snprintf(fw_req->name, sizeof(fw_req->name),
>>>> +			 FW_NAME_PREFIX "%08x_%08x_%08x_%08x_%s.%s",
>>>> +			 intf->ddbl1_manufacturer_id, intf->ddbl1_product_id,
>>>> +			 intf->vendor_id, intf->product_id, tag, format);
>>>> +	}
>>>>    	dev_info(fw_download->parent, "Requested firmware package '%s'\n",
>>>>    		 fw_req->name);
>>>> @@ -225,7 +232,7 @@ static int fw_download_find_firmware(struct gb_operation *op)
>>>>    	struct gb_fw_download_find_firmware_request *request;
>>>>    	struct gb_fw_download_find_firmware_response *response;
>>>>    	struct fw_request *fw_req;
>>>> -	const char *tag;
>>>> +	const char *tag, *format;
>>>>    	if (op->request->payload_size != sizeof(*request)) {
>>>>    		dev_err(fw_download->parent,
>>> We have changed the sizeof(*request) but we haven't changed
>>> ->payload_size so how can this ever be true?  Did you test this change?
>>
>> The request originates in greybus node. The payload size here is calculate
>> from the greybus message header. It is not a hard coded value. So as long as
>> the node sets it correctly, it will work fine.
> I guess, how was this working for other people then?  It seems like a
> behavior change.


Well, it is technically a breaking change, if any device was already 
using fw download protocol. With that said, I have no idea who is using 
greybus right now. And since the changes are in staging drivers, it 
probably is fine.

I don't really know if the spec came first or linux implementation. But 
one of them is currently incorrect.

Just to clarify, greybus-for-zephyr is not the original or source of 
truth implementation of greybus. I just found the inconsistency between 
what the spec says, and what Linux kernel implements and thought that 
spec should probably have higher priority.


Best Regards,

Ayush Singh