Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio-snd: missing stop_operating when paused capture hits XRUN --
commit 6569a6d061b2d8bd7d031671c88c23ff7eb99711 Author: Joonho Lee <[email protected]> ALSA: virtio-snd: fix dsnoop capture with staged buffers This commit updates virtio-snd staged capture to detect free-running mode when stop_threshold equals boundary, allowing dsnoop clients to consume the capture ring without advancing appl_ptr. It also validates unread ring occupancy when transitioning back to bounded capture and triggers an XRUN if outstanding data fills the ring. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index d50924b792f65..622a5d7706b44 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -893,6 +905,13 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream > *substream, int command) > if (rc) { > vss->stopped = true; > substream->runtime->stop_operating = true; > + if (vss->capture_staging && > + vss->direction == SNDRV_PCM_STREAM_CAPTURE > && > + command == SNDRV_PCM_TRIGGER_PAUSE_RELEASE) > { > + /* Pause undo leaves ALSA paused; wake > its readers. */ > + vss->xfer_xrun = true; > + > virtsnd_pcm_schedule_period_elapsed(vss); > + } > return rc; > } > vss->xfer_enabled = true; [ ... ] > @@ -1244,14 +1269,18 @@ static int virtsnd_pcm_capture_staged_ack(struct > virtio_pcm_substream *vss, > snd_pcm_uframes_t appl_ptr; > snd_pcm_sframes_t diff; > size_t bytes; > + u64 outstanding; > u64 next_acked_total; > + bool free_running; > > if (!vss->runtime_active || !runtime || !runtime->control || > !runtime->dma_area) > return -EBADFD; > if (vss->xfer_xrun) > return -EPIPE; > - if (!virtsnd_pcm_capture_counters_valid(vss)) { > + rc = virtsnd_pcm_capture_refresh_mode_locked(vss, > + &free_running); > + if (rc || !virtsnd_pcm_capture_counters_valid(vss)) { > vss->xfer_xrun = true; > virtsnd_pcm_schedule_period_elapsed(vss); > return -EPIPE; [Severity: High] What happens if virtsnd_pcm_capture_staged_ack() in sound/virtio/virtio_pcm_ops.c detects an overflow or validation failure while the substream is in SNDRV_PCM_STATE_PAUSED? Suppose a staged capture stream runs in free-running mode until outstanding >= vss->buffer_bytes, and userspace pauses the stream via SNDRV_PCM_IOCTL_PAUSE. While paused, userspace lowers stop_threshold via SNDRV_PCM_IOCTL_SW_PARAMS (which does not call snd_pcm_update_state() because snd_pcm_running() is false) and advances appl_ptr via read(), snd_pcm_forward(), or snd_pcm_sync_ptr(). This invokes pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack() -> virtsnd_pcm_capture_staged_ack(), where virtsnd_pcm_capture_refresh_mode_locked() sees outstanding >= vss->buffer_bytes and returns -EPIPE. Unlike the SNDRV_PCM_TRIGGER_PAUSE_RELEASE error path in virtsnd_pcm_trigger(), virtsnd_pcm_capture_staged_ack() sets vss->xfer_xrun = true and schedules vss->elapsed_period without setting vss->stopped = true or substream->runtime->stop_operating = true: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_capture_staged_ack() { ... rc = virtsnd_pcm_capture_refresh_mode_locked(vss, &free_running); if (rc || !virtsnd_pcm_capture_counters_valid(vss)) { vss->xfer_xrun = true; virtsnd_pcm_schedule_period_elapsed(vss); return -EPIPE; } ... } When pcm_lib_apply_appl_ptr() receives -EPIPE, it calls __snd_pcm_xrun() -> snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN) (which is also what virtsnd_pcm_period_elapsed() in sound/virtio/virtio_pcm.c calls on its paused_xrun path). Because snd_pcm_running(substream) is false when runtime->state is SNDRV_PCM_STATE_PAUSED, snd_pcm_stop() transitions runtime->state to SNDRV_PCM_STATE_XRUN without calling ops->trigger(SNDRV_PCM_TRIGGER_STOP) or setting runtime->stop_operating = true. When userspace then calls snd_pcm_prepare() to recover from XRUN, ALSA skips both the SNDRV_PCM_STATE_PAUSED cleanup block and virtsnd_pcm_sync_stop(): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare() { ... scoped_guard(spinlock_irqsave, &vss->lock) stopped = vss->stopped; if (stopped) { dev_err(&vdev->dev, "SID %u: stream release failed\n", vss->sid); substream->runtime->stop_operating = true; return -EBADFD; } if (!vss->suspended) { if (virtsnd_pcm_msg_pending_num(vss)) { dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", vss->sid); return -EBADFD; } ... } In default native pause mode, virtsnd_pcm_msg_pending_num(vss) is still non-zero, so virtsnd_pcm_prepare() fails with -EBADFD without setting runtime->stop_operating, leaving RX buffers queued in the virtqueue across subsequent prepare(), hw_params(), hw_free(), and close() calls. In capture_pause_restart mode, vss->stopped is true, so the first virtsnd_pcm_prepare() call also fails with -EBADFD. Should virtsnd_pcm_capture_staged_ack() (or virtsnd_pcm_period_elapsed() when handling paused_xrun) also mark vss->stopped = true and runtime->stop_operating = true when transitioning a paused stream to XRUN, or should virtsnd_pcm_prepare() arm runtime->stop_operating when virtsnd_pcm_msg_pending_num(vss) is non-zero? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
