Re: [PATCH v4 3/3] media: iris: Add Gen2 firmware autodetect and fallback

[Dmitry Baryshkov]

On Sun, May 31, 2026 at 12:43:18AM +0100, bod@xxxxxxxxxx wrote:
> On 2026-04-29 17:39 +0530, Dikshita Agarwal wrote:
> > Some Iris platforms support both Gen1 and Gen2 HFI firmware images.
> > Update the firmware loading logic to handle this generically by
> > preferring Gen2 when available, while safely falling back to Gen1
> > when required.
> > 
> > The firmware loading logic is updated with the following priority:
> > 1. Device Tree (`firmware-name`): If specified, load unconditionally.
> > 2. Gen2 default : If no DT override exists, select the Gen2 firmware
> >    descriptor when present and attempt to load the corresponding
> >    firmware image.
> > 3. Gen1 Fallback: If loading the Gen2 firmware fails and a Gen1
> >    descriptor is available, retry with the Gen1 firmware image.
> > 
> > When a platform provides both Gen1 and Gen2 firmware descriptors and the
> > firmware is loaded via a DT override, the driver detects the
> > firmware generation at runtime before authentication by inspecting
> > the firmware data. The firmware is classified as Gen2 if the
> > QC_IMAGE_VERSION_STRING starts with "vfw" or matches the
> > "video-firmware.N.M" format with N >= 2.
> > 
> > If a Gen1 firmware image is detected in this case, the driver switches
> > to the Gen1 firmware descriptor and associated platform data so that
> > the correct HFI implementation is used.
> > 
> > This change makes firmware generation detection platform‑agnostic,
> > preserves DT overrides, prefers newer Gen2 firmware when available,
> > and maintains compatibility with platforms that only support Gen1.
> > 
> > Co-developed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Dikshita Agarwal <dikshita.agarwal@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/media/platform/qcom/iris/iris_firmware.c   | 105 +++++++++++++++++----
> >  .../platform/qcom/iris/iris_platform_common.h      |   6 +-
> >  .../media/platform/qcom/iris/iris_platform_vpu2.c  |  11 ++-
> >  .../media/platform/qcom/iris/iris_platform_vpu3x.c |   8 +-
> >  drivers/media/platform/qcom/iris/iris_probe.c      |   4 -
> >  drivers/media/platform/qcom/iris/iris_vidc.c       |   3 +
> >  6 files changed, 105 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> > index 1a476146d7580849d7b68c7c15dd7f82f89a680b..64a2170bf538a6d291b3d909f5563408a3a75e50 100644
> > --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> > @@ -16,20 +16,95 @@
> >  
> >  #define MAX_FIRMWARE_NAME_SIZE	128
> >  
> > -static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> > +/* Detect Gen2 firmware by scanning the blob for:
> > + *   QC_IMAGE_VERSION_STRING=<version>
> > + * and then checking:
> > + *   - version starts with "vfw", OR
> > + *   - version matches "video-firmware.N.M" with N >= 2
> > + */
> > +
> > +static bool iris_detect_gen2_from_fwdata(const u8 *data, size_t size)
> > +{
> > +	const char *marker = "QC_IMAGE_VERSION_STRING=";
> > +	const size_t mlen = strlen(marker);
> > +	int major = 0, minor = 0;
> > +	char version_buf[64];
> > +	size_t max;
> > +
> > +	max = (size > mlen) ? size - mlen : 0;
> > +	for (size_t i = 0; i < max; i++) {
> 
> Iterating character by character through the string is boilerplate you
> don't need.
> 
> size = 27;
> mlen = 24;
> max = 3;
> 
> for (i = 0; i < max = 3; i++)
> 
> i = 2;
> 
> =>
> 
> 
> > +		if (!memcmp(data + i, marker, mlen)) {
> > +			const char *found = (const char *)(data + i + mlen);
> > +
> > +			strscpy(version_buf, found, sizeof(version_buf));
> 
> This strscpy can exceed the extent of the data buffer here because the
> bounds check is the sizeof(version_buf) and all you've bounds checked is
> the start of the string not its extent by this point.
> 
> found = data + 2 + mlen - strscpy copies from data+2 to data+64 which is an
> overflow.

This can be made more robust, but I think it would be enough to stop
scanning sizeof(version_buf) + strlen(marker) bytes before the end.

> 
> > +			if (!strncmp(version_buf, "vfw", 3))
> > +				return true;
> > +			if (sscanf(version_buf, "video-firmware.%d.%d", &major, &minor) == 2 &&
> > +			    major >= 2)
> > +				return true;
> > +			break;
> 
> Right so it is valid to find maker only once with this break

Yes, there is just one version per firmware file.

> 
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> 
> const char * const marker = "QC_IMAGE_VERSION_STRING=";
> const char * const terminator = data + size;
> size_t marker_len = strlen(marker);
> size_t marker_off;
> char *fat_buf;
> char *version;
> bool ret = false;
> 
> version = strnstr(data, marker, size);

strnstr is defined to search for the substring in another string. There
is no promise that it will work if data contains \0 chars (which would
terminate the string). And the memmem is not defined for the Linux
kernel.

> if (version) {
> 	marker_off = version - data;
> 	version += marker_len;
> 	size -= marker_off + marker_len;
> 
> 	if (version < terminator-3) {
> 		/* This is safe because we bounds check */
> 		if (strncmp("vfw", version, size) == 0)
> 			return true;
> 	}
> 
> 	/* To do your sscanf() you need to create a zeorised buffer */
> 	fat_buf = kzalloc(size+1, GFP_KERNEL);
> 	if (!fat_buf)
> 		return false;
> 
> 	memcpy(fat_buf, version, size);

Creating a copy of about the half of the image is definitely an
overkill.

> 
> 	/* sscanf is now guaranteed to terminate on NULL */
> 	if (sscanf(fat_buf, "video-firmware.%d.%d", &major, &minor) == 2) {

I think we can replace this with string comparisons too. No need to
sscanf it.

WDYT?

> 		if (major >= 2) {
> 			ret = true;
> 			goto free_mem;
> 		}
> 	}
> }
> 
> free_mem:
> 	if (fat_buf)
> 		kfree(fat_buf);
> 	return ret;
> 

-- 
With best wishes
Dmitry