Re: [PATCH 2/3] media: rockchip: Add IEP2 deinterlacer driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] media: rockchip: iep2: lost frame-done IRQ in iep_irq() when
  OSD_MAX fires
- [Medium] media: rockchip: iep2: unreleased prev and second dst buf in
  iep_fail_job()
- [Medium] media: rockchip: iep2: race and use-after-free in
  iep_remove() on unbind
--

commit 33c094e881e4e76b67c9126fbcce91524e080828
Author: Arun Iyengar <aiyengar1990@gmail.com>

media: rockchip: Add IEP2 deinterlacer driver

This commit adds a V4L2 mem2mem driver for the Rockchip IEP2 deinterlacer on
RK3568 and RK3588 SoCs, supporting bob and motion-adaptive deinterlacing at
frame and field rates.

> diff --git a/drivers/media/platform/rockchip/iep2/iep2.c b/drivers/media/platform/rockchip/iep2/iep2.c
> new file mode 100644
> index 0000000000000..26f5fec02b679
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/iep2/iep2.c
[ ... ]
> +static void iep_job_done(struct iep_ctx *ctx, enum vb2_buffer_state state)
> +{
> +	struct iep_dev *iep = ctx->iep;
> +	struct vb2_v4l2_buffer *src = ctx->job_src;
> +	unsigned int i;
> +
> +	for (i = 0; i < ctx->job_ndst; i++) {
> +		v4l2_m2m_buf_done(ctx->job_dst[i], state);
> +		ctx->job_dst[i] = NULL;
> +	}
> +	ctx->job_ndst = 0;
> +	ctx->job_src = NULL;
> +
> +	if (ctx->prev) {
> +		v4l2_m2m_buf_done(ctx->prev, VB2_BUF_STATE_DONE);
> +		ctx->prev = NULL;
> +	}
> +
> +	if (src) {
> +		if (ctx->job_keep_src && state == VB2_BUF_STATE_DONE)
> +			ctx->prev = src;
> +		else
> +			v4l2_m2m_buf_done(src, state);
> +	}
> +	ctx->job_keep_src = false;
> +
> +	pm_runtime_put_autosuspend(iep->dev);
> +	v4l2_m2m_job_finish(iep->m2m_dev, ctx->fh.m2m_ctx);
> +}
> +
> +static void iep_fail_job(struct iep_ctx *ctx)
> +{
> +	struct vb2_v4l2_buffer *src, *dst;
> +
> +	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +	if (src)
> +		v4l2_m2m_buf_done(src, VB2_BUF_STATE_ERROR);
> +	if (dst)
> +		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_ERROR);
> +	v4l2_m2m_job_finish(ctx->iep->m2m_dev, ctx->fh.m2m_ctx);
> +}

[Severity: Medium]
When pm_runtime_resume_and_get() fails in iep_device_run(), does
iep_fail_job() leave ctx->prev and the second field-rate CAPTURE buffer
queued?

In motion-adaptive mode, failing src without first completing and clearing
ctx->prev returns the current OUTPUT buffer ahead of the reference buffer held
from the previous frame.  In field-rate mode, iep_job_ready() waits for two
CAPTURE buffers, so leaving ctx->prev on src_vq->queued_list and the second
CAPTURE buffer on dst_vq->queued_list keeps both queues non-empty while no new
job can run, causing poll() or blocking VIDIOC_DQBUF to hang even after
iep_resume() calls v4l2_m2m_resume().

[ ... ]
> +static irqreturn_t iep_irq(int irq, void *data)
> +{
> +	struct iep_dev *iep = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 status;
> +
> +	/* Shared with the IOMMU: registers are only accessible during a job */
> +	spin_lock(&iep->irq_lock);
> +	if (!iep->busy)
> +		goto out;
> +
> +	status = iep_read(iep, IEP2_INT_STS);
> +	if (!status) {
> +		/* IOMMU fault: the IEP would retry the read forever */
> +		if (++iep->foreign_irqs < IEP_MAX_FOREIGN_IRQS)
> +			goto out;
> +
> +		iep_write(iep, IEP2_INT_EN, 0);
> +		iep_write(iep, IEP2_CONFIG0, IEP2_ACLK_SRESET_P);
> +		iep->busy = false;
> +		iep->irq_status = IEP_IRQ_IOMMU_FAULT;
> +		ret = IRQ_WAKE_THREAD;
> +		goto out;
> +	}
> +
> +	iep_write(iep, IEP2_INT_CLR, 0xffffffff);
> +	ret = IRQ_HANDLED;
> +
> +	if (status & IEP2_INT_VALID) {
> +		iep->busy = false;
> +		iep->irq_status = status;
> +		ret = IRQ_WAKE_THREAD;
> +	}
> +out:
> +	spin_unlock(&iep->irq_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Can writing 0xffffffff to IEP2_INT_CLR drop a frame-done interrupt when
IEP2_INT_OSD_MAX fires first?

iep_hw_config() enables IEP2_INT_OSD_MAX in IEP2_INT_EN, but IEP2_INT_VALID
only includes IEP2_INT_TIMEOUT, IEP2_INT_BUS_ERROR and IEP2_INT_FRM_DONE.  If
IEP2_INT_OSD_MAX triggers iep_irq() and IEP2_INT_FRM_DONE asserts between
reading IEP2_INT_STS and writing 0xffffffff to IEP2_INT_CLR, the frame-done
status is cleared in hardware without waking iep_irq_thread(), stalling the
job until the 500 ms watchdog times out and resets the block.  Clearing only
status (or not enabling IEP2_INT_OSD_MAX) would avoid losing the completion.

[ ... ]
> +static void iep_remove(struct platform_device *pdev)
> +{
> +	struct iep_dev *iep = platform_get_drvdata(pdev);
> +
> +	v4l2_m2m_suspend(iep->m2m_dev);
> +	video_unregister_device(&iep->vfd);
> +	cancel_delayed_work_sync(&iep->watchdog_work);
> +
> +	/* Disabling runtime PM drops a pending autosuspend */
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		iep_runtime_suspend(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	if (iep->empty_domain)
> +		iommu_domain_free(iep->empty_domain);
> +	iep->empty_domain = NULL;
> +}

[Severity: Medium]
Can v4l2_m2m_suspend() race with iep_release() and read a freed m2m_ctx?

v4l2_m2m_suspend() (also called from iep_suspend() without iep->mutex) reads
m2m_dev->curr_ctx under job_spinlock, drops the spinlock, and waits on
curr_ctx->finished:

CPU0                              CPU1
iep_remove()
  v4l2_m2m_suspend(iep->m2m_dev)
    curr_ctx = m2m_dev->curr_ctx
                                  iep_release()
                                    mutex_lock(&iep->mutex)
                                    v4l2_m2m_ctx_release(ctx->fh.m2m_ctx)
                                      wait_event(m2m_ctx->finished, ...)
                                      /* job finishes */
                                      kfree(m2m_ctx)
    wait_event(curr_ctx->finished,
      !(curr_ctx->job_flags & ...))  <-- use-after-free

In addition, iep_remove() never acquires iep->mutex, so an ioctl that entered
__video_do_ioctl() before video_unregister_device() cleared
V4L2_FL_REGISTERED can still be running under iep->mutex while iep_remove()
disables runtime PM, unprepares clocks, and frees iep->empty_domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-iep2-v1-0-a8edde5a21e1@gmail.com?part=2