Re: [PATCH 06/17] media: rockchip: rga: move power handling to device_run

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Autosuspend is configured but never enabled, causing `pm_runtime_put_autosuspend()` to act like an immediate `pm_runtime_put()`.

Pre-existing issues:
- [Critical] The `rga` structure is freed by devres on device removal but accessed via V4L2 file ops later.
- [High] Missing hardware state validation in shared interrupt handler allows register access on powered-off device.
- [High] Missing IRQ synchronization during runtime suspend allows hardware power-down while IRQ handler is mid-flight.
- [High] Asynchronous pm_runtime_put() is dangerously canceled by an immediate pm_runtime_disable() during module removal, leaking power.
- [High] Missing return after WARN_ON(!ctx) leads to immediate NULL pointer dereference.
- [High] `dma_sync_single_for_device()` is illegally called on coherent memory.
--

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

media: rockchip: rga: move power handling to device_run

Move the power handling to the device_run function in preparation for
enabling multiple cores. Switch to autosuspend to avoid powering down
the core when the next frame is scheduled in the next 50ms.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 8c03422d669cf..0eff558d7f133 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c

[ ... ]

> @@ -81,6 +89,8 @@ static irqreturn_t rga_isr(int irq, void *prv)
>  		v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
>  		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
>  		v4l2_m2m_job_finish(rga->m2m_dev, ctx->fh.m2m_ctx);
> +
> +		pm_runtime_put_autosuspend(rga->dev);

[Severity: Medium]
Is the idle timer being reset properly here?

It appears we are missing a call to pm_runtime_mark_last_busy() before the
put operation. Without it, the idle timer might not be reset, defeating the
intended autosuspend optimization.

[Severity: High]
This is a pre-existing issue, but I noticed the shared interrupt handler
might execute hardware operations blindly. Does rga_isr() need to verify
the device is powered on before touching hardware?

Since the device can use shared IRQs, rga_isr() can be invoked
asynchronously if another device on the same line triggers an interrupt,
even when the RGA device is suspended. Should we use
pm_runtime_get_if_active() to ensure we don't read from powered-down
registers?

[Severity: High]
This is also a pre-existing issue, but looking at rga_isr(), if ctx is NULL,
does the code fall through to a NULL pointer dereference?

rga_isr() checks for a NULL context:

WARN_ON(!ctx);
core->curr = NULL;
src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);

Should there be a return statement after the WARN_ON(!ctx) to prevent
dereferencing ctx in the subsequent calls?

>  	}
>  
>  	return IRQ_HANDLED;
> @@ -797,6 +807,7 @@ static int rga_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "Unable to parse OF data\n");
>  
> +	pm_runtime_set_autosuspend_delay(rga->dev, 50);
>  	pm_runtime_enable(rga->dev);

[Severity: Medium]
Are we missing a call to pm_runtime_use_autosuspend() here?

Without enabling the autosuspend flag, the PM core might ignore the 50ms
delay, causing pm_runtime_put_autosuspend() to fall back to an immediate
suspend behavior.

[ ... ]

I also noticed a few other pre-existing issues in the driver while reviewing
this patch:

[Severity: High]
This isn't a bug introduced by this patch, but does the driver need to
synchronize IRQs during runtime suspend?

In rga_runtime_suspend(), the driver disables clocks using
clk_bulk_disable_unprepare(). Because the driver uses shared IRQs, could
the hardware be powered down while an executing rga_isr() is mid-flight on
another CPU? Should synchronize_irq() be called before shutting down the
hardware?

[Severity: High]
This is a pre-existing issue, but does device removal safely cancel pending
suspend operations?

During module removal, rga_core_remove() calls pm_runtime_disable().
Because the driver uses pm_runtime_put_autosuspend(), an asynchronous
suspend might be pending. Could this immediate disable cancel the pending
work without executing it, leaving the hardware active and leaking power?
Would using pm_runtime_put_sync() ensure the suspend completes before
removal?

[Severity: Critical]
This is a pre-existing issue, but is there a potential use-after-free
related to the rga structure lifecycle?

The rga structure is allocated using devm_kzalloc() during probe, meaning
it is freed automatically on device removal. However, if a user process
holds the video device open, the V4L2 file descriptor remains valid. When
the process later closes the file, rga_release() is called and accesses the
freed memory, for example by acquiring the mutex:

mutex_lock(&rga->mutex);

Should the allocation be tied to the v4l2_device or video_device lifecycle
instead of devres?

[Severity: High]
This is a pre-existing issue, but I noticed a potential DMA API violation in
rga_hw_start().

The command buffer is allocated as write-combined coherent memory using
dma_alloc_attrs() with DMA_ATTR_WRITE_COMBINE. Later, the driver attempts
to sync this buffer to the device:

dma_sync_single_for_device(..., ctx->cmdbuf_phy, ...);

Since the sync functions are intended for streaming DMA mappings and not
coherent allocations, could this corrupt internal state or trigger DMA API
debug warnings?

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