Re: [PATCH v8 09/14] ALSA: virtio: add staged capture accounting and lifecycle recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_prepare_locked() fails after playback pre-buffering
- [High] ALSA: virtio: virtsnd_pcm_trigger() sends PCM_STOP when stream is not running
- [High] ALSA: virtio: virtsnd_pcm_start_failed() marks unprepared stream stopped
- [Medium] ALSA: virtio: missed period wakeup in virtsnd_pcm_period_elapsed()
- [Medium] ALSA: virtio: unreleased device stream on failed re-prepare in virtsnd_pcm_prepare_locked()
- [Medium] ALSA: virtio: reset_epoch TOCTOU in virtsnd_pcm_send_sync() helpers
- [Low] ALSA: virtio: virtsnd_pcm_trigger() mixes goto cleanup with guard() helpers
--

commit 775188574a684cd4f7321eccd1403d5cf535ff0a
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: add staged capture accounting and lifecycle recovery

This commit introduces staged capture with separate produced, observed, and
consumed positions using request-owned buffers, along with a free-running
capture policy for boundary stop thresholds. It also adds a lifecycle mutex,
reset epoch tracking, and an opt-in pause-restart state machine to handle
control transitions and device reset recovery.

> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index be3893de40a55..ecd017edba72c 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -310,8 +310,98 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
>  {
>  	struct virtio_pcm_substream *vss =
>  		container_of(work, struct virtio_pcm_substream, elapsed_period);
> +	struct snd_pcm_substream *substream = vss->substream;
> +	struct snd_pcm_runtime *runtime;
> +	size_t period_bytes;
> +	u64 period_floor;
> +	u64 snapshot;
> +	bool staged_capture;
> +	bool paused_xrun = false;
> +	bool notify = false;
> +	bool requeue = false;

[ ... ]

> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		runtime = substream->runtime;
> +		if (!vss->runtime_active ||
> +		    (!vss->xfer_enabled && !vss->xfer_xrun) || !runtime)
> +			return;
> +
> +		period_bytes = snd_pcm_lib_period_bytes(substream);
> +		if (vss->xfer_xrun) {
> +			notify = true;
> +		} else if (!period_bytes || period_bytes >= vss->buffer_bytes ||
> +			   !virtsnd_pcm_capture_counters_valid(vss) ||
> +			   vss->capture_notified_periods >
> +				div64_ul(vss->capture_copied_total, period_bytes)) {
> +			vss->xfer_xrun = true;
> +			notify = true;
> +		} else {
> +			period_floor = div64_ul(vss->capture_copied_total, period_bytes);
> +			if (period_floor > vss->capture_notified_periods) {
> +				/* Reserve crossed boundaries before the callback. */
> +				vss->capture_notified_periods = period_floor;
> +				notify = true;
> +			}
> +		}
> +
> +		if (notify) {
> +			snapshot = vss->capture_copied_total;
> +			vss->capture_callback_snapshot = snapshot;
> +			vss->capture_callback_snapshot_active = true;
> +		}
> +	}

[Severity: Medium]
When virtsnd_capture_staging and virtsnd_capture_allow_short are enabled, can
gating period notifications strictly on integer multiples of period_bytes in
virtsnd_pcm_period_elapsed() miss a period wakeup and trigger a ring_capacity
xrun?

When virtsnd_pcm_period_elapsed() sets vss->capture_callback_snapshot to
vss->capture_copied_total (or when virtsnd_pcm_cp_pointer() is called
directly), any sub-period tail is exposed to the reader and acknowledged via
virtsnd_pcm_cp_ack() -> virtsnd_pcm_capture_staged_ack().

If a subsequent short completion in virtsnd_pcm_capture_complete() crosses the
next integer multiple of period_bytes while total unacknowledged data
(vss->capture_copied_total - vss->capture_acked_total) is still less than one
period, virtsnd_pcm_period_elapsed() advances vss->capture_notified_periods to
period_floor and calls snd_pcm_period_elapsed_under_stream_lock(), which leaves
the reader sleeping because available frames are below avail_min.

If another short completion then arrives within that same period_floor
interval and brings unacknowledged bytes in vss->capture_ready to at least
period_bytes without crossing the next integer period boundary,
period_floor > vss->capture_notified_periods is false and notify remains
false. On a two-period buffer, leaving at least period_bytes unacknowledged in
vss->capture_ready causes the next full-period completion in
virtsnd_pcm_capture_complete() to fail:

	else if (!free_running &&
		 audio_bytes >= vss->buffer_bytes - vss->capture_ready)
		invalid_reason = "ring_capacity";

Should virtsnd_pcm_period_elapsed() also notify when unacknowledged bytes
reach period_bytes even if period_floor has not advanced?

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index f74bf365065b6..66560d82b7f8e 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -97,14 +128,238 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
>  };
>  
>  static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream);
> +static int virtsnd_pcm_sync_stop_locked(struct snd_pcm_substream *substream);
> +static int virtsnd_pcm_pause_restart_sync_stop(struct snd_pcm_substream *substream);
>  
> -static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss)
> +static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss, u64 epoch)
>  {
> +	bool notify;
> +
>  	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting || vss->reset_epoch != epoch)
> +			return;
>  		vss->xfer_enabled = false;
>  		vss->stopped = true;
> +		notify = vss->runtime_active && vss->capture_staging &&
> +			 vss->direction == SNDRV_PCM_STREAM_CAPTURE;
> +		if (notify)
> +			vss->xfer_xrun = true;
>  	}
>  	vss->substream->runtime->stop_operating = true;
> +	if (notify)
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +}

[Severity: High]
Does unconditionally setting vss->stopped = true in virtsnd_pcm_start_failed()
cause virtsnd_pcm_sync_stop_locked() to send an invalid release command when a
stream is not prepared on the device?

After a device reset and restore (virtsnd_reset_vqs() -> virtsnd_restore()),
or after a failed re-prepare in virtsnd_pcm_prepare_locked(), vss->prepared is
false, vss->stopped is false, and vss->msg_count is 0, while the substream
runtime state can still be paused or prepared.

If userspace then drops, prepares, or closes a paused substream (calling
snd_pcm_pause() -> virtsnd_pcm_trigger()), or starts a prepared substream,
virtsnd_pcm_trigger() sees !vss->prepared and jumps to start_failed:

	if (vss->stopped || !vss->prepared || vss->xfer_xrun) {
		rc = vss->xfer_xrun ? -EPIPE : -EBADFD;
		goto start_failed;
	}

virtsnd_pcm_start_failed() then sets vss->stopped = true and
runtime->stop_operating = true even though the device stream was never
prepared in the current epoch and has no queued messages. When
virtsnd_pcm_sync_stop_locked() subsequently runs, need_release evaluates to
true because vss->stopped is set, so it sends a release control message for an
unprepared stream. If the device rejects that release request,
virtsnd_pcm_sync_stop_locked() returns an error with vss->stopped still true,
wedging subsequent prepare, hw_params, and open calls.

Should virtsnd_pcm_start_failed() only set vss->stopped and
runtime->stop_operating when vss->prepared || vss->msg_count is true?

[ ... ]

> +static int virtsnd_pcm_send_sync(struct virtio_pcm_substream *vss,
> +				 struct virtio_snd_msg *msg)
> +{
> +	u64 epoch;
> +	int rc;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting) {
> +			virtsnd_ctl_msg_unref(msg);
> +			return -ECANCELED;
> +		}
> +		epoch = vss->reset_epoch;
> +	}
> +	rc = virtsnd_ctl_msg_send_sync(vss->snd, msg);
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (epoch != vss->reset_epoch || vss->resetting)
> +			return -ECANCELED;
> +	}
> +	return rc;
> +}
> +
> +static int
> +virtsnd_pcm_pause_restart_send(struct virtio_pcm_substream *vss,
> +			       struct virtio_snd_msg *msg,
> +			       unsigned long allowed_states,
> +			       enum virtio_pcm_pause_restart_state submitted_state,
> +			       enum virtio_pcm_pause_restart_state success_state)
> +{
> +	enum virtio_pcm_pause_restart_state state;
> +	bool uncertain;
> +	u64 epoch;
> +	int rc;
> +
> +	if (!virtsnd_pcm_pause_restart_enabled(vss))
> +		return virtsnd_pcm_send_sync(vss, msg);
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		epoch = vss->reset_epoch;
> +		state = vss->pause_restart_state;
> +		if (vss->resetting) {
> +			rc = -ECANCELED;
> +		} else if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED) {
> +			rc = -EIO;
> +		} else if (virtsnd_pcm_pause_restart_transitioning(state)) {
> +			vss->pause_restart_state =
> +				VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
> +			rc = -EIO;
> +		} else if (!(allowed_states & BIT(state))) {
> +			rc = -EBADFD;
> +		} else {
> +			vss->pause_restart_state = submitted_state;
> +			rc = 0;
> +		}
> +	}

[Severity: Medium]
Can re-sampling vss->reset_epoch inside virtsnd_pcm_send_sync() and
virtsnd_pcm_pause_restart_send() allow a control request from before a device
reset to be sent in the new epoch?

Callers such as virtsnd_pcm_trigger(), virtsnd_pcm_prepare_locked(),
virtsnd_pcm_sync_stop_locked(), virtsnd_pcm_pause_release_flush(),
virtsnd_pcm_pause_restart_start(), virtsnd_pcm_pause_restart_pause(), and
virtsnd_pcm_pause_restart_sync_stop() check substream state (and in the start
paths, queue virtqueue buffers) under vss->lock while recording
epoch = vss->reset_epoch, then drop vss->lock to call cancel_work_sync() or
virtsnd_pcm_ctl_msg_alloc() before calling virtsnd_pcm_send_sync() or
virtsnd_pcm_pause_restart_send().

If virtsnd_reset_vqs() and virtsnd_restore() both complete during that
unlocked window, vss->resetting is false again and vss->reset_epoch has been
incremented. Because virtsnd_pcm_send_sync() and
virtsnd_pcm_pause_restart_send() overwrite their local epoch variable with the
new vss->reset_epoch instead of checking the caller's epoch, they can send a
stale start, stop, release, or prepare command to the freshly restored device,
or fail the state check against the reset pause_restart_state with -EBADFD
instead of -ECANCELED.

Should virtsnd_pcm_send_sync() and virtsnd_pcm_pause_restart_send() take the
caller's expected epoch parameter and return -ECANCELED if vss->reset_epoch no
longer matches?

[ ... ]

> @@ -277,27 +587,90 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,

[ ... ]

>  static int virtsnd_pcm_prepare_locked(struct snd_pcm_substream *substream)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
>  	struct virtio_device *vdev = vss->snd->vdev;
> -	struct virtio_snd_msg *msg;
> +	bool pause_restart = virtsnd_pcm_pause_restart_enabled(vss);
> +	bool free_running;
> +	int rc;
>  	bool stopped;
> +	bool suspended;
> +	u64 epoch;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting)
> +			return -ECANCELED;
> +		stopped = vss->stopped;
> +	}
> +	if (stopped) {\n+		rc = virtsnd_pcm_sync_stop_locked(substream);
> +		if (rc)
> +			return rc;
> +	}
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		vss->prepared = false;
> +		epoch = vss->reset_epoch;
> +		suspended = vss->suspended;
> +	}
> +
> +	if (pause_restart) {
> +		enum virtio_pcm_pause_restart_state state;
> +
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			state = vss->pause_restart_state;
> +		if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
> +		    virtsnd_pcm_pause_restart_transitioning(state)) {
> +			virtsnd_pcm_pause_restart_fail(vss, true, epoch);
> +			return -EIO;
> +		}
> +	}
>  
>  	scoped_guard(spinlock_irqsave, &vss->lock)
>  		stopped = vss->stopped;
> @@ -308,44 +681,320 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  		return -EBADFD;
>  	}
>  
> -	if (!vss->suspended) {
> +	if (!suspended) {
>  		if (virtsnd_pcm_msg_pending_num(vss)) {
>  			dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
>  				vss->sid);
>  			return -EBADFD;
>  		}
>  
> -		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> -		vss->hw_ptr = 0;

[Severity: High]
What happens in virtsnd_pcm_prepare_locked() if a playback stream queues
at least one period while in the prepared state before reaching
runtime->start_threshold, and userspace then calls snd_pcm_drop() followed by
snd_pcm_prepare() (or calls snd_pcm_prepare() directly)?

During playback pre-buffering, pcm_lib_apply_appl_ptr() ->
virtsnd_pcm_pb_ack() -> snd_pcm_indirect_playback_transfer() ->
virtsnd_pcm_trans_copy() -> virtsnd_pcm_msg_send() submits full periods to the
tx virtqueue and increments vss->msg_count while the stream is still in the
prepared state and vss->stopped is false. Because snd_pcm_do_stop() only
invokes trigger(stop) when the stream is running or paused, vss->stopped
remains false.

When virtsnd_pcm_prepare_locked() runs (and similarly in
virtsnd_pcm_sync_stop()), it only checks if (stopped) before calling
virtsnd_pcm_sync_stop_locked(substream), skipping the release flush even
though vss->msg_count > 0. It then clears vss->prepared = false and fails the
virtsnd_pcm_msg_pending_num(vss) check with -EBADFD, leaving pending messages
in the queue with vss->stopped == false and vss->prepared == false so
subsequent prepare or write calls cannot recover until hw_params or hw_free.

Should virtsnd_pcm_prepare_locked() and virtsnd_pcm_sync_stop() also run
virtsnd_pcm_sync_stop_locked() when vss->msg_count > 0?

>  	} else {
>  		struct snd_pcm_runtime *runtime = substream->runtime;
>  		unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
>  		unsigned int period_bytes = snd_pcm_lib_period_bytes(substream);
> -		int rc;
>  
>  		rc = virtsnd_pcm_dev_set_params(vss, buffer_bytes, period_bytes,
>  						runtime->channels,
>  						runtime->format, runtime->rate);
> -		if (rc)
> +		if (rc) {
> +			if (pause_restart && rc != -ECANCELED)
> +				virtsnd_pcm_pause_restart_fail(vss, false, epoch);
>  			return rc;
> +		}
>  	}
>  
> -	vss->xfer_xrun = false;
> -	vss->suspended = false;
> -	vss->msg_count = 0;
> -	virtsnd_pcm_msg_reset(vss);
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (epoch != vss->reset_epoch || vss->resetting)
> +			return -ECANCELED;
> +		vss->prepared = false;
> +		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> +		vss->hw_ptr = 0;
> +		if (!pause_restart)
> +			vss->xfer_xrun = false;
> +		vss->suspended = false;
> +		vss->msg_count = 0;
> +		virtsnd_pcm_msg_reset(vss);
> +		memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
> +		vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes;
> +		vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes;
> +		if (vss->capture_staging) {
> +			vss->capture_boundary = substream->runtime->boundary;
> +			rc = virtsnd_pcm_capture_refresh_mode_locked(vss,
> +							     &free_running);
> +			if (rc)
> +				return rc;
> +		}
> +	}
>  
> -	memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
> -	vss->pcm_indirect.sw_buffer_size =
> -		vss->pcm_indirect.hw_buffer_size =
> -		snd_pcm_lib_buffer_bytes(substream);
> +	rc = virtsnd_pcm_dev_prepare(vss);
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (epoch != vss->reset_epoch || vss->resetting)
> +			return -ECANCELED;
> +		vss->prepared = !rc;
> +	}

[Severity: Medium]
In virtsnd_pcm_prepare_locked(), if the substream was already prepared on the
device (vss->prepared was true, vss->stopped was false) and userspace calls
prepare again without pause_restart enabled, what happens if
virtsnd_pcm_dev_prepare() fails (or if virtsnd_pcm_dev_set_params() or
virtsnd_pcm_capture_refresh_mode_locked() fails earlier)?

Because vss->prepared is cleared to false at the start of
virtsnd_pcm_prepare_locked() without recording that the device stream was
already prepared, a failure when !pause_restart leaves both vss->prepared and
vss->stopped false. When virtsnd_pcm_hw_free() later runs on close:

	scoped_guard(spinlock_irqsave, &vss->lock) {
		vss->stopped |= vss->prepared;
		vss->prepared = false;
		vss->runtime_active = false;
		vss->xfer_enabled = false;
	}
	rc = virtsnd_pcm_sync_stop_locked(substream);

virtsnd_pcm_sync_stop_locked() sees vss->prepared, vss->stopped, and
vss->msg_count all false/zero and skips sending the release command to the
device. Should a failed re-prepare preserve the need to release a previously
prepared device stream (or release it before re-preparing)?

[ ... ]

> @@ -372,38 +1042,51 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)

[ ... ]

>  	case SNDRV_PCM_TRIGGER_SUSPEND:
> -		vss->suspended = true;
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			vss->suspended = true;
>  		fallthrough;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		scoped_guard(spinlock_irqsave, &vss->lock) {
> +			if (!vss->prepared)
> +				return 0;
>  			if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH)
>  				vss->stopped = true;
>  			vss->xfer_enabled = false;
>  		}
>  
>  		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
>  						GFP_KERNEL);
>  		if (!msg)
>  			return -ENOMEM;
>  
> -		return virtsnd_ctl_msg_send_sync(snd, msg);
> +		return virtsnd_pcm_send_sync(vss, msg);

[Severity: High]
When virtsnd_pcm_pause_restart_enabled(vss) is false, can the stop and
pause_push cases in virtsnd_pcm_trigger() send a stop control request when the
device stream is not running?

Unlike virtsnd_pcm_pause_restart_stop(), which returns 0 when the state is not
running, this path only checks if (!vss->prepared). That appears to allow
sending an invalid or duplicate stop request in two situations:

1. When a running stream is paused via pause_push, virtsnd_pcm_trigger() sends
a stop request to the device while leaving vss->prepared = true and setting
vss->xfer_enabled = false. If that paused stream is then stopped (for example
via snd_pcm_drop(), close, or paused xrun handling in
virtsnd_pcm_period_elapsed() -> snd_pcm_stop()), snd_pcm_do_stop() invokes
virtsnd_pcm_trigger() with the stop command while vss->prepared is still true,
sending a second stop request to a stream that is already stopped on the
device.

2. If start or pause_release fails in virtsnd_pcm_trigger() on an unlinked
stream, virtsnd_pcm_start_failed() leaves vss->prepared = true. In
snd_pcm_action_single(), the error path calls ops->undo_action()
(snd_pcm_undo_start() or snd_pcm_undo_pause()), which invokes
virtsnd_pcm_trigger() with stop or pause_push and sends a stop request even
though start failed.

Also, if pause_push fails in virtsnd_pcm_ctl_msg_alloc() or
virtsnd_pcm_send_sync(), vss->xfer_enabled has already been cleared to false
without marking the stream stopped, and snd_pcm_action_single() will call
snd_pcm_undo_pause() -> virtsnd_pcm_trigger(pause_release) on the still-running
device stream. Should the non-pause-restart stop/pause path check whether the
stream was actually running before sending the stop request, and call
virtsnd_pcm_start_failed() if sending stop for pause_push fails?

>  	default:
>  		return -EINVAL;
>  	}
> +
> +start_failed:
> +	virtsnd_pcm_start_failed(vss, epoch);
> +	return rc;
> +}

[Severity: Low]
In virtsnd_pcm_trigger(), the start and pause_release case uses
scoped_guard(spinlock_irqsave, &queue->lock) and guard(spinlock)(&vss->lock)
and jumps out of the guarded scope via goto start_failed. Since
include/linux/cleanup.h discourages mixing goto labels with guard() and
scoped_guard() in the same function, could virtsnd_pcm_trigger() break out of
the scoped_guard block and handle the error without a goto label?

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