Re: [PATCH v3] media: ti: vpe: quiesce overflow recovery before freeing streams

[Yemike Abhilash Chandra]

Hi Hans,
Thanks for the review.

On 15/07/26 18:25, hverkuil+cisco@xxxxxxxxxx wrote:
> On 13/07/2026 21:56, Fan Wu wrote:
> > The VIP overflow recovery worker is armed from the hardirq handler when a
> > FIFO overflow is detected, and the list-complete path looks the stream up
> > through the VPDMA list private pointer. Both keep touching stream, port
> > and device state; the recovery worker also resets the parser and VPDMA,
> > repopulates the descriptor list, and re-enables the per-list IRQs.
> >
> > vip_stop_streaming() masks and clears the per-list IRQs, but it neither
> > synchronizes the hardirq handler nor disables recovery_work. An overflow
> > IRQ that has already queued recovery_work, or a list-complete IRQ in
> > flight when the stream is torn down, can therefore still dereference the
> > stream after its resources are released: the descriptor list is freed by
> > vip_release_stream() on file release, and the stream itself by
> > free_stream() on unbind/remove.
> >
> > Drain the recovery worker and the IRQ handler at both teardown points
> > through a shared vip_quiesce_stream() helper, before any stream-owned
> > resource is released. disable_work_sync() cancels pending recovery_work,
> > drains a running instance, and raises its disable depth, so a subsequent
> > schedule_work() issued by a racing IRQ handler is rejected at the
> > workqueue scheduler: recovery_work cannot be requeued after
> > disable_work_sync() takes effect. The worker may still re-enable the
> > per-list IRQs before disable_work_sync() returns; disable_irqs() then
> > masks those sources and synchronize_irq() waits for any in-flight handler
> > that still dereferences stream state. recovery_work is created disabled
> > and enabled in vip_start_streaming() before IRQs, pairing the enable with
> > the teardown disable across the streaming lifecycle.
> >
> > Return the released slot's value instead of the array base in
> > vpdma_hwlist_release(), and clear the slot so subsequent list-complete
> > lookups cannot recover the freed stream through the stale slot value.
> >
> > This issue was found by an in-house static analysis tool and confirmed by
> > manual code review.
> >
> > Fixes: fc2873aa4a21 ("media: ti: vpe: Add the VIP driver")
> > Assisted-by: Codex:gpt-5.6
> > Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Replace the per-stream irq_rearm_allowed flag and the repeated IRQ
> >    disable/synchronize_irq() in vip_quiesce_stream() with the workqueue
> >    disable-depth API, as suggested by Yemike Abhilash Chandra.
> >    disable_work_sync() cancels pending recovery_work, drains a running
> >    instance, and blocks any future schedule_work() from a racing IRQ
> >    handler at the scheduler; this also closes a window the v2 double-drain
> >    left open, where its second IRQ drain (synchronize_irq) waited for the
> >    in-flight handler but did not cancel the recovery_work it had requeued,
> >    so that work could run after free.
> > - Create recovery_work disabled and enable it in vip_start_streaming()
> >    before IRQs.
> > - In vip_stop_streaming(), quiesce before stopping the parser: a worker
> >    drained by disable_work_sync() may re-enable the parser before exiting, so
> >    stopping the parser first would be undone.
> >
> > Changes in v2:
> > - Drain the overflow recovery worker at both teardown points through a
> >    shared vip_quiesce_stream() helper: vip_stop_streaming() (file release
> >    path) and free_stream() (unbind/remove). v1 drained only in
> >    free_stream().
> > - Document how the issue was found and that the patch was prepared with
> >    LLM assistance (Assisted-by trailer and body note).
> >
> > Link: https://lore.kernel.org/r/20260708013738.110752-1-fanwu01@xxxxxxxxxx/
> > ---
> >   drivers/media/platform/ti/vpe/vip.c   | 37 ++++++++++++++++++++++++---
> >   drivers/media/platform/ti/vpe/vpdma.c |  3 ++-
> >   2 files changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
> > index cb0a5a07a3d4..30e9a85d4cf5 100644
> > --- a/drivers/media/platform/ti/vpe/vip.c
> > +++ b/drivers/media/platform/ti/vpe/vip.c
> > @@ -814,6 +814,22 @@ static void clear_irqs(struct vip_dev *dev, int irq_num, int list_num)
> >   	vpdma_clear_list_stat(dev->shared->vpdma, irq_num, dev->slice_id);
> >   }
> >   
> > +/*
> > + * Quiesce recovery work and per-list IRQs before releasing stream resources.
> > + * disable_work_sync() prevents the overflow handler from requeueing recovery
> > + * work. Mask and synchronize IRQs afterwards because a running worker may
> > + * have re-enabled them before exiting.
> > + */
> > +static void vip_quiesce_stream(struct vip_stream *stream)
> > +{
> > +	struct vip_dev *dev = stream->port->dev;
> > +
> > +	disable_work_sync(&stream->recovery_work);
> > +	disable_irqs(dev, dev->slice_id, stream->list_num);
> > +	clear_irqs(dev, dev->slice_id, stream->list_num);
> > +	synchronize_irq(dev->irq);
> > +}
> > +
> >   static void populate_desc_list(struct vip_stream *stream)
> >   {
> >   	struct vip_port *port = stream->port;
> > @@ -2428,6 +2444,7 @@ static int vip_start_streaming(struct vb2_queue *vq, unsigned int count)
> >   		goto err;
> >   
> >   	stream->num_recovery = 0;
> > +	enable_work(&stream->recovery_work);
> >   
> >   	clear_irqs(dev, dev->slice_id, stream->list_num);
> >   	enable_irqs(dev, dev->slice_id, stream->list_num);
> > @@ -2452,13 +2469,17 @@ static void vip_stop_streaming(struct vb2_queue *vq)
> >   	struct vip_dev *dev = port->dev;
> >   	int ret;
> >   
> > +	/*
> > +	 * A running recovery worker may re-enable the parser, so quiesce it
> > +	 * and its IRQ handler before stopping the parser or releasing the
> > +	 * descriptor list.
> > +	 */
> > +	vip_quiesce_stream(stream);
> > +
> >   	vip_parser_stop_imm(port, true);
> >   	vip_enable_parser(port, false);
> >   	unset_fmt_params(stream);
> >   
> > -	disable_irqs(dev, dev->slice_id, stream->list_num);
> > -	clear_irqs(dev, dev->slice_id, stream->list_num);
> > -
> >   	if (port->subdev) {
> >   		ret = v4l2_subdev_call(port->subdev, video, s_stream, 0);
> >   		if (ret)
> > @@ -3074,6 +3095,8 @@ static int alloc_stream(struct vip_port *port, int stream_id, int vfl_type)
> >   		goto do_free_hwlist;
> >   
> >   	INIT_WORK(&stream->recovery_work, vip_overflow_recovery_work);
> > +	/* Start disabled; vip_start_streaming() enables it before IRQs. */
> > +	disable_work(&stream->recovery_work);
> >   
> >   	INIT_LIST_HEAD(&stream->vidq);
> >   
> > @@ -3139,6 +3162,13 @@ static void free_stream(struct vip_stream *stream)
> >   		return;
> >   
> >   	dev = stream->port->dev;
> > +	/*
> > +	 * Unpublish the stream and quiesce its IRQ handler and recovery worker
> > +	 * before releasing stream-owned resources.
> > +	 */
> > +	stream->port->cap_streams[stream->stream_id] = NULL;
> > +	vip_quiesce_stream(stream);
>
> Shouldn't these two lines be swapped? It feels dangerous to set that pointer
> to NULL while IRQ handlers might still be running.
>
> I want to see a 'Tested-by' from someone before I accept this patch.

Acknowledged, I will be doing that.

Thanks and Regards,
Yemike Abhilash Chandra

> > +
> >   	/* Free up the Drop queue */
> >   	list_for_each_safe(pos, q, &stream->dropq) {
> >   		buf = list_entry(pos,
> > @@ -3150,7 +3180,6 @@ static void free_stream(struct vip_stream *stream)
> >   
> >   	video_unregister_device(stream->vfd);
> >   	vpdma_hwlist_release(dev->shared->vpdma, stream->list_num);
> > -	stream->port->cap_streams[stream->stream_id] = NULL;
> >   	kfree(stream);
> >   }
> >   
> > diff --git a/drivers/media/platform/ti/vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c
> > index 573aa83f62eb..f9f5b2f1ee1a 100644
> > --- a/drivers/media/platform/ti/vpe/vpdma.c
> > +++ b/drivers/media/platform/ti/vpe/vpdma.c
> > @@ -988,7 +988,8 @@ void *vpdma_hwlist_release(struct vpdma_data *vpdma, int list_num)
> >   
> >   	spin_lock_irqsave(&vpdma->lock, flags);
> >   	vpdma->hwlist_used[list_num] = false;
> > -	priv = vpdma->hwlist_priv;
> > +	priv = vpdma->hwlist_priv[list_num];
> > +	vpdma->hwlist_priv[list_num] = NULL;
>
> Hmm, the return pointer is not actually used anywhere. I'd rather turn this into a
> void function.
>
> I also don't think it is needed to set vpdma->hwlist_priv[list_num] to NULL. Although you
> could use that as an alternative for hwlist_used and just drop hwlist_used.
>
> In any case, this change has nothing to do with the other changes and so it should be
> in a separate patch.
>
> Regards,
>
> 	Hans
>
> >   	spin_unlock_irqrestore(&vpdma->lock, flags);
> >   
> >   	return priv;