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

[Fan Wu]

Hi Yemike,

Thanks for the review.

I have not reproduced this on hardware. The issue was reported by our
in-house static analysis tool, and I manually audited the driver to confirm
the lifetime race. The overflow IRQ can queue recovery_work, while the
list-complete IRQ handler can obtain a stale stream pointer through the
VPDMA list private data (vpdma_hwlist_get_priv()). vip_stop_streaming()
only masks and clears the IRQs; it does not wait for an in-flight handler
and does not cancel recovery_work. Both the handler and the worker can
still dereference stream-owned state, and the worker may also re-enable
the IRQs, so free_stream() needs to drain the handler/work item before
releasing the stream-owned resources.

Your suggestion makes sense. I will update v2 so that
vip_overflow_recovery_work() checks whether the vb2 queue is still active
before re-enabling IRQs/restarting the parser. With that guard in place,
free_stream() should then only need the first disable/clear/synchronize_irq()
followed by cancel_work_sync(), and the second disable/clear/synchronize_irq()
sequence can be dropped.

Regarding your tooling question: in addition to the in-house static
analysis tool that found the bug, I used the Codex coding assistant
(gpt-5.5) to help prepare the patch. The final submitted change was
reviewed and edited by me, and I take full responsibility for it.

I will document this in the v2 commit message, for example:

  This issue was found by an in-house static analysis tool.

  Assisted-by: Codex:gpt-5.5

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;