Re: [PATCH] staging: greybus: fw-download: Fix find firmware req
From: Alex Elder
Date: Fri Oct 24 2025 - 12:35:16 EST
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@xxxxxxxxxxxxxxx>
---
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