Re: [PATCH v3 2/2] 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:
- [High] ALSA: virtio-snd: missing stop_operating when paused capture hits XRUN
--

commit 6569a6d061b2d8bd7d031671c88c23ff7eb99711
Author: Joonho Lee <mynameisjune111@gmail.com>

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