Re: [PATCH v24 12/16] ALSA: virtio: use request-owned capture and serialize recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: virtio: virtsnd_pcm_ack_state() drops buffer_accessing
  gate check
- [Low] ALSA: virtio: commit message describes removal of non-existent
  module params
--

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

ALSA: virtio: use request-owned capture and serialize recovery

This commit switches capture substreams to request-owned buffers with
in-order completion commits and serializes PCM control state transitions,
recovery, and reset handling across epochs.

> Replace the direct/staged split with a common request-owned capture
> path. Remove capture_staging, capture_allow_short and
> capture_allow_empty. Packed status decoding and RELEASE/reprepare pause
> are still explicit, default-off compatibility options. This changes the
> default RX memory and copy cost.

[Severity: Low]
Is this paragraph left over from an earlier development branch?  Neither
the baseline nor earlier commits in the series define capture_staging,
capture_allow_short, capture_allow_empty, or a direct/staged split, while
capture_packed_status and capture_pause_restart are newly added by this
patch rather than retained.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index a5a682534acf7..da4e98836721d 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -733,116 +1840,397 @@ static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss)
>  	vss->substream->runtime->stop_operating = true;
>  }
>  
> -/* The caller holds the PCM lock, also used by DROP and state transitions. */
> -static int virtsnd_pcm_ack_state(struct snd_pcm_substream *substream)
> +/* Caller holds vss->lock; a paused ACK must not trigger linked XRUN. */

[ ... ]

> +/* 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:
> +		/*
> +		 * pcm_lib_apply_appl_ptr() interprets -EPIPE as a new XRUN
> +		 * and calls __snd_pcm_xrun(), which stops the whole linked
> +		 * group. The stream is already XRUN; do not repeat that action.
> +		 */
> +		return -EBADFD;
> +	case SNDRV_PCM_STATE_PREPARED:
> +	case SNDRV_PCM_STATE_RUNNING:
> +	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING:
> +		return 0;
> +	default:
> +		return -EBADFD;
> +	}
> +}

[Severity: Medium]
Can dropping the atomic_read(&substream->runtime->buffer_accessing) < 0
check here let a concurrent SNDRV_PCM_IOCTL_SYNC_PTR admit an ACK while a
non-atomic operation holds the buffer access lock?

snd_pcm_sync_ptr() does not check runtime->buffer_accessing before calling
pcm_lib_apply_appl_ptr() -> substream->ops->ack().  Relying only on
vss->preparing and vss->indirect_reset_observing leaves unguarded windows
where buffer_accessing is negative but those flags are not set on the
substream yet:

CPU0                                  CPU1
snd_pcm_prepare(s0) [linked s0, s1]
  snd_pcm_group_buffer_access_lock()
    s1->runtime->buffer_accessing = -1
  snd_pcm_do_prepare(s0)
    virtsnd_pcm_prepare(s0) [sleeps]
                                      snd_pcm_sync_ptr(s1)
                                        pcm_lib_apply_appl_ptr(s1)
                                          virtsnd_pcm_pb_ack(s1)
                                            /* s1->preparing is false */
                                            virtsnd_pcm_msg_send(vss1)
  snd_pcm_do_prepare(s1)
    virtsnd_pcm_prepare(s1)

Here s1 is already in SNDRV_PCM_STATE_PREPARED when the linked prepare
starts, so CPU1 queues new TX descriptors on s1 after the group buffer
access gate was closed.  Similar windows exist across linked
snd_pcm_reset() before or after a member's snd_pcm_lib_reset() call, and
in snd_pcm_hw_params() or snd_pcm_hw_free() on a PREPARED substream before
virtsnd_pcm_sync_stop_locked() sets vss->preparing.

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