Re: [PATCH v7 11/16] ALSA: virtio-snd: fix dsnoop capture with staged buffers

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261028094132.20364-1-mynameisjune111@gmail.com?part=11