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 <[email protected]> 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/[email protected]?part=4
