Re: [PATCH v6 01/18] media: iris: Add Gen2 firmware autodetect and fallback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unprotected concurrent modification of shared core capability structures in `iris_open()`.
- [High] Lockless reads of `core->iris_firmware_desc` and `core->iris_firmware_data` during ioctls can race with their reassignment.
- [Low] Missing `firmware_request_nowarn()` causes misleading dmesg errors during normal fallback.
--

--- Patch [1]: [PATCH v6 01/18] media: iris: Add Gen2 firmware autodetect and fallback ---
commit 877740428251d1096cd14181b1ab991b0027ac73
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Add Gen2 firmware autodetect and fallback

This commit updates the firmware loading logic to prefer Gen2 firmware,
detect the firmware generation from the firmware image itself, and safely
fallback to Gen1 if necessary.

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index 1a476146d7580..965384fdd339c 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
[ ... ]
> +static const struct firmware *iris_detect_firmware(struct iris_core *core,
> +						   const char **fw_name)
> +{
> +	const struct firmware *firmware;
> +	bool has_both_gens;
> +	int ret;
> +
> +	*fw_name = NULL;
> +	if (core->iris_platform_data->firmware_desc_gen2)
> +		core->iris_firmware_desc = core->iris_platform_data->firmware_desc_gen2;

[Severity: High]
Since this patch changes these pointers from being statically assigned in
the probe function to being dynamically mutated during iris_core_init(), can
this race with lockless reads during ioctls?

For example, if a STREAMON ioctl executes concurrently holding only
inst->lock, it might read the pointer locklessly in
iris_fill_internal_buf_info():

drivers/media/platform/qcom/iris/iris_buffer.c:iris_fill_internal_buf_info()
{
        ...
        buffers->size = inst->core->iris_firmware_desc->get_vpu_buffer_size(
                inst, buffer_type);
        ...
}

If sys_error_handler executes iris_core_init() concurrently holding
core->lock, could this result in dereferencing an invalid or mismatched
function pointer?

> +	else if (core->iris_platform_data->firmware_desc_gen1)
> +		core->iris_firmware_desc = core->iris_platform_data->firmware_desc_gen1;
> +	else
> +		return ERR_PTR(-EINVAL);
> +
> +	has_both_gens = core->iris_platform_data->firmware_desc_gen2 &&
> +		core->iris_platform_data->firmware_desc_gen1;
> +
> +	ret = of_property_read_string_index(dev_of_node(core->dev), "firmware-name", 0, fw_name);
> +	if (ret) {
> +		*fw_name = core->iris_firmware_desc->fwname;
> +		ret = request_firmware(&firmware, *fw_name, core->dev);

[Severity: Low]
Since the Gen2 firmware is optional and the driver intentionally falls back
to Gen1, will using request_firmware() here instead of
firmware_request_nowarn() print a misleading "Direct firmware load failed"
error to the kernel log on Gen1-only platforms?

> +		if (ret && has_both_gens) {
> +			core->iris_firmware_desc = core->iris_platform_data->firmware_desc_gen1;
> +			*fw_name = core->iris_firmware_desc->fwname;
> +			ret = request_firmware(&firmware, *fw_name, core->dev);
> +		}
> +
> +		return ret ? ERR_PTR(ret) : firmware;
> +	}
[ ... ]
> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index 14d63dc76c9ba..372408b894c19 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
[ ... ]
> @@ -196,6 +197,8 @@ int iris_open(struct file *filp)
>  		goto fail_m2m_release;
>  	}
>  
> +	iris_session_init_caps(core);

[Severity: High]
Can concurrent opens race to write to the shared device-wide array
core->inst_fw_caps_dec?

This patch moves iris_session_init_caps() from the probe function to
iris_open(). Since iris_open() does not hold core->lock when calling this
function, multiple concurrent opens will unconditionally write to the
same shared array in iris_session_init_caps():

drivers/media/platform/qcom/iris/iris_ctrls.c:iris_session_init_caps()
{
        ...
        core->inst_fw_caps_dec[cap_id].step_or_mask = caps[i].step_or_mask;
        ...
}

The racing values are then copied into the instance in iris_vdec_inst_init(),
which could result in copying torn reads or corrupted bounds:

drivers/media/platform/qcom/iris/iris_vdec.c:iris_vdec_inst_init()
{
        ...
        memcpy(&inst->fw_caps[0], &core->inst_fw_caps_dec[0],
               INST_FW_CAP_MAX * sizeof(struct platform_inst_fw_cap));
        ...
}

> +
>  	if (inst->domain == DECODER)
>  		ret = iris_vdec_inst_init(inst);
>  	else if (inst->domain == ENCODER)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-iris-ar50lt-v6-0-374f0a46c23b@oss.qualcomm.com?part=1