Re: [PATCH v7 18/28] media: rockchip: rga: use card type to specify rga type

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Changing the V4L2 capability `card` name from `"rockchip-rga"` to `"rga2"` breaks existing userspace applications matching on the original card name.
- [Medium] The patch updates the `card` field in `vidioc_querycap` to use `card_type` but fails to update the `name` field in the underlying `struct video_device`. This leaves sysfs and the ioctl out of sync.

Pre-existing issues:
- [High] The driver incorrectly uses dma_sync_single_for_device() on a DMA buffer allocated with dma_alloc_attrs(..., DMA_ATTR_WRITE_COMBINE).
- [High] The interrupt handler warns if the context or buffers are NULL, but fails to abort, leading to immediate kernel panics upon dereferencing them.
- [High] MMIO registers are unconditionally accessed without ensuring that device power domains and clocks are enabled.
- [High] Hardware resets are asserted and deasserted before device clocks are enabled.
--

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

media: rockchip: rga: use card type to specify rga type

This commit adds a card_type field to the rga_hw struct to prepare for
RGA3 support. It updates the V4L2 capability card field to use this
hardware-specific type string, intending to allow userspace to
differentiate between RGA2 and RGA3 device nodes.

> In preparation of the RGA3 support add a filed to the rga_hw struct
> to specify the desired card type value. This allows the user to
> differentiate the RGA2 and RGA3 video device nodes.

[Severity: Medium]
Will this leave the sysfs name out of sync with the V4L2 capability,
preventing the differentiation goal described here?

During device registration in rga_probe(), the struct video_device
inherits its name from rga_videodev.name, which remains hardcoded to
"rockchip-rga". Consequently, /sys/class/video4linux/videoX/name will
still return "rockchip-rga" for all versions, which prevents udev rules
from differentiating the nodes using sysfs attributes. Should vfd->name
be updated dynamically during probe?

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 22954bbae55fc..91775b43ff617 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -354,8 +354,10 @@ static const struct v4l2_file_operations rga_fops = {
>  static int
>  vidioc_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
>  {
> +	struct rockchip_rga *rga = video_drvdata(file);
> +
>  	strscpy(cap->driver, RGA_NAME, sizeof(cap->driver));
> -	strscpy(cap->card, "rockchip-rga", sizeof(cap->card));
> +	strscpy(cap->card, rga->hw->card_type, sizeof(cap->card));

[Severity: Critical]
Does changing the card name from "rockchip-rga" to "rga2" break existing
userspace applications?

The V4L2 capability card string is part of the user-space ABI. Existing
userspace tools or udev rules expecting the original string might fail to
recognize the device, potentially causing an ABI breakage.

>  	strscpy(cap->bus_info, "platform:rga", sizeof(cap->bus_info));
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does this driver incorrectly use
dma_sync_single_for_device() on a DMA buffer allocated with
dma_alloc_attrs(..., DMA_ATTR_WRITE_COMBINE)?

Calling streaming DMA sync functions on addresses returned by coherent
allocations violates the DMA API. Should a memory barrier like wmb() be
used instead?

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
fail to abort when the context is NULL?

Looking at rga_isr(), if rga->curr is NULL, the code emits a WARN_ON(!ctx)
but does not return, and proceeds to dereference ctx in ctx->fh.m2m_ctx.
Can this lead to an immediate kernel panic if a spurious interrupt arrives?

[Severity: High]
This is a pre-existing issue, but are MMIO registers unconditionally
accessed without ensuring device power domains and clocks are enabled?

The driver relies on pm_runtime for clock management and drops its PM
reference at the end of rga_probe(). It appears pm_runtime_get_sync()
is not called before writing to hardware registers in the device_run()
callback or when opening the device. Can this trigger a synchronous
external abort if a shared or spurious interrupt arrives while the device
is suspended?

[Severity: High]
This isn't a bug introduced by this patch, but are hardware resets asserted
and deasserted before device clocks are enabled?

In rga_parse_dt(), the core, axi, and ahb reset controllers are triggered
before pm_runtime_enable() is called to enable the RGA clocks. Since
hardware reset logic often requires clocks to be active, does toggling
resets with gated clocks leave the hardware in an undefined state?

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