Re: [PATCH v13 10/12] ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Medium] ALSA: virtio: missing stop_operating arming on -EPIPE in ACK and start paths
- [Medium] ALSA: virtio: skipped NOMMU post-STOP copy in virtsnd_pcm_msg_complete()
- [Medium] ALSA: virtio: cancel_work_sync() in virtsnd_reset_vqs() stalls ctl cleanup
- [Medium] ALSA: virtio: missing stop_operating on XRUN in pause_restart trigger
- [Medium] ALSA: virtio: delayed preparing gate and stale release_required in prepare
- [Medium] ALSA: virtio: virtsnd_pcm_pause_restart_stop() skips setting suspended
- [Medium] ALSA: virtio: virtsnd_pcm_indirect_reset() skips cursor rebase on error
- [Medium] ALSA: virtio: virtsnd_pcm_capture_staged_ack() rejects sub-byte carryover
- [Medium] ALSA: virtio: unaligned byte ACK in virtsnd_pcm_capture_staged_ack()
--

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

ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

This commit introduces request-owned staged capture buffers with separate
produced, observed, and consumed position tracking, and serializes PCM
lifecycle transitions and ACK accounting using lifecycle_lock and the PCM
stream lock. It also adds reset callbacks for playback and capture streams
and refines XRUN and teardown handling across device resets and pause
transitions.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index aec4959c973ca..f32e9fb9e8b1d 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -382,8 +399,16 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd)
>  		queue->vqueue = NULL;
>  		spin_unlock_irqrestore(&queue->lock, flags);
>  	}
> -
>  	vdev->config->del_vqs(vdev);
> +	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
> +		struct virtio_pcm_substream *vss = &snd->substreams[i];
> +
> +		cancel_work_sync(&vss->elapsed_period);

[Severity: Medium]
Can calling cancel_work_sync(&vss->elapsed_period) inside virtsnd_reset_vqs()
stall freeze or remove for the full virtsnd_msg_timeout_ms duration?

In virtsnd_freeze() and virtsnd_remove():

  virtsnd_ctl_msg_cancel_all(snd);
  virtsnd_reset_vqs(snd);
  virtsnd_ctl_msg_cleanup(snd);

virtsnd_ctl_msg_cancel_all() marks pending control messages with
msg->cancelled = true, but does not complete msg->notify; waking the waiters
only happens later in virtsnd_ctl_msg_cleanup() after virtsnd_reset_vqs()
returns.

Because vpcm->pcm->nonatomic is true, virtsnd_pcm_period_elapsed() acquires
the sleeping PCM stream lock via guard(pcm_stream_lock_irqsave)(substream).
If another thread holds that PCM stream lock while waiting in
virtsnd_ctl_msg_send() on an already-submitted control message (such as in
virtsnd_pcm_trigger()), or if virtsnd_pcm_period_elapsed() itself entered
snd_pcm_stop() -> virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP) ->
virtsnd_ctl_msg_send_sync() just before virtsnd_reset_vqs() set
vss->resetting = true, the device has already been reset and will never
complete that control message.

Waiting in cancel_work_sync(&vss->elapsed_period) before
virtsnd_ctl_msg_cleanup(snd) prevents msg->notify from being completed until
wait_for_completion_timeout(&msg->notify, js) expires. Should control message
waiters be woken before synchronously canceling elapsed_period work?

[ ... ]

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index b94ab24f5ffef..d6510d813f3ca 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -540,19 +880,52 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  				     size_t written_bytes)
>  {
>  	struct virtio_pcm_substream *vss = msg->substream;
> +	size_t completed_bytes;
> +	u32 latency_bytes;
>  
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    vss->capture_staging) {
> +		virtsnd_pcm_capture_complete(msg, written_bytes);
> +		return;
> +	}
> +
> +	/* Only completed TX requests and valid RX audio advance the pointer. */
>  	guard(spinlock)(&vss->lock);
> +	/* Empty STOP returns retire ownership without inventing captured data. */
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    (!vss->prepared ||
> +	     (vss->stopped && written_bytes <= sizeof(*msg->status)))) {
> +		msg->length = 0;
> +		vss->msg_count--;
> +		if (!vss->msg_count)
> +			wake_up_all(&vss->msg_empty);
> +		return;
> +	}
> +	completed_bytes = msg->length;
> +	latency_bytes = le32_to_cpu(msg->status->latency_bytes);
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +		struct snd_pcm_runtime *runtime = vss->substream->runtime;
> +		size_t frame_bytes = runtime->frame_bits / gcd(runtime->frame_bits, 8);
> +
> +		if (virtsnd_pcm_capture_status(msg, written_bytes, frame_bytes,
> +					       &completed_bytes, &latency_bytes) ||
> +		    !completed_bytes) {
> +			vss->xfer_xrun = true;
> +			msg->length = 0;
> +			vss->msg_count--;
> +			if (!vss->msg_count)
> +				wake_up_all(&vss->msg_empty);
> +			virtsnd_pcm_schedule_period_elapsed(vss);
> +			return;
> +		}
> +	}
>  #ifndef CONFIG_MMU
>  	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> -	    vss->xfer_enabled && written_bytes > sizeof(msg->status)) {
> +	    vss->prepared && !vss->stopped &&
> +	    written_bytes > sizeof(*msg->status)) {
>  		struct snd_pcm_runtime *runtime =
>  			vss->substream ? vss->substream->runtime : NULL;
> -		size_t reported_bytes = written_bytes - sizeof(msg->status);
> +		size_t reported_bytes = written_bytes - sizeof(*msg->status);
>  		size_t data_bytes = min_t(size_t, reported_bytes, msg->length);
>  
>  		if (!runtime || !runtime->dma_area || !vss->owned_dma_area ||
> @@ -568,26 +941,16 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  			       vss->owned_dma_area + msg->data_offset, data_bytes);
>  	}
>  #endif
> +	vss->hw_ptr += completed_bytes;
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +		/* A full unseen ring cannot be represented by the modulo pointer. */
> +		if (check_add_overflow(vss->capture_copied_total,
> +				       (u64)completed_bytes,
> +				       &vss->capture_copied_total) ||

[Severity: Medium]
On !CONFIG_MMU systems, does checking !vss->stopped before copying from
vss->owned_dma_area leave stale data in runtime->dma_area on post-STOP
capture completions?

At the top of virtsnd_pcm_msg_complete(), post-STOP capture completions with
written_bytes > sizeof(*msg->status) are intentionally kept so that in-flight
payloads (such as during capture SNDRV_PCM_STATE_DRAINING after
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP) sets vss->stopped = true) advance
vss->hw_ptr and vss->capture_copied_total by completed_bytes.

However, the #ifndef CONFIG_MMU bounce-buffer copy block is gated on
vss->prepared && !vss->stopped. When vss->stopped is true, memcpy() into
runtime->dma_area is skipped even though vss->hw_ptr and
vss->capture_copied_total are advanced, exposing stale or zeroed buffer
contents to userspace. Should the !vss->stopped check be removed from the
!CONFIG_MMU copy condition (and data_bytes use completed_bytes)?

[ ... ]

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index f4b45bddfb09c..05b38d8a67d7a 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -288,28 +622,95 @@ 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 stopped;
> +	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;
> +		epoch = vss->reset_epoch;
> +		stopped = vss->stopped || vss->release_required || vss->msg_count;
> +	}
> +	if (stopped) {
> +		rc = virtsnd_pcm_sync_stop_locked(substream);
> +		if (rc)
> +			return rc;
> +	}
> +	scoped_guard(pcm_stream_lock_irqsave, substream) {
> +		guard(spinlock_irqsave)(&vss->lock);
> +		if (vss->resetting || vss->reset_epoch != epoch)
> +			return -ECANCELED;
> +		vss->prepared = false;
> +		vss->preparing = true;
> +		suspended = vss->suspended;
> +	}

[Severity: Medium]
When re-preparing a substream that is already in SNDRV_PCM_STATE_PREPARED,
can a concurrent SNDRV_PCM_IOCTL_SYNC_PTR ioctl race with
virtsnd_pcm_sync_stop_locked() before vss->preparing is set?

When snd_pcm_prepare() is called on a prepared substream (where
vss->release_required is true), ALSA core holds runtime->buffer_mutex but not
the PCM stream lock, and runtime->state remains SNDRV_PCM_STATE_PREPARED
until snd_pcm_post_prepare().

In virtsnd_pcm_prepare_locked(), virtsnd_pcm_sync_stop_locked() runs before
the PCM stream lock is taken to set vss->preparing = true and
vss->prepared = false. Inside virtsnd_pcm_sync_stop_locked() (and
virtsnd_pcm_pause_restart_sync_stop()), vss->stopped is set to true while
vss->prepared remains true across the sleeping VIRTIO_SND_R_PCM_RELEASE
request:

  virtsnd_pcm_prepare_locked()
    virtsnd_pcm_sync_stop_locked()
      vss->stopped = true;
      /* vss->prepared is still true, vss->preparing is still false */
      virtsnd_pcm_send_sync(vss, msg); /* sleeps */

If another thread calls snd_pcm_sync_ptr() -> pcm_lib_apply_appl_ptr() during
this window, it acquires the PCM stream lock, passes virtsnd_pcm_ack_state(),
sees vss->preparing == false, vss->prepared == true, and vss->stopped == true
in virtsnd_pcm_pb_ack_locked() or virtsnd_pcm_cp_ack_locked(), and returns
-EPIPE, causing pcm_lib_apply_appl_ptr() to call __snd_pcm_xrun(substream)
mid-prepare.

Additionally, virtsnd_pcm_prepare_locked() sets vss->release_required = true
even when pause_restart is enabled, but neither virtsnd_pcm_release_wait()
nor virtsnd_pcm_pause_restart_sync_stop() clears vss->release_required,
causing stopped to be true on every subsequent prepare. Should vss->preparing
be armed under the PCM stream lock before calling
virtsnd_pcm_sync_stop_locked(), and should vss->release_required be cleared
when pause_restart releases the stream?

[ ... ]

> +static int virtsnd_pcm_pause_restart_start(struct virtio_pcm_substream *vss,
> +					   bool resume)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
>  	struct virtio_snd_msg *msg;
> +	enum virtio_pcm_pause_restart_state state;
> +	bool reset_required;
> +	bool invalid_state;
> +	bool xrun;
>  	int rc = 0;
> +	u64 epoch;
>  
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting)
> +			return -ECANCELED;
> +		epoch = vss->reset_epoch;
> +		state = vss->pause_restart_state;
> +		xrun = vss->xfer_xrun;
[ ... ]
> +	}
> +	if (reset_required) {
> +		virtsnd_pcm_pause_restart_fail(vss, true, epoch);
> +		return -EIO;
> +	}
> +	if (xrun)
> +		return -EPIPE;
[ ... ]
> +static int virtsnd_pcm_pause_restart_pause(struct virtio_pcm_substream *vss)
> +{
> +	struct virtio_snd_msg *msg;
> +	enum virtio_pcm_pause_restart_state state;
> +	int rc;
> +	u64 epoch;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting)
> +			return -ECANCELED;
> +		epoch = vss->reset_epoch;
> +		state = vss->pause_restart_state;
> +		if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) {
> +			if (vss->xfer_xrun)
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			return vss->xfer_xrun ? -EPIPE : 0;
> +		}

[Severity: Medium]
In virtsnd_pcm_pause_restart_start() and virtsnd_pcm_pause_restart_pause(),
does returning -EPIPE on a latched XRUN without arming vss->stopped and
runtime->stop_operating prevent subsequent DROP or prepare teardown from
running sync_stop?

In virtsnd_pcm_pause_restart_start(), if xrun is true on entry, it returns
-EPIPE directly without calling virtsnd_pcm_pause_restart_failed(vss, epoch)
(unlike the later vss->xfer_xrun check when queueing capture buffers, and
unlike the native trigger path which calls virtsnd_pcm_start_failed()). When
SNDRV_PCM_TRIGGER_START fails with -EPIPE on a linked substream,
snd_pcm_do_start() sets runtime->state to SNDRV_PCM_STATE_XRUN and
snd_pcm_action_group() only runs undo_action on group members preceding the
failed stream, leaving vss->stopped and runtime->stop_operating false on the
failed stream so SNDRV_PCM_IOCTL_DROP will not release the prepared stream on
the device.

Similarly, if a paused stream has vss->xfer_xrun latched and snd_pcm_drop()
or snd_pcm_prepare() calls snd_pcm_pause(substream, false) under the PCM
stream lock, virtsnd_pcm_pause_restart_start() returns -EPIPE and
snd_pcm_undo_pause() -> virtsnd_pcm_pause_restart_pause() also returns -EPIPE
in the VIRTSND_PCM_PAUSE_RESTART_RELEASED branch without setting
runtime->stop_operating = true (unlike the
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT branch just below it). Because
runtime->state remains SNDRV_PCM_STATE_PAUSED and runtime->stop_operating is
false, both snd_pcm_do_stop() and snd_pcm_sync_stop() skip teardown.

Should both of these -EPIPE paths call virtsnd_pcm_pause_restart_failed(vss,
epoch) (or arm vss->stopped and runtime->stop_operating)?

[ ... ]

> +static int virtsnd_pcm_pause_restart_stop(struct virtio_pcm_substream *vss,
> +					  bool suspend)
> +{
> +	struct virtio_snd_msg *msg;
> +	enum virtio_pcm_pause_restart_state state;
> +	int rc;
> +	u64 epoch;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting)
> +			return -ECANCELED;
> +		epoch = vss->reset_epoch;
> +		if (!vss->prepared && !vss->msg_count)
> +			return 0;
> +		if (suspend)
> +			vss->suspended = true;

[Severity: Medium]
In virtsnd_pcm_pause_restart_stop(), if suspend is true while !vss->prepared
&& !vss->msg_count, won't returning early skip setting vss->suspended = true?

In the native trigger path in virtsnd_pcm_trigger(), SNDRV_PCM_TRIGGER_SUSPEND
sets vss->suspended = true before checking if (!vss->prepared) return 0. Here
in virtsnd_pcm_pause_restart_stop(), if (!vss->prepared && !vss->msg_count)
returns 0 before setting vss->suspended = true. If a stream in pause_restart
mode has vss->prepared cleared and messages drained (for example, while paused
in VIRTSND_PCM_PAUSE_RESTART_RELEASED or after a failed pause transition) and
then receives SNDRV_PCM_TRIGGER_SUSPEND, vss->suspended remains false, so
staged pointer/ACK callbacks will not observe the suspended state and a
subsequent prepare will skip re-sending VIRTIO_SND_R_PCM_SET_PARAMS. Should
if (suspend) vss->suspended = true precede the early return check?

[ ... ]

> -static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
> +static int virtsnd_pcm_pb_ack_locked(struct snd_pcm_substream *substream)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
>  	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
>  	struct virtsnd_pcm_transfer transfer = {};
>  	int rc;
>  
> -	rc = virtsnd_pcm_ack_state(substream);
> -	if (rc)
> -		return rc;
> -
>  	scoped_guard(spinlock_irqsave, &queue->lock) {
>  		guard(spinlock)(&vss->lock);
> +		if (vss->preparing)
> +			return -EBADFD;
> +		if (vss->xfer_xrun)
> +			return -EPIPE;
> +		if (!vss->prepared)
> +			return -EBADFD;
>  		transfer.indirect = vss->pcm_indirect;
>  		if (vss->stopped) {
>  			rc = -EPIPE;
> @@ -626,21 +1609,163 @@ static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
>  	return 0;
>  }
[ ... ]
> +static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
> +					  struct snd_pcm_runtime *runtime)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	int rc = 0;
> +
> +	/*
> +	 * The RX callback holds the queue lock while draining completions.
> +	 * Account ALSA's read under the substream lock alone so completions later
> +	 * in that drain observe the newly released ring space.
> +	 */
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		snd_pcm_uframes_t appl_ptr;
> +		snd_pcm_sframes_t diff;
> +		size_t bytes;
> +		u64 outstanding;
> +		u64 next_acked_total;
> +		bool free_running;
[ ... ]
> +		appl_ptr = runtime->control->appl_ptr;
> +		diff = appl_ptr - vss->pcm_indirect.appl_ptr;
> +		if (diff < -(snd_pcm_sframes_t)(runtime->boundary / 2))
> +			diff += runtime->boundary;
> +		if (diff < 0 ||
> +		    (snd_pcm_uframes_t)diff > runtime->buffer_size) {
> +			vss->xfer_xrun = true;
> +			virtsnd_pcm_schedule_period_elapsed(vss);
> +			return -EPIPE;
> +		}

[Severity: Medium]
Can carrying over an unconsumed sub-byte frame in vss->pcm_indirect.appl_ptr
cause a subsequent full-buffer ACK to fail this diff > runtime->buffer_size
check with a false -EPIPE?

For a sub-byte frame format such as 1-channel SNDRV_PCM_FORMAT_IMA_ADPCM
(where frame_bits == 4), acknowledging 1 frame yields
bytes = frames_to_bytes(runtime, 1) = 0, leaving vss->pcm_indirect.appl_ptr
lagging runtime->control->appl_ptr (old_appl_ptr) by 1 frame.

If the application then advances appl_ptr by runtime->buffer_size frames in
free-running mode (which pcm_lib_apply_appl_ptr() allows because
appl_ptr - old_appl_ptr == runtime->buffer_size, and which equals
vss->buffer_bytes in bytes), diff = appl_ptr - vss->pcm_indirect.appl_ptr
becomes runtime->buffer_size + 1. Checking
(snd_pcm_uframes_t)diff > runtime->buffer_size then rejects the ACK with
-EPIPE and latches vss->xfer_xrun before frames_to_bytes(runtime, diff) is
computed.

> +
> +		/* ACK includes any subperiod tail already copied to the ring. */
> +		bytes = frames_to_bytes(runtime, diff);
> +		outstanding = vss->capture_copied_total -
> +			      vss->capture_acked_total;
> +		if ((!free_running && bytes > vss->capture_ready) ||
> +		    (free_running &&
> +		     (bytes > vss->buffer_bytes || bytes > outstanding)) ||
> +		    check_add_overflow(vss->capture_acked_total, (u64)bytes,
> +				       &next_acked_total) ||
> +		    next_acked_total > vss->capture_pointer_seen_total) {
> +			vss->xfer_xrun = true;
> +			virtsnd_pcm_schedule_period_elapsed(vss);
> +			return -EPIPE;
> +		}
> +
> +		vss->capture_acked_total = next_acked_total;
> +		if (!free_running)
> +			vss->capture_ready -= bytes;
> +		/* Keep an unconsumed sub-byte frame for the next acknowledgment. */
> +		vss->pcm_indirect.appl_ptr += bytes_to_frames(runtime, bytes);

[Severity: Medium]
What happens here when an odd number of frames is acknowledged on a
multi-channel fractional-byte format (such as 3-, 5-, or 7-channel
SNDRV_PCM_FORMAT_IMA_ADPCM where frame_bits > 8 and frame_bits % 8 != 0)?

For example, with 3-channel IMA_ADPCM (frame_bits == 12, 2 frames == 3 bytes),
if diff == 1 frame, frames_to_bytes(runtime, 1) is 1 byte (or if diff == 3
frames, bytes is 4 bytes). That unaligned byte count is committed to
vss->capture_acked_total and subtracted from vss->capture_ready, but
bytes_to_frames(runtime, bytes) then truncates 1 byte to 0 frames (or 4 bytes
to 2 frames == 3 bytes) when advancing vss->pcm_indirect.appl_ptr.

Because vss->pcm_indirect.appl_ptr does not advance for that partial frame's
byte, the next ACK will include that frame again in diff and double-count the
byte in vss->capture_acked_total, desynchronizing vss->capture_acked_total
from vss->pcm_indirect.appl_ptr and triggering a spurious -EPIPE XRUN when
next_acked_total exceeds vss->capture_pointer_seen_total or bytes exceeds
vss->capture_ready. Should bytes be rounded down to a whole number of frames
(for instance, frames_to_bytes(runtime, bytes_to_frames(runtime, bytes)))
before updating vss->capture_acked_total and vss->capture_ready?

[ ... ]

> +static int virtsnd_pcm_cp_ack_locked(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct virtsnd_pcm_transfer transfer = {};
> +	int rc;
> +
> +	if (vss->capture_staging) {
> +		rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
> +		if (rc == -EPIPE) {
> +			guard(spinlock_irqsave)(&vss->lock);
> +			virtsnd_pcm_capture_arm_paused_stop(vss);
> +		}
> +		return rc;
> +	}
> +
>  	scoped_guard(spinlock_irqsave, &queue->lock) {
>  		guard(spinlock)(&vss->lock);
> +		if (vss->preparing)
> +			return -EBADFD;
> +		if (vss->xfer_xrun)
> +			return -EPIPE;
> +		if (!vss->prepared)
> +			return -EBADFD;
>  		transfer.indirect = vss->pcm_indirect;
> -		if (vss->stopped) {
> +		if (vss->xfer_xrun ||
> +		    (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING)) {
>  			rc = -EPIPE;
>  		} else {
>  			rc = snd_pcm_indirect_capture_transfer(substream,

[Severity: Medium]
When virtsnd_pcm_pb_ack_locked() or virtsnd_pcm_cp_ack_locked() returns
-EPIPE on a non-running substream (such as in SNDRV_PCM_STATE_PAUSED or
SNDRV_PCM_STATE_PREPARED), can leaving vss->stopped and
runtime->stop_operating unset cause a subsequent SNDRV_PCM_IOCTL_DROP to skip
virtsnd_pcm_sync_stop()?

In ALSA core, when pcm_lib_apply_appl_ptr() receives -EPIPE from ops->ack(),
it calls __snd_pcm_xrun(substream) -> snd_pcm_stop(substream,
SNDRV_PCM_STATE_XRUN). In snd_pcm_do_stop(), ALSA only invokes
ops->trigger(SNDRV_PCM_TRIGGER_STOP) and sets runtime->stop_operating = true
when snd_pcm_running(substream) is true. Once runtime->state becomes
SNDRV_PCM_STATE_XRUN, virtsnd_pcm_period_elapsed() also sees
runtime->state != SNDRV_PCM_STATE_PAUSED and does not call
virtsnd_pcm_capture_arm_paused_stop().

There are two places where -EPIPE is returned from ACK without arming
vss->stopped and runtime->stop_operating:

1. In both virtsnd_pcm_pb_ack_locked() and virtsnd_pcm_cp_ack_locked(), the
   early check:

     if (vss->xfer_xrun)
             return -EPIPE;

   returns before reaching virtsnd_pcm_transfer_failed(vss) (which also makes
   the second vss->xfer_xrun check in virtsnd_pcm_cp_ack_locked() unreachable).

2. In virtsnd_pcm_cp_ack_locked() for staged capture, when
   virtsnd_pcm_capture_staged_ack() returns -EPIPE,
   virtsnd_pcm_capture_arm_paused_stop() only sets vss->stopped = true and
   runtime->stop_operating = true when runtime->state == SNDRV_PCM_STATE_PAUSED,
   leaving SNDRV_PCM_STATE_PREPARED (and capture SNDRV_PCM_STATE_DRAINING)
   unarmed.

If the application then calls SNDRV_PCM_IOCTL_DROP, snd_pcm_drop() sees
runtime->state == SNDRV_PCM_STATE_XRUN with runtime->stop_operating == false
and transitions to SNDRV_PCM_STATE_SETUP without calling snd_pcm_sync_stop(),
leaving the device stream prepared/active and any queued descriptors
unreleased until prepare or hw_free. Should these -EPIPE paths always arm
vss->stopped and runtime->stop_operating via virtsnd_pcm_transfer_failed()?

[ ... ]

> +static int virtsnd_pcm_indirect_reset(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct virtsnd_pcm_transfer transfer = {};
> +	snd_pcm_uframes_t paused_ptr = 0;
> +	bool paused = runtime->state == SNDRV_PCM_STATE_PAUSED;
> +	int rc;
> +
> +	/* Only PCM-locked callbacks access this observation flag. */
> +	vss->indirect_reset_observing = true;
> +	rc = snd_pcm_lib_reset(substream);
> +	vss->indirect_reset_observing = false;
> +	if (rc)
> +		return rc;
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	vss->preparing = false;
> +	if (vss->resetting || vss->xfer_xrun) {
> +		virtsnd_pcm_capture_reset_error(vss, runtime);
> +		return 0;
> +	}
> +	if (!snd_pcm_running(substream) && !paused) {
> +		vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr;
> +		vss->pcm_indirect.sw_ready = 0;
> +		return 0;
> +	}
> +	if (!vss->prepared || vss->stopped) {
> +		virtsnd_pcm_transfer_failed(vss);
> +		vss->xfer_xrun = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}
> +
> +	transfer.indirect = vss->pcm_indirect;
> +	if (paused) {
> +		/* Observe and reclaim paused completions in one queue transaction. */
> +		vss->capture_pointer_seen_total = vss->capture_copied_total;
> +		vss->indirect_reset_observing = true;
> +		paused_ptr = snd_pcm_indirect_capture_pointer(substream,
> +							      &transfer.indirect, vss->hw_ptr);
> +		vss->indirect_reset_observing = false;
> +		if (paused_ptr == SNDRV_PCM_POS_XRUN) {
> +			virtsnd_pcm_transfer_failed(vss);
> +			vss->xfer_xrun = true;
> +			virtsnd_pcm_schedule_period_elapsed(vss);
> +			return 0;
> +		}
> +		runtime->status->hw_ptr = paused_ptr;
> +	}
> +	transfer.indirect.sw_ready = 0;
> +	/* Consume no application delta while returning discarded RX space. */
> +	transfer.indirect.appl_ptr = runtime->control->appl_ptr;
> +	rc = snd_pcm_indirect_capture_transfer(substream, &transfer.indirect,
> +					       virtsnd_pcm_trans_copy);
> +	if (!rc)
> +		rc = transfer.error;
> +	if (rc) {
> +		virtsnd_pcm_transfer_failed(vss);
> +		vss->xfer_xrun = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}

[Severity: Medium]
In virtsnd_pcm_indirect_reset(), shouldn't the three error paths after
snd_pcm_lib_reset() also rebase vss->pcm_indirect.appl_ptr and clear
vss->pcm_indirect.sw_ready?

The commit message notes:

  Complete capture's private cursor rebase on RESET error paths while
  retaining the XRUN latch and notification. This permits the core's
  matching appl_ptr update without claiming stream recovery.

While the first error branch (vss->resetting || vss->xfer_xrun) calls
virtsnd_pcm_capture_reset_error(vss, runtime) to set
vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr and
vss->pcm_indirect.sw_ready = 0, the other three error branches
(!vss->prepared || vss->stopped, paused_ptr == SNDRV_PCM_POS_XRUN, and rc != 0
from snd_pcm_indirect_capture_transfer()) return 0 to snd_pcm_reset_substream()
without updating vss->pcm_indirect.appl_ptr or vss->pcm_indirect.sw_ready,
leaving vss->pcm_indirect desynchronized from runtime->control->appl_ptr.
Should those three branches also call virtsnd_pcm_capture_reset_error(vss,
runtime) after setting vss->xfer_xrun = true?

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