drivers/media/platform/ti/vpe/vip.c | 37 ++++++++++++++++++++++++--- drivers/media/platform/ti/vpe/vpdma.c | 3 ++- 2 files changed, 35 insertions(+), 5 deletions(-)
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.1On 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;
Hi Hans,
Thanks for the review.
On 15/07/26 18:25, hverkuil+cisco@kernel.org wrote:
> 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.
>
Acknowledged, I will be doing that.
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 +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;
>
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.1Hi,
Fan, Thanks for the v4.
Hans, Apologies for the delay in testing this.
On 16/07/26 17:02, 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>
The changes looks good to me. Hence
Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
Tested-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> (on
AM572X-EVM)
Test logs:
https://gist.github.com/Yemike-Abhilash-Chandra/76ff7b621001187283f28fcba15a23e4
Branch:
https://github.com/Yemike-Abhilash-Chandra/linux/commits/VIP_testing/
Thanks and Regards,
Yemike Abhilash Chandra
> ---
> 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);
> }
>
© 2016 - 2026 Red Hat, Inc.