Re: [PATCH v10 14/14] ALSA: virtio: rebase ordinary capture accounting on RESET

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ALSA: virtio: missing xfer_xrun checks in virtsnd_pcm_indirect_reset() and ACK
- [High] ALSA: virtio: RX msg->length corruption on PAUSE_RELEASE after paused RESET
- [Medium] ALSA: virtio: unlocked paused pointer update in virtsnd_pcm_indirect_reset()
--

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

ALSA: virtio: rebase ordinary capture accounting on RESET

This commit adds an indirect reset callback for ordinary capture streams so
that indirect accounting follows the common ALSA reset observation while
preserving fixed DMA ring offsets. It also suppresses nested ACK refills
during pointer observation and preserves latched capture XRUN errors across
late I/O completions.

Link: https://sashiko.dev/#/patchset/20261029085032.70689-1-mynameisjune111%40gmail.com?part=14

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 8060858d88203..4ab73c5e78f8e 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -1738,6 +1746,77 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	return virtsnd_pcm_sync_stop_locked(substream);
>  }
>  
> +/*
> + * The PCM lock covers indirect accounting and core completion. Once lib_reset
> + * changes hw_ptr, complete the core rebase even if refilling fails. The latched
> + * XRUN rejects subsequent pointer/ACK operations; the worker publishes XRUN to
> + * paused streams too. RESET success therefore does not imply stream recovery.
> + */
> +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;
> +	unsigned int hw_ptr;
> +	int rc;
> +
> +	/* Only PCM-locked callbacks access this observation flag. */
> +	vss->indirect_reset_observing = true;
> +	if (paused) {
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			hw_ptr = vss->hw_ptr;
> +		paused_ptr = snd_pcm_indirect_capture_pointer(substream,
> +						&vss->pcm_indirect, hw_ptr);
> +	}

[Severity: Medium]
When resetting a paused stream in virtsnd_pcm_indirect_reset(), vss->hw_ptr
is sampled under a temporary vss->lock guard and
snd_pcm_indirect_capture_pointer() mutates vss->pcm_indirect before
queue->lock and vss->lock are acquired below.

Because snd_pcm_running(substream) is false in SNDRV_PCM_STATE_PAUSED,
snd_pcm_lib_reset() does not inspect vss->pcm_indirect or call
substream->ops->pointer(), and simply zeros runtime->status->hw_ptr and
runtime->hw_ptr_wrap.

If a late RX completion interrupt arrives via
virtsnd_pcm_rx_notify_cb()->virtsnd_pcm_msg_complete() between releasing
vss->lock here and acquiring queue->lock and vss->lock below, vss->hw_ptr
advances and msg->length is cleared to 0 while vss->xfer_enabled is false.
Wouldn't virtsnd_pcm_indirect_reset() then rebase runtime->status->hw_ptr to
the stale paused_ptr and miss re-enqueueing the newly completed RX
descriptor?

In addition, since snd_pcm_indirect_capture_pointer() mutates
vss->pcm_indirect (hw_io, hw_ready, sw_io) before the locked checks for
vss->resetting, !vss->prepared, and vss->stopped, can those early returns
leave vss->pcm_indirect out of sync with runtime->status->hw_ptr?

> +	rc = snd_pcm_lib_reset(substream);
> +	vss->indirect_reset_observing = false;
> +	if (rc)
> +		return rc;
> +	if (!snd_pcm_running(substream) &&
> +	    runtime->state != SNDRV_PCM_STATE_PAUSED)
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting)
> +		return 0;
> +	if (!vss->prepared || vss->stopped ||
> +	    (paused && paused_ptr == SNDRV_PCM_POS_XRUN)) {
> +		virtsnd_pcm_transfer_failed(vss);
> +		vss->xfer_xrun = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}

[Severity: High]
As reported by Sashiko on v9 [1], what happens if vss->xfer_xrun is already
latched when virtsnd_pcm_indirect_reset() or virtsnd_pcm_cp_ack_locked()
runs?

In sound/virtio/virtio_pcm.c:virtsnd_pcm_event(), receiving
VIRTIO_SND_EVT_PCM_XRUN on an active ordinary capture stream sets
vss->xfer_xrun = true without setting vss->stopped = true:

sound/virtio/virtio_pcm.c:virtsnd_pcm_event() {
	case VIRTIO_SND_EVT_PCM_XRUN:
		scoped_guard(spinlock, &vss->lock) {
			if (vss->xfer_enabled) {
				vss->xfer_xrun = true;
				if (vss->capture_staging)
					virtsnd_pcm_schedule_period_elapsed(vss);
			}
		}
		break;
}

And with the change in virtsnd_pcm_msg_complete(), capture I/O completions no
longer clear vss->xfer_xrun. If the stream is paused via
SNDRV_PCM_TRIGGER_PAUSE_PUSH before a pointer query observes the XRUN (or if
VIRTIO_SND_EVT_PCM_XRUN arrives on a running stream just after
snd_pcm_lib_reset()), virtsnd_pcm_indirect_reset() calls
snd_pcm_indirect_capture_pointer() directly instead of
virtsnd_pcm_cp_pointer() and does not check vss->xfer_xrun in this condition
(unlike virtsnd_pcm_reset()). Won't it skip virtsnd_pcm_transfer_failed() and
virtsnd_pcm_schedule_period_elapsed(), overwrite runtime->status->hw_ptr with
paused_ptr, and re-enqueue RX descriptors onto the virtqueue despite the
latched XRUN?

Similarly, in sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_cp_ack_locked(), the
ordinary capture path checks vss->stopped but not vss->xfer_xrun:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_cp_ack_locked() {
		if (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING) {
			rc = -EPIPE;
		} else {
			rc = snd_pcm_indirect_capture_transfer(substream,
							       &transfer.indirect,
							       virtsnd_pcm_trans_copy);
		}
}

If an ACK operation does not first pass through virtsnd_pcm_cp_pointer()
(such as reading buffered frames while paused or SNDRV_PCM_IOCTL_SYNC_PTR
without HWSYNC), won't virtsnd_pcm_cp_ack_locked() succeed and enqueue RX
buffers instead of rejecting the ACK with -EPIPE?

> +
> +	/* Discard completions observed through the paused device pointer too. */
> +	if (runtime->state == SNDRV_PCM_STATE_PAUSED)
> +		runtime->status->hw_ptr = paused_ptr;
> +
> +	transfer.indirect = vss->pcm_indirect;
> +	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);

[Severity: High]
When virtsnd_pcm_indirect_reset() runs while the stream is in
SNDRV_PCM_STATE_PAUSED, snd_pcm_indirect_capture_transfer() invokes
virtsnd_pcm_trans_copy()->virtsnd_pcm_msg_send() to re-enqueue completed RX
descriptors into the virtqueue, setting their msg->length to period_bytes
while vss->device_running is false (cleared during
SNDRV_PCM_TRIGGER_PAUSE_PUSH in virtsnd_pcm_stop_device()).

When the stream is subsequently resumed via SNDRV_PCM_TRIGGER_PAUSE_RELEASE,
virtsnd_pcm_trigger() calls virtsnd_pcm_start_queue(), which sees
!vss->device_running and calls virtsnd_pcm_msg_send(vss, 0,
vss->buffer_bytes) across the entire buffer:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_start_queue() {
		if (vss->capture_staging)
			rc = virtsnd_pcm_capture_send(vss);
		else if (!vss->device_running)
			rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
}

In sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_send(), msg->length += n is
executed unconditionally before checking if msg->length == period_bytes:

sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_send() {
		msg->length += n;
		if (msg->length == period_bytes) {
			...
		}
}

For every RX descriptor already queued in the virtqueue with
msg->length == period_bytes, doesn't this increment msg->length to
2 * period_bytes on PAUSE_RELEASE (and similarly corrupt msg->length if
unread completed descriptors are re-submitted by virtsnd_pcm_start_queue()
and later ACKed by the application)?

[1] https://lore.kernel.org/r/20261029085032.70689-15-mynameisjune111@gmail.com

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