Re: [PATCH v17 10/12] ALSA: virtio: serialize staged capture lifecycle and defer paused XRUNs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio: missing xfer_xrun latch in virtsnd_pcm_transfer_failed()
--

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

ALSA: virtio: serialize staged capture lifecycle and defer paused XRUNs

This commit introduces request-owned RX staging for capture streams,
serializes PCM lifecycle operations with lifecycle_lock, and defers XRUN
handling for paused linked streams until resume.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 706d59d8c7201..36f8128a3f30c 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -637,132 +1739,515 @@ static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss)
>  	vss->substream->runtime->stop_operating = true;
>  }
>  
> +/* Caller holds vss->lock; a paused ACK must not trigger linked XRUN. */
> +static int virtsnd_pcm_ack_xrun_locked(struct virtio_pcm_substream *vss,
> +				       struct snd_pcm_runtime *runtime)
> +{
> +	vss->xfer_xrun = true;
> +	if (runtime->state == SNDRV_PCM_STATE_PAUSED) {
> +		vss->xfer_enabled = false;
> +		runtime->stop_operating = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return -EBADFD;
> +	}
> +	return -EPIPE;
> +}
> +
> +static int virtsnd_pcm_pb_ack_locked(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 = {};
> +	bool paused;
> +	int rc;
> +
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		paused = runtime->state == SNDRV_PCM_STATE_PAUSED;
> +		if (vss->preparing)
> +			return -EBADFD;
> +		/* A latched XRUN must not re-arm sync_stop once teardown began. */
> +		if (vss->stopped && vss->xfer_xrun)
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		} else {
> +			if (!vss->prepared || vss->stopped)
> +				return -EBADFD;
> +			transfer.indirect = vss->pcm_indirect;
> +			rc = snd_pcm_indirect_playback_transfer(substream,
> +								&transfer.indirect,
> +								virtsnd_pcm_trans_copy);
> +		}
> +		if (!rc)
> +			rc = transfer.error;
> +		if (rc) {
> +			if (paused)
> +				rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +			else
> +				virtsnd_pcm_transfer_failed(vss);
> +		} else {
> +			vss->pcm_indirect = transfer.indirect;
> +		}
> +	}

[ ... ]

> +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 = {};
> +	bool paused;
> +	int rc;
> +
> +	if (vss->capture_staging) {
> +		rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
> +		if (rc == -EPIPE) {
> +			guard(spinlock_irqsave)(&vss->lock);
> +			virtsnd_pcm_transfer_failed(vss);
> +		}
> +		return rc;
> +	}
> +
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		paused = runtime->state == SNDRV_PCM_STATE_PAUSED;
> +		if (vss->preparing)
> +			return -EBADFD;
> +		/* A latched XRUN must not re-arm sync_stop once teardown began. */
> +		if (vss->stopped && vss->xfer_xrun)
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		} else {
> +			if (!vss->prepared ||
> +			    (vss->stopped &&
> +			     runtime->state != SNDRV_PCM_STATE_DRAINING))
> +				return -EBADFD;
> +			transfer.indirect = vss->pcm_indirect;
> +			rc = snd_pcm_indirect_capture_transfer(substream,
> +							       &transfer.indirect,
> +							       virtsnd_pcm_trans_copy);
> +		}
> +		if (!rc)
> +			rc = transfer.error;
> +		if (rc) {
> +			if (paused)
> +				rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +			else
> +				virtsnd_pcm_transfer_failed(vss);
> +		} else {
> +			vss->pcm_indirect = transfer.indirect;
> +		}
> +	}

[Severity: Medium]
Should virtsnd_pcm_transfer_failed() in sound/virtio/virtio_pcm_ops.c also
latch vss->xfer_xrun = true?

When a non-paused indirect playback or capture ACK transfer fails in
virtsnd_pcm_pb_ack_locked() or virtsnd_pcm_cp_ack_locked() (for example,
when snd_pcm_indirect_playback_transfer() or
snd_pcm_indirect_capture_transfer() returns -EPIPE on a negative appl_ptr
diff, or virtsnd_pcm_msg_send() sets transfer.error), the error path calls
virtsnd_pcm_transfer_failed():

    static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss)
    {
            vss->xfer_enabled = false;
            vss->stopped = true;
            vss->substream->runtime->stop_operating = true;
    }

Unlike the paused path (which calls virtsnd_pcm_ack_xrun_locked()) and the
staged capture path (where virtsnd_pcm_capture_staged_ack() calls
virtsnd_pcm_ack_xrun_locked() before virtsnd_pcm_transfer_failed()),
virtsnd_pcm_transfer_failed() leaves vss->xfer_xrun false while vss->prepared
and vss->stopped are true.

Because vss->xfer_xrun is not set:

1. In sound/virtio/virtio_pcm_msg.c, in-flight non-empty RX completions
arriving in virtsnd_pcm_msg_complete() for a non-staged capture stream bypass
the early retirement check:

    if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
        (!vss->prepared || vss->xfer_xrun ||
         (vss->stopped && written_bytes <= sizeof(*msg->status)))) {

and continue copying data and advancing vss->hw_ptr and
vss->capture_copied_total after the failed ACK rolled back vss->pcm_indirect
and appl_ptr.

2. If the capture stream is in SNDRV_PCM_STATE_DRAINING, the stopped-plus-XRUN
admission check (if (vss->stopped && vss->xfer_xrun) return -EBADFD) in
virtsnd_pcm_cp_ack_locked() is not armed.

3. Any call to virtsnd_pcm_pb_pointer() or virtsnd_pcm_cp_pointer() before
teardown (such as across the stream lock drop when locking a linked group in
__snd_pcm_xrun()) does not see vss->xfer_xrun set, so it invokes
snd_pcm_indirect_playback_pointer() or snd_pcm_indirect_capture_pointer()
instead of returning SNDRV_PCM_POS_XRUN.

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