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

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

Hi Fan,
Thanks for the v2.

On 08/07/26 07:10, Fan Wu wrote:
> 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 see. Thanks for checking this.

> - 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.
> 

I still feel that there might be a better solution, while I was wondering
I found [1].

Rather than adding a second cancel round, I would suggest dropping the flag
entirely and using disable_work_sync()/enable_work() instead.

disable_work_sync() cancels pending work, waits for a running one,
and additionally makes any subsequent schedule_work() on it a no-op
until enable_work() is called.

So the quiesce_stream can now be

	1.disable_work_sync(&stream->recovery_work);
	2.disable_irqs(dev, dev->slice_id, stream->list_num);
	3.clear_irqs(dev, dev->slice_id, stream->list_num);
	4.synchronize_irq(dev->irq);

Also note that the disable count must stay balanced across start_streaming
and stop_streaming calls, for that we need to do:

- alloc_stream() should do disable_work(&stream->recovery_work) right after
   INIT_WORK(), so the work's resting state is disabled.
- vip_start_streaming(): enable_work(&stream->recovery_work)
- vip_quiesce_stream(): disable_work_sync() as above.

> - 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.
>

I agree.

[1]: https://lore.kernel.org/all/20240227172852.2386358-1-tj@kernel.org/

Thanks and Regards,
Yemike Abhilash Chandra

> Thanks,
> Fan
> 
>> On Jul 7, 2026, at 16:54, Yemike Abhilash Chandra <y-abhilashchandra@ti.com> 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@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;
>