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

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

[Fan Wu]

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.
+	 */
+	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);
+
 	/* 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;
-- 
2.34.1

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

[Yemike Abhilash Chandra]

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;

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@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;

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

[Fan Wu]

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, repopulates the descriptor list, 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: the descriptor
list is freed by vip_release_stream() on file release, and the stream
itself by free_stream() on unbind/remove.

Drain the IRQ handler and recovery work at both teardown points through
a shared vip_quiesce_stream() helper, before any stream-owned resource
is released: vip_stop_streaming() (the vb2 stop reached on file release)
and free_stream() (reached on unbind/remove) each disable IRQs for the
list, wait for any in-flight handler, cancel the worker, then disable
and synchronize one more time. vip_overflow_recovery_work() checks a
per-stream irq_rearm_allowed flag before it re-enables IRQs and
restarts the parser, so the worker does not re-arm the capture path
once streaming is stopping; the flag cannot close the check-then-act
window on its own, so the second disable/synchronize remains as the
backstop.

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.

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.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---

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, where vip_release_stream() frees the descriptor list) and
  free_stream() (unbind/remove). v1 drained only in free_stream().
- vip_overflow_recovery_work() checks a per-stream irq_rearm_allowed
  flag before re-enabling IRQs (suggested by Yemike Abhilash Chandra);
  keep the final disable/clear/synchronize_irq() as a backstop, since the
  flag check is not a synchronization primitive.
- Document how the issue was found and that the patch was prepared with
  LLM assistance (Assisted-by trailer and body note).

 drivers/media/platform/ti/vpe/vip.c   | 56 +++++++++++++++++++++++++--
 drivers/media/platform/ti/vpe/vip.h   |  1 +
 drivers/media/platform/ti/vpe/vpdma.c |  3 +-
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
index cb0a5a07a3d4..82a1b3f8f1ac 100644
--- a/drivers/media/platform/ti/vpe/vip.c
+++ b/drivers/media/platform/ti/vpe/vip.c
@@ -814,6 +814,30 @@ 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);
 }
 
+/*
+ * Drain the overflow IRQ handler and recovery worker for this stream
+ * before its resources are released. disable_irqs() masks both the
+ * parser-overflow and the list-complete IRQ for this list.
+ * vip_overflow_recovery_work() checks irq_rearm_allowed before it
+ * re-enables IRQs, but it may have passed that check just before the
+ * flag is cleared and then enabled IRQs before cancel_work_sync()
+ * returns, so disable and synchronize one more time. Reached on
+ * close(fd) through vip_stop_streaming() and on unbind/remove through
+ * free_stream().
+ */
+static void vip_quiesce_stream(struct vip_stream *stream)
+{
+	struct vip_dev *dev = stream->port->dev;
+
+	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);
+}
+
 static void populate_desc_list(struct vip_stream *stream)
 {
 	struct vip_port *port = stream->port;
@@ -1041,6 +1065,16 @@ static void vip_overflow_recovery_work(struct work_struct *work)
 	populate_desc_list(stream);
 	stream->num_recovery++;
 	if (stream->num_recovery < 5) {
+		/*
+		 * Streaming may have stopped while this work was pending or
+		 * running. If re-arming is no longer allowed, leave the
+		 * interrupts disabled and return instead of restarting the
+		 * capture path. vip_stop_streaming() and free_stream() clear
+		 * the flag before tearing the path down.
+		 */
+		if (!READ_ONCE(stream->irq_rearm_allowed))
+			return;
+
 		/* Reload the vpdma */
 		vip_load_vpdma_list_fifo(stream);
 
@@ -2428,6 +2462,7 @@ static int vip_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err;
 
 	stream->num_recovery = 0;
+	WRITE_ONCE(stream->irq_rearm_allowed, true);
 
 	clear_irqs(dev, dev->slice_id, stream->list_num);
 	enable_irqs(dev, dev->slice_id, stream->list_num);
@@ -2452,12 +2487,18 @@ static void vip_stop_streaming(struct vb2_queue *vq)
 	struct vip_dev *dev = port->dev;
 	int ret;
 
+	/*
+	 * The stream is going down: forbid the recovery worker from
+	 * re-arming the capture path and drain any in-flight overflow IRQ
+	 * handler and worker before the descriptor list is freed by
+	 * vip_release_stream().
+	 */
+	WRITE_ONCE(stream->irq_rearm_allowed, false);
 	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);
+	vip_quiesce_stream(stream);
 
 	if (port->subdev) {
 		ret = v4l2_subdev_call(port->subdev, video, s_stream, 0);
@@ -3139,6 +3180,16 @@ static void free_stream(struct vip_stream *stream)
 		return;
 
 	dev = stream->port->dev;
+	/*
+	 * Unbind/remove path: drop the stream from cap_streams[] so a
+	 * racing overflow handler misses the lookup, then drain the IRQ
+	 * handler and recovery worker (shared with vip_stop_streaming())
+	 * before releasing the stream-owned resources.
+	 */
+	WRITE_ONCE(stream->irq_rearm_allowed, false);
+	stream->port->cap_streams[stream->stream_id] = NULL;
+	vip_quiesce_stream(stream);
+
 	/* Free up the Drop queue */
 	list_for_each_safe(pos, q, &stream->dropq) {
 		buf = list_entry(pos,
@@ -3150,7 +3201,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/vip.h b/drivers/media/platform/ti/vpe/vip.h
index 20525369955d..6976affc9ece 100644
--- a/drivers/media/platform/ti/vpe/vip.h
+++ b/drivers/media/platform/ti/vpe/vip.h
@@ -215,6 +215,7 @@ struct vip_stream {
 	char			name[16];
 	struct work_struct	recovery_work;
 	int			num_recovery;
+	bool			irq_rearm_allowed;
 	enum v4l2_field		field;		/* current field */
 	unsigned int		sequence;	/* current frame/field seq */
 	enum v4l2_field		sup_field;	/* supported field value */
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;
-- 
2.34.1

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@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;

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

[Yemike Abhilash Chandra]

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

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

[Fan Wu]

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. recovery_work is created disabled
and enabled in vip_start_streaming() before IRQs, pairing the enable with
the teardown disable across the streaming lifecycle.

Return the released slot's value instead of the array base in
vpdma_hwlist_release(), and clear the slot so subsequent list-complete
lookups cannot recover the freed stream through the stale slot value.

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")
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
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, as suggested by Yemike Abhilash Chandra.
  disable_work_sync() cancels pending recovery_work, drains a running
  instance, and blocks any future schedule_work() from a racing IRQ
  handler at the scheduler; this also closes a window the v2 double-drain
  left open, where its second IRQ drain (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 ++++++++++++++++++++++++---
 drivers/media/platform/ti/vpe/vpdma.c |  3 ++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
index cb0a5a07a3d4..30e9a85d4cf5 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;
+	/*
+	 * Unpublish the stream and quiesce its IRQ handler and recovery worker
+	 * before releasing stream-owned resources.
+	 */
+	stream->port->cap_streams[stream->stream_id] = NULL;
+	vip_quiesce_stream(stream);
+
 	/* 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);
 }
 
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;
-- 
2.34.1

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

[hverkuil+cisco]

On 13/07/2026 21:56, 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. recovery_work is created disabled
> and enabled in vip_start_streaming() before IRQs, pairing the enable with
> the teardown disable across the streaming lifecycle.
> 
> Return the released slot's value instead of the array base in
> vpdma_hwlist_release(), and clear the slot so subsequent list-complete
> lookups cannot recover the freed stream through the stale slot value.
> 
> 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")
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
> 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, as suggested by Yemike Abhilash Chandra.
>   disable_work_sync() cancels pending recovery_work, drains a running
>   instance, and blocks any future schedule_work() from a racing IRQ
>   handler at the scheduler; this also closes a window the v2 double-drain
>   left open, where its second IRQ drain (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 ++++++++++++++++++++++++---
>  drivers/media/platform/ti/vpe/vpdma.c |  3 ++-
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/vpe/vip.c b/drivers/media/platform/ti/vpe/vip.c
> index cb0a5a07a3d4..30e9a85d4cf5 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;
> +	/*
> +	 * Unpublish the stream and quiesce its IRQ handler and recovery worker
> +	 * before releasing stream-owned resources.
> +	 */
> +	stream->port->cap_streams[stream->stream_id] = NULL;
> +	vip_quiesce_stream(stream);

Shouldn't these two lines be swapped? It feels dangerous to set that pointer
to NULL while IRQ handlers might still be running.

I want to see a 'Tested-by' from someone before I accept this patch.

> +
>  	/* 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);
>  }
>  
> 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;

Hmm, the return pointer is not actually used anywhere. I'd rather turn this into a
void function.

I also don't think it is needed to set vpdma->hwlist_priv[list_num] to NULL. Although you
could use that as an alternative for hwlist_used and just drop hwlist_used.

In any case, this change has nothing to do with the other changes and so it should be
in a separate patch.

Regards,

	Hans

>  	spin_unlock_irqrestore(&vpdma->lock, flags);
>  
>  	return priv;

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

[Fan Wu]

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

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);
 }
 
-- 
2.34.1