Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] ALSA: virtio: virtsnd_pcm_prepare_locked() fails after playback pre-buffering - [High] ALSA: virtio: virtsnd_pcm_trigger() sends PCM_STOP when stream is not running - [High] ALSA: virtio: virtsnd_pcm_start_failed() marks unprepared stream stopped - [Medium] ALSA: virtio: missed period wakeup in virtsnd_pcm_period_elapsed() - [Medium] ALSA: virtio: unreleased device stream on failed re-prepare in virtsnd_pcm_prepare_locked() - [Medium] ALSA: virtio: reset_epoch TOCTOU in virtsnd_pcm_send_sync() helpers - [Low] ALSA: virtio: virtsnd_pcm_trigger() mixes goto cleanup with guard() helpers --
commit 775188574a684cd4f7321eccd1403d5cf535ff0a Author: Joonho Lee <[email protected]> ALSA: virtio: add staged capture accounting and lifecycle recovery This commit introduces staged capture with separate produced, observed, and consumed positions using request-owned buffers, along with a free-running capture policy for boundary stop thresholds. It also adds a lifecycle mutex, reset epoch tracking, and an opt-in pause-restart state machine to handle control transitions and device reset recovery. > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index be3893de40a55..ecd017edba72c 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c > @@ -310,8 +310,98 @@ static void virtsnd_pcm_period_elapsed(struct > work_struct *work) > { > struct virtio_pcm_substream *vss = > container_of(work, struct virtio_pcm_substream, elapsed_period); > + struct snd_pcm_substream *substream = vss->substream; > + struct snd_pcm_runtime *runtime; > + size_t period_bytes; > + u64 period_floor; > + u64 snapshot; > + bool staged_capture; > + bool paused_xrun = false; > + bool notify = false; > + bool requeue = false; [ ... ] > + scoped_guard(spinlock_irqsave, &vss->lock) { > + runtime = substream->runtime; > + if (!vss->runtime_active || > + (!vss->xfer_enabled && !vss->xfer_xrun) || !runtime) > + return; > + > + period_bytes = snd_pcm_lib_period_bytes(substream); > + if (vss->xfer_xrun) { > + notify = true; > + } else if (!period_bytes || period_bytes >= vss->buffer_bytes || > + !virtsnd_pcm_capture_counters_valid(vss) || > + vss->capture_notified_periods > > + div64_ul(vss->capture_copied_total, > period_bytes)) { > + vss->xfer_xrun = true; > + notify = true; > + } else { > + period_floor = div64_ul(vss->capture_copied_total, > period_bytes); > + if (period_floor > vss->capture_notified_periods) { > + /* Reserve crossed boundaries before the > callback. */ > + vss->capture_notified_periods = period_floor; > + notify = true; > + } > + } > + > + if (notify) { > + snapshot = vss->capture_copied_total; > + vss->capture_callback_snapshot = snapshot; > + vss->capture_callback_snapshot_active = true; > + } > + } [Severity: Medium] When virtsnd_capture_staging and virtsnd_capture_allow_short are enabled, can gating period notifications strictly on integer multiples of period_bytes in virtsnd_pcm_period_elapsed() miss a period wakeup and trigger a ring_capacity xrun? When virtsnd_pcm_period_elapsed() sets vss->capture_callback_snapshot to vss->capture_copied_total (or when virtsnd_pcm_cp_pointer() is called directly), any sub-period tail is exposed to the reader and acknowledged via virtsnd_pcm_cp_ack() -> virtsnd_pcm_capture_staged_ack(). If a subsequent short completion in virtsnd_pcm_capture_complete() crosses the next integer multiple of period_bytes while total unacknowledged data (vss->capture_copied_total - vss->capture_acked_total) is still less than one period, virtsnd_pcm_period_elapsed() advances vss->capture_notified_periods to period_floor and calls snd_pcm_period_elapsed_under_stream_lock(), which leaves the reader sleeping because available frames are below avail_min. If another short completion then arrives within that same period_floor interval and brings unacknowledged bytes in vss->capture_ready to at least period_bytes without crossing the next integer period boundary, period_floor > vss->capture_notified_periods is false and notify remains false. On a two-period buffer, leaving at least period_bytes unacknowledged in vss->capture_ready causes the next full-period completion in virtsnd_pcm_capture_complete() to fail: else if (!free_running && audio_bytes >= vss->buffer_bytes - vss->capture_ready) invalid_reason = "ring_capacity"; Should virtsnd_pcm_period_elapsed() also notify when unacknowledged bytes reach period_bytes even if period_floor has not advanced? > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index f74bf365065b6..66560d82b7f8e 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -97,14 +128,238 @@ 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); > > -static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss) > +static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss, u64 > epoch) > { > + bool notify; > + > scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting || vss->reset_epoch != epoch) > + return; > vss->xfer_enabled = false; > vss->stopped = true; > + notify = vss->runtime_active && vss->capture_staging && > + vss->direction == SNDRV_PCM_STREAM_CAPTURE; > + if (notify) > + vss->xfer_xrun = true; > } > vss->substream->runtime->stop_operating = true; > + if (notify) > + virtsnd_pcm_schedule_period_elapsed(vss); > +} [Severity: High] Does unconditionally setting vss->stopped = true in virtsnd_pcm_start_failed() cause virtsnd_pcm_sync_stop_locked() to send an invalid release command when a stream is not prepared on the device? After a device reset and restore (virtsnd_reset_vqs() -> virtsnd_restore()), or after a failed re-prepare in virtsnd_pcm_prepare_locked(), vss->prepared is false, vss->stopped is false, and vss->msg_count is 0, while the substream runtime state can still be paused or prepared. If userspace then drops, prepares, or closes a paused substream (calling snd_pcm_pause() -> virtsnd_pcm_trigger()), or starts a prepared substream, virtsnd_pcm_trigger() sees !vss->prepared and jumps to start_failed: if (vss->stopped || !vss->prepared || vss->xfer_xrun) { rc = vss->xfer_xrun ? -EPIPE : -EBADFD; goto start_failed; } virtsnd_pcm_start_failed() then sets vss->stopped = true and runtime->stop_operating = true even though the device stream was never prepared in the current epoch and has no queued messages. When virtsnd_pcm_sync_stop_locked() subsequently runs, need_release evaluates to true because vss->stopped is set, so it sends a release control message for an unprepared stream. If the device rejects that release request, virtsnd_pcm_sync_stop_locked() returns an error with vss->stopped still true, wedging subsequent prepare, hw_params, and open calls. Should virtsnd_pcm_start_failed() only set vss->stopped and runtime->stop_operating when vss->prepared || vss->msg_count is true? [ ... ] > +static int virtsnd_pcm_send_sync(struct virtio_pcm_substream *vss, > + struct virtio_snd_msg *msg) > +{ > + u64 epoch; > + int rc; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting) { > + virtsnd_ctl_msg_unref(msg); > + return -ECANCELED; > + } > + epoch = vss->reset_epoch; > + } > + rc = virtsnd_ctl_msg_send_sync(vss->snd, msg); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + } > + return rc; > +} > + > +static int > +virtsnd_pcm_pause_restart_send(struct virtio_pcm_substream *vss, > + struct virtio_snd_msg *msg, > + unsigned long allowed_states, > + enum virtio_pcm_pause_restart_state > submitted_state, > + enum virtio_pcm_pause_restart_state > success_state) > +{ > + enum virtio_pcm_pause_restart_state state; > + bool uncertain; > + u64 epoch; > + int rc; > + > + if (!virtsnd_pcm_pause_restart_enabled(vss)) > + return virtsnd_pcm_send_sync(vss, msg); > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + epoch = vss->reset_epoch; > + state = vss->pause_restart_state; > + if (vss->resetting) { > + rc = -ECANCELED; > + } else if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED) { > + rc = -EIO; > + } else if (virtsnd_pcm_pause_restart_transitioning(state)) { > + vss->pause_restart_state = > + VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; > + rc = -EIO; > + } else if (!(allowed_states & BIT(state))) { > + rc = -EBADFD; > + } else { > + vss->pause_restart_state = submitted_state; > + rc = 0; > + } > + } [Severity: Medium] Can re-sampling vss->reset_epoch inside virtsnd_pcm_send_sync() and virtsnd_pcm_pause_restart_send() allow a control request from before a device reset to be sent in the new epoch? Callers such as virtsnd_pcm_trigger(), virtsnd_pcm_prepare_locked(), virtsnd_pcm_sync_stop_locked(), virtsnd_pcm_pause_release_flush(), virtsnd_pcm_pause_restart_start(), virtsnd_pcm_pause_restart_pause(), and virtsnd_pcm_pause_restart_sync_stop() check substream state (and in the start paths, queue virtqueue buffers) under vss->lock while recording epoch = vss->reset_epoch, then drop vss->lock to call cancel_work_sync() or virtsnd_pcm_ctl_msg_alloc() before calling virtsnd_pcm_send_sync() or virtsnd_pcm_pause_restart_send(). If virtsnd_reset_vqs() and virtsnd_restore() both complete during that unlocked window, vss->resetting is false again and vss->reset_epoch has been incremented. Because virtsnd_pcm_send_sync() and virtsnd_pcm_pause_restart_send() overwrite their local epoch variable with the new vss->reset_epoch instead of checking the caller's epoch, they can send a stale start, stop, release, or prepare command to the freshly restored device, or fail the state check against the reset pause_restart_state with -EBADFD instead of -ECANCELED. Should virtsnd_pcm_send_sync() and virtsnd_pcm_pause_restart_send() take the caller's expected epoch parameter and return -ECANCELED if vss->reset_epoch no longer matches? [ ... ] > @@ -277,27 +587,90 @@ static int virtsnd_pcm_hw_params(struct > snd_pcm_substream *substream, [ ... ] > static int virtsnd_pcm_prepare_locked(struct snd_pcm_substream *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_device *vdev = vss->snd->vdev; > - struct virtio_snd_msg *msg; > + bool pause_restart = virtsnd_pcm_pause_restart_enabled(vss); > + bool free_running; > + int rc; > bool stopped; > + bool suspended; > + u64 epoch; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting) > + return -ECANCELED; > + stopped = vss->stopped; > + } > + if (stopped) {\n+ rc = > virtsnd_pcm_sync_stop_locked(substream); > + if (rc) > + return rc; > + } > + scoped_guard(spinlock_irqsave, &vss->lock) { > + vss->prepared = false; > + epoch = vss->reset_epoch; > + suspended = vss->suspended; > + } > + > + if (pause_restart) { > + enum virtio_pcm_pause_restart_state state; > + > + scoped_guard(spinlock_irqsave, &vss->lock) > + state = vss->pause_restart_state; > + if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || > + virtsnd_pcm_pause_restart_transitioning(state)) { > + virtsnd_pcm_pause_restart_fail(vss, true, epoch); > + return -EIO; > + } > + } > > scoped_guard(spinlock_irqsave, &vss->lock) > stopped = vss->stopped; > @@ -308,44 +681,320 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) > return -EBADFD; > } > > - if (!vss->suspended) { > + if (!suspended) { > if (virtsnd_pcm_msg_pending_num(vss)) { > dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", > vss->sid); > return -EBADFD; > } > > - vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > - vss->hw_ptr = 0; [Severity: High] What happens in virtsnd_pcm_prepare_locked() if a playback stream queues at least one period while in the prepared state before reaching runtime->start_threshold, and userspace then calls snd_pcm_drop() followed by snd_pcm_prepare() (or calls snd_pcm_prepare() directly)? During playback pre-buffering, pcm_lib_apply_appl_ptr() -> virtsnd_pcm_pb_ack() -> snd_pcm_indirect_playback_transfer() -> virtsnd_pcm_trans_copy() -> virtsnd_pcm_msg_send() submits full periods to the tx virtqueue and increments vss->msg_count while the stream is still in the prepared state and vss->stopped is false. Because snd_pcm_do_stop() only invokes trigger(stop) when the stream is running or paused, vss->stopped remains false. When virtsnd_pcm_prepare_locked() runs (and similarly in virtsnd_pcm_sync_stop()), it only checks if (stopped) before calling virtsnd_pcm_sync_stop_locked(substream), skipping the release flush even though vss->msg_count > 0. It then clears vss->prepared = false and fails the virtsnd_pcm_msg_pending_num(vss) check with -EBADFD, leaving pending messages in the queue with vss->stopped == false and vss->prepared == false so subsequent prepare or write calls cannot recover until hw_params or hw_free. Should virtsnd_pcm_prepare_locked() and virtsnd_pcm_sync_stop() also run virtsnd_pcm_sync_stop_locked() when vss->msg_count > 0? > } else { > struct snd_pcm_runtime *runtime = substream->runtime; > unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > unsigned int period_bytes = snd_pcm_lib_period_bytes(substream); > - int rc; > > rc = virtsnd_pcm_dev_set_params(vss, buffer_bytes, period_bytes, > runtime->channels, > runtime->format, runtime->rate); > - if (rc) > + if (rc) { > + if (pause_restart && rc != -ECANCELED) > + virtsnd_pcm_pause_restart_fail(vss, false, > epoch); > return rc; > + } > } > > - vss->xfer_xrun = false; > - vss->suspended = false; > - vss->msg_count = 0; > - virtsnd_pcm_msg_reset(vss); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + vss->prepared = false; > + vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > + vss->hw_ptr = 0; > + if (!pause_restart) > + vss->xfer_xrun = false; > + vss->suspended = false; > + vss->msg_count = 0; > + virtsnd_pcm_msg_reset(vss); > + memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect)); > + vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes; > + vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes; > + if (vss->capture_staging) { > + vss->capture_boundary = substream->runtime->boundary; > + rc = virtsnd_pcm_capture_refresh_mode_locked(vss, > + &free_running); > + if (rc) > + return rc; > + } > + } > > - memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect)); > - vss->pcm_indirect.sw_buffer_size = > - vss->pcm_indirect.hw_buffer_size = > - snd_pcm_lib_buffer_bytes(substream); > + rc = virtsnd_pcm_dev_prepare(vss); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + vss->prepared = !rc; > + } [Severity: Medium] In virtsnd_pcm_prepare_locked(), if the substream was already prepared on the device (vss->prepared was true, vss->stopped was false) and userspace calls prepare again without pause_restart enabled, what happens if virtsnd_pcm_dev_prepare() fails (or if virtsnd_pcm_dev_set_params() or virtsnd_pcm_capture_refresh_mode_locked() fails earlier)? Because vss->prepared is cleared to false at the start of virtsnd_pcm_prepare_locked() without recording that the device stream was already prepared, a failure when !pause_restart leaves both vss->prepared and vss->stopped false. When virtsnd_pcm_hw_free() later runs on close: scoped_guard(spinlock_irqsave, &vss->lock) { vss->stopped |= vss->prepared; vss->prepared = false; vss->runtime_active = false; vss->xfer_enabled = false; } rc = virtsnd_pcm_sync_stop_locked(substream); virtsnd_pcm_sync_stop_locked() sees vss->prepared, vss->stopped, and vss->msg_count all false/zero and skips sending the release command to the device. Should a failed re-prepare preserve the need to release a previously prepared device stream (or release it before re-preparing)? [ ... ] > @@ -372,38 +1042,51 @@ static int virtsnd_pcm_trigger(struct > snd_pcm_substream *substream, int command) [ ... ] > 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; > } > > msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, > GFP_KERNEL); > if (!msg) > return -ENOMEM; > > - return virtsnd_ctl_msg_send_sync(snd, msg); > + return virtsnd_pcm_send_sync(vss, msg); [Severity: High] When virtsnd_pcm_pause_restart_enabled(vss) is false, can the stop and pause_push cases in virtsnd_pcm_trigger() send a stop control request when the device stream is not running? Unlike virtsnd_pcm_pause_restart_stop(), which returns 0 when the state is not running, this path only checks if (!vss->prepared). That appears to allow sending an invalid or duplicate stop request in two situations: 1. When a running stream is paused via pause_push, virtsnd_pcm_trigger() sends a stop request to the device while leaving vss->prepared = true and setting vss->xfer_enabled = false. If that paused stream is then stopped (for example via snd_pcm_drop(), close, or paused xrun handling in virtsnd_pcm_period_elapsed() -> snd_pcm_stop()), snd_pcm_do_stop() invokes virtsnd_pcm_trigger() with the stop command while vss->prepared is still true, sending a second stop request to a stream that is already stopped on the device. 2. If start or pause_release fails in virtsnd_pcm_trigger() on an unlinked stream, virtsnd_pcm_start_failed() leaves vss->prepared = true. In snd_pcm_action_single(), the error path calls ops->undo_action() (snd_pcm_undo_start() or snd_pcm_undo_pause()), which invokes virtsnd_pcm_trigger() with stop or pause_push and sends a stop request even though start failed. Also, if pause_push fails in virtsnd_pcm_ctl_msg_alloc() or virtsnd_pcm_send_sync(), vss->xfer_enabled has already been cleared to false without marking the stream stopped, and snd_pcm_action_single() will call snd_pcm_undo_pause() -> virtsnd_pcm_trigger(pause_release) on the still-running device stream. Should the non-pause-restart stop/pause path check whether the stream was actually running before sending the stop request, and call virtsnd_pcm_start_failed() if sending stop for pause_push fails? > default: > return -EINVAL; > } > + > +start_failed: > + virtsnd_pcm_start_failed(vss, epoch); > + return rc; > +} [Severity: Low] In virtsnd_pcm_trigger(), the start and pause_release case uses scoped_guard(spinlock_irqsave, &queue->lock) and guard(spinlock)(&vss->lock) and jumps out of the guarded scope via goto start_failed. Since include/linux/cleanup.h discourages mixing goto labels with guard() and scoped_guard() in the same function, could virtsnd_pcm_trigger() break out of the scoped_guard block and handle the error without a goto label? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
