Re: [PATCH 09/17] media: rockchip: rga: use components to manage multiple cores

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] v4l2 `m2m_dev` is freed while the shared interrupt handler remains active, causing a use-after-free.

Pre-existing issues:
- [High] Missing `synchronize_irq()` in runtime suspend and unsafe hardware access in shared IRQ handler.
- [High] Out-of-order `pm_runtime_enable` and `platform_set_drvdata` causes a NULL pointer dereference in `rga_runtime_resume`.
--

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

media: rockchip: rga: use components to manage multiple cores

Use component helpers to manage multiple cores and aggregate them into a
central master device. This gives us a dedicated master device and
ensures that all cores (components) are properly set up before creating
the video device.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 15d095a1d1973..178f45b8da940 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
[ ... ]
> +static int rga_core_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +
> +	ret = component_add(&pdev->dev, &rga_core_ops);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register component: %d", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
This is a pre-existing issue, but can the initialization order leave the
driver vulnerable to sysfs-triggered PM transitions?

During probe, pm_runtime_enable() is called before the driver data is set:

drivers/media/platform/rockchip/rga/rga.c:
	pm_runtime_set_autosuspend_delay(core->dev, 50);
	pm_runtime_enable(core->dev);
	...
	platform_set_drvdata(pdev, core);

If a concurrent PM state transition to 'on' occurs (e.g., via a sysfs write)
before platform_set_drvdata() runs, the PM core will trigger
rga_runtime_resume().

Will this result in a NULL pointer dereference when dev_get_drvdata() is
called?

drivers/media/platform/rockchip/rga/rga.c:rga_runtime_resume() {
	struct rga_core *core = dev_get_drvdata(dev);

	return clk_bulk_prepare_enable(core->num_clks, core->clks);
}

[ ... ]
>  static int __maybe_unused rga_runtime_suspend(struct device *dev)
>  {
>  	struct rga_core *core = dev_get_drvdata(dev);
> @@ -933,7 +912,7 @@ static int __maybe_unused rga_runtime_resume(struct device *dev)
>  	return clk_bulk_prepare_enable(core->num_clks, core->clks);
>  }
>  
> -static const struct dev_pm_ops rga_pm = {
> +static const struct dev_pm_ops rga_core_pm = {
>  	SET_RUNTIME_PM_OPS(rga_runtime_suspend,
>  			   rga_runtime_resume, NULL)
>  };

[Severity: High]
This is a pre-existing issue, but does this runtime suspend callback lack
a synchronize_irq() call?

When the hardware lacks an internal IOMMU, the driver requests a shared
interrupt line (IRQF_SHARED). If a shared interrupt fires while the device
is runtime-suspended, the interrupt handler rga_isr() unconditionally
reads hardware registers via rga_handle_irq() without verifying if the
device is powered on using pm_runtime_get_if_active().

Does disabling clocks here in rga_runtime_suspend():

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

allow the interrupt handler to execute mid-flight while the hardware is
powered down, potentially leading to a synchronous external abort?

[ ... ]
> +static void rga_unbind(struct device *dev)
> +{
> +	component_unbind_all(dev, NULL);
> +}

[Severity: Critical]
Does unbinding the master device cause a use-after-free in the active
interrupt handler?

Because the componentization moved the v4l2 m2m_dev allocation to the master
device, but the devres-managed IRQ request is on the core component device,
the shared interrupt handler rga_isr() remains active indefinitely.

When the master device is unbound, rga->m2m_dev is freed:

drivers/media/platform/rockchip/rga/rga.c:rga_unbind() {
    ...
	v4l2_m2m_release(rga->m2m_dev);
    ...
}

If a shared interrupt fires after the master is unbound, will rga_isr()
access the freed m2m_dev and rga structures when calling
v4l2_m2m_job_finish()?

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