Re: [PATCH 11/17] media: rockchip: rga: move video device to the master

[sashiko-bot@kernel.org]

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

New issues:
- [High] The master component bind function (`rga_bind`) fails to unbind previously bound components when a subsequent V4L2 initialization step fails.
- [High] Improper coupling of component lifecycle and devres managed allocations leads to potential resource leaks or use-after-free upon master probe failure.
- [Low] The `rga_core_bind` function uses an unregistered `v4l2_device` for logging, resulting in a missing device name prefix.

Pre-existing issues:
- [High] The driver conditionally uses a shared interrupt (`IRQF_SHARED`) but lacks necessary PM runtime synchronizations, risking invalid hardware access.
--

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

media: rockchip: rga: move video device to the master

This commit moves the video device allocation and registration to the master
component bind function. This prepares the driver for binding multiple cores
to the master device, ensuring the v4l2 device is only registered after all
cores have successfully bound.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 11912bf5b6906..952377ae467f5 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
[ ... ]
> @@ -789,33 +788,11 @@ static int rga_core_bind(struct device *dev, struct device *master, void *data)
>  		goto err_put_clk;
>  	}
>  
> -	ret = v4l2_device_register(&pdev->dev, &rga->v4l2_dev);
> -	if (ret)
> -		goto err_put_clk;
[ ... ]
>  	ret = pm_runtime_resume_and_get(core->dev);
>  	if (ret < 0)
> -		goto rel_m2m;
> +		goto err_put_clk;

[Severity: High]
This is a pre-existing issue, but the driver appears to use IRQF_SHARED
conditionally for its interrupts. Does rga_isr() need to use
pm_runtime_get_if_active() to ensure the device is not runtime-suspended
before accessing hardware registers?

Additionally, does rga_runtime_suspend() need to call synchronize_irq()
before disabling clocks to prevent in-flight shared IRQs from executing
while hardware is powered down?

[ ... ]
> @@ -937,6 +892,7 @@ static struct platform_driver rga_core_pdrv = {
>  static int rga_bind(struct device *dev)
>  {
>  	struct rockchip_rga *rga = dev_get_drvdata(dev);
> +	struct video_device *vfd;
>  	int ret;
>  
>  	ret = component_bind_all(dev, rga);
> @@ -945,11 +901,59 @@ static int rga_bind(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = v4l2_device_register(dev, &rga->v4l2_dev);

[Severity: Low]
Since component_bind_all() executes rga_core_bind() before
v4l2_device_register() initializes rga->v4l2_dev.name, does the v4l2_info()
logging call inside rga_core_bind() end up printing an empty device name
prefix?

[Severity: High]
Since rga_core_probe() allocates the shared IRQ using devm_request_irq(),
the IRQ remains active and tied to the core device's devres lifecycle.

If rga_bind() returns an error, the master probe fails and its devres
unwinds, freeing the master's rga structure. If the shared IRQ fires
after this point, could rga_isr() trigger a use-after-free or NULL pointer
dereference when accessing core->rga?

[ ... ]
> +rel_m2m:
> +	v4l2_m2m_release(rga->m2m_dev);
> +rel_vdev:
> +	video_device_release(vfd);
> +unreg_v4l2_dev:
> +	v4l2_device_unregister(&rga->v4l2_dev);
> +	return ret;
>  }

[Severity: High]
If initialization fails at v4l2_device_register() or later, the function
jumps to these error paths. Should component_unbind_all() be called here
to prevent leaving the child cores in a bound state and leaking resources
acquired during their bind callbacks?

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