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

[Fan Wu]

Hi Yemike,

A quick follow-up to my earlier reply: [PATCH v2] is in this thread. I
ended up changing the approach after looking more closely, and found one
extra issue:

- I used a per-stream irq_rearm_allowed flag instead of vb2_is_streaming().
  vb2 clears q->streaming only after stop_streaming() returns, so
  vb2_is_streaming() can still return true while stop is in progress; a
  flag cleared at the start of stop_streaming()/free_stream() does not lag.

- I could not drop the second disable/clear/synchronize_irq() after all.
  The flag check is not a synchronization primitive: the worker may read
  the flag as true just before teardown clears it and then enable IRQs
  before cancel_work_sync() returns, so the final quiesce stays as the
  backstop. It is centralized in vip_quiesce_stream(), so the sequence is
  not open-coded in multiple places.

- While reworking this, I found that v1 only drained the worker in the
  unbind/remove path. On file release, vip_release_stream() frees the
  descriptor list while the worker can still reach it via
  populate_desc_list(). v2 drains from vip_stop_streaming() too.

Thanks,
Fan

> On Jul 7, 2026, at 16:54, Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx> wrote:
> 
> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
>> ---
>>  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;