Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] ALSA: virtio: false XRUN on capture drain in virtsnd_pcm_msg_complete() - [High] ALSA: virtio: buffer_mutex trylock in ack corrupts indirect PCM state - [High] ALSA: virtio: buffer_mutex trylock in virtsnd_pcm_pb_ack() fails during RESET - [Medium] ALSA: virtio: staged capture drops RX completions during pause in virtsnd_pcm_capture_complete() - [Medium] ALSA: virtio: missed XRUN wakeup when RX ring fills during pause in virtsnd_pcm_msg_complete() - [Medium] ALSA: virtio: virtsnd_pcm_event() skips period work for ordinary capture XRUN events - [Medium] ALSA: virtio: virtsnd_pcm_start_queue() drops period notifications - [Medium] ALSA: virtio: failed STOP in virtsnd_pcm_trigger() misses xfer_xrun latch - [Medium] ALSA: virtio: premature state reset in virtsnd_pcm_prepare_locked() - [Medium] ALSA: virtio: missing stopped check in virtsnd_pcm_capture_staged_ack() --
commit 89969d05596ec30bdd3e3e6560275f8867f0911d 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 monotonic position counters, while separating device running state, RELEASE obligations, and reset epochs from ALSA runtime state. It also serializes PCM lifecycle callbacks with lifecycle_lock and updates pause, XRUN, and period wakeup handling across ordinary and staged capture paths. Link: https://sashiko.dev/#/patchset/20261030052629.27742-1-mynameisjune111%40gmail.com?part=9 > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index be3893de40a55..aed94a892aaa4 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c [ ... ] > @@ -517,8 +630,11 @@ void virtsnd_pcm_event(struct virtio_snd *snd, struct > virtio_snd_event *event) > break; > case VIRTIO_SND_EVT_PCM_XRUN: > scoped_guard(spinlock, &vss->lock) { > - if (vss->xfer_enabled) > + if (vss->xfer_enabled) { > vss->xfer_xrun = true; > + if (vss->capture_staging) > + > virtsnd_pcm_schedule_period_elapsed(vss); > + } > } > break; > } [Severity: Medium] When VIRTIO_SND_EVT_PCM_XRUN arrives in virtsnd_pcm_event() while vss->xfer_enabled is true, why is virtsnd_pcm_schedule_period_elapsed(vss) only called when vss->capture_staging is set? In virtsnd_pcm_period_elapsed(), ordinary capture now checks vss->xfer_xrun and calls snd_pcm_progress_under_stream_lock(substream), which observes SNDRV_PCM_POS_XRUN from virtsnd_pcm_cp_pointer() and triggers __snd_pcm_xrun(). However, if the device sends VIRTIO_SND_EVT_PCM_XRUN on an ordinary capture substream (!vss->capture_staging) and stops completing RX buffers, skipping virtsnd_pcm_schedule_period_elapsed(vss) here leaves ALSA unaware of the XRUN until a read timeout expires. Should this check vss->direction == SNDRV_PCM_STREAM_CAPTURE instead of vss->capture_staging? > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index b94ab24f5ffef..cdd26729568a1 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -478,6 +530,290 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream > *vss, unsigned long offset, [ ... ] > +static void virtsnd_pcm_capture_complete(struct virtio_pcm_msg *msg, > + size_t written_bytes) > +{ > + struct virtio_pcm_substream *vss = msg->substream; > + size_t audio_bytes = 0; > + size_t next_ready = 0; > + u64 next_copied_total = 0; > + bool free_running = false; > + bool got_audio = false; > + > + guard(spinlock)(&vss->lock); > + vss->msg_count--; > + if (vss->runtime_active && vss->xfer_enabled && !vss->xfer_xrun) { > + struct snd_pcm_runtime *runtime = vss->substream->runtime; [Severity: Medium] In virtsnd_pcm_capture_complete(), does gating completion processing on vss->xfer_enabled drop in-flight RX completions during a native pause transition? When userspace triggers SNDRV_PCM_TRIGGER_PAUSE_PUSH on an active staged capture stream (with virtsnd_capture_pause_restart disabled), virtsnd_pcm_trigger() clears vss->xfer_enabled = false under vss->lock while keeping vss->prepared = true and vss->stopped = false, and then calls virtsnd_pcm_stop_device() to send VIRTIO_SND_R_PCM_STOP synchronously: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() { ... 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); } While VIRTIO_SND_R_PCM_STOP is in flight (both when STOP succeeds and when the device rejects STOP), the device can complete already-queued RX requests. Unlike ordinary capture in virtsnd_pcm_msg_complete(), which checks vss->prepared && !vss->stopped for NOMMU copyout and still updates vss->hw_ptr, vss->capture_copied_total, and vss->xfer_xrun when vss->xfer_enabled is false, virtsnd_pcm_capture_complete() skips status validation, overrun checks, memcpy() into runtime->dma_area, and counter updates, and then resets msg->length = 0. When pause is rejected or later resumed via SNDRV_PCM_TRIGGER_PAUSE_RELEASE, virtsnd_pcm_capture_enqueue() zeroes and re-queues those descriptors, permanently losing the captured audio frames and any device status or ring overrun error. [ ... ] > @@ -580,14 +924,22 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > * the actual SG payload capacity before accounting them. > */ > if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK || > - written_bytes <= sizeof(msg->status)) { > - vss->hw_ptr += msg->length; > - } else { > - size_t data_bytes = min_t(size_t, > - written_bytes - sizeof(msg->status), > - msg->length); > - > - vss->hw_ptr += data_bytes; > + written_bytes <= sizeof(*msg->status)) > + completed_bytes = msg->length; > + else > + completed_bytes = min_t(size_t, > + written_bytes - sizeof(*msg->status), > + msg->length); > + 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; > } [Severity: High] Can this unconditional update of vss->capture_copied_total in virtsnd_pcm_msg_complete() trigger a false XRUN during capture drain? When an application calls snd_pcm_drain() (SNDRV_PCM_IOCTL_DRAIN) on a running ordinary capture stream with unread frames in the buffer, snd_pcm_do_drain_init() transitions the stream to SNDRV_PCM_STATE_DRAINING and invokes virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP), which sets vss->stopped = true and vss->xfer_enabled = false and sends VIRTIO_SND_R_PCM_STOP. In SNDRV_PCM_STATE_DRAINING, snd_pcm_running(substream) is false and virtsnd_pcm_cp_pointer() is no longer called, so vss->capture_pointer_seen_total stops advancing. However, virtsnd_pcm_msg_complete() continues to add completed_bytes (including a full period msg->length when written_bytes <= sizeof(*msg->status) for empty or status-only completions) to vss->hw_ptr and vss->capture_copied_total even when vss->stopped is true. When the seeded RX ring descriptors complete after STOP or RELEASE, vss->capture_copied_total - vss->capture_pointer_seen_total reaches vss->buffer_bytes and latches vss->xfer_xrun = true. Subsequent draining reads via snd_pcm_readi() or SNDRV_PCM_IOCTL_SYNC_PTR then call virtsnd_pcm_cp_ack_locked(), which checks: if (vss->xfer_xrun || (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING)) { rc = -EPIPE; } and aborts the capture drain with -EPIPE, discarding the already-captured audio frames. > if (vss->hw_ptr >= vss->buffer_bytes) > @@ -595,7 +947,9 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > > msg->length = 0; > > - vss->xfer_xrun = false; > + /* Preserve a latched capture error until prepare recovers it. */ > + if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK) > + vss->xfer_xrun = false; > vss->msg_count--; > > if (vss->xfer_enabled) { > @@ -603,8 +957,8 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > > runtime->delay = > bytes_to_frames(runtime, > - le32_to_cpu(msg->status.latency_bytes)); > - schedule_work(&vss->elapsed_period); > + > le32_to_cpu(msg->status->latency_bytes)); > + virtsnd_pcm_schedule_period_elapsed(vss); > } else if (!vss->msg_count) { > wake_up_all(&vss->msg_empty); > } [Severity: Medium] What happens if in-flight ordinary capture RX completions fill the unread ring while SNDRV_PCM_TRIGGER_PAUSE_PUSH is waiting for VIRTIO_SND_R_PCM_STOP and the STOP request succeeds? During SNDRV_PCM_TRIGGER_PAUSE_PUSH, virtsnd_pcm_trigger() clears vss->xfer_enabled = false before calling virtsnd_pcm_stop_device(vss, epoch). If the device completes queued RX requests before processing VIRTIO_SND_R_PCM_STOP and vss->capture_copied_total - vss->capture_pointer_seen_total reaches vss->buffer_bytes, virtsnd_pcm_msg_complete() latches vss->xfer_xrun = true, but skips virtsnd_pcm_schedule_period_elapsed(vss) because vss->xfer_enabled is false. When virtsnd_pcm_stop_device() returns 0, virtsnd_pcm_trigger() only checks if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) and returns 0 without scheduling elapsed_period. ALSA transitions runtime->state to SNDRV_PCM_STATE_PAUSED, and the paused_xrun handler in virtsnd_pcm_period_elapsed() never runs to move the stream to SNDRV_PCM_STATE_XRUN, leaving readers in wait_for_avail() sleeping indefinitely. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index ad602b6b6530b..d587c8f699553 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -320,47 +706,383 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) [ ... ] > + 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; > + } > + } > + > + rc = virtsnd_pcm_dev_prepare(vss, epoch); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + vss->prepared = !rc; > + if (!rc) > + vss->release_required = true; > + } > + if (pause_restart) { > + if (rc) { > + if (rc == -ECANCELED) > + return rc; > + virtsnd_pcm_pause_restart_fail(vss, false, epoch); > + } else { > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting || vss->reset_epoch != epoch) > + return -ECANCELED; > + vss->stopped = false; > + vss->xfer_xrun = false; > + } > + } > + } > + > + return rc; > +} [Severity: Medium] In virtsnd_pcm_prepare_locked(), if virtsnd_pcm_dev_prepare() fails when pause_restart is false, doesn't clearing vss->xfer_xrun and resetting vss->pcm_indirect and the capture counters before sending VIRTIO_SND_R_PCM_PREPARE leave the driver out of sync with ALSA's runtime state? When userspace invokes SNDRV_PCM_IOCTL_PREPARE on a capture substream in SNDRV_PCM_STATE_XRUN or SNDRV_PCM_STATE_DRAINING, snd_pcm_do_prepare() calls virtsnd_pcm_prepare_locked(). Before virtsnd_pcm_dev_prepare() is called, this block clears vss->xfer_xrun = false (when !pause_restart), calls virtsnd_pcm_msg_reset(vss), and zeroes vss->pcm_indirect. If virtsnd_pcm_dev_prepare() then returns an error, snd_pcm_do_prepare() aborts before snd_pcm_do_reset() and snd_pcm_post_prepare(), leaving runtime->state and runtime->control->appl_ptr unchanged. Because only the pause_restart branch restores vss->xfer_xrun = true and vss->stopped = true on failure: - If the stream was in SNDRV_PCM_STATE_XRUN, the latched vss->xfer_xrun state is lost even though prepare failed. - If the stream was in SNDRV_PCM_STATE_DRAINING, runtime->state remains SNDRV_PCM_STATE_DRAINING with runtime->control->appl_ptr at its pre-prepare value while vss->pcm_indirect is zeroed and vss->prepared, vss->stopped, and vss->xfer_xrun are all false; a subsequent read or SNDRV_PCM_IOCTL_SYNC_PTR in SNDRV_PCM_STATE_DRAINING bypasses the !vss->prepared and vss->stopped checks in virtsnd_pcm_cp_ack_locked() and subtracts appl_ptr from a zeroed vss->pcm_indirect.sw_ready. [ ... ] > +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->capture_rx_seeded) { > + rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes); > + if (!rc) > + vss->capture_rx_seeded = true; > + } > + } > if (!rc) { > - guard(spinlock_irqsave)(&vss->lock); > - vss->release_required = true; > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + !vss->capture_staging) { > + size_t period = > snd_pcm_lib_period_bytes(vss->substream); > + > + /* A synthetic resume notification is not a new period > IRQ. */ > + vss->capture_notified_periods = period ? > + div64_ul(vss->capture_copied_total, period) : 0; > + } > + vss->xfer_enabled = true; > } > return rc; > } [Severity: Medium] Does overwriting vss->capture_notified_periods in virtsnd_pcm_start_queue() drop period notifications for RX periods that completed while vss->xfer_enabled was false? During SNDRV_PCM_TRIGGER_PAUSE_PUSH, virtsnd_pcm_trigger() clears vss->xfer_enabled = false before calling virtsnd_pcm_stop_device(). If one or more RX periods complete in virtsnd_pcm_msg_complete() while vss->xfer_enabled is false (either before STOP takes effect or when STOP is rejected by the device), vss->capture_copied_total advances across a period boundary without scheduling vss->elapsed_period. When virtsnd_pcm_start_queue() subsequently runs on rejected PAUSE_PUSH recovery or on SNDRV_PCM_TRIGGER_PAUSE_RELEASE, it overwrites vss->capture_notified_periods with div64_ul(vss->capture_copied_total, period) before virtsnd_pcm_period_elapsed() executes. When virtsnd_pcm_period_elapsed() then runs: sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() { ... partial = vss->xfer_xrun || period_floor <= vss->capture_notified_periods; if (!partial) vss->capture_notified_periods = period_floor; } period_floor <= vss->capture_notified_periods is forced to true (partial = true), so it calls snd_pcm_progress_under_stream_lock() instead of snd_pcm_period_elapsed_under_stream_lock(), losing the period notification, runtime->hw_ptr_interrupt update, and PCM timer tick for the completed periods. > @@ -377,72 +1099,226 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) [ ... ] > 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) { > + 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] If virtsnd_pcm_stop_device() fails during SNDRV_PCM_TRIGGER_STOP on a capture stream, should vss->xfer_xrun also be latched here? When userspace invokes SNDRV_PCM_IOCTL_DRAIN on a running capture stream with unread frames, snd_pcm_do_drain_init() calls snd_pcm_do_stop(substream, SNDRV_PCM_STATE_DRAINING), which ignores the return value of substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP) and sets runtime->state to SNDRV_PCM_STATE_DRAINING. When virtsnd_pcm_stop_device() fails, vss->device_running remains true. Unlike virtsnd_pcm_pause_restart_stop(), which calls virtsnd_pcm_pause_restart_failed() to set vss->xfer_xrun = true, normal-mode virtsnd_pcm_trigger() only handles rc != 0 for SNDRV_PCM_TRIGGER_PAUSE_PUSH and leaves vss->xfer_xrun false. Because both virtsnd_pcm_capture_staged_ack() and virtsnd_pcm_cp_ack_locked() allow ACK in SNDRV_PCM_STATE_DRAINING when vss->stopped is true unless vss->xfer_xrun is set, draining capture reads and ACKs can proceed while the device is still running and DMA-writing into the capture buffer. [ ... ] > @@ -606,17 +1560,134 @@ static int virtsnd_pcm_pb_ack(struct > snd_pcm_substream *substream) > return 0; > } > > -static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream) > +/* The PCM lock is held; exclude every member's buffer reconfiguration. */ > +static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + int rc; > + > + if (!mutex_trylock(&runtime->buffer_mutex)) > + return -EBADFD; > + rc = virtsnd_pcm_pb_ack_locked(substream); > + mutex_unlock(&runtime->buffer_mutex); > + return rc; > +} [Severity: High] Can failing mutex_trylock(&runtime->buffer_mutex) in virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() corrupt vss->pcm_indirect when another thread transiently holds runtime->buffer_mutex on a running stream? In sound/core/pcm_native.c, snd_pcm_hw_refine_with_lock(), snd_pcm_hw_params(), snd_pcm_hw_free(), and snd_pcm_action_nonatomic() acquire runtime->buffer_mutex via snd_pcm_buffer_access_lock() (or mutex_lock_nested() on linked group members in snd_pcm_action_group()) before acquiring the PCM stream lock to check whether runtime->state is SNDRV_PCM_STATE_RUNNING. Meanwhile, virtsnd_pcm_period_elapsed(), snd_pcm_status64(), and snd_pcm_sync_ptr() hold the PCM stream lock without incrementing runtime->buffer_accessing. If virtsnd_pcm_period_elapsed() or snd_pcm_status64() runs while another thread holds runtime->buffer_mutex waiting for the PCM stream lock, virtsnd_pcm_pb_pointer() or virtsnd_pcm_cp_pointer() calls snd_pcm_indirect_playback_pointer() or snd_pcm_indirect_capture_pointer(), which first mutates vss->pcm_indirect in place (updating hw_io, hw_ready, and sw_io) and then invokes substream->ops->ack(substream). When mutex_trylock(&runtime->buffer_mutex) fails and returns -EBADFD, snd_pcm_indirect_playback_pointer() and snd_pcm_indirect_capture_pointer() ignore the error (only checking err == -EPIPE), leaving vss->pcm_indirect half-updated without running snd_pcm_indirect_playback_transfer() or snd_pcm_indirect_capture_transfer(). Concurrent SNDRV_PCM_IOCTL_SYNC_PTR calls without HWSYNC will also fail with -EBADFD whenever another thread is in the middle of an ioctl that locks buffer_mutex before checking stream state. > + > +static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss, > + struct snd_pcm_runtime *runtime) > +{ > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + int rc = 0; > + > + /* > + * The RX callback holds the queue lock while draining completions. > + * Account ALSA's read under the substream lock alone so completions > later > + * in that drain observe the newly released ring space. > + */ > + scoped_guard(spinlock_irqsave, &vss->lock) { > + snd_pcm_uframes_t appl_ptr; > + snd_pcm_sframes_t diff; > + size_t bytes; > + u64 outstanding; > + u64 next_acked_total; > + bool free_running; > + > + if (!vss->runtime_active || !runtime || !runtime->control || > + !runtime->dma_area || vss->suspended) > + return -EBADFD; > + if (!vss->prepared && runtime->state != > SNDRV_PCM_STATE_DRAINING && > + !(runtime->state == SNDRV_PCM_STATE_PAUSED && > + virtsnd_pcm_pause_restart_enabled(vss) && > + vss->pause_restart_state == > VIRTSND_PCM_PAUSE_RESTART_RELEASED)) > + return -EBADFD; > + if (vss->xfer_xrun) > + return -EPIPE; [Severity: Medium] Should virtsnd_pcm_capture_staged_ack() also check vss->stopped (or verify that runtime->state permits acknowledgments), similar to virtsnd_pcm_cp_ack_locked()? When ALSA transitions a running staged capture substream to SNDRV_PCM_STATE_XRUN without the virtio driver setting vss->xfer_xrun (for example, when snd_pcm_update_state() detects avail >= runtime->stop_threshold, or when userspace issues SNDRV_PCM_IOCTL_XRUN via snd_pcm_xrun()), __snd_pcm_xrun() calls snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN), which invokes virtsnd_pcm_trigger(substream, SNDRV_PCM_TRIGGER_STOP) without calling snd_pcm_sync_stop(). That sets vss->stopped = true and vss->xfer_enabled = false while leaving vss->prepared = true and vss->xfer_xrun = false. Similarly, if SNDRV_PCM_IOCTL_DROP is called while in SNDRV_PCM_STATE_PREPARED, snd_pcm_do_stop() transitions runtime->state to SNDRV_PCM_STATE_SETUP without setting stop_operating, leaving vss->prepared = true. If userspace then invokes SNDRV_PCM_IOCTL_SYNC_PTR without SNDRV_PCM_SYNC_PTR_HWSYNC or SNDRV_PCM_SYNC_PTR_APPL, snd_pcm_sync_ptr() calls pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack() -> virtsnd_pcm_capture_staged_ack(). Because virtsnd_pcm_capture_staged_ack() does not check (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING), it accepts the ACK and advances vss->capture_acked_total, vss->capture_ready, and vss->pcm_indirect.appl_ptr while the stream is stopped in SNDRV_PCM_STATE_XRUN or SNDRV_PCM_STATE_SETUP. [ ... ] > @@ -641,6 +1712,50 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream > *substream) > return 0; > } > > +/* > + * Linked prepare locks each member's buffer_mutex, while buffer_accessing is > + * negative only on its initiating member. ACK must exclude every prepare. > + * The PCM lock is already held, so a failed trylock must return immediately. > + */ > +static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + int rc; > + > + if (!mutex_trylock(&runtime->buffer_mutex)) > + return -EBADFD; > + rc = virtsnd_pcm_cp_ack_locked(substream); > + mutex_unlock(&runtime->buffer_mutex); > + return rc; > +} [Severity: High] Won't mutex_trylock(&runtime->buffer_mutex) in virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() always fail during SNDRV_PCM_IOCTL_RESET on a running stream? When userspace invokes SNDRV_PCM_IOCTL_RESET, snd_pcm_reset() calls snd_pcm_action_nonatomic(&snd_pcm_action_reset, substream, ACTION_ARG_IGNORE), which acquires substream->runtime->buffer_mutex via snd_pcm_buffer_access_lock() (and locks runtime->buffer_mutex on all linked group members in snd_pcm_action_group()). While holding runtime->buffer_mutex on the same thread, snd_pcm_do_reset() calls snd_pcm_lib_ioctl_reset() (or snd_pcm_lib_reset()). When the stream is running, snd_pcm_lib_ioctl_reset() acquires the PCM stream lock and calls snd_pcm_update_hw_ptr(substream) -> substream->ops->pointer(substream). For playback (virtsnd_pcm_pb_pointer()) and ordinary capture (virtsnd_pcm_cp_pointer()), snd_pcm_indirect_playback_pointer() and snd_pcm_indirect_capture_pointer() first mutate vss->pcm_indirect in place (updating hw_io, hw_ready, and sw_io) and then invoke substream->ops->ack(substream). Because the calling thread already holds runtime->buffer_mutex, mutex_trylock(&runtime->buffer_mutex) in virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() is guaranteed to fail and return -EBADFD. snd_pcm_indirect_playback_pointer() and snd_pcm_indirect_capture_pointer() ignore -EBADFD (only checking err == -EPIPE) and skip snd_pcm_indirect_playback_transfer() / snd_pcm_indirect_capture_transfer() after vss->pcm_indirect has already been modified. Immediately afterwards, snd_pcm_post_reset() sets runtime->control->appl_ptr = runtime->status->hw_ptr without updating vss->pcm_indirect.appl_ptr, causing the next playback ACK or period completion to compute a negative diff = runtime->control->appl_ptr - rec->appl_ptr and abort the stream with -EPIPE. Later commits in this series ("ALSA: pcm: add an optional reset operation under the stream lock", "ALSA: virtio: synchronize staged capture accounting during RESET", and "ALSA: virtio: rebase ordinary capture accounting on RESET") add .reset callbacks for capture streams, but playback streams in virtsnd_pcm_ops[0] still have no .reset callback or bypass in virtsnd_pcm_pb_ack(), leaving SNDRV_PCM_IOCTL_RESET broken on running playback streams. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
