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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi,
Thanks for the patch

On 07/07/26 07:22, Fan Wu wrote:
> The VIP overflow recovery work 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 and can re-enable overflow interrupts.
> 
> vip_stop_streaming() masks and clears the per-list IRQs, but it neither
> synchronizes the hardirq handler nor cancels recovery_work. If an
> overflow IRQ has already queued recovery_work, or a list-complete IRQ is
> in flight when the stream is torn down, the handler or worker can still
> dereference the stream after its resources are released.
> 
> free_stream() owns the stream lifetime, so drain the IRQ handler and
> recovery work there before freeing stream-owned resources: drop the
> stream from cap_streams[], disable IRQs for its list (disable_irqs()
> masks both the parser-overflow and the list-complete IRQ), wait for any
> in-flight handler, cancel the worker, then disable and sync again
> because the worker may have re-enabled interrupts while it ran. Only
> then are the drop queue, video device and VPDMA list released and the
> stream freed.
> 
> Additionally clear the VPDMA list private pointer in vpdma_hwlist_release
> (and return the released slot's value instead of the array base), so
> later list-complete handling cannot recover a freed stream through a
> stale private pointer.
> 
> Fixes: fc2873aa4a21 ("media: ti: vpe: Add the VIP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>   drivers/media/platform/ti/vpe/vip.c   | 20 +++++++++++++++++++-
>   drivers/media/platform/ti/vpe/vpdma.c |  3 ++-
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
> index cb0a5a07a3d4..9c5bf91ade1b 100644
> --- a/drivers/media/platform/ti/vpe/vip.c
> +++ b/drivers/media/platform/ti/vpe/vip.c
> @@ -3139,6 +3139,25 @@ static void free_stream(struct vip_stream *stream)
>   		return;
>   
>   	dev = stream->port->dev;
> +	/*
> +	 * Quiesce the overflow IRQ and recovery work for this stream
> +	 * before releasing its resources: the handler and the worker
> +	 * both keep touching stream, port and device state. disable_irqs()
> +	 * masks both the parser-overflow and the list-complete IRQ for
> +	 * this list. Drop the stream from cap_streams[] first so a racing
> +	 * overflow handler misses the lookup, wait for any in-flight
> +	 * handler, cancel the worker, then disable and sync again because
> +	 * the worker may have re-enabled interrupts while it ran.
> +	 */

Did you able to reproduce this?

I am not sure if it is reproducible in practice? I will try to reproduce
this with hardware, (again I am not really sure how to simulate the
overflow to trigger the overflow recovery) but in the meantime few
comments.

> +	stream->port->cap_streams[stream->stream_id] = NULL;
> +	disable_irqs(dev, dev->slice_id, stream->list_num);
> +	clear_irqs(dev, dev->slice_id, stream->list_num);
> +	synchronize_irq(dev->irq);
> +	cancel_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);
> +

Having synchronize_irq and etc twice seems ugly, I understand the reason.

But in vip_overflow_recovery_work before actually enabling the irqs again,
Can you check if the queue is currently active? and If not we can choose
not enable the irqs again, thereby eliminating need for second
synchronize_irq, disable_irqs call?

and also are you using any LLM or other tool? if so, please document the
same in the commit message.

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 +3169,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;
>   	spin_unlock_irqrestore(&vpdma->lock, flags);
>   
>   	return priv;