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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pm: missing synchronous suspend in rkjpegd_remove leaks clocks
- [High] media: rkjpegd: resetting hardware before disabling IRQ causes kernel panic
- [Medium] media: rkjpegd: valid JPEGs with >= 127 bytes of trailing garbage are incorrectly rejected
- [Medium] pm: missing pm_runtime_mark_last_busy() causes autosuspend thrashing
--

commit d2574a6f04bf2bf05abc30d32244135c74b9546e
Author: Sascha Hauer <s.hauer@pengutronix.de>
Subject: media: rockchip: Add JPEG decoder driver

This commit adds a driver for the Rockchip VDPU720 JPEG hardware decoder.
It exposes a V4L2 M2M device to decode JPEG bitstreams into NV12 format.

> diff --git a/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
> new file mode 100644
> index 0000000000000..e3a6c820ad1ed
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c

[ ... ]

> +static void rkjpegd_job_finish(struct rkjpegd_ctx *ctx,
> +			       enum vb2_buffer_state state)
> +{
> +	struct rkjpegd_dev *jpegd = ctx->dev;
> +
> +	pm_runtime_put_autosuspend(jpegd->dev);

[Severity: Medium]
Will this suspend the device immediately instead of using the configured
100ms autosuspend delay?

The device's last busy timestamp is not updated with
pm_runtime_mark_last_busy() before queuing the autosuspend here. Without
it, the PM core calculates the expiration time based on an uninitialized
or stale timestamp, which can cause the device to suspend immediately and
thrash between back-to-back frames.

> +
> +	rkjpegd_job_finish_no_pm(ctx, state);
> +}

[ ... ]

> +static irqreturn_t rkjpegd_vdpu720_irq(int irq, void *dev_id)
> +{

[ ... ]

> +	rkjpegd_irq_done(jpegd, state);
> +	ret = IRQ_HANDLED;
> +
> +out_put:
> +	pm_runtime_put_autosuspend(jpegd->dev);

[Severity: Medium]
Similar to the issue in rkjpegd_job_finish() above, is it intentional to omit
pm_runtime_mark_last_busy() here?

This also queues an autosuspend without updating the last busy timestamp,
which can defeat the autosuspend delay and cause unnecessary power cycling.

> +
> +	return ret;
> +}

[ ... ]

> +static bool rkjpegd_has_eoi(const void *data, u32 start, u32 len)
> +{
> +	u8 tail[128];
> +	u32 n, i;
> +
> +	while (len > start) {
> +		n = min_t(u32, len - start, sizeof(tail));
> +		memcpy(tail, data + len - n, n);
> +
> +		for (i = n; i && (tail[i - 1] == 0x00 || tail[i - 1] == 0xff); i--)
> +			;
> +
> +		len -= n - i;
> +		if (i)
> +			break;
> +	}
> +
> +	n = min_t(u32, len - start, sizeof(tail));
> +	memcpy(tail, data + len - n, n);
> +
> +	for (i = 0; i + 1 < n; i++)
> +		if (tail[i] == 0xff && tail[i + 1] == 0xd9)
> +			return true;

[Severity: Medium]
Can this inadvertently reject valid JPEG frames?

The search for the 0xff 0xd9 EOI marker only inspects the final 128 bytes
after stripping the 0x00 and 0xff padding. If a valid JPEG has 127 or more
bytes of trailing garbage (such as EXIF metadata or digital signatures),
the EOI marker will fall completely outside this search window and the
function will return false.

> +
> +	return false;
> +}

[ ... ]

> +static void rkjpegd_remove(struct platform_device *pdev)
> +{
> +	struct rkjpegd_dev *jpegd = platform_get_drvdata(pdev);
> +
> +	rkjpegd_v4l2_cleanup(jpegd);
> +
> +	reset_control_assert(jpegd->resets);

[Severity: High]
Does asserting the hardware reset before disabling the IRQ and PM runtime
introduce a race condition?

If a spurious interrupt fires while the IP block is held in reset, the
rkjpegd_vdpu720_irq() handler will run. Because PM runtime is still active,
pm_runtime_get_if_active() will succeed, and the handler will try to read
from the hardware (VDPU720_REG_INT), which can cause a bus stall or kernel
panic.

> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);

[Severity: High]
Could this leak the hardware clocks?

Disabling PM runtime here cancels the pending autosuspend timer, but it
leaves the device in the active power state with a usage count of 0.
Without an explicit synchronous suspend (like pm_runtime_suspend() or
pm_runtime_put_sync_suspend()) before pm_runtime_disable(), the
rkjpegd_runtime_suspend() callback is never executed, and
clk_bulk_disable_unprepare() is bypassed.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-rockchip-jpegdec-v5-0-30658833cb68@pengutronix.de?part=2