Re: [PATCH v7 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 3 potential issue(s) to consider:

New issues:
- [Critical] Accessing hardware registers in the shared interrupt handler while the device is runtime suspended causes a bus fault and system crash.
- [High] The interrupt handler unconditionally returns `IRQ_HANDLED`, violating shared interrupt semantics and breaking spurious interrupt detection.

Pre-existing issues:
- [High] Missing early returns after `WARN_ON` checks leads to deterministic NULL pointer dereferences in the interrupt handler.
--

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

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

This commit updates the RGA driver to request a shared interrupt when
paired with an external IOMMU, allowing both drivers to share the IRQ line.

> 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)
[ ... ]
> -	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,

[Severity: Critical]
If the external IOMMU triggers an interrupt while the RGA device is idle
and its clocks are disabled via rga_runtime_suspend(), will rga_isr() still
attempt to read the hardware registers?

Since rga->hw->handle_irq(rga) directly reads RGA_INT, could this cause an
external abort or bus fault? Does the ISR need to verify the device's
power state with pm_runtime_active() before accessing registers?

[Severity: High]
With IRQF_SHARED added, does rga_isr() correctly return IRQ_NONE when the
RGA hardware didn't generate the interrupt?

If it unconditionally returns IRQ_HANDLED even when rga->hw->handle_irq(rga)
returns false, could this defeat the spurious interrupt detection mechanism
and lead to lockups during an interrupt storm?

[Severity: High]
This is a pre-existing issue, but does adding IRQF_SHARED make the
WARN_ON(!ctx) check in rga_isr() susceptible to a NULL pointer dereference?

If an IOMMU interrupt fires while RGA is idle and rga->curr is NULL, and
reading gated registers returns 0xFFFFFFFF, handle_irq might incorrectly
return true. Since the WARN_ON(!ctx) lacks an early return, will this
proceed to call v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx) and dereference
the NULL pointer?

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

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