Re: [PATCH 15/17] media: rockchip: rga: schedule jobs to multiple cores

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition in device_run() error path and unhandled early return permanently deadlock the mem2mem queue.
- [Medium] Data race on core->curr pointer when clearing job assignments.

Pre-existing issues:
- [High] Out-of-bounds array write on unbind/rebind of components because rga->num_cores is never reset.
- [High] Missing pm_runtime_get_if_active() before hardware access in shared IRQ handler.
- [High] Missing synchronize_irq() in runtime suspend callback for shared IRQ.
--

commit f03021dff03be4350cbe7b0ab4ce4a9a54a7602f
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: schedule jobs to multiple cores

This commit modifies the Rockchip RGA driver to schedule jobs across
multiple cores to better utilize hardware. A dedicated spinlock is added
to protect the selection of the next free core from race conditions.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 9cebb461b3fd2..f00b7f99f2521 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -38,15 +38,31 @@ static void device_run(void *prv)
>  {
>  	struct rga_ctx *ctx = prv;
>  	struct rockchip_rga *rga = ctx->rga;
> -	struct rga_core *core = rga->cores[0];
> +	struct rga_core *core = NULL;
>  	struct vb2_v4l2_buffer *src, *dst;
>  	unsigned long flags;
>  	int ret;
> +	unsigned int i;
> +
> +	spin_lock_irqsave(&rga->cores_lock, flags);
> +	for (i = 0; i < rga->num_cores; i++) {
> +		if (!rga->cores[i]->curr) {
> +			core = rga->cores[i];
> +			core->curr = ctx;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&rga->cores_lock, flags);
> +
> +	WARN_ONCE(!core, "No free core although max parallel jobs matches the core count!\n");
> +	if (!core)
> +		return;
>  
>  	ret = pm_runtime_resume_and_get(core->dev);
>  	if (ret < 0) {
>  		v4l2_m2m_buf_done_and_job_finish(rga->m2m_dev, ctx->fh.m2m_ctx,
>  						 VB2_BUF_STATE_ERROR);
> +		core->curr = NULL;

[Severity: High]
Could this error path introduce a race condition that permanently deadlocks the
mem2mem queue?

v4l2_m2m_buf_done_and_job_finish() is called before core->curr is cleared,
which asynchronously schedules the next job via a workqueue.

If the worker executes device_run() on another CPU before the current CPU marks
the core as free, the worker will see the core as busy. If no other core is
free, device_run() will trigger the WARN_ONCE and return early without
notifying the v4l2 mem2mem framework:

    if (!core)
        return;

Because the new job is already marked as TRANS_RUNNING, returning early without
signaling job completion permanently hangs the job queue, and can cause a
D-state deadlock if userspace attempts to close the device file descriptor.

[Severity: Medium]
Additionally, could there be a data race when clearing core->curr here?

While cores_lock is acquired when scanning and assigning a core earlier in
device_run(), it is not held when releasing the core in this error path.
Concurrent unprotected reads and writes to core->curr across CPUs could lead to
undefined behavior.

>  		return;
>  	}

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but does modifying core->curr in
rga_isr() without holding cores_lock introduce a data race?

Since cores_lock was introduced to protect against concurrent selections,
shouldn't it also be held when releasing the core in the interrupt handler?

drivers/media/platform/rockchip/rga/rga.c:rga_isr() {
    ...
    core->curr = NULL;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the unbind/rebind path
cause an out-of-bounds array write in rga->cores[]?

During component bind, rga_core_bind() appends to the rga->cores array:

drivers/media/platform/rockchip/rga/rga.c:rga_core_bind() {
    ...
    rga->cores[rga->num_cores++] = core;
    ...
}

However, the master unbind callback and the component system do not reset
rga->num_cores to 0:

drivers/media/platform/rockchip/rga/rga.c:rga_unbind() {
    ...
    component_unbind_all(dev, NULL);
}

The struct rockchip_rga structure is not freed when a component is unbound. If
a component is unbound and subsequently re-bound, rga_core_bind() will continue
incrementing rga->num_cores past the initially allocated length, corrupting
memory.

[Severity: High]
This isn't a bug introduced by this patch, but is the driver missing a
pm_runtime_get_if_active() check before accessing hardware in the shared IRQ
handler?

The driver registers rga_isr() with IRQF_SHARED if there is no internal IOMMU.
When an interrupt is triggered by another device on the shared line, rga_isr()
blindly calls handle_irq(), which reads hardware registers:

drivers/media/platform/rockchip/rga/rga.c:rga_isr() {
    ...
    if (rga->hw->handle_irq(core)) {
    ...
}

If the RGA core is runtime-suspended and its clocks are disabled, accessing its
registers will cause an external abort / SError.

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing
synchronize_irq() in the runtime suspend callback for shared IRQs?

In rga_runtime_suspend(), the driver disables clocks unconditionally:

drivers/media/platform/rockchip/rga/rga.c:rga_runtime_suspend() {
    ...
    clk_bulk_disable_unprepare(core->num_clks, core->clks);
    ...
}

Because the driver can use a shared IRQ, an interrupt handler could be
mid-flight on another CPU when the clocks are disabled, causing an external
abort when the handler accesses the now-unclocked hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-spu-rga3multicore-v1-0-3ec2b15675f7@pengutronix.de?part=15