Re: [PATCH v21 04/12] ALSA: virtio: Propagate PCM send failures through ACK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: virtio: premature ACK admission in virtsnd_pcm_reset()
  during prepare
- [Low] ALSA: virtio: misplaced Fixes trailer and inaccurate commit
  message
--

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

> ALSA: virtio: Propagate PCM send failures through ACK
> 
> virtsnd_pcm_msg_send() can fail while queueing a transfer. Restore the
> failed message's length and notify the device about earlier messages
> queued by the same call, so they do not remain stranded.
> 
> Propagate errors through the indirect transfer callback and commit its
> private cursor only after successful submission. Genuine transfer errors
> arm stop_operating and return -EPIPE so ALSA can retire the stream.
> 
> Close ACK admission under the PCM stream lock before sleeping teardown.
> Keep the prepared state protected by the substream lock and reject ACK
> through prepare and parameter changes. Reopen admission in the reset
> callback while the core holds the PCM lock for its pointer rebase. An
> ACK racing with teardown returns -EBADFD and preserves stop state.
> 
> Keep the polling feature path unchanged.
> 
> Fixes: fe981e67568c ("ALSA: virtio: use ack callback")
> 
> Reject a stopped playback ACK before the XRUN path can rearm
> stop_operating. For capture, allow the ordinary stopped tail only during
> DRAINING and only while no transfer XRUN is latched.
> 
> Latch a transfer failure before stopping PCM work. Once core reports the member XRUN, ACK returns -EBADFD to avoid another linked XRUN. ACK during prepare remains rejected until the core state reaches PREPARED.
> Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>

[Severity: Low]
This isn't a runtime bug, but the Fixes: trailer sits in the middle of the
commit message rather than in the trailer block above Signed-off-by:, and
the final paragraph states that ACK returns -EBADFD once the core reports
XRUN whereas virtsnd_pcm_ack_state() returns -EPIPE for
SNDRV_PCM_STATE_XRUN in this patch.

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

[ ... ]

> @@ -461,41 +530,168 @@ 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_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;
> +
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		if (vss->preparing || !vss->prepared)
> +			return -EBADFD;
> +		/* Keep only the post-STOP capture tail available during drain. */
> +		if (vss->stopped &&
> +		    (vss->xfer_xrun ||
> +		     substream->runtime->state != SNDRV_PCM_STATE_DRAINING))
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = -EPIPE;
> +		} else {
> +			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)
> +			virtsnd_pcm_transfer_failed(vss);
> +		else
> +			vss->pcm_indirect = transfer.indirect;
> +	}
> +	if (rc) {
> +		dev_err(&vss->snd->vdev->dev,
> +			"SID %u: capture ACK transfer failed: %d\n",
> +			vss->sid, rc);
> +		return -EPIPE;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * virtsnd_pcm_reset() - Reset PCM pointers and complete prepare admission.
> + * @substream: Kernel ALSA substream.
> + *
> + * Context: PCM stream lock held.
> + * Return: 0 on success, -errno on failure.
> + */
> +static int virtsnd_pcm_reset(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	int rc;
> +
> +	rc = snd_pcm_lib_reset(substream);
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->preparing) {
> +			vss->prepared = !rc;
> +			vss->preparing = false;
> +		}
> +	}
>  
> -	return snd_pcm_indirect_capture_transfer(substream, &vss->pcm_indirect,
> -						 virtsnd_pcm_trans_copy);
> +	return rc;
>  }

[Severity: Medium]
Can virtsnd_pcm_reset() reopen ACK admission on a substream before
snd_pcm_post_prepare() transitions the group to SNDRV_PCM_STATE_PREPARED?

CPU0                                  CPU1
snd_pcm_prepare(s0)
  snd_pcm_action_nonatomic(...)
    snd_pcm_do_prepare(s1)
      virtsnd_pcm_sync_stop(s1)
        vss1->stopped = false
      virtsnd_pcm_prepare(s1)
      snd_pcm_reset_substream(s1, ...)
        virtsnd_pcm_reset(s1)
          vss1->prepared = true
          vss1->preparing = false
    snd_pcm_do_prepare(s2)
      ... (sleeps or fails)
                                      snd_pcm_sync_ptr(s1)
                                        pcm_lib_apply_appl_ptr(s1, ...)
                                          virtsnd_pcm_pb_ack(s1) /
                                          virtsnd_pcm_cp_ack(s1)
                                            <-- accepted before post_prepare

In snd_pcm_action_group(), snd_pcm_do_prepare() runs sequentially on each
linked member before snd_pcm_post_prepare() runs on any of them, and
snd_pcm_action_nonatomic() only sets runtime->buffer_accessing to -1 on
the ioctl target substream (and snd_pcm_sync_ptr() does not check
buffer_accessing at all).  If a linked member s1 entered prepare from
PREPARED, PAUSED, or capture DRAINING, virtsnd_pcm_reset(s1) sets
vss1->prepared = true and vss1->preparing = false and releases s1's PCM
stream lock before s2 completes snd_pcm_do_prepare(s2).

While s2 sleeps in snd_pcm_do_prepare(s2) (or permanently if s2 fails
prepare, since snd_pcm_action_prepare has no undo_action), playback ACKs
on s1 queue new TX buffers to the virtqueue before prepare completes.  In
addition, because virtsnd_pcm_cp_ack() only checks runtime->state !=
SNDRV_PCM_STATE_DRAINING when vss->stopped is true, a capture substream
re-prepared from DRAINING has vss1->stopped == false after
virtsnd_pcm_sync_stop(s1) and accepts capture ACKs while runtime->state is
still DRAINING, driving vss1->pcm_indirect.sw_ready negative.

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