Re: [PATCH v6 2/4] media: rockchip: Add JPEG decoder driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] media: rkjpegd: user-triggerable WARN_ON_ONCE in error path
- [High] pm: kernel crash from spurious IRQ reading unclocked registers
- [High] media: rkjpegd: IRQ handler wipes out watchdog soft reset
- [Medium] pm: resource leak due to missing synchronous suspend in remove
- [Medium] media: rkjpegd: data race on ctx->sequence_cap between QBUF and IRQ
--

commit e33f32166562ddeac3fb6446b67084c1666b6b89
Author: Sascha Hauer <s.hauer@pengutronix.de>

media: rockchip: Add JPEG decoder driver

Add a driver for the JPEG hardware decoder Rockchip integrates into a
number of its SoCs, exposing one V4L2 M2M device implementing the
stateful decoder interface.

> diff --git a/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
> --- a/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
> +++ b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c

[ ... ]

> @@ -1388,8 +1388,9 @@ static void rkjpegd_job_finish_no_pm(struct rkjpegd_ctx *ctx,
>  
>  	src->sequence = ctx->sequence_out++;
>  	dst->sequence = ctx->sequence_cap++;
>  

[Severity: Medium]
Does this create a data race on ctx->sequence_cap?

Here in rkjpegd_job_finish_no_pm(), ctx->sequence_cap is incremented in
hardirq context protected by drain_lock. However, it is also incremented in
rkjpegd_last_buffer_done() under vdev_lock in process context.

If userspace issues a VIDIOC_QBUF on the capture queue while a drain is
ongoing, this might lead to duplicate sequence numbers or skipped counts
since the differing locks provide no mutual exclusion.

>  	/* GStreamer takes an empty capture buffer for the end of the stream. */
>  	vb2_set_plane_payload(&dst->vb2_buf, 0,
>  			      ctx->dst_fmt.plane_fmt[0].sizeimage);

[Severity: High]
Could this trigger a kernel WARN_ON_ONCE when handling an undersized buffer?

If userspace queues a frame with a larger resolution, rkjpegd_vdpu720_run()
detects the undersized capture buffer and fails with -EINVAL. The error path
jumps here, which unconditionally sets the payload size to the new, larger
ctx->dst_fmt.plane_fmt[0].sizeimage.

Because sizeimage exceeds the underlying plane length, setting it via
vb2_set_plane_payload() will trigger a warning.

[ ... ]

> @@ -1405,6 +1405,7 @@ static irqreturn_t rkjpegd_vdpu720_irq(int irq, void *dev_id)
>  	struct rkjpegd_dev *jpegd = dev_id;
>  	enum vb2_buffer_state state;
>  	u32 status;
>  
>  	status = rkjpegd_read(jpegd, VDPU720_REG_INT);
>  

[Severity: High]
Could a spurious interrupt cause a synchronous bus fault here?

The IRQ is requested via devm_request_irq() in rkjpegd_probe() before
pm_runtime_enable() is called and before resets are deasserted. If a
spurious interrupt is pending from a prior bootloader or kexec stage, the
IRQ handler fires immediately and reads VDPU720_REG_INT while the device
clocks are still disabled.

Should the handler use pm_runtime_get_if_active() to guard access, and the
IRQ be requested after PM initialization is complete?

>  	/* First phase of the IRQ clear, see VDPU720_IRQ_CLR_KEEP. */
>  	rkjpegd_write(jpegd, status & VDPU720_IRQ_CLR_KEEP, VDPU720_REG_INT);
>  
>  	if (!(status & VDPU720_IRQ_RAW))
>  		return IRQ_NONE;
>  
>  	rkjpegd_write(jpegd, 0, VDPU720_REG_INT);

[Severity: High]
Can the IRQ handler wipe out a concurrent soft reset requested by the
watchdog?

If the watchdog timeout executes vdpu720_soft_reset(), it writes
VDPU720_SOFT_RST_EN to this register. If a hardware IRQ fires concurrently,
this IRQ handler reads the register and then unconditionally writes 0 to
clear it.

This seems to wipe out the VDPU720_SOFT_RST_EN bit before the watchdog
finishes waiting for VDPU720_SOFT_RST_RDY, potentially leaving the block
wedged.

[ ... ]

> @@ -2121,6 +2121,7 @@ static void rkjpegd_remove(struct platform_device *pdev)
>  	reset_control_assert(jpegd->resets);
>  	/* Suspends at once, a pending autosuspend does not keep it active. */
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  }

[Severity: Medium]
Does this leave the hardware active and leak clocks on module unload?

If a job finishes and calls pm_runtime_put_autosuspend(), a delayed suspend
is queued. If the driver is removed before the timer fires,
pm_runtime_disable() cancels the pending autosuspend, but does not suspend
the active device.

Should the driver explicitly suspend the device synchronously before or
after disabling PM runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-rockchip-jpegdec-v6-0-25f16dea1697@pengutronix.de?part=2