Re: [PATCH] media: hws: Wait for IRQ handler before returning buffers

[Ben Hoff]

I reproduced this on HWS hardware.

Without the patch, the synchronized STREAMOFF/VDONE test produced vb2
ownership warnings. With the patch applied, 500 attempts entered the
instrumented 250 ms post-snapshot VDONE window without an ownership
warning or capture failure.

Reviewed-by: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
Tested-by: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>

On Sat, Aug 8, 2026 at 11:08 AM Hao-Qun Huang <alvinhuang0603@xxxxxxxxx> wrote:
>
> hws_stop_streaming() disables capture and then collects the active and
> queued buffers straight away. Clearing cap_active and setting
> stop_requested only stops a VDONE handler that has not checked them
> yet; one already running on another CPU has passed those checks and
> cannot be recalled.
>
> That handler snapshots v->active into a local pointer and drops
> irq_lock before it touches the buffer, so stop_streaming can run in
> between. Without a next_prepared buffer both paths complete the same
> buffer, and the second vb2_buffer_done() hits the WARN_ON for a buffer
> that is no longer active. With a next_prepared buffer the snapshot is
> the only remaining reference to the old active buffer, so
> stop_streaming returns without it and vb2 reports "stop_streaming
> operation is leaving buffer %u in active state" before completing it
> with an error.
>
> Either way the driver breaks the vb2 rule that stop_streaming has to
> give back every buffer it owns before it returns.
>
> Wait for the handler once the hardware is disabled and before the
> buffers are collected. The live mode change and the channel cleanup
> paths already do this around the same collect helper.
>
> Fixes: ba07fd2f5742 ("media: pci: add AVMatrix HWS capture driver")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Hao-Qun Huang <alvinhuang0603@xxxxxxxxx>
> ---
> Found by code inspection; I do not have an HWS card, so this is not
> reproduced on hardware. What convinced me is the asymmetry inside the
> driver itself: the live mode change path calls synchronize_irq() before
> the same hws_video_collect_done_locked() helper, and hws_stop_streaming()
> does not.
>
>  drivers/media/pci/hws/hws_video.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/pci/hws/hws_video.c b/drivers/media/pci/hws/hws_video.c
> index 18e4bc6901d3..7f7e51040926 100644
> --- a/drivers/media/pci/hws/hws_video.c
> +++ b/drivers/media/pci/hws/hws_video.c
> @@ -1292,6 +1292,8 @@ static void hws_stop_streaming(struct vb2_queue *q)
>         WRITE_ONCE(v->stop_requested, true);
>
>         hws_enable_video_capture(v->parent, v->channel_index, false);
> +       if (hws->irq >= 0)
> +               synchronize_irq(hws->irq);
>
>         /* 2) Collect in-flight + queued under the IRQ lock */
>         spin_lock_irqsave(&v->irq_lock, flags);
> --
> 2.43.0
>