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

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

[Ayush Singh]

According to the Greybus Spec published here [0], the Greybus firmware
download find firmware request should have both tag and format as
request arguments. However, currently, the linux kernel seems to have
defined this request incorrectly since format is missing.

The patch adds format to request and am using it as the file extension of
the firmware.

[0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
According to the Greybus Spec published here [0], the Greybus firmware
download find firmware request should have both tag and format as
request arguments. However, currently, the linux kernel seems to have
defined this request incorrectly since format is missing.

The patch adds format to request and am using it as the file extension of
the firmware.

I came across the bug while working on greybus-for-zephyr [1], to get it
ready for upstreaming as zephyr module.

Open Questions
***************

1. Handle empty format

Not sure what to do in case format is just NULL. Should the request
fail? There is no reason to not support firmware without extension. So
personally, I don't think it should be treated as error.

[0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
[1]: https://github.com/Ayush1325/greybus-for-zephyr
---
 drivers/staging/greybus/fw-download.c     | 31 ++++++++++++++++++++++++-------
 include/linux/greybus/greybus_protocols.h |  2 ++
 2 files changed, 26 insertions(+), 7 deletions(-)

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) {
+		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,
@@ -245,7 +252,17 @@ static int fw_download_find_firmware(struct gb_operation *op)
 		return -EINVAL;
 	}
 
-	fw_req = find_firmware(fw_download, tag);
+	format = (const char *)request->format;
+
+	/* firmware_format must be null-terminated */
+	if (strnlen(format, GB_FIRMWARE_FORMAT_MAX_SIZE) ==
+	    GB_FIRMWARE_FORMAT_MAX_SIZE) {
+		dev_err(fw_download->parent,
+			"firmware-format is not null-terminated\n");
+		return -EINVAL;
+	}
+
+	fw_req = find_firmware(fw_download, tag, format);
 	if (IS_ERR(fw_req))
 		return PTR_ERR(fw_req);
 
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
index 820134b0105c2caf8cea3ff0985c92d48d3a2d4c..48d91154847dbc7d3c01081eadc69f96dbe41a9f 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -214,10 +214,12 @@ struct gb_apb_request_cport_flags {
 #define GB_FW_DOWNLOAD_TYPE_RELEASE_FIRMWARE	0x03
 
 #define GB_FIRMWARE_TAG_MAX_SIZE		10
+#define GB_FIRMWARE_FORMAT_MAX_SIZE		10
 
 /* firmware download find firmware request/response */
 struct gb_fw_download_find_firmware_request {
 	__u8			firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE];
+	__u8			format[GB_FIRMWARE_FORMAT_MAX_SIZE];
 } __packed;
 
 struct gb_fw_download_find_firmware_response {

---
base-commit: aaa9c3550b60d6259d6ea8b1175ade8d1242444e
change-id: 20251022-gb-fw-a5db68692b5d

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

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

[Dan Carpenter]

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.


> +		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?

regards,
dan carpenter

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

[Ayush Singh]

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 am using the zephyr greybus firmware for my testing [2] using a 
BeaglePlay [3] and BeagleConnect Freedom [4].


[2]: https://github.com/Ayush1325/greybus-for-zephyr/pull/46

[3]: https://www.beagleboard.org/boards/beagleplay

[4]: https://www.beagleboard.org/boards/beagleconnect-freedom


>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Ayush Singh]

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

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

[Dan Carpenter]

On Wed, Oct 22, 2025 at 07:56:35PM +0530, Ayush Singh wrote:
> 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.

Ok, this is a question that many people face in one way or another.

Unless you have proof that there are no users, then we have to assume that
there are users.  An example of proof would be, there is a bug which
prevents the module from probing and no one has complained for over a year.
Just because code is in staging doesn't mean there are no users.

Sometimes code is in staging because the user interface is bad, and in
that case we need to update the usespace tools to handle the new interface
as well as the old one and then we can change the kernel.  That's something
we can do for staging code, but we hate to do it and once code leaves
staging then the rules become more strict.

We don't really care about the spec, it's good to support the spec but
what we really support are users.  We can't break the code for existing
users.

So we need to add if statements to support both formats.

	if (op->request->payload_size != sizeof(*request) &&
	    op->request->payload_size != OLD_SIZE) {

etc.

regards,
dan carpenter

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

[kernel test robot]

Hi Ayush,

kernel test robot noticed the following build warnings:

[auto build test WARNING on aaa9c3550b60d6259d6ea8b1175ade8d1242444e]

url:    https://github.com/intel-lab-lkp/linux/commits/Ayush-Singh/staging-greybus-fw-download-Fix-find-firmware-req/20251022-153520
base:   aaa9c3550b60d6259d6ea8b1175ade8d1242444e
patch link:    https://lore.kernel.org/r/20251022-gb-fw-v1-1-183b18500cd5%40beagleboard.org
patch subject: [PATCH] staging: greybus: fw-download: Fix find firmware req
config: arm64-randconfig-002-20251023 (https://download.01.org/0day-ci/archive/20251023/202510231209.JEFfW9s0-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251023/202510231209.JEFfW9s0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510231209.JEFfW9s0-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/staging/greybus/fw-download.c:14:
   drivers/staging/greybus/fw-download.c: In function 'gb_fw_download_request_handler':
>> drivers/staging/greybus/firmware.h:14:24: warning: '%s' directive output may be truncated writing up to 9 bytes into a region of size between 6 and 15 [-Wformat-truncation=]
    #define FW_NAME_PREFIX "gmp_"
                           ^~~~~~
   drivers/staging/greybus/fw-download.c:188:5: note: in expansion of macro 'FW_NAME_PREFIX'
        FW_NAME_PREFIX "%08x_%08x_%08x_%08x_%s.%s",
        ^~~~~~~~~~~~~~
   drivers/staging/greybus/fw-download.c:188:44: note: format string is defined here
        FW_NAME_PREFIX "%08x_%08x_%08x_%08x_%s.%s",
                                               ^~
   drivers/staging/greybus/fw-download.c:187:3: note: 'snprintf' output between 42 and 60 bytes into a destination of size 56
      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);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +14 drivers/staging/greybus/firmware.h

cca222076738962 Viresh Kumar       2016-04-28  13  
350e3ac2ceb6964 Joel Porquet       2016-07-21 @14  #define FW_NAME_PREFIX	"gmp_"
8a704565ebda960 Greg Kroah-Hartman 2016-07-20  15  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Greg Kroah-Hartman]

On Wed, Oct 22, 2025 at 12:57:57PM +0530, Ayush Singh wrote:
> According to the Greybus Spec published here [0], the Greybus firmware
> download find firmware request should have both tag and format as
> request arguments. However, currently, the linux kernel seems to have
> defined this request incorrectly since format is missing.

Odd, I don't remember that at all, but it was changed here:
	https://github.com/projectara/greybus-spec/commit/773b9e0d6cc84a3c7a9f79ea353a552bd66d9de3

maybe we never actually implemented it?

> 
> The patch adds format to request and am using it as the file extension of
> the firmware.
> 
> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
> According to the Greybus Spec published here [0], the Greybus firmware
> download find firmware request should have both tag and format as
> request arguments. However, currently, the linux kernel seems to have
> defined this request incorrectly since format is missing.
> 
> The patch adds format to request and am using it as the file extension of
> the firmware.
> 
> I came across the bug while working on greybus-for-zephyr [1], to get it
> ready for upstreaming as zephyr module.
> 
> Open Questions
> ***************
> 
> 1. Handle empty format
> 
> Not sure what to do in case format is just NULL. Should the request
> fail? There is no reason to not support firmware without extension. So
> personally, I don't think it should be treated as error.
> 
> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
> [1]: https://github.com/Ayush1325/greybus-for-zephyr

As this is a AP-specific thing, it's whatever you want to do I think.
You can handle NULL there, or anything else, it's up to the firmware and
userspace to coordinate this, right?

> ---
>  drivers/staging/greybus/fw-download.c     | 31 ++++++++++++++++++++++++-------
>  include/linux/greybus/greybus_protocols.h |  2 ++
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> 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) {
> +		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,
> @@ -245,7 +252,17 @@ static int fw_download_find_firmware(struct gb_operation *op)
>  		return -EINVAL;
>  	}
>  
> -	fw_req = find_firmware(fw_download, tag);
> +	format = (const char *)request->format;
> +
> +	/* firmware_format must be null-terminated */
> +	if (strnlen(format, GB_FIRMWARE_FORMAT_MAX_SIZE) ==
> +	    GB_FIRMWARE_FORMAT_MAX_SIZE) {
> +		dev_err(fw_download->parent,
> +			"firmware-format is not null-terminated\n");
> +		return -EINVAL;
> +	}
> +
> +	fw_req = find_firmware(fw_download, tag, format);
>  	if (IS_ERR(fw_req))
>  		return PTR_ERR(fw_req);
>  
> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> index 820134b0105c2caf8cea3ff0985c92d48d3a2d4c..48d91154847dbc7d3c01081eadc69f96dbe41a9f 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -214,10 +214,12 @@ struct gb_apb_request_cport_flags {
>  #define GB_FW_DOWNLOAD_TYPE_RELEASE_FIRMWARE	0x03
>  
>  #define GB_FIRMWARE_TAG_MAX_SIZE		10
> +#define GB_FIRMWARE_FORMAT_MAX_SIZE		10
>  
>  /* firmware download find firmware request/response */
>  struct gb_fw_download_find_firmware_request {
>  	__u8			firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE];
> +	__u8			format[GB_FIRMWARE_FORMAT_MAX_SIZE];

Build issues asside (see the 0-day bot report), I am loath to change a
user api like this at the moment, without some sort of guarantee that
this isn't going to break anything.

But, these days, I think your implementation might be one of the few
"real" greybus users that is still alive.  The old phones that used the
protocol are no longer being sold from what I can tell, and the "greybus
over IP" stuff didn't actually get used anywhere outside of cool demos
that I know of.

So we might be ok?  Or, can you live without any such "format" need?
Have you handled downloading firmware already without this?

thanks,

greg k-h

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

[Ayush Singh]

On 10/23/25 3:34 PM, Greg Kroah-Hartman wrote:

> On Wed, Oct 22, 2025 at 12:57:57PM +0530, Ayush Singh wrote:
>> According to the Greybus Spec published here [0], the Greybus firmware
>> download find firmware request should have both tag and format as
>> request arguments. However, currently, the linux kernel seems to have
>> defined this request incorrectly since format is missing.
> Odd, I don't remember that at all, but it was changed here:
> 	https://github.com/projectara/greybus-spec/commit/773b9e0d6cc84a3c7a9f79ea353a552bd66d9de3
>
> maybe we never actually implemented it?
>
>> The patch adds format to request and am using it as the file extension of
>> the firmware.
>>
>> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>> According to the Greybus Spec published here [0], the Greybus firmware
>> download find firmware request should have both tag and format as
>> request arguments. However, currently, the linux kernel seems to have
>> defined this request incorrectly since format is missing.
>>
>> The patch adds format to request and am using it as the file extension of
>> the firmware.
>>
>> I came across the bug while working on greybus-for-zephyr [1], to get it
>> ready for upstreaming as zephyr module.
>>
>> Open Questions
>> ***************
>>
>> 1. Handle empty format
>>
>> Not sure what to do in case format is just NULL. Should the request
>> fail? There is no reason to not support firmware without extension. So
>> personally, I don't think it should be treated as error.
>>
>> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
>> [1]: https://github.com/Ayush1325/greybus-for-zephyr
> As this is a AP-specific thing, it's whatever you want to do I think.
> You can handle NULL there, or anything else, it's up to the firmware and
> userspace to coordinate this, right?
>
>> ---
>>   drivers/staging/greybus/fw-download.c     | 31 ++++++++++++++++++++++++-------
>>   include/linux/greybus/greybus_protocols.h |  2 ++
>>   2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> 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) {
>> +		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,
>> @@ -245,7 +252,17 @@ static int fw_download_find_firmware(struct gb_operation *op)
>>   		return -EINVAL;
>>   	}
>>   
>> -	fw_req = find_firmware(fw_download, tag);
>> +	format = (const char *)request->format;
>> +
>> +	/* firmware_format must be null-terminated */
>> +	if (strnlen(format, GB_FIRMWARE_FORMAT_MAX_SIZE) ==
>> +	    GB_FIRMWARE_FORMAT_MAX_SIZE) {
>> +		dev_err(fw_download->parent,
>> +			"firmware-format is not null-terminated\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	fw_req = find_firmware(fw_download, tag, format);
>>   	if (IS_ERR(fw_req))
>>   		return PTR_ERR(fw_req);
>>   
>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>> index 820134b0105c2caf8cea3ff0985c92d48d3a2d4c..48d91154847dbc7d3c01081eadc69f96dbe41a9f 100644
>> --- a/include/linux/greybus/greybus_protocols.h
>> +++ b/include/linux/greybus/greybus_protocols.h
>> @@ -214,10 +214,12 @@ struct gb_apb_request_cport_flags {
>>   #define GB_FW_DOWNLOAD_TYPE_RELEASE_FIRMWARE	0x03
>>   
>>   #define GB_FIRMWARE_TAG_MAX_SIZE		10
>> +#define GB_FIRMWARE_FORMAT_MAX_SIZE		10
>>   
>>   /* firmware download find firmware request/response */
>>   struct gb_fw_download_find_firmware_request {
>>   	__u8			firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE];
>> +	__u8			format[GB_FIRMWARE_FORMAT_MAX_SIZE];
> Build issues asside (see the 0-day bot report), I am loath to change a
> user api like this at the moment, without some sort of guarantee that
> this isn't going to break anything.
>
> But, these days, I think your implementation might be one of the few
> "real" greybus users that is still alive.  The old phones that used the
> protocol are no longer being sold from what I can tell, and the "greybus
> over IP" stuff didn't actually get used anywhere outside of cool demos
> that I know of.
>
> So we might be ok?  Or, can you live without any such "format" need?
> Have you handled downloading firmware already without this?
>
> thanks,
>
> greg k-h


Well, I don't really need the format. It's a bit odd that that file 
extension is currently hardcoded, but it's not like file extension needs 
to mean anything. Just found that things were different from spec, hence 
the patch. The fw-download and management implementation I have does not 
care about the extension anyway.

As for downloading firmware. I have an implementation. It can transfer 
30K of firmware. But then it runs out of networking packets. So I have 
not yet done a complete OTA. The implementation is technically there for 
the whole process, but can't promise it will work. I will look into 
where zephyr seems to be leaking the networking packets, but until that 
is fixed, it probably cannot do the complete firmware transfer (704K).

I am planning to make the greybus module an official zephyr module. And 
there are plans to use greybus for i2c based extension boards. 
Hopefully, it will bring more eyes and hands to work greybus.


I can make the patch not break old behavior as suggested by Dan. 
Alternatively, I am also fine without it. However, we should probably 
update the spec in that case.


Best Regards,

Ayush Singh

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

[Alex Elder]

On 10/23/25 5:04 AM, Greg Kroah-Hartman wrote:
> On Wed, Oct 22, 2025 at 12:57:57PM +0530, Ayush Singh wrote:
>> According to the Greybus Spec published here [0], the Greybus firmware
>> download find firmware request should have both tag and format as
>> request arguments. However, currently, the linux kernel seems to have
>> defined this request incorrectly since format is missing.
> 
> Odd, I don't remember that at all, but it was changed here:
> 	https://github.com/projectara/greybus-spec/commit/773b9e0d6cc84a3c7a9f79ea353a552bd66d9de3
> 
> maybe we never actually implemented it?

To be fair, it was the very last commit to the spec before the
program was canceled.  So it would not surprise me too much
if the kernel commit didn't get made.

					-Alex

> 
>>
>> The patch adds format to request and am using it as the file extension of
>> the firmware.
>>
>> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>> According to the Greybus Spec published here [0], the Greybus firmware
>> download find firmware request should have both tag and format as
>> request arguments. However, currently, the linux kernel seems to have
>> defined this request incorrectly since format is missing.
>>
>> The patch adds format to request and am using it as the file extension of
>> the firmware.
>>
>> I came across the bug while working on greybus-for-zephyr [1], to get it
>> ready for upstreaming as zephyr module.
>>
>> Open Questions
>> ***************
>>
>> 1. Handle empty format
>>
>> Not sure what to do in case format is just NULL. Should the request
>> fail? There is no reason to not support firmware without extension. So
>> personally, I don't think it should be treated as error.
>>
>> [0]: https://github.com/projectara/greybus-spec/blob/ac47bc32dce1256489a82ff1f463fb979e9684ee/source/device_class/firmware.rst?plain=1#L152
>> [1]: https://github.com/Ayush1325/greybus-for-zephyr
> 
> As this is a AP-specific thing, it's whatever you want to do I think.
> You can handle NULL there, or anything else, it's up to the firmware and
> userspace to coordinate this, right?
> 
>> ---
>>   drivers/staging/greybus/fw-download.c     | 31 ++++++++++++++++++++++++-------
>>   include/linux/greybus/greybus_protocols.h |  2 ++
>>   2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> 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) {
>> +		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,
>> @@ -245,7 +252,17 @@ static int fw_download_find_firmware(struct gb_operation *op)
>>   		return -EINVAL;
>>   	}
>>   
>> -	fw_req = find_firmware(fw_download, tag);
>> +	format = (const char *)request->format;
>> +
>> +	/* firmware_format must be null-terminated */
>> +	if (strnlen(format, GB_FIRMWARE_FORMAT_MAX_SIZE) ==
>> +	    GB_FIRMWARE_FORMAT_MAX_SIZE) {
>> +		dev_err(fw_download->parent,
>> +			"firmware-format is not null-terminated\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	fw_req = find_firmware(fw_download, tag, format);
>>   	if (IS_ERR(fw_req))
>>   		return PTR_ERR(fw_req);
>>   
>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>> index 820134b0105c2caf8cea3ff0985c92d48d3a2d4c..48d91154847dbc7d3c01081eadc69f96dbe41a9f 100644
>> --- a/include/linux/greybus/greybus_protocols.h
>> +++ b/include/linux/greybus/greybus_protocols.h
>> @@ -214,10 +214,12 @@ struct gb_apb_request_cport_flags {
>>   #define GB_FW_DOWNLOAD_TYPE_RELEASE_FIRMWARE	0x03
>>   
>>   #define GB_FIRMWARE_TAG_MAX_SIZE		10
>> +#define GB_FIRMWARE_FORMAT_MAX_SIZE		10
>>   
>>   /* firmware download find firmware request/response */
>>   struct gb_fw_download_find_firmware_request {
>>   	__u8			firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE];
>> +	__u8			format[GB_FIRMWARE_FORMAT_MAX_SIZE];
> 
> Build issues asside (see the 0-day bot report), I am loath to change a
> user api like this at the moment, without some sort of guarantee that
> this isn't going to break anything.
> 
> But, these days, I think your implementation might be one of the few
> "real" greybus users that is still alive.  The old phones that used the
> protocol are no longer being sold from what I can tell, and the "greybus
> over IP" stuff didn't actually get used anywhere outside of cool demos
> that I know of.
> 
> So we might be ok?  Or, can you live without any such "format" need?
> Have you handled downloading firmware already without this?
> 
> thanks,
> 
> greg k-h