Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio-snd: incomplete XRUN handling in virtsnd_pcm_trigger() --
commit aaa1fb5901e460bfc006edef14710cb64e262e44 Author: Joonho Lee <[email protected]> ALSA: virtio-snd: fix dsnoop capture with staged buffers This commit adds free-running staged capture policy support when stop_threshold equals the prepared boundary so dsnoop capture streams do not fail capacity checks. It also reconstructs unread occupancy when returning to bounded capture and handles XRUN state transitions across paused streams. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index d50924b792f65..cbf4e479d295f 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; [Severity: Medium] What happens in virtsnd_pcm_trigger() if a staged capture stream using native pause (with virtsnd_capture_pause_restart disabled) hits another failure or prior XRUN during SNDRV_PCM_TRIGGER_PAUSE_RELEASE? Unlike virtsnd_pcm_pause_restart_start(), the native trigger path in virtsnd_pcm_trigger() does not check vss->xfer_xrun under vss->lock before queueing requests: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: queue = virtsnd_pcm_queue(vss); scoped_guard(spinlock_irqsave, &queue->lock) { guard(spinlock)(&vss->lock); if (vss->stopped) { substream->runtime->stop_operating = true; return -EBADFD; } if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { if (vss->capture_staging) { if (!vss->runtime_active) rc = -EBADFD; else rc = virtsnd_pcm_capture_send(vss); If vss->xfer_xrun was already latched before SNDRV_PCM_TRIGGER_PAUSE_PUSH while virtsnd_pcm_period_elapsed() was waiting on the PCM stream lock, won't SNDRV_PCM_TRIGGER_PAUSE_RELEASE succeed, set vss->xfer_enabled = true, and send VIRTIO_SND_R_PCM_START on a stream that is already in XRUN (causing subsequent completions in virtsnd_pcm_capture_complete() to be dropped)? Additionally, if virtsnd_pcm_capture_send() succeeds during SNDRV_PCM_TRIGGER_PAUSE_RELEASE, but virtsnd_pcm_ctl_msg_alloc() or virtsnd_ctl_msg_send_sync() fails afterwards, virtsnd_pcm_trigger() calls virtsnd_pcm_start_failed(vss, false): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_start_failed() { scoped_guard(spinlock_irqsave, &vss->lock) { vss->xfer_enabled = false; vss->stopped = true; if (pause_restart) vss->xfer_xrun = true; } vss->substream->runtime->stop_operating = true; } Because pause_restart is false here (and the early vss->stopped check also returns -EBADFD directly), neither path sets vss->xfer_xrun = true or calls virtsnd_pcm_schedule_period_elapsed(vss). When ALSA undoes the failed pause release and leaves runtime->state in SNDRV_PCM_STATE_PAUSED with vss->stopped == true, could readers blocked in wait_for_avail() or snd_pcm_poll() be left sleeping without virtsnd_pcm_period_elapsed() ever moving the stream to SNDRV_PCM_STATE_XRUN? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
