Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] ALSA: virtio: duplicate RX buffer enqueue on non-staged pause release - [Medium] ALSA: virtio: detached virtqueue race in virtsnd_reset_vqs() and notify_cb - [Medium] ALSA: virtio: incomplete pause rejection recovery in virtsnd_pcm_trigger() - [Medium] ALSA: virtio: missing virtsnd_pcm_start_failed() on running trigger error --
commit 4e7eab1d20a52ea1742792d7fd3cc04245ceba41 Author: Joonho Lee <[email protected]> ALSA: virtio: add staged capture accounting and lifecycle recovery This commit adds staged capture accounting using request-owned DMA buffers and introduces lifecycle recovery and epoch tracking across PCM operations and device resets. Link: https://sashiko.dev/#/patchset/20261029085032.70689-1-mynameisjune111%40gmail.com?part=9 > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index df7091b74a521..07d66225ce04a 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c > @@ -348,6 +348,22 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd) > struct virtio_device *vdev = snd->vdev; > unsigned int i; > > + for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) { > + struct virtio_pcm_substream *vss = &snd->substreams[i]; > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + vss->reset_epoch++; > + vss->resetting = true; > + vss->prepared = false; > + vss->device_running = false; > + vss->release_required = false; > + vss->control_uncertain = false; > + vss->capture_reset_observing = false; > + vss->xfer_enabled = false; > + vss->suspended = true; > + } > virtio_reset_device(vdev); > > /* Deleting a virtqueue does not unmap its outstanding descriptors. */ > @@ -364,9 +380,20 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd) > spin_unlock_irqrestore(&queue->lock, flags); > } > > - vdev->config->del_vqs(vdev); > - for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) > + for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) { > + guard(spinlock_irqsave)(&snd->queues[i].lock); > snd->queues[i].vqueue = NULL; > + } [Severity: Medium] As reported by Sashiko on v9 [1], can an IRQ callback still observe a non-NULL queue->vqueue after its buffers have been detached in virtsnd_reset_vqs()? In virtsnd_reset_vqs(), the first loop over VIRTIO_SND_VQ_MAX detaches unused buffers under queue->lock and then drops the lock: for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) { struct virtio_snd_queue *queue = &snd->queues[i]; unsigned long flags; void *msg; spin_lock_irqsave(&queue->lock, flags); if (queue->vqueue) while ((msg = virtqueue_detach_unused_buf(queue->vqueue))) if (i == VIRTIO_SND_VQ_TX || i == VIRTIO_SND_VQ_RX) virtsnd_pcm_msg_cancel(msg); spin_unlock_irqrestore(&queue->lock, flags); } Only in the separate loop above does virtsnd_reset_vqs() re-acquire snd->queues[i].lock to set snd->queues[i].vqueue = NULL. Because virtqueue_detach_unused_buf_split() clears vq->split.desc_state[i].data = NULL without advancing vq->last_used_idx, if virtsnd_pcm_notify_cb() acquires queue->lock between those two loops, it will see queue->vqueue != NULL and call virtqueue_get_buf() on a ring whose descriptor state was already cleared, triggering BAD_RING() in virtqueue_get_buf_ctx_split() if the device wrote a used entry just before virtio_reset_device(). In addition, virtsnd_ctl_notify_cb() in sound/virtio/virtio_ctl_msg.c and virtsnd_event_notify_cb() in sound/virtio/virtio_card.c do not check queue->vqueue under queue->lock at all before calling virtqueue_get_buf(), virtqueue_enable_cb(), or virtsnd_event_send(). Should queue->vqueue = NULL be set inside the same queue->lock critical section that calls virtqueue_detach_unused_buf(), and should virtsnd_ctl_notify_cb() and virtsnd_event_notify_cb() also check if (!queue->vqueue) under queue->lock? [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index de11bf162006e..8a6499d0cfea7 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > +static int virtsnd_pcm_start_queue(struct virtio_pcm_substream *vss, u64 > epoch) > +{ > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + int rc = 0; > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + if (vss->resetting || vss->reset_epoch != epoch) > + return -ECANCELED; > + if (vss->stopped || !vss->prepared || vss->xfer_xrun) > + return vss->xfer_xrun ? -EPIPE : -EBADFD; > + if (vss->control_uncertain) > + return -EIO; > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { > + if (vss->capture_staging) > + rc = vss->runtime_active ? > virtsnd_pcm_capture_send(vss) : -EBADFD; > + else if (!vss->device_running) > + rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes); > + } [Severity: High] On a non-staged capture stream, won't resuming from pause via SNDRV_PCM_TRIGGER_PAUSE_RELEASE re-submit the entire capture buffer while descriptors from before the pause are still queued or hold unread data? When SNDRV_PCM_TRIGGER_START initially starts a non-staged capture stream, virtsnd_pcm_start_queue() calls virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes) to queue all vss->nmsgs descriptors with msg->length = period_bytes. When SNDRV_PCM_TRIGGER_PAUSE_PUSH pauses the stream, virtsnd_pcm_stop_device() sends VIRTIO_SND_R_PCM_STOP and sets vss->device_running = false without releasing the pending RX descriptors. When SNDRV_PCM_TRIGGER_PAUSE_RELEASE later resumes the stream, virtsnd_pcm_start_queue() sees !vss->device_running == true and calls virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes) again across the whole buffer: msg->length += n; if (msg->length == period_bytes) { For descriptors still pending in the virtqueue, msg->length is incremented to 2 * period_bytes, corrupting vss->hw_ptr accounting on error status and preventing subsequent ACK re-submissions. For descriptors that already completed before or during pause and are waiting for userspace to read and ACK them, virtsnd_pcm_msg_send() re-queues them into the RX virtqueue, allowing the device to overwrite unread audio samples via DMA. > + if (!rc) > + vss->xfer_enabled = true; > + return rc; > +} > + > /** > * virtsnd_pcm_trigger() -Start/stop/pause/resume the PCM substream. > * @substream: Kernel ALSA substream. > @@ -377,68 +1087,219 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) > static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int > command) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > - struct virtio_snd *snd = vss->snd; > - struct virtio_snd_queue *queue; > struct virtio_snd_msg *msg; > int rc = 0; > + bool running; > + u64 epoch; > + > + scoped_guard(spinlock_irqsave, &vss->lock) > + epoch = vss->reset_epoch; > + > + if (virtsnd_pcm_pause_restart_enabled(vss)) { > + switch (command) { > + case SNDRV_PCM_TRIGGER_START: > + return virtsnd_pcm_pause_restart_start(vss, false); > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + return virtsnd_pcm_pause_restart_start(vss, true); > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + return virtsnd_pcm_pause_restart_pause(vss); > + case SNDRV_PCM_TRIGGER_STOP: > + return virtsnd_pcm_pause_restart_stop(vss, false); > + case SNDRV_PCM_TRIGGER_SUSPEND: > + return virtsnd_pcm_pause_restart_stop(vss, true); > + default: > + return -EINVAL; > + } > + } > > switch (command) { > case SNDRV_PCM_TRIGGER_START: > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > - queue = virtsnd_pcm_queue(vss); > - > - scoped_guard(spinlock_irqsave, &queue->lock) { > - guard(spinlock)(&vss->lock); > - if (vss->stopped) { > - substream->runtime->stop_operating = true; > - return -EBADFD; > - } > - if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) > - rc = virtsnd_pcm_msg_send(vss, 0, > vss->buffer_bytes); > - if (rc) { > - vss->stopped = true; > - substream->runtime->stop_operating = true; > - return rc; > - } > - vss->xfer_enabled = true; > + scoped_guard(spinlock_irqsave, &vss->lock) > + running = vss->device_running; > + /* Refill RX descriptors returned during a rejected native > pause. */ > + if (running) > + return virtsnd_pcm_start_queue(vss, epoch); [Severity: Medium] As reported by Sashiko on v9 [1], what happens when running is true here and virtsnd_pcm_start_queue(vss, epoch) fails, or when it succeeds on a staged capture stream? When SNDRV_PCM_TRIGGER_PAUSE_PUSH is rejected by the device in virtsnd_pcm_stop_device(), vss->device_running remains true. When SNDRV_PCM_TRIGGER_PAUSE_RELEASE subsequently runs (either from snd_pcm_undo_pause(), which ignores the trigger return value, or a later resume on a linked substream), virtsnd_pcm_trigger() sees running == true and returns virtsnd_pcm_start_queue(vss, epoch) directly. If virtsnd_pcm_start_queue() fails (for example, if virtsnd_pcm_capture_refresh_mode_locked() returns -EPIPE after a stop_threshold update, or virtqueue_add_sgs() fails while refilling descriptors completed after PAUSE_PUSH), this early return bypasses virtsnd_pcm_start_failed(vss, epoch), leaving vss->xfer_enabled, vss->stopped, substream->runtime->stop_operating, and vss->xfer_xrun unlatched and failing to schedule elapsed_period work to wake blocked staged-capture readers. Conversely, if virtsnd_pcm_start_queue() succeeds for staged capture on this running == true path, returning early skips the virtsnd_pcm_schedule_period_elapsed(vss) call at the end of the SNDRV_PCM_TRIGGER_START / SNDRV_PCM_TRIGGER_PAUSE_RELEASE case. > + rc = virtsnd_pcm_start_queue(vss, epoch); > + if (rc) { > + virtsnd_pcm_start_failed(vss, epoch); > + return rc; > } > > msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, > - GFP_KERNEL); > + GFP_KERNEL, epoch); > if (!msg) { > - virtsnd_pcm_start_failed(vss); > + virtsnd_pcm_start_failed(vss, epoch); > return -ENOMEM; > } > > - rc = virtsnd_ctl_msg_send_sync(snd, msg); > - if (rc) > - virtsnd_pcm_start_failed(vss); > + rc = virtsnd_pcm_send_sync(vss, msg); > + if (rc && rc != -ECANCELED) > + virtsnd_pcm_start_failed(vss, epoch); > > + if (!rc) { > + guard(spinlock_irqsave)(&vss->lock); > + if (vss->resetting || vss->reset_epoch != epoch) > + return -ECANCELED; > + vss->device_running = true; > + } > + if (!rc && vss->capture_staging) > + virtsnd_pcm_schedule_period_elapsed(vss); > return rc; > case SNDRV_PCM_TRIGGER_SUSPEND: > - vss->suspended = true; > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->suspended = true; > fallthrough; > case SNDRV_PCM_TRIGGER_STOP: > case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > scoped_guard(spinlock_irqsave, &vss->lock) { > + if (!vss->prepared) > + return 0; > if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) > vss->stopped = true; > vss->xfer_enabled = false; > + if (!vss->device_running) > + return 0; > + } > + > + rc = virtsnd_pcm_stop_device(vss, epoch); > + if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH && > + virtsnd_pcm_start_queue(vss, epoch)) { > + guard(spinlock_irqsave)(&vss->lock); > + if (vss->reset_epoch == epoch && !vss->resetting) { > + vss->xfer_xrun = true; > + vss->stopped = true; > + substream->runtime->stop_operating = true; > + virtsnd_pcm_schedule_period_elapsed(vss); > + } > + } > + return rc; [Severity: Medium] As reported by Sashiko on v9 [1], can a non-staged capture stream stall if VIRTIO_SND_R_PCM_STOP is rejected during SNDRV_PCM_TRIGGER_PAUSE_PUSH? Before calling virtsnd_pcm_stop_device(vss, epoch), SNDRV_PCM_TRIGGER_PAUSE_PUSH sets vss->xfer_enabled = false. While VIRTIO_SND_R_PCM_STOP is in flight, the device is still running and can complete queued RX buffers. In virtsnd_pcm_msg_complete(), completions for non-staged capture advance vss->hw_ptr, clear msg->length = 0, and decrement vss->msg_count, but skip virtsnd_pcm_schedule_period_elapsed(vss) because vss->xfer_enabled is false. When virtsnd_pcm_stop_device() fails and leaves vss->device_running = true, both virtsnd_pcm_start_queue(vss, epoch) here and the subsequent SNDRV_PCM_TRIGGER_PAUSE_RELEASE recovery path check: if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { if (vss->capture_staging) rc = vss->runtime_active ? virtsnd_pcm_capture_send(vss) : -EBADFD; else if (!vss->device_running) rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes); } if (!rc) vss->xfer_enabled = true; Because vss->device_running is still true, virtsnd_pcm_start_queue() only sets vss->xfer_enabled = true without scheduling elapsed_period work or refilling descriptors. If all queued RX descriptors completed while vss->xfer_enabled was false, vss->msg_count is 0 and no period work is scheduled, so ALSA never runs pointer/ack to re-enqueue RX buffers and the capture stream stalls indefinitely. [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
