[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@xxxxxxxxxxxxxxx
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
---

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