Re: [PATCH v6 21/28] media: rockchip: rga: share the interrupt when an external iommu is used

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Accessing hardware registers without ensuring power/clock state in shared interrupt handler.
- [High] Unconditional `IRQ_HANDLED` return from shared interrupt handler defeats spurious IRQ detection.
- [High] Data race on the `RGA_INT` register due to lockless ISR execution.
- [High] Early IRQ registration introduces a NULL pointer dereference window.
--

commit 9f93e8eca57c7b5671e83f97e16709aa91786176
Author: Michael Olbrich <m.olbrich@pengutronix.de>

media: rockchip: rga: share the interrupt when an external iommu is used

The RGA3 and the corresponding iommu share the interrupt. So in that
case, request a shared interrupt so that the iommu driver can request
it as well.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index e3c99c3f7c5be..cda3cecb1ce8c 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -767,7 +767,8 @@ static int rga_probe(struct platform_device *pdev)
>  		goto err_put_clk;
>  	}
>  
> -	ret = devm_request_irq(rga->dev, irq, rga_isr, 0,
> +	ret = devm_request_irq(rga->dev, irq, rga_isr,
> +			       rga_has_internal_iommu(rga) ? 0 : IRQF_SHARED,
>  			       dev_name(rga->dev), rga);

By enabling IRQF_SHARED, the rga_isr handler can be triggered by interrupts
from other devices (like the IOMMU) at any time. Does the current ISR safely
handle these shared interrupts?

Looking at rga_isr(), it always returns IRQ_HANDLED even when the interrupt
was not generated by this device:

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

	return IRQ_HANDLED;
}

Should the handler return IRQ_NONE when it determines the interrupt wasn't
generated by its hardware, to avoid defeating the kernel's spurious IRQ
detection?

Additionally, because the IRQ is requested here before the driver is fully
initialized, could an immediate shared interrupt cause a NULL pointer
dereference?

drivers/media/platform/rockchip/rga/rga.c:rga_isr() {
	...
	if (rga->hw->handle_irq(rga)) {
		...
		struct rga_ctx *ctx = rga->curr;

		WARN_ON(!ctx);

		rga->curr = NULL;

		src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
	...
}

If the hardware happens to have a pending interrupt bit, rga->curr will be
NULL, and the code assigns NULL to ctx, then attempts to dereference
ctx->fh.m2m_ctx.

Could this early registration also cause the ISR to attempt to access
hardware registers while the device's clocks and power domains are disabled?

Because the driver uses PM runtime, the device may be suspended when a shared
interrupt fires. Is it necessary to verify pm_runtime_active(rga->dev)
before reading registers in the ISR to avoid a synchronous external abort?

Finally, could this lead to a data race on the RGA_INT register?

With IRQF_SHARED, rga_isr() can run asynchronously on another CPU while
device_run() is executing:

drivers/media/platform/rockchip/rga/rga.c:device_run() {
	...
	rga->hw->start(rga, vb_to_rga(src), vb_to_rga(dst));
}

If device_run() calls rga->hw->start() to write 0x600 to the RGA_INT register
to enable interrupts, while rga_isr() locklessly performs a read-modify-write
on RGA_INT, could the ISR silently overwrite the configuration with 0,
disabling RGA interrupts mid-job?

>  	if (ret < 0) {
>  		dev_err(rga->dev, "failed to request irq\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=21