Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] ALSA: virtio: missed playback wakeup and wrap in virtsnd_pcm_trigger_inner() --
commit 1965297bcd244a03fcb4a5994d8a256a3575a92b Author: Joonho Lee <[email protected]> ALSA: virtio: add opt-in staged capture and recovery This commit adds an opt-in request-owned staged capture path with compatibility options and updates PCM control serialization, pause/resume handling, and reset callbacks. > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index 0c56f7f4af0f6..4528259597a36 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -536,78 +876,103 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, [ ... ] > + vss->hw_ptr += completed_bytes; > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { > + /* A full unseen ring cannot be represented by the modulo > pointer. */ > + if (check_add_overflow(vss->capture_copied_total, > + (u64)completed_bytes, > + &vss->capture_copied_total) || > + vss->capture_copied_total < vss->capture_pointer_seen_total > || > + vss->capture_copied_total - vss->capture_pointer_seen_total > >= > + vss->buffer_bytes) > + vss->xfer_xrun = true; > + } > > + if (vss->hw_ptr >= vss->buffer_bytes) > + vss->hw_ptr -= vss->buffer_bytes; > + > msg->length = 0; > + > + /* Keep latched errors until prepare establishes a fresh epoch. */ > vss->msg_count--; > > - if (vss->xfer_enabled) { > - if (!capture_invalid) { > - struct snd_pcm_runtime *runtime = > vss->substream->runtime; > + if (vss->xfer_enabled || > + (vss->direction == SNDRV_PCM_STREAM_CAPTURE && vss->xfer_xrun)) { > + struct snd_pcm_runtime *runtime = vss->substream->runtime; > > - runtime->delay = > - bytes_to_frames(runtime, > - > le32_to_cpu(msg->status.latency_bytes)); > - } > - schedule_work(&vss->elapsed_period); > + runtime->delay = bytes_to_frames(runtime, latency_bytes); > + virtsnd_pcm_schedule_period_elapsed(vss); > } > if (!vss->msg_count) > wake_up_all(&vss->msg_empty); > } [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 1496725cba005..d953cd4babf6b 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > + switch (command) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + scoped_guard(spinlock_irqsave, &vss->lock) > + running = vss->device_running; > + /* Preserve queued requests and unread RX data across native > pause. */ > + rc = virtsnd_pcm_start_queue(vss, epoch); > + if (rc) { > + virtsnd_pcm_trigger_failed(vss, epoch); > + return rc; > } > + > + if (running) { > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) > + virtsnd_pcm_schedule_period_elapsed(vss); > + return 0; > + } > + > + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, > + GFP_KERNEL, epoch); > if (!msg) { > - virtsnd_pcm_start_failed(vss); > + virtsnd_pcm_trigger_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_trigger_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->direction == SNDRV_PCM_STREAM_CAPTURE) > + 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; > } > > - msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, > - GFP_KERNEL); > - if (!msg) > - return -ENOMEM; > + rc = virtsnd_pcm_stop_device(vss, epoch); > + if (rc && command != SNDRV_PCM_PAUSE_PUSH) { > + guard(spinlock_irqsave)(&vss->lock); > > - return virtsnd_ctl_msg_send_sync(snd, msg); > + if (vss->reset_epoch == epoch && !vss->resetting) { > + vss->xfer_xrun = true; > + substream->runtime->stop_operating = true; > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) > + > virtsnd_pcm_schedule_period_elapsed(vss); > + } > + } > + if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) { > + if (substream->runtime->state != > SNDRV_PCM_STATE_RUNNING) { > + /* Failed resume rollback must leave a paused > queue closed. */ > + virtsnd_pcm_trigger_failed(vss, epoch); > + } else if (virtsnd_pcm_start_queue(vss, epoch)) { > + virtsnd_pcm_trigger_failed(vss, epoch); > + } else if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { > + virtsnd_pcm_schedule_period_elapsed(vss); > + } > + } > + return rc; [Severity: Medium] Can pausing a playback stream stall playback when in-flight TX descriptors complete while vss->xfer_enabled is false? SNDRV_PCM_TRIGGER_PAUSE_PUSH clears vss->xfer_enabled before virtsnd_pcm_stop_device() sends VIRTIO_SND_R_PCM_STOP. Any TX descriptors that complete in virtsnd_pcm_msg_complete() while vss->xfer_enabled is false advance vss->hw_ptr and decrement vss->msg_count without scheduling vss->elapsed_period, and any work already queued returns early in virtsnd_pcm_period_elapsed() while the substream is paused. When playback resumes via SNDRV_PCM_TRIGGER_PAUSE_RELEASE (or recovers from a failed pause push while still running), virtsnd_pcm_trigger_inner() only schedules vss->elapsed_period for capture. If all queued TX descriptors completed while paused (vss->msg_count == 0), no further TX interrupt will arrive to run virtsnd_pcm_pb_pointer(), reclaim vss->pcm_indirect.hw_ready, or submit pending vss->pcm_indirect.sw_ready bytes. In addition, virtsnd_pcm_msg_complete() only checks for a full-buffer unseen completion wrap on capture. Because virtsnd_pcm_playback_ack_validate() allows queuing a full buffer of TX descriptors, completing all of them before virtsnd_pcm_pb_pointer() runs wraps vss->hw_ptr modulo vss->buffer_bytes back to vss->pcm_indirect.hw_io, so snd_pcm_indirect_playback_pointer() observes zero progress and leaves vss->pcm_indirect.hw_ready stuck at vss->buffer_bytes. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
