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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 28/07/2026 13:06, hverkuil+cisco@kernel.org wrote:
> On 16/07/2026 13:32, 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. In vip_stop_streaming() the helper
>> runs before the parser is stopped, since a worker drained by
>> disable_work_sync() may re-enable the parser before exiting and would
>> otherwise undo the stop. recovery_work is created disabled and enabled in
>> vip_start_streaming() before IRQs, pairing the enable with the teardown
>> disable across the streaming lifecycle.
>>
>> 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")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Codex:gpt-5.6
>> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
>> ---
>> Changes in v4:
>> - Drop the unrelated vpdma_hwlist_release() cleanup from this fix (Hans
>>   Verkuil).
>> - In free_stream(), call vip_quiesce_stream() before clearing
>>   cap_streams[], so the stream remains published while in-flight IRQ
>>   handling is drained (Hans Verkuil).
> 
> Ah, I already merged v3, so v4 doesn't apply.
> 
> Can you post a new patch that sits on top of the current media-committers/next
> tree?

Sorry, my bad. Just ignore, I made a mistake.

Regards,

	Hans

> 
> Thank you,
> 
> 	Hans
> 
>>
>> 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 (disable_work_sync/enable_work/disable_work), as
>>   suggested by Yemike Abhilash Chandra. This also closes a window the v2
>>   double-drain left open, where its second 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@zju.edu.cn/
>> ---
>>  drivers/media/platform/ti/vpe/vip.c | 37 +++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
>> index cb0a5a07a3d4..673f9addfade 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;
>> +	/*
>> +	 * Quiesce the IRQ handler and recovery worker, then drop the stream
>> +	 * from cap_streams[], before releasing stream-owned resources.
>> +	 */
>> +	vip_quiesce_stream(stream);
>> +	stream->port->cap_streams[stream->stream_id] = NULL;
>> +
>>  	/* 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);
>>  }
>>  
> 
>