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