Re: [PATCH v10 10/14] media: iris: Add support to select core for dual core platforms

[sashiko-bot@kernel.org]

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

New issues:
- [High] The MBPS core load limit is bypassed during stream setup, and core selection is improperly locked to the MBPF check, contradicting the patch description.
- [High] Cross-instance lockless access to format and frame rate properties during core load calculation.

Pre-existing issues:
- [High] Lockless traversal of the `core->instances` list in `iris_check_session_supported` can cause list corruption or read-after-free bugs.
- [High] `iris_add_session` silently fails to add sessions exceeding `max_session_count`, leaving the session in an invalid state and violating the API.
- [High] Cross-instance lockless access to 64-bit power requirements (`min_freq` and `max_input_data_size`).
--

--- Patch [10]: [PATCH v10 10/14] media: iris: Add support to select core for dual core platforms ---
commit c60a40ca7a5f98a3b80a85f1100a2fa1ecc2f020
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add support to select core for dual core platforms
    
On platforms with two video codec cores, select the hardware core for
a new session based on the current Macroblocks Per Frame (MBPF) and
Macroblocks Per Second (MBPS) load on each core. The selected core is
communicated to the firmware via the HFI_PROP_CORE_ID property at
stream-on time.
Since both cores share the same clock source, the required clock
frequency is the maximum of the aggregated frequencies across both
cores.
The total session count limit is scaled by the number of cores, since
each core independently supports sessions up to its own limit.

> diff --git a/drivers/media/platform/qcom/iris/iris_power.c b/drivers/media/platform/qcom/iris/iris_power.c
> index 91aa21d4070eb..a875647b31628 100644
> --- a/drivers/media/platform/qcom/iris/iris_power.c
> +++ b/drivers/media/platform/qcom/iris/iris_power.c
> @@ -75,14 +75,12 @@ static int iris_vote_interconnects(struct iris_inst *inst)
>  	return iris_set_interconnects(inst);
>  }
>  
> -static int iris_set_clocks(struct iris_inst *inst)
> +static u64 iris_get_required_freq(struct iris_inst *inst)
>  {
>  	struct iris_core *core = inst->core;
>  	struct iris_inst *instance;
>  	u64 freq = 0;
> -	int ret;
>  
> -	mutex_lock(&core->lock);
>  	list_for_each_entry(instance, &core->instances, list) {
>  		if (!instance->max_input_data_size)
>  			continue;
> @@ -90,6 +88,22 @@ static int iris_set_clocks(struct iris_inst *inst)
>  		freq += instance->power.min_freq;
>  	}

[Severity: High]
This is a pre-existing issue, but does reading a 64-bit value concurrently
across instances risk returning torn reads?

Functions like iris_get_required_freq() and the newly introduced
iris_vpu36_get_required_freq() iterate over core->instances with core->lock
held to aggregate instance->power.min_freq.

However, inst->power.min_freq and inst->max_input_data_size appear to be
written concurrently by iris_scale_clocks() which only holds inst->lock.

Could this data race lead to passing an invalid aggregated frequency to the
clock framework?

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_utils.c b/drivers/media/platform/qcom/iris/iris_utils.c
> index ba5c8dc1280c2..4608e3f288326 100644
> --- a/drivers/media/platform/qcom/iris/iris_utils.c
> +++ b/drivers/media/platform/qcom/iris/iris_utils.c
> @@ -101,40 +109,52 @@ struct iris_inst *iris_get_instance(struct iris_core *core, u32 session_id)
>  	return NULL;
>  }
>  
> -int iris_check_core_mbpf(struct iris_inst *inst)
> +static int iris_check_core_load(struct iris_inst *inst, bool mbpf)
>  {
> -	struct iris_core *core = inst->core;
> +	const struct iris_platform_data *platform_data = inst->core->iris_platform_data;
> +	u32 max_load = mbpf ? platform_data->max_core_mbpf : platform_data->max_core_mbps;
>  	struct iris_inst *instance;
> -	u32 total_mbpf = 0;
> +	u32 total_load = 0;
>  
> -	mutex_lock(&core->lock);
> -	list_for_each_entry(instance, &core->instances, list)
> -		total_mbpf += iris_get_mbpf(instance);
> -	mutex_unlock(&core->lock);
> +	list_for_each_entry(instance, &inst->core->instances, list)
> +		total_load += mbpf ? iris_get_mbpf(instance) : iris_get_mbps(instance);

[Severity: High]
Does iterating over all sessions and calling iris_get_mbpf() or
iris_get_mbps() read non-atomic properties across instances safely?

If the aggregating thread reads fmt_src, crop, and frame rate properties of
other individual sessions without holding their respective inst->lock, could
this result in torn reads if they are concurrently updated during
VIDIOC_S_FMT or VIDIOC_S_PARM?

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index a2ea2d67f60d0..dbb89396e6514 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -176,6 +176,10 @@ int iris_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
>  	if (ret)
>  		goto error;
>  
> +	ret = iris_set_core_id(inst);
> +	if (ret)
> +		goto error;
> +

[Severity: High]
This is a pre-existing issue, but I noticed that iris_check_session_supported()
is called just before this in iris_vb2_start_streaming() while holding only
inst->lock. It iterates over the global core->instances list:

drivers/media/platform/qcom/iris/iris_vb2.c:iris_check_session_supported() {
    ...
    list_for_each_entry(instance, &core->instances, list) {
        if (instance == inst)
            found = true;
    }
    ...
}

Since core->instances is modified by iris_add_session() and
iris_remove_session() under core->lock, can traversing it concurrently
without core->lock lead to list corruption or read-after-free bugs?

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index 26df872fb4fa1..52855f6c7deaa 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> @@ -41,16 +41,20 @@ static void iris_v4l2_fh_deinit(struct iris_inst *inst, struct file *filp)
>  
>  static void iris_add_session(struct iris_inst *inst)
>  {
> +	const struct iris_platform_data *plat = inst->core->iris_platform_data;
> +	u32 max_session_count = plat->max_session_count;
>  	struct iris_core *core = inst->core;
>  	struct iris_inst *iter;
>  	u32 count = 0;
>  
> +	max_session_count *= max(plat->num_cores, 1);
> +
>  	mutex_lock(&core->lock);
>  
>  	list_for_each_entry(iter, &core->instances, list)
>  		count++;
>  
> -	if (count < core->iris_platform_data->max_session_count)
> +	if (count < max_session_count)
>  		list_add_tail(&inst->list, &core->instances);
>  
>  	mutex_unlock(&core->lock);

[Severity: High]
This is a pre-existing issue, but if count is greater than or equal to
max_session_count, this function silently returns without adding the instance
to the list and without propagating an error code like -EBUSY to iris_open().

Does this allow the open() syscall to incorrectly succeed, returning a valid
file descriptor to userspace while leaving the session off the internal
tracking list?

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 5212f7d4e606c..ec35ad4f86102 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -333,6 +333,70 @@ static void iris_vpu36_program_bootup_registers(struct iris_core *core)
>  	writel(0x0, core->reg_base + CPU_CS_SCIACMDARG3);
>  }
>  
> +static int iris_vpu36_check_core_load(struct iris_inst *inst, bool mbpf)
> +{
> +	const struct iris_platform_data *platform_data = inst->core->iris_platform_data;
> +	u32 max_load = mbpf ? platform_data->max_core_mbpf : platform_data->max_core_mbps;
> +	u32 max_session_cnt = platform_data->max_session_count;
> +	u32 core0_session_cnt = 0, core1_session_cnt = 0;
> +	u32 core0_load = 0, core1_load = 0;
> +	bool select_core0, select_core1;
> +	struct iris_inst *instance;
> +	u32 load, new_load;
> +
> +	list_for_each_entry(instance, &inst->core->instances, list) {
> +		load = mbpf ? iris_get_mbpf(instance) : iris_get_mbps(instance);
> +
> +		if (instance->core_id == IRIS_VCODEC0) {
> +			core0_load += load;
> +			core0_session_cnt++;
> +		} else if (instance->core_id == IRIS_VCODEC1) {
> +			core1_load += load;
> +			core1_session_cnt++;
> +		}
> +	}
> +
> +	if (inst->core_id == IRIS_VCODEC0)
> +		return core0_load <= max_load ? 0 : -ENOMEM;
> +	else if (inst->core_id == IRIS_VCODEC1)
> +		return core1_load <= max_load ? 0 : -ENOMEM;
> +
> +	new_load = mbpf ? iris_get_mbpf(inst) : iris_get_mbps(inst);
> +
> +	select_core0 = core0_load + new_load <= max_load && core0_session_cnt < max_session_cnt;
> +	select_core1 = core1_load + new_load <= max_load && core1_session_cnt < max_session_cnt;
> +
> +	if (select_core0 && select_core1)
> +		inst->core_id = (core0_load <= core1_load) ? IRIS_VCODEC0 : IRIS_VCODEC1;

[Severity: High]
If iris_check_session_supported() exclusively invokes iris_check_core_mbpf()
to validate the session, and this new function permanently sets inst->core_id
based on the MBPF load alone, does it omit the MBPS check entirely?

If MBPS is checked sequentially later, wouldn't valid sessions be falsely
rejected because the selection algorithm only considers MBPF here?

> +	else if (select_core0)
> +		inst->core_id = IRIS_VCODEC0;
> +	else if (select_core1)
> +		inst->core_id = IRIS_VCODEC1;
> +	else
> +		return -ENOMEM;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-glymur-v10-0-de451559e88b@oss.qualcomm.com?part=10