Re: [PATCH v7 07/28] media: rockchip: rga: use clk_bulk api

[sashiko-bot@kernel.org]

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

New issues:
- [High] The transition to `devm_clk_bulk_get_all()` removes the strict requirement for device clocks, creating a silent failure that leads to a system crash if clocks are missing in the device tree.

Pre-existing issues:
- [Critical] The device removal process incorrectly orders the teardown steps and destroys memory while the V4L2 device may still be open in userspace, leading to a Use-After-Free.
- [Critical] The interrupt handler accesses hardware registers without ensuring the device is in an active power state.
- [High] Hardware resets are asserted and deasserted while the core, AXI, and AHB clocks are disabled.
- [High] The driver calls a streaming DMA sync function on a coherent DMA allocation and uses an out-of-bounds size.
--

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

media: rockchip: rga: use clk_bulk api
    
Use the clk_bulk API to avoid code duplication for each of the three
clocks.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index fea63b94c5f3d..4e710a050cb7c 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
[ ... ]
> @@ -771,23 +733,12 @@ static int rga_parse_dt(struct rockchip_rga *rga)
>  	udelay(1);
>  	reset_control_deassert(ahb_rst);

[Severity: High]
This is a pre-existing issue, but are these hardware resets effective when
the clocks are disabled?

The core, axi, and ahb reset lines are pulsed here before the corresponding
clocks are enabled. For synchronous hardware blocks like AXI/AHB and IP
cores, the clock usually must be active for the reset signal to propagate
correctly through the internal state machine.

Could pulsing resets while clocks are gated leave the hardware in an
undefined state and lead to DMA corruption or hardware hangs?

> -	rga->sclk = devm_clk_get(rga->dev, "sclk");
> -	if (IS_ERR(rga->sclk)) {
> -		dev_err(rga->dev, "failed to get sclk clock\n");
> -		return PTR_ERR(rga->sclk);
> -	}
[ ... ]
> +	ret = devm_clk_bulk_get_all(rga->dev, &rga->clks);
> +	if (ret < 0) {
> +		dev_err(rga->dev, "failed to get clocks\n");
> +		return ret;
>  	}
> +	rga->num_clks = ret;

[Severity: High]
Does this missing clock check lead to a synchronous external abort?

devm_clk_bulk_get_all() returns the number of clocks successfully obtained,
or 0 if none are specified in the device tree.

By only checking ret < 0, a return value of 0 is treated as a success,
setting num_clks = 0. The driver will then successfully call
clk_bulk_prepare_enable(0, ...) which does nothing, and immediately attempt
to access unclocked hardware registers.

On ARM platforms, accessing unclocked hardware domains can trigger a
synchronous external abort (bus fault).

Should this code assert that ret > 0 to ensure the mandatory clocks were
acquired?

[ ... ]

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

[Severity: Critical]
This isn't a bug introduced by this patch, but does the teardown order in
rga_remove() cause a use-after-free?

The driver unregisters the video device (video_unregister_device) and
completely tears down the context (v4l2_m2m_release), relying on devres to
free the rockchip_rga structure upon returning.

However, video_unregister_device does not forcefully close existing
userspace file descriptors. If a userspace process holds an open FD and
later closes it, rga_release() executes and dereferences the already freed
rga structure.

[Severity: Critical]
This is a pre-existing issue, but can rga_isr() access unclocked hardware
when the device is runtime-suspended?

The interrupt handler rga_isr() unconditionally reads the hardware register
RGA_INT. A subsequent patch in this series changes the driver to request a
shared interrupt (IRQF_SHARED).

This means the ISR will be invoked whenever the IOMMU (or another sharing
device) triggers an interrupt, which can occur while the RGA device is
runtime-suspended and its clocks are disabled. Reading from unclocked RGA
registers causes a synchronous external abort.

Should the handler use pm_runtime_get_if_active() before accessing hardware?

[Severity: High]
This isn't a bug introduced by this patch, but does rga_cmd_set() perform a
streaming DMA sync on a coherent allocation?

cmdbuf_virt is allocated using dma_alloc_attrs() with DMA_ATTR_WRITE_COMBINE,
returning a coherent DMA allocation. However, in rga_cmd_set(), the driver
calls dma_sync_single_for_device() on this buffer.

Streaming DMA sync functions must not be used on coherent allocations. Doing
so violates the DMA API and can trigger system faults on strict
architectures.

Furthermore, it synchronizes PAGE_SIZE bytes, but the buffer size is only
RGA_CMDBUF_SIZE (0x80 bytes). Could this create an out-of-bounds DMA
operation that corrupts adjacent memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-spu-rga3-v7-0-3f33e8c7145f@pengutronix.de?part=7