Re: [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_cp_ack() fails with -EPIPE during capture drain
--

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

ALSA: virtio: Propagate PCM send failures through ACK

This commit updates virtsnd_pcm_msg_send() to roll back the message length
and notify the virtqueue for previously queued messages if adding a scatterlist
fails. It also propagates transfer errors through the playback and capture
ack callbacks, marking the stream stopped and returning an xrun error on
failure.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1091100a4c982..a40ea394bf174 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -466,36 +466,100 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
>  static int virtsnd_pcm_cp_ack(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;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		transfer.indirect = vss->pcm_indirect;
> +		if (vss->stopped) {
> +			rc = -EPIPE;
> +		} else {
> +			rc = snd_pcm_indirect_capture_transfer(substream,
> +							       &transfer.indirect,
> +							       virtsnd_pcm_trans_copy);
> +		}

[Severity: High]
Does checking vss->stopped here break capture stream draining?

When userspace drains a running capture substream that still has unread
frames in the ring buffer, snd_pcm_do_drain_init() stops the hardware stream
and places the substream into the draining state so userspace can read the
remaining captured data:

sound/core/pcm_native.c:snd_pcm_do_drain_init() {
	...
		if (runtime->state == SNDRV_PCM_STATE_RUNNING) {
			snd_pcm_state_t new_state;

			new_state = snd_pcm_capture_avail(runtime) > 0 ?
				SNDRV_PCM_STATE_DRAINING : SNDRV_PCM_STATE_SETUP;
			snd_pcm_do_stop(substream, new_state);
			snd_pcm_post_stop(substream, new_state);
		}
}

snd_pcm_do_stop() invokes virtsnd_pcm_trigger(), which sets vss->stopped:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() {
	...
	case SNDRV_PCM_TRIGGER_STOP:
		vss->stopped = true;
		fallthrough;
}

When userspace subsequently reads those remaining buffered frames via
__snd_pcm_lib_xfer(), advances appl_ptr via snd_pcm_forward(), or synchronizes
via snd_pcm_sync_ptr(), pcm_lib_apply_appl_ptr() invokes substream->ops->ack():

sound/core/pcm_lib.c:pcm_lib_apply_appl_ptr() {
	...
	runtime->control->appl_ptr = appl_ptr;
	if (substream->ops->ack) {
		ret = substream->ops->ack(substream);
		if (ret < 0) {
			runtime->control->appl_ptr = old_appl_ptr;
			if (ret == -EPIPE)
				__snd_pcm_xrun(substream);
			return ret;
		}
	}
}

Because vss->stopped is already true during capture drain, virtsnd_pcm_cp_ack()
will unconditionally fail and return -EPIPE, causing pcm_lib_apply_appl_ptr()
to revert appl_ptr, trigger __snd_pcm_xrun(), and prevent userspace from
reading the remaining captured frames.

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