Re: [PATCH v16 04/12] 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:
- [Medium] ALSA: virtio: premature virtsnd_pcm_ack_state() check in ACK callbacks
--

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

ALSA: virtio: Propagate PCM send failures through ACK

This commit propagates message submission failures from virtsnd_pcm_msg_send()
through the indirect PCM transfer callback in the playback and capture ACK
handlers. It also introduces prepared and preparing state tracking to gate ACK
admission across teardown, prepare, and pointer reset operations.

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

[ ... ]

> @@ -461,41 +530,167 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)

[ ... ]

> +/* The caller holds the PCM lock, also used by DROP and state transitions. */
> +static int virtsnd_pcm_ack_state(struct snd_pcm_substream *substream)
> +{
> +	switch (substream->runtime->state) {
> +	case SNDRV_PCM_STATE_XRUN:
> +		return -EPIPE;
> +	case SNDRV_PCM_STATE_PREPARED:
> +	case SNDRV_PCM_STATE_RUNNING:
> +	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING:
> +		if (atomic_read(&substream->runtime->buffer_accessing) < 0)
> +			return -EBADFD;
> +		return 0;
> +	default:
> +		return -EBADFD;
> +	}
> +}
>  
>  static int virtsnd_pcm_pb_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;
> +
> +	rc = virtsnd_pcm_ack_state(substream);
> +	if (rc)
> +		return rc;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		if (vss->preparing || !vss->prepared)
> +			return -EBADFD;

[ ... ]

>  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;
> +
> +	rc = virtsnd_pcm_ack_state(substream);
> +	if (rc)
> +		return rc;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		if (vss->preparing || !vss->prepared)
> +			return -EBADFD;

[Severity: Medium]
Should virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() check vss->preparing and
vss->prepared under vss->lock before evaluating virtsnd_pcm_ack_state()?

When snd_pcm_prepare() is invoked on a substream in SNDRV_PCM_STATE_XRUN, the
ALSA core leaves runtime->state as SNDRV_PCM_STATE_XRUN throughout
snd_pcm_do_prepare() (across the sleeping virtsnd_pcm_sync_stop() and
virtsnd_pcm_prepare() calls) until snd_pcm_post_prepare() runs.

Because virtsnd_pcm_ack_state() checks SNDRV_PCM_STATE_XRUN before
vss->preparing and vss->prepared are checked, a concurrent
SNDRV_PCM_IOCTL_SYNC_PTR (or rewind/forward) updating appl_ptr via
pcm_lib_apply_appl_ptr() during XRUN recovery receives -EPIPE instead of
-EBADFD. That causes pcm_lib_apply_appl_ptr() to call __snd_pcm_xrun()
mid-prepare, forcing any linked substreams into SNDRV_PCM_STATE_XRUN and
recording a spurious XRUN.

Similarly, when linked substreams are prepared via snd_pcm_action_group(),
snd_pcm_do_prepare() (including virtsnd_pcm_reset(), which sets
vss->prepared = true and vss->preparing = false) completes on each member
before snd_pcm_post_prepare() updates runtime->state to
SNDRV_PCM_STATE_PREPARED, and while snd_pcm_buffer_access_lock() still holds
buffer_accessing < 0 on the primary substream. While a later linked member
sleeps in snd_pcm_do_prepare(), an earlier member that has already finished
virtsnd_pcm_reset() still has runtime->state set to SNDRV_PCM_STATE_SETUP or
SNDRV_PCM_STATE_XRUN (or buffer_accessing < 0 when re-prepared from
SNDRV_PCM_STATE_PREPARED), causing virtsnd_pcm_ack_state() to reject a
concurrent SNDRV_PCM_IOCTL_SYNC_PTR with -EBADFD or -EPIPE.

Although the later commit ("ALSA: virtio: add staged capture and serialize
PCM lifecycle accounting") removes the buffer_accessing check from
virtsnd_pcm_ack_state(), virtsnd_pcm_ack_state() is still evaluated before
vss->preparing and vss->prepared, leaving the SNDRV_PCM_STATE_XRUN and
SNDRV_PCM_STATE_SETUP ordering issues in place.

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