Re: [PATCH v6 12/18] media: iris: add minimal GET_PROPERTY implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent V4L2 ioctls can wipe out unconsumed completion tokens via `reinit_completion()`, causing threads to timeout or wake up on the wrong firmware response.
--

--- Patch [12]: [PATCH v6 12/18] media: iris: add minimal GET_PROPERTY implementation ---
commit ef2e1b5777ca6613617c7857a2dbbf08d615a9fa
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: add minimal GET_PROPERTY implementation

AR50Lt with the Gen1 firmware requires host to read
HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS property, otherwise it doesn't
update internal data and fails the HFI_CMD_SESSION_LOAD_RESOURCES
command. Implement minimal support for querying the properties from the
firmware.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 7674b47ad6c49..99e82e5510abe 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -1117,10 +1117,31 @@ static int iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 p
>  	return 0;
>  }
>  
> +static int iris_hfi_gen1_session_get_property(struct iris_inst *inst, u32 packet_type)
> +{
> +	struct hfi_session_get_property_pkt pkt;
> +	int ret;
> +
> +	pkt.shdr.hdr.size = sizeof(pkt);
> +	pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_GET_PROPERTY;
> +	pkt.shdr.session_id = inst->session_id;
> +	pkt.num_properties = 1;
> +	pkt.data = packet_type;
> +
> +	reinit_completion(&inst->completion);

[Severity: High]
This is a pre-existing issue, but does calling reinit_completion() here
risk wiping out an unconsumed completion token from a concurrent V4L2
ioctl?

When thread A sends a command and enters iris_wait_for_session_response(),
it drops the instance lock during the wait:

drivers/media/platform/qcom/iris/iris_utils.c:iris_wait_for_session_response() {
    ...
	mutex_unlock(&inst->lock);
	ret = wait_for_completion_timeout(done, msecs_to_jiffies(HW_RESPONSE_TIMEOUT_VALUE));
	mutex_lock(&inst->lock);
    ...
}

If thread B handles a concurrent ioctl and sends its own command, it will
acquire the freed lock and call reinit_completion() on the same shared
inst->completion token.

If the firmware response for thread A's command arrives just before thread
B calls reinit_completion(), thread B will reset the completion state and
leave thread A waiting, eventually causing it to spuriously time out and
transition the instance state to IRIS_INST_ERROR.

Could this lead to incorrectly pairing firmware responses with commands or
a permanent denial of service for the V4L2 instance?

> +
> +	ret = iris_hfi_queue_cmd_write(inst->core, &pkt, pkt.shdr.hdr.size);
> +	if (ret)
> +		return ret;
> +
> +	return iris_wait_for_session_response(inst, false);
> +}

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