Thank you for your contribution! Sashiko AI review found 5 potential issues to consider: - [Medium] ALSA: virtio: missing playback XRUN state in virtsnd_pcm_start_failed() - [Medium] ALSA: virtio: PAUSE_PUSH rollback re-enables queue in virtsnd_pcm_trigger_inner() - [Medium] ALSA: virtio: paused ACK validation skips XRUN latch in virtsnd_pcm_cp_ack_locked() - [Medium] ALSA: virtio: unaligned appl_ptr in virtsnd_pcm_capture_staged_ack() passes hw_ptr - [Low] ALSA: virtio: commit message omits staged capture, reset, and param changes --
commit e0eac3208f73f40e886712be3453157819f1fd9f Author: Joonho Lee <[email protected]> > ALSA: virtio: gate starts during PCM prepare > > Use the prepare-completion callback to keep ACK admission closed until > every linked member finishes device prepare and the core pointer reset. > Clear the gate after success or failure, and keep failed members > unprepared so a later ACK cannot use stale accounting. > > Keep START and PAUSE_RELEASE admission checks at the virtio trigger and > deferred restart entry points. Preserve the existing capture lifecycle > and reset rules. > > Assisted-by: LLM > Signed-off-by: Joonho Lee <[email protected]> [Severity: Low] Does the commit message match the actual changes in this patch? While the changelog describes gating ACK and start admission during prepare and claims to preserve the existing capture lifecycle and reset rules, the patch adds a request-owned staged capture path, five new module parameters (capture_staging, capture_packed_status, capture_allow_short, capture_allow_empty, and capture_pause_restart), a pause-restart state machine, custom reset callbacks (virtsnd_pcm_playback_reset(), virtsnd_pcm_indirect_reset(), and virtsnd_pcm_reset()), and manual DMA buffer management replacing snd_pcm_set_managed_buffer_all(). > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index b9728d04ce608..b2a569866d5c9 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -97,46 +131,265 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = > { > }; > > static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream); > +static int virtsnd_pcm_sync_stop_locked(struct snd_pcm_substream *substream); > +static int virtsnd_pcm_pause_restart_sync_stop(struct snd_pcm_substream > *substream); > > -/** > - * virtsnd_pcm_close_ack() - Update PCM prepare admission state. > - * @substream: Kernel ALSA substream. > - * @preparing: True while driver prepare and the core pointer reset are > active. > - * > - * Context: Process context. > - */ > -static void virtsnd_pcm_close_ack(struct snd_pcm_substream *substream, > - bool preparing) > +static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss, u64 > epoch) > { > - struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + bool notify; > > - scoped_guard(pcm_stream_lock_irqsave, substream) { > - guard(spinlock_irqsave)(&vss->lock); > - vss->prepared = false; > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting || vss->reset_epoch != epoch) > + return; > vss->xfer_enabled = false; > - vss->preparing = preparing; > + vss->stopped = vss->release_required || vss->msg_count; > + notify = vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + (!vss->capture_staging || vss->runtime_active); > + if (notify) > + vss->xfer_xrun = true; > } > + vss->substream->runtime->stop_operating = true; > + if (notify) > + virtsnd_pcm_schedule_period_elapsed(vss); > } [Severity: Medium] Should virtsnd_pcm_start_failed() also latch vss->xfer_xrun and schedule vss->elapsed_period for playback streams? When SNDRV_PCM_TRIGGER_START or SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails on a playback substream, vss->stopped is set to true while vss->xfer_xrun remains false. A subsequent SNDRV_PCM_TRIGGER_PAUSE_RELEASE (such as during snd_pcm_drop() or snd_pcm_prepare() on a paused stream) then skips virtsnd_pcm_defer_resume_xrun() and fails in virtsnd_pcm_start_queue() with -EBADFD, and a start failure during snd_pcm_do_drain_init() or snd_pcm_undo_pause() leaves the stream in RUNNING or DRAINING without virtsnd_pcm_pb_pointer() ever reporting SNDRV_PCM_POS_XRUN. [ ... ] > + rc = virtsnd_pcm_stop_device(vss, epoch); > + if (rc && command != SNDRV_PCM_TRIGGER_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 (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); > + } > + } else if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) { > + virtsnd_pcm_schedule_period_elapsed(vss); > + } > + } > + return rc; [Severity: Medium] Can a failed SNDRV_PCM_TRIGGER_PAUSE_PUSH re-enable the queue while substream->runtime->state is SNDRV_PCM_STATE_PAUSED? When resuming a linked group from pause fails on a later member, snd_pcm_undo_pause() calls trigger(SNDRV_PCM_TRIGGER_PAUSE_PUSH) on earlier members while their runtime->state is still SNDRV_PCM_STATE_PAUSED and ignores the return value. If virtsnd_pcm_stop_device() fails during that rollback, calling virtsnd_pcm_start_queue() leaves vss->xfer_enabled set and capture buffers queued on a paused substream instead of latching vss->xfer_xrun and vss->stopped. [ ... ] > + appl_ptr = runtime->control->appl_ptr; > + diff = appl_ptr - vss->pcm_indirect.appl_ptr; > + if (diff < -(snd_pcm_sframes_t)(runtime->boundary / 2)) > + diff += runtime->boundary; > + if (diff < 0) > + return virtsnd_pcm_ack_xrun_locked(vss, runtime); > + appl_diff = diff; > + ack_frames = appl_diff - appl_diff % runtime->min_align; > + if (ack_frames > runtime->buffer_size || > + (appl_diff > runtime->buffer_size && > + appl_diff - runtime->buffer_size >= runtime->min_align)) { > + return virtsnd_pcm_ack_xrun_locked(vss, runtime); > + } > + > + /* ACK includes any subperiod tail already copied to the ring. > */ > + bytes = frames_to_bytes(runtime, ack_frames); > + outstanding = vss->capture_copied_total - > + vss->capture_acked_total; > + if ((!free_running && bytes > vss->capture_ready) || > + (free_running && > + (bytes > vss->buffer_bytes || bytes > outstanding)) || > + check_add_overflow(vss->capture_acked_total, (u64)bytes, > + &next_acked_total) || > + next_acked_total > vss->capture_pointer_seen_total) { > + return virtsnd_pcm_ack_xrun_locked(vss, runtime); > + } > + > + vss->capture_acked_total = next_acked_total; > + if (!free_running) > + vss->capture_ready -= bytes; > + /* Keep any unaligned frame tail for the next acknowledgment. */ > + vss->pcm_indirect.appl_ptr += ack_frames; > + if (vss->pcm_indirect.appl_ptr >= runtime->boundary) > + vss->pcm_indirect.appl_ptr -= runtime->boundary; > + } [Severity: Medium] Can an unaligned appl_ptr update in virtsnd_pcm_capture_staged_ack() advance runtime->control->appl_ptr past runtime->status->hw_ptr? For sub-byte frame formats where runtime->min_align > 1 (such as 1-channel SNDRV_PCM_FORMAT_IMA_ADPCM), rounding appl_diff down to ack_frames and returning 0 lets pcm_lib_apply_appl_ptr() commit the unaligned runtime->control->appl_ptr even when bytes == vss->capture_ready and no unread sample covers the tail frame. That puts appl_ptr ahead of hw_ptr and causes snd_pcm_capture_avail() to underflow near runtime->boundary. Rejecting unaligned appl_diff with -EBADFD, as virtsnd_pcm_capture_ack_validate() does, would prevent the partial-frame advance. [ ... ] > + scoped_guard(spinlock_irqsave, &queue->lock) { > + guard(spinlock)(&vss->lock); > + paused = runtime->state == SNDRV_PCM_STATE_PAUSED; > + if (vss->preparing) > + return -EBADFD; > + /* A latched XRUN must not re-arm sync_stop once teardown > began. */ > + if (vss->stopped && vss->xfer_xrun) > + return -EBADFD; > + if (vss->xfer_xrun) { > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + } else { > + if (!vss->prepared || > + (vss->stopped && > + runtime->state != SNDRV_PCM_STATE_DRAINING)) > + return -EBADFD; > + rc = virtsnd_pcm_capture_ack_validate(vss, runtime, > + &skip_transfer); > + if (rc == -EBADFD || (rc && paused)) > + return -EBADFD; > + if (!rc && skip_transfer) > + return 0; > + if (!rc) { > transfer.indirect = vss->pcm_indirect; > rc = > snd_pcm_indirect_capture_transfer(substream, > > &transfer.indirect, > > virtsnd_pcm_trans_copy); > } > } > if (!rc) > rc = transfer.error; > - if (rc) > - virtsnd_pcm_transfer_failed(vss); > - else > + if (rc) { > + if (paused) > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + else > + virtsnd_pcm_transfer_failed(vss); > + } else { > vss->pcm_indirect = transfer.indirect; > + } > } [Severity: Medium] Does returning -EBADFD on (rc && paused) right after virtsnd_pcm_capture_ack_validate() skip latching the paused XRUN? When virtsnd_pcm_capture_ack_validate() returns -EPIPE while paused, the early return bypasses virtsnd_pcm_ack_xrun_locked() below, so vss->xfer_xrun is never latched and the XRUN is not deferred to pause release as in virtsnd_pcm_pb_ack_locked(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
