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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] media: rockchip: iep2: race in iep_device_run() releasing
  irq_lock early
--

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

media: rockchip: Add IEP2 deinterlacer driver

This commit adds a V4L2 mem2mem driver for the Rockchip IEP2 deinterlacer
found on the RK3568 and RK3588 SoCs.

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

[ ... ]

> +static void iep_device_run(void *priv)
> +{

[ ... ]

> +	spin_lock_irqsave(&iep->irq_lock, flags);
> +	iep->busy = true;
> +	iep->irq_status = 0;
> +	iep->foreign_irqs = 0;
> +	spin_unlock_irqrestore(&iep->irq_lock, flags);
> +
> +	schedule_delayed_work(&iep->watchdog_work,
> +			      msecs_to_jiffies(IEP_WATCHDOG_MS));
> +
> +	writel(IEP2_FRM_EN, iep->regs + IEP2_FRM_START);
> +}

[Severity: Medium]
Can releasing iep->irq_lock before scheduling iep->watchdog_work and
writing IEP2_FRM_EN race with iep_watchdog() or shared interrupts?

CPU0                                  CPU1
iep_device_run()
  spin_lock_irqsave(&iep->irq_lock)
  iep->busy = true
  spin_unlock_irqrestore(&iep->irq_lock)
  schedule_delayed_work(&iep->watchdog_work, ...)
  <preempted>
                                      iep_watchdog()
                                        iep->busy = false
                                        iep_reset(iep)
                                        iep_job_done(ctx, ...)
                                          v4l2_m2m_buf_done(...)
                                          pm_runtime_put_autosuspend(...)
  writel(IEP2_FRM_EN, ...)  <-- starts DMA on returned buffers / unclocked regs

Similarly, if IEP_MAX_FOREIGN_IRQS shared interrupts arrive on the shared
IRQ line right after spin_unlock_irqrestore(), iep_irq() sees iep->busy
and wakes iep_irq_thread(), which finishes the job before iep_device_run()
queues iep->watchdog_work and writes IEP2_FRM_START.

Keeping schedule_delayed_work() and writel(IEP2_FRM_EN, ...) inside the
iep->irq_lock critical section would make publishing iep->busy atomic with
starting the hardware and watchdog timer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-iep2-v2-0-3d8798cd9a55@gmail.com?part=2