Re: [PATCH v2 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:
- [Medium] ALSA: virtio-snd: unlatched XRUN in virtsnd_pcm_trigger() on pause release
--

commit 84dba89c8170c6b6702f2442748d799772bffa04
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio-snd: fix dsnoop capture with staged buffers

This commit adds free-running staged-capture policy handling when
stop_threshold equals boundary so dsnoop capture streams do not fail with
ring_capacity rejections. It also reconstructs unread ring occupancy when
returning from free-running to bounded capture mode and returns -EPIPE if
outstanding unacknowledged data already fills the ring.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 663f7695b5be8..e7258bce2e6f0 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> +/* Caller holds vss->lock. */
> +int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss,
> +					    bool *free_running)
> +{
> +	struct snd_pcm_runtime *runtime;
> +	snd_pcm_uframes_t stop_threshold;
> +	u64 outstanding;
> +	bool new_free_running;
> +
> +	if (!vss->capture_staging || !vss->runtime_active || !vss->substream)
> +		return -EBADFD;
> +
> +	runtime = vss->substream->runtime;
> +	if (!runtime || !vss->buffer_bytes || !vss->capture_boundary)
> +		return -EBADFD;
> +
> +	/* Validate the currently committed policy before observing a transition. */
> +	if (!virtsnd_pcm_capture_counters_valid(vss))
> +		return -EIO;
> +
> +	stop_threshold = READ_ONCE(runtime->stop_threshold);
> +	new_free_running = stop_threshold == vss->capture_boundary;
> +	if (new_free_running != vss->capture_free_running) {
> +		outstanding = vss->capture_copied_total -
> +			      vss->capture_acked_total;
> +		if (new_free_running) {
> +			vss->capture_ready = 0;
> +		} else {
> +			if (outstanding >= vss->buffer_bytes)
> +				return -EPIPE;
> +			vss->capture_ready = outstanding;
> +		}
> +		vss->capture_free_running = new_free_running;
> +	}
> +
> +	*free_running = new_free_running;
> +	return 0;
> +}
[ ... ]
> +int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss)
> +{
> +	bool free_running;
> +	int rc;
> +
> +	rc = virtsnd_pcm_capture_refresh_mode_locked(vss, &free_running);
> +	if (rc)
> +		return rc;
> +
> +	return virtsnd_pcm_capture_enqueue(vss, free_running);
> +}

[Severity: Medium]
Can a staged capture substream get stuck in SNDRV_PCM_STATE_PAUSED with
vss->stopped = true and vss->xfer_xrun = false when
virtsnd_pcm_capture_refresh_mode_locked() returns -EPIPE during pause
release?

Suppose a staged capture stream (with virtsnd_capture_pause_restart=false)
runs in free-running mode until capture_copied_total - capture_acked_total >=
vss->buffer_bytes, is paused via SNDRV_PCM_IOCTL_PAUSE(1), and then has
stop_threshold updated below boundary via snd_pcm_sw_params(). Because the
substream is in SNDRV_PCM_STATE_PAUSED, snd_pcm_running(substream) is false
and snd_pcm_sw_params() skips snd_pcm_update_state().

When the stream is subsequently resumed via SNDRV_PCM_IOCTL_PAUSE(0),
snd_pcm_do_pause() calls virtsnd_pcm_trigger(substream,
SNDRV_PCM_TRIGGER_PAUSE_RELEASE), which calls virtsnd_pcm_capture_send() ->
virtsnd_pcm_capture_refresh_mode_locked() and receives -EPIPE:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() {
	...
	case SNDRV_PCM_TRIGGER_START:
	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
		...
			if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
				if (vss->capture_staging) {
					if (!vss->runtime_active)
						rc = -EBADFD;
					else
						rc = virtsnd_pcm_capture_send(vss);
				} else {
					rc = virtsnd_pcm_msg_send(vss, 0,
								  vss->buffer_bytes);
				}
			}
			if (rc) {
				vss->stopped = true;
				substream->runtime->stop_operating = true;
				return rc;
			}
}

Unlike snd_pcm_do_start(), snd_pcm_do_pause() does not transition
runtime->state to SNDRV_PCM_STATE_XRUN on -EPIPE; snd_pcm_action_single()
invokes snd_pcm_undo_pause() and leaves runtime->state in
SNDRV_PCM_STATE_PAUSED.

In sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed(), transitioning a
paused staged-capture stream to SNDRV_PCM_STATE_XRUN relies on vss->xfer_xrun
being latched and vss->elapsed_period being scheduled:

sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() {
	...
	if (!snd_pcm_running(substream)) {
		scoped_guard(spinlock_irqsave, &vss->lock) {
			runtime = substream->runtime;
			paused_xrun = runtime &&
				      runtime->state == SNDRV_PCM_STATE_PAUSED &&
				      vss->runtime_active && vss->xfer_xrun;
		}
		if (paused_xrun) {
			/* Wake readers without sending STOP for a paused stream. */
			snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
		}
		return;
	}
}

Because virtsnd_pcm_trigger() sets vss->stopped = true without setting
vss->xfer_xrun = true or scheduling vss->elapsed_period (unlike the
virtsnd_pcm_pause_restart_start() -> virtsnd_pcm_pause_restart_fail() path),
won't any reader blocked in wait_for_avail() remain asleep in
SNDRV_PCM_STATE_PAUSED while subsequent pause-release attempts fail with
-EBADFD?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025051301.54640-1-mynameisjune111@gmail.com?part=2