* [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:09 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start Joonho Lee
` (9 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
virtsnd_pcm_msg_send() can fail while queueing a transfer. Restore the
failed message's length and notify the device about earlier messages
queued by the same call, so they do not remain stranded.
Propagate that failure through the indirect PCM transfer callback instead
of discarding it. Commit the updated indirect state only after successful
submission and stop further transfers when a send fails. Return an XRUN
error to ALSA so the stream can recover.
Keep the polling feature path unchanged.
Fixes: fe981e67568c ("ALSA: virtio: use ack callback")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Changes in v3:
- No code or commit-message changes from v2.
sound/virtio/virtio_pcm_msg.c | 12 +++++
sound/virtio/virtio_pcm_ops.c | 82 +++++++++++++++++++++++++++++++----
2 files changed, 85 insertions(+), 9 deletions(-)
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 72103f606716..5371f4985fd0 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -260,6 +260,18 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
dev_err(&vdev->dev,
"SID %u: failed to send I/O message\n",
vss->sid);
+ msg->length -= n;
+
+ /*
+ * Do not leave earlier messages queued without notifying
+ * the device when a later message could not be added.
+ */
+ if (msg_count != vss->msg_count &&
+ !(vss->features &
+ (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
+ virtqueue_kick_prepare(vqueue))
+ virtqueue_notify(vqueue);
+
return rc;
}
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index 1091100a4c98..a40ea394bf17 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -466,36 +466,100 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
vss->hw_ptr);
}
+struct virtsnd_pcm_transfer {
+ struct snd_pcm_indirect indirect;
+ int error;
+};
+
static void virtsnd_pcm_trans_copy(struct snd_pcm_substream *substream,
struct snd_pcm_indirect *rec, size_t bytes)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct virtsnd_pcm_transfer *transfer =
+ container_of(rec, struct virtsnd_pcm_transfer, indirect);
+
+ if (transfer->error)
+ return;
- virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
+ transfer->error = virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
+}
+
+/* Caller holds vss->lock. */
+static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss)
+{
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ vss->substream->runtime->stop_operating = true;
}
static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct virtsnd_pcm_transfer transfer = {};
+ int rc;
- guard(spinlock_irqsave)(&queue->lock);
- guard(spinlock)(&vss->lock);
+ scoped_guard(spinlock_irqsave, &queue->lock) {
+ guard(spinlock)(&vss->lock);
+ transfer.indirect = vss->pcm_indirect;
+ if (vss->stopped) {
+ rc = -EPIPE;
+ } else {
+ rc = snd_pcm_indirect_playback_transfer(substream,
+ &transfer.indirect,
+ virtsnd_pcm_trans_copy);
+ }
+ if (!rc)
+ rc = transfer.error;
+ if (rc)
+ virtsnd_pcm_transfer_failed(vss);
+ else
+ vss->pcm_indirect = transfer.indirect;
+ }
+ if (rc) {
+ dev_err(&vss->snd->vdev->dev,
+ "SID %u: playback ACK transfer failed: %d\n",
+ vss->sid, rc);
+ /* ALSA treats -EPIPE as an XRUN in ACK and pointer paths. */
+ return -EPIPE;
+ }
- return snd_pcm_indirect_playback_transfer(substream, &vss->pcm_indirect,
- virtsnd_pcm_trans_copy);
+ return 0;
}
static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct virtsnd_pcm_transfer transfer = {};
+ int rc;
- guard(spinlock_irqsave)(&queue->lock);
- guard(spinlock)(&vss->lock);
+ scoped_guard(spinlock_irqsave, &queue->lock) {
+ guard(spinlock)(&vss->lock);
+ transfer.indirect = vss->pcm_indirect;
+ if (vss->stopped) {
+ rc = -EPIPE;
+ } else {
+ 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
+ vss->pcm_indirect = transfer.indirect;
+ }
+ if (rc) {
+ dev_err(&vss->snd->vdev->dev,
+ "SID %u: capture ACK transfer failed: %d\n",
+ vss->sid, rc);
+ /* ALSA treats -EPIPE as an XRUN in ACK and pointer paths. */
+ return -EPIPE;
+ }
- return snd_pcm_indirect_capture_transfer(substream, &vss->pcm_indirect,
- virtsnd_pcm_trans_copy);
+ return 0;
}
/* PCM substream operators map. */
--
2.54.0 (Apple Git-157)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK
2026-10-27 23:43 ` [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK Joonho Lee
@ 2026-10-28 0:09 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:09 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_cp_ack() fails with -EPIPE during capture drain
--
commit 7b35fc6d816e5d3f7f39daa43f235ee425fb93dd
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Propagate PCM send failures through ACK
This commit updates virtsnd_pcm_msg_send() to roll back the message length
and notify the virtqueue for previously queued messages if adding a scatterlist
fails. It also propagates transfer errors through the playback and capture
ack callbacks, marking the stream stopped and returning an xrun error on
failure.
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1091100a4c982..a40ea394bf174 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -466,36 +466,100 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
> static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
> {
> struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> + struct virtsnd_pcm_transfer transfer = {};
> + int rc;
>
> - guard(spinlock_irqsave)(&queue->lock);
> - guard(spinlock)(&vss->lock);
> + scoped_guard(spinlock_irqsave, &queue->lock) {
> + guard(spinlock)(&vss->lock);
> + transfer.indirect = vss->pcm_indirect;
> + if (vss->stopped) {
> + rc = -EPIPE;
> + } else {
> + rc = snd_pcm_indirect_capture_transfer(substream,
> + &transfer.indirect,
> + virtsnd_pcm_trans_copy);
> + }
[Severity: High]
Does checking vss->stopped here break capture stream draining?
When userspace drains a running capture substream that still has unread
frames in the ring buffer, snd_pcm_do_drain_init() stops the hardware stream
and places the substream into the draining state so userspace can read the
remaining captured data:
sound/core/pcm_native.c:snd_pcm_do_drain_init() {
...
if (runtime->state == SNDRV_PCM_STATE_RUNNING) {
snd_pcm_state_t new_state;
new_state = snd_pcm_capture_avail(runtime) > 0 ?
SNDRV_PCM_STATE_DRAINING : SNDRV_PCM_STATE_SETUP;
snd_pcm_do_stop(substream, new_state);
snd_pcm_post_stop(substream, new_state);
}
}
snd_pcm_do_stop() invokes virtsnd_pcm_trigger(), which sets vss->stopped:
sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() {
...
case SNDRV_PCM_TRIGGER_STOP:
vss->stopped = true;
fallthrough;
}
When userspace subsequently reads those remaining buffered frames via
__snd_pcm_lib_xfer(), advances appl_ptr via snd_pcm_forward(), or synchronizes
via snd_pcm_sync_ptr(), pcm_lib_apply_appl_ptr() invokes substream->ops->ack():
sound/core/pcm_lib.c:pcm_lib_apply_appl_ptr() {
...
runtime->control->appl_ptr = appl_ptr;
if (substream->ops->ack) {
ret = substream->ops->ack(substream);
if (ret < 0) {
runtime->control->appl_ptr = old_appl_ptr;
if (ret == -EPIPE)
__snd_pcm_xrun(substream);
return ret;
}
}
}
Because vss->stopped is already true during capture drain, virtsnd_pcm_cp_ack()
will unconditionally fail and return -EPIPE, causing pcm_lib_apply_appl_ptr()
to revert appl_ptr, trigger __snd_pcm_xrun(), and prevent userspace from
reading the remaining captured frames.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
2026-10-27 23:43 ` [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:18 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset Joonho Lee
` (8 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
Capture trigger queues I/O messages before PCM_START. If the START
message cannot be allocated or sent, transfers may remain queued, and
ALSA does not invoke the action undo callback for the failed trigger.
Mark the stream stopped and disable transfers for every failed START
path. Arm runtime->stop_operating so sync_stop releases pending I/O and
prevents prepare, hw_params, or START retries until release succeeds.
The stopped flag is shared with ACK callbacks that can run without the
ALSA stream lock. Protect every stopped access with vss->lock. Read the
pending message count under the same lock in open and hw_params. In
sync_stop, decide whether RELEASE is needed and mark the stream stopped
in one locked transition; leave it stopped when release or draining fails.
Always cancel elapsed-period work in sync_stop, even when no RELEASE
is needed.
Fixes: da76e9f3e43a ("ALSA: virtio: PCM substream operators")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Changes in v3:
- Protect all stopped accesses with vss->lock. Keep the RELEASE
decision and stopped transition in the same critical section.
sound/virtio/virtio_pcm.h | 4 +-
sound/virtio/virtio_pcm_ops.c | 81 +++++++++++++++++++++++++++++------
2 files changed, 69 insertions(+), 16 deletions(-)
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 8837dfc12828..8048b14e6c80 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -26,13 +26,13 @@ struct virtio_pcm_msg;
* @hw: Kernel ALSA substream hardware descriptor.
* @elapsed_period: Kernel work to handle the elapsed period state.
* @lock: Spinlock that protects fields shared by interrupt handlers and
- * substream operators.
+ * substream operators, including @stopped.
* @buffer_bytes: Current buffer size in bytes.
* @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
* @xfer_enabled: Data transfer state (0 - off, 1 - on).
* @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
* @stopped: True if the substream is stopped and must be released on the device
- * side.
+ * side; protected by @lock.
* @suspended: True if the substream is suspended and must be reconfigured on
* the device side at resume.
* @msgs: Allocated I/O messages.
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index a40ea394bf17..970499fe419b 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -96,6 +96,15 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream);
+static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss)
+{
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ }
+ vss->substream->runtime->stop_operating = true;
+}
+
/**
* virtsnd_pcm_open() - Open the PCM substream.
* @substream: Kernel ALSA substream.
@@ -115,7 +124,8 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS);
- vss->stopped = !!virtsnd_pcm_msg_pending_num(vss);
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->stopped |= !!vss->msg_count;
vss->suspended = false;
/*
@@ -217,13 +227,25 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_device *vdev = vss->snd->vdev;
+ unsigned int msg_count;
+ bool stopped;
int rc;
- if (virtsnd_pcm_msg_pending_num(vss)) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ msg_count = vss->msg_count;
+ stopped = vss->stopped;
+ }
+ if (msg_count) {
dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
vss->sid);
return -EBADFD;
}
+ if (stopped) {
+ dev_err(&vdev->dev, "SID %u: stream release failed\n",
+ vss->sid);
+ substream->runtime->stop_operating = true;
+ return -EBADFD;
+ }
rc = virtsnd_pcm_dev_set_params(vss, params_buffer_bytes(hw_params),
params_period_bytes(hw_params),
@@ -273,6 +295,16 @@ static int virtsnd_pcm_prepare(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 stopped;
+
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ stopped = vss->stopped;
+ if (stopped) {
+ dev_err(&vdev->dev, "SID %u: stream release failed\n",
+ vss->sid);
+ substream->runtime->stop_operating = true;
+ return -EBADFD;
+ }
if (!vss->suspended) {
if (virtsnd_pcm_msg_pending_num(vss)) {
@@ -338,31 +370,40 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
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)
+ if (rc) {
+ vss->stopped = true;
+ substream->runtime->stop_operating = true;
return rc;
+ }
vss->xfer_enabled = true;
}
msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
GFP_KERNEL);
if (!msg) {
- guard(spinlock_irqsave)(&vss->lock);
- vss->xfer_enabled = false;
-
+ virtsnd_pcm_start_failed(vss);
return -ENOMEM;
}
- return virtsnd_ctl_msg_send_sync(snd, msg);
+ rc = virtsnd_ctl_msg_send_sync(snd, msg);
+ if (rc)
+ virtsnd_pcm_start_failed(vss);
+
+ return rc;
case SNDRV_PCM_TRIGGER_SUSPEND:
vss->suspended = true;
fallthrough;
case SNDRV_PCM_TRIGGER_STOP:
- vss->stopped = true;
- fallthrough;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
scoped_guard(spinlock_irqsave, &vss->lock) {
+ if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH)
+ vss->stopped = true;
vss->xfer_enabled = false;
}
@@ -393,21 +434,31 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
struct virtio_snd *snd = vss->snd;
struct virtio_snd_msg *msg;
unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
+ bool need_release;
int rc;
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->xfer_enabled = false;
+ need_release = vss->stopped || vss->msg_count;
+ if (need_release)
+ vss->stopped = true;
+ }
cancel_work_sync(&vss->elapsed_period);
-
- if (!vss->stopped)
+ if (!need_release)
return 0;
msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE,
GFP_KERNEL);
- if (!msg)
+ if (!msg) {
+ substream->runtime->stop_operating = true;
return -ENOMEM;
+ }
rc = virtsnd_ctl_msg_send_sync(snd, msg);
- if (rc)
+ if (rc) {
+ substream->runtime->stop_operating = true;
return rc;
+ }
/*
* The spec states that upon receipt of the RELEASE command "the device
@@ -421,11 +472,13 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
if (rc <= 0) {
dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n",
vss->sid);
+ substream->runtime->stop_operating = true;
return !rc ? -ETIMEDOUT : rc;
}
- vss->stopped = false;
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->stopped = false;
return 0;
}
--
2.54.0 (Apple Git-157)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start
2026-10-27 23:43 ` [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start Joonho Lee
@ 2026-10-28 0:18 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:18 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: unrecoverable stream state after failed suspend sync_stop()
- [Medium] ALSA: virtio: unprepared stream left startable if virtsnd_pcm_prepare() fails
--
commit ff74351afcffdc15b798435ae2935317313176c5
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Recover PCM state after a failed start
This commit recovers virtio PCM substream state when a start trigger fails by
disabling transfers, marking the stream as stopped under vss->lock, and
arming runtime->stop_operating so virtsnd_pcm_sync_stop() releases pending
I/O before subsequent prepare, hw_params, or start operations.
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index a40ea394bf174..970499fe419b4 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -115,7 +124,8 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
> snd_pcm_hw_constraint_integer(substream->runtime,
> SNDRV_PCM_HW_PARAM_PERIODS);
>
> - vss->stopped = !!virtsnd_pcm_msg_pending_num(vss);
> + scoped_guard(spinlock_irqsave, &vss->lock)
> + vss->stopped |= !!vss->msg_count;
> vss->suspended = false;
>
> /*
[ ... ]
> @@ -273,6 +295,16 @@ static int virtsnd_pcm_prepare(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 stopped;
> +
> + scoped_guard(spinlock_irqsave, &vss->lock)
> + stopped = vss->stopped;
> + if (stopped) {
> + dev_err(&vdev->dev, "SID %u: stream release failed\n",
> + vss->sid);
> + substream->runtime->stop_operating = true;
> + return -EBADFD;
> + }
>
> if (!vss->suspended) {
> if (virtsnd_pcm_msg_pending_num(vss)) {
[Severity: High]
Can a failed virtsnd_pcm_sync_stop() during system suspend leave the PCM
substream in a permanently unrecoverable state after resume?
During system suspend, do_pcm_suspend() -> snd_pcm_suspend_all() runs:
snd_pcm_do_suspend()
virtsnd_pcm_trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND)
vss->suspended = true;
vss->stopped = true;
snd_pcm_sync_stop(substream, false)
virtsnd_pcm_sync_stop(substream)
and ignores the return value of snd_pcm_sync_stop(). In
virtsnd_pcm_sync_stop(), allocating or sending VIRTIO_SND_R_PCM_RELEASE or
waiting in wait_event_interruptible_timeout() for vss->msg_empty can fail or
be interrupted (for example, returning -ERESTARTSYS when a signal or the
freezer is pending, or -ETIMEDOUT while vss->msg_count > 0). With this
change, virtsnd_pcm_sync_stop() leaves vss->stopped = true and re-arms
substream->runtime->stop_operating = true.
Subsequently, virtsnd_freeze() resets the device and deletes the virtqueues
without clearing vss->stopped or vss->msg_count:
virtsnd_freeze()
virtio_reset_device(vdev);
vdev->config->del_vqs(vdev);
which returns all device-side streams to their initial unconfigured state.
Previously, post-resume snd_pcm_prepare() could recover because
runtime->stop_operating had been cleared by snd_pcm_sync_stop(),
virtsnd_pcm_prepare() did not check vss->stopped, and the vss->suspended
branch skipped the pending-message check, re-sent
VIRTIO_SND_R_PCM_SET_PARAMS to the reset device, and reset
vss->msg_count = 0.
With this patch, when userspace calls snd_pcm_prepare() after resume:
1. Because runtime->stop_operating was re-armed, snd_pcm_do_prepare() calls
virtsnd_pcm_sync_stop() first. Seeing vss->stopped == true,
virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to a freshly reset,
unconfigured stream before VIRTIO_SND_R_PCM_SET_PARAMS has been sent, and
waits on vss->msg_empty for a stale vss->msg_count on the recreated empty
virtqueue, timing out with -ETIMEDOUT.
2. Even if virtsnd_pcm_prepare() is reached, checking if (stopped) before
if (!vss->suspended) returns -EBADFD immediately and makes the
vss->suspended recovery path unreachable.
3. Reconfiguring via virtsnd_pcm_hw_params() is also rejected by the new
stopped and msg_count checks, and closing and reopening the substream via
virtsnd_pcm_open() preserves the stale vss->stopped = true (due to
vss->stopped |= !!vss->msg_count), clears vss->suspended = false, and
calls virtsnd_pcm_sync_stop() again.
Later commits in this series ("ALSA: virtio: Retain queued messages through
device reset" and "ALSA: virtio: wait through signals for PCM teardown")
detach unused buffers in virtsnd_freeze() and make the control message wait
non-interruptible, but wait_event_interruptible_timeout() in
virtsnd_pcm_sync_stop() remains interruptible and virtsnd_freeze() still does
not clear vss->stopped when virtio_reset_device() resets the device-side
stream state. If virtsnd_pcm_sync_stop() fails during suspend, post-resume
sync_stop() will still send VIRTIO_SND_R_PCM_RELEASE to an unconfigured
stream before SET_PARAMS and block prepare(), hw_params(), and open() if the
reset device rejects RELEASE in its initial state.
[ ... ]
> @@ -393,21 +434,31 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
> struct virtio_snd *snd = vss->snd;
> struct virtio_snd_msg *msg;
> unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
> + bool need_release;
> int rc;
>
> + scoped_guard(spinlock_irqsave, &vss->lock) {
> + vss->xfer_enabled = false;
> + need_release = vss->stopped || vss->msg_count;
> + if (need_release)
> + vss->stopped = true;
> + }
> cancel_work_sync(&vss->elapsed_period);
> -
> - if (!vss->stopped)
> + if (!need_release)
> return 0;
>
> msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE,
> GFP_KERNEL);
> - if (!msg)
> + if (!msg) {
> + substream->runtime->stop_operating = true;
> return -ENOMEM;
> + }
>
> rc = virtsnd_ctl_msg_send_sync(snd, msg);
> - if (rc)
> + if (rc) {
> + substream->runtime->stop_operating = true;
> return rc;
> + }
>
> /*
> * The spec states that upon receipt of the RELEASE command "the device
> @@ -421,11 +472,13 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
> if (rc <= 0) {
> dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n",
> vss->sid);
> + substream->runtime->stop_operating = true;
>
> return !rc ? -ETIMEDOUT : rc;
> }
>
> - vss->stopped = false;
> + scoped_guard(spinlock_irqsave, &vss->lock)
> + vss->stopped = false;
>
> return 0;
> }
[Severity: Medium]
What happens if virtsnd_pcm_sync_stop() succeeds after a failed start, but
the subsequent VIRTIO_SND_R_PCM_PREPARE command in virtsnd_pcm_prepare()
fails?
In sound/core/pcm_native.c:snd_pcm_do_start():
err = substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_START);
/* XRUN happened during the start */
if (err == -EPIPE)
__snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_XRUN);
return err;
When virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_START) fails with an error other
than -EPIPE (such as -ENOMEM from virtsnd_pcm_ctl_msg_alloc(), or an error
from virtsnd_pcm_msg_send() or virtsnd_ctl_msg_send_sync()),
substream->runtime->state remains SNDRV_PCM_STATE_PREPARED while
virtsnd_pcm_start_failed() sets vss->stopped = true and
runtime->stop_operating = true.
When userspace then calls snd_pcm_prepare() to recover:
1. snd_pcm_do_prepare() calls snd_pcm_sync_stop(substream, true), which
clears runtime->stop_operating = false and runs virtsnd_pcm_sync_stop().
2. virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to the device
(moving the device stream out of the prepared state into the released
state), waits for queued messages to drain, and clears
vss->stopped = false.
3. snd_pcm_do_prepare() then calls virtsnd_pcm_prepare(), which allocates and
sends VIRTIO_SND_R_PCM_PREPARE:
msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
GFP_KERNEL);
if (!msg)
return -ENOMEM;
return virtsnd_ctl_msg_send_sync(vss->snd, msg);
If allocating or sending VIRTIO_SND_R_PCM_PREPARE fails here,
snd_pcm_do_prepare() returns the error without changing
substream->runtime->state. At that point, the device stream is in the
released (unprepared) state, but substream->runtime->state is still
SNDRV_PCM_STATE_PREPARED, runtime->stop_operating is false, and vss->stopped
is false.
Wouldn't a subsequent SNDRV_PCM_IOCTL_START, write(), or
SNDRV_PCM_IOCTL_SYNC_PTR bypass both ALSA's state check and the vss->stopped
check in virtsnd_pcm_trigger(), queueing I/O messages and sending
VIRTIO_SND_R_PCM_START to an unprepared device stream?
Note that a later commit in this series ("ALSA: virtio: stage VZ capture and
restart paused streams") handles virtsnd_pcm_dev_prepare() failure when
pause_restart is true, but leaves the standard (!pause_restart) path
unhandled.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
2026-10-27 23:43 ` [PATCH v5 01/11] ALSA: virtio: Propagate PCM send failures through ACK Joonho Lee
2026-10-27 23:43 ` [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:15 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership Joonho Lee
` (7 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
Keep the control queue available through snd_card_free() so close-time
STOP and RELEASE commands can still be submitted. Block new control
requests and mark pending messages canceled before reset, then release
their queue references only after reset and queue deletion.
For PCM queues, detach each remaining virtqueue buffer after the device
has stopped DMA and retire its message without reporting a normal
completion. Detach all queues before deleting them so outstanding DMA
mappings are unmapped and no token remains for a late device completion.
Fixes: 9d45e514da88 ("ALSA: virtio: handling control messages")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Changes in v3:
- No code or commit-message changes from v2.
sound/virtio/virtio_card.c | 44 ++++++++++++++++++++++---
sound/virtio/virtio_card.h | 2 ++
sound/virtio/virtio_ctl_msg.c | 62 +++++++++++++++++++++++++++++++----
sound/virtio/virtio_ctl_msg.h | 4 +++
sound/virtio/virtio_pcm.h | 2 ++
sound/virtio/virtio_pcm_msg.c | 29 ++++++++++++++++
6 files changed, 131 insertions(+), 12 deletions(-)
diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
index 6f35276416fe..df7091b74a52 100644
--- a/sound/virtio/virtio_card.c
+++ b/sound/virtio/virtio_card.c
@@ -337,6 +337,38 @@ static int virtsnd_probe(struct virtio_device *vdev)
return rc;
}
+/**
+ * virtsnd_reset_vqs() - Stop device access and unmap outstanding buffers.
+ * @snd: VirtIO sound device.
+ *
+ * Context: Any context that permits to sleep.
+ */
+static void virtsnd_reset_vqs(struct virtio_snd *snd)
+{
+ struct virtio_device *vdev = snd->vdev;
+ unsigned int i;
+
+ virtio_reset_device(vdev);
+
+ /* Deleting a virtqueue does not unmap its outstanding descriptors. */
+ 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);
+ }
+
+ vdev->config->del_vqs(vdev);
+ for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)
+ snd->queues[i].vqueue = NULL;
+}
+
/**
* virtsnd_remove() - Remove VirtIO and ALSA devices.
* @vdev: VirtIO parent device.
@@ -349,13 +381,14 @@ static void virtsnd_remove(struct virtio_device *vdev)
unsigned int i;
virtsnd_disable_event_vq(snd);
- virtsnd_ctl_msg_cancel_all(snd);
if (snd->card)
snd_card_free(snd->card);
- virtio_reset_device(vdev);
- vdev->config->del_vqs(vdev);
+ virtsnd_ctl_msg_cancel_all(snd);
+
+ virtsnd_reset_vqs(snd);
+ virtsnd_ctl_msg_cleanup(snd);
for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
struct virtio_pcm_substream *vss = &snd->substreams[i];
@@ -383,8 +416,8 @@ static int virtsnd_freeze(struct virtio_device *vdev)
virtsnd_disable_event_vq(snd);
virtsnd_ctl_msg_cancel_all(snd);
- virtio_reset_device(vdev);
- vdev->config->del_vqs(vdev);
+ virtsnd_reset_vqs(snd);
+ virtsnd_ctl_msg_cleanup(snd);
for (i = 0; i < snd->nsubstreams; ++i)
cancel_work_sync(&snd->substreams[i].elapsed_period);
@@ -412,6 +445,7 @@ static int virtsnd_restore(struct virtio_device *vdev)
return rc;
virtio_device_ready(vdev);
+ virtsnd_ctl_msg_enable(snd);
virtsnd_enable_event_vq(snd);
diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h
index 3ceee4e416fc..bd065bd00d25 100644
--- a/sound/virtio/virtio_card.h
+++ b/sound/virtio/virtio_card.h
@@ -47,6 +47,7 @@ struct virtio_kctl {
* @queues: Virtqueue wrappers.
* @card: ALSA sound card.
* @ctl_msgs: Pending control request list.
+ * @ctl_msgs_blocked: Control requests are blocked during teardown or suspend.
* @event_msgs: Device events.
* @pcm_list: VirtIO PCM device list.
* @jacks: VirtIO jacks.
@@ -64,6 +65,7 @@ struct virtio_snd {
struct virtio_snd_queue queues[VIRTIO_SND_VQ_MAX];
struct snd_card *card;
struct list_head ctl_msgs;
+ bool ctl_msgs_blocked;
struct virtio_snd_event *event_msgs;
struct list_head pcm_list;
struct virtio_jack *jacks;
diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
index 6433c870f271..7114bea4033a 100644
--- a/sound/virtio/virtio_ctl_msg.c
+++ b/sound/virtio/virtio_ctl_msg.c
@@ -15,6 +15,7 @@
* @list: Pending message list entry.
* @notify: Request completed notification.
* @ref_count: Reference count used to manage a message lifetime.
+ * @cancelled: Request canceled during device teardown.
*/
struct virtio_snd_msg {
struct scatterlist sg_request;
@@ -22,6 +23,7 @@ struct virtio_snd_msg {
struct list_head list;
struct completion notify;
refcount_t ref_count;
+ bool cancelled;
};
/**
@@ -100,7 +102,7 @@ struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
INIT_LIST_HEAD(&msg->list);
init_completion(&msg->notify);
- /* This reference is dropped in virtsnd_ctl_msg_complete(). */
+ /* The caller owns this reference; send adds a queue reference. */
refcount_set(&msg->ref_count, 1);
return msg;
@@ -134,6 +136,7 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
int rc;
virtsnd_ctl_msg_ref(msg);
+ /* This reference stays with the queue until completion or teardown. */
/* Set the default status in case the message was canceled. */
response->code = cpu_to_le32(VIRTIO_SND_S_IO_ERR);
@@ -147,12 +150,17 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
psgs[nouts + nins++] = in_sgs;
scoped_guard(spinlock_irqsave, &queue->lock) {
- rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
- GFP_ATOMIC);
+ if (snd->ctl_msgs_blocked)
+ rc = -ENODEV;
+ else
+ rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins,
+ msg, GFP_ATOMIC);
if (!rc) {
notify = virtqueue_kick_prepare(queue->vqueue);
list_add_tail(&msg->list, &snd->ctl_msgs);
+ if (notify)
+ virtqueue_notify(queue->vqueue);
}
}
@@ -169,9 +177,6 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
goto on_exit;
}
- if (notify)
- virtqueue_notify(queue->vqueue);
-
if (nowait)
goto on_exit;
@@ -187,6 +192,11 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
goto on_exit;
}
+ if (READ_ONCE(msg->cancelled)) {
+ rc = -ENODEV;
+ goto on_exit;
+ }
+
switch (le32_to_cpu(response->code)) {
case VIRTIO_SND_S_OK:
rc = 0;
@@ -227,9 +237,33 @@ void virtsnd_ctl_msg_complete(struct virtio_snd_msg *msg)
* virtsnd_ctl_msg_cancel_all() - Cancel all pending control messages.
* @snd: VirtIO sound device.
*
- * Context: Any context.
+ * Context: Any context. Blocks new senders and marks pending requests canceled.
+ * Existing senders are woken when their request completes or after
+ * the device has been reset.
*/
void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd)
+{
+ struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
+ struct virtio_snd_msg *msg;
+
+ guard(spinlock_irqsave)(&queue->lock);
+ snd->ctl_msgs_blocked = true;
+ list_for_each_entry(msg, &snd->ctl_msgs, list) {
+ WRITE_ONCE(msg->cancelled, true);
+ }
+}
+
+/**
+ * virtsnd_ctl_msg_cleanup() - Release messages left after queue teardown.
+ * @snd: VirtIO sound device.
+ *
+ * This must be called after device reset, buffer detachment, and del_vqs(),
+ * when no queue callback or DMA access can still reach a message. The queue
+ * object and its lock must remain alive until this function returns.
+ *
+ * Context: Any context.
+ */
+void virtsnd_ctl_msg_cleanup(struct virtio_snd *snd)
{
struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
@@ -243,6 +277,20 @@ void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd)
}
}
+/**
+ * virtsnd_ctl_msg_enable() - Allow control requests after queue restoration.
+ * @snd: VirtIO sound device.
+ *
+ * Context: Any context.
+ */
+void virtsnd_ctl_msg_enable(struct virtio_snd *snd)
+{
+ struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
+
+ guard(spinlock_irqsave)(&queue->lock);
+ snd->ctl_msgs_blocked = false;
+}
+
/**
* virtsnd_ctl_query_info() - Query the item configuration from the device.
* @snd: VirtIO sound device.
diff --git a/sound/virtio/virtio_ctl_msg.h b/sound/virtio/virtio_ctl_msg.h
index 7f4db044f28e..d917435409d0 100644
--- a/sound/virtio/virtio_ctl_msg.h
+++ b/sound/virtio/virtio_ctl_msg.h
@@ -68,6 +68,10 @@ static inline int virtsnd_ctl_msg_send_async(struct virtio_snd *snd,
void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd);
+void virtsnd_ctl_msg_cleanup(struct virtio_snd *snd);
+
+void virtsnd_ctl_msg_enable(struct virtio_snd *snd);
+
void virtsnd_ctl_msg_complete(struct virtio_snd_msg *msg);
int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 8048b14e6c80..e31f090a0124 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -120,6 +120,8 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss);
void virtsnd_pcm_msg_reset(struct virtio_pcm_substream *vss);
+void virtsnd_pcm_msg_cancel(struct virtio_pcm_msg *msg);
+
int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
unsigned long bytes);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 5371f4985fd0..762480363b86 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -307,6 +307,35 @@ unsigned int virtsnd_pcm_msg_pending_num(struct virtio_pcm_substream *vss)
return vss->msg_count;
}
+/**
+ * virtsnd_pcm_msg_cancel() - Cancel a message detached during device reset.
+ * @msg: Message returned by virtqueue_detach_unused_buf().
+ *
+ * Reset cancellation retires the queue token without treating it as a device
+ * completion. In particular, it does not advance the PCM hardware pointer or
+ * schedule period work.
+ *
+ * Context: Process context with the PCM queue lock held.
+ */
+void virtsnd_pcm_msg_cancel(struct virtio_pcm_msg *msg)
+{
+ struct virtio_pcm_substream *vss = msg->substream;
+ bool wake;
+
+ {
+ guard(spinlock)(&vss->lock);
+ msg->length = 0;
+ vss->xfer_enabled = false;
+ if (WARN_ON_ONCE(!vss->msg_count))
+ return;
+ vss->msg_count--;
+ wake = !vss->msg_count;
+ }
+
+ if (wake)
+ wake_up_all(&vss->msg_empty);
+}
+
/**
* virtsnd_pcm_msg_complete() - Complete an I/O message.
* @msg: I/O message.
--
2.54.0 (Apple Git-157)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset
2026-10-27 23:43 ` [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset Joonho Lee
@ 2026-10-28 0:15 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:15 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: NULL vqueue dereference in virtsnd_pcm_msg_send() after freeze
- [High] ALSA: virtio: virtsnd_reset_vqs() leaves stale substream state on reset
--
commit 00a0945381a2dc9d6b91d5ecac16bfd8128b97f3
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Retain queued messages through device reset
This patch retains the control virtqueue through snd_card_free() during
device removal, blocks new control messages and cancels pending ones across
device reset, and detaches unused virtqueue buffers in virtsnd_reset_vqs()
before deleting the virtqueues.
> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 6f35276416fed..df7091b74a521 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -337,6 +337,38 @@ static int virtsnd_probe(struct virtio_device *vdev)
> return rc;
> }
>
> +/**
> + * virtsnd_reset_vqs() - Stop device access and unmap outstanding buffers.
> + * @snd: VirtIO sound device.
> + *
> + * Context: Any context that permits to sleep.
> + */
> +static void virtsnd_reset_vqs(struct virtio_snd *snd)
> +{
> + struct virtio_device *vdev = snd->vdev;
> + unsigned int i;
> +
> + virtio_reset_device(vdev);
> +
> + /* Deleting a virtqueue does not unmap its outstanding descriptors. */
> + 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);
> + }
> +
> + vdev->config->del_vqs(vdev);
> + for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)
> + snd->queues[i].vqueue = NULL;
> +}
[Severity: High]
Can this lead to a NULL pointer dereference in virtsnd_pcm_msg_send() after
virtsnd_freeze() clears snd->queues[i].vqueue?
During system sleep, do_pcm_suspend() -> snd_pcm_suspend_all() leaves
vss->stopped as false (either because the substream was not running, because
virtsnd_pcm_sync_stop() flushed the queue and cleared vss->stopped, or
because virtsnd_pcm_msg_cancel() only clears vss->xfer_enabled).
virtsnd_freeze() then calls virtsnd_reset_vqs(), which deletes the
virtqueues and sets snd->queues[i].vqueue = NULL, but unlike the control
queue, nothing blocks submissions to the TX/RX PCM queues.
If virtsnd_restore() fails (for example in virtsnd_find_vqs() or feature
negotiation), snd->queues[VIRTIO_SND_VQ_TX/RX].vqueue remains NULL. If
userspace then issues SNDRV_PCM_IOCTL_SYNC_PTR with an advanced appl_ptr,
snd_pcm_common_ioctl() only checks for SNDRV_PCM_STATE_DISCONNECTED (and
snd_power_wait() is a no-op since virtio-snd does not update
card->power_state), calling:
snd_pcm_sync_ptr()
-> pcm_lib_apply_appl_ptr()
-> virtsnd_pcm_pb_ack() / virtsnd_pcm_cp_ack()
In sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pb_ack():
scoped_guard(spinlock_irqsave, &queue->lock) {
guard(spinlock)(&vss->lock);
transfer.indirect = vss->pcm_indirect;
if (vss->stopped) {
rc = -EPIPE;
} else {
rc = snd_pcm_indirect_playback_transfer(substream,
&transfer.indirect,
virtsnd_pcm_trans_copy);
}
Because vss->stopped is false, this calls virtsnd_pcm_trans_copy() ->
virtsnd_pcm_msg_send():
struct virtqueue *vqueue = virtsnd_pcm_queue(vss)->vqueue;
...
rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg, GFP_ATOMIC);
which passes a NULL vqueue to virtqueue_add_sgs() under the spinlocks. Even
when virtsnd_restore() succeeds, the same SNDRV_PCM_IOCTL_SYNC_PTR -> ack()
path can queue PCM buffers onto the newly recreated virtqueue while the
substream is still in SNDRV_PCM_STATE_SUSPENDED (or SNDRV_PCM_STATE_SETUP)
before VIRTIO_SND_R_PCM_SET_PARAMS and VIRTIO_SND_R_PCM_PREPARE have been
sent to the reset device.
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 5371f4985fd06..762480363b861 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -307,6 +307,35 @@ unsigned int virtsnd_pcm_msg_pending_num(struct virtio_pcm_substream *vss)
> return vss->msg_count;
> }
>
> +/**
> + * virtsnd_pcm_msg_cancel() - Cancel a message detached during device reset.
> + * @msg: Message returned by virtqueue_detach_unused_buf().
> + *
> + * Reset cancellation retires the queue token without treating it as a device
> + * completion. In particular, it does not advance the PCM hardware pointer or
> + * schedule period work.
> + *
> + * Context: Process context with the PCM queue lock held.
> + */
> +void virtsnd_pcm_msg_cancel(struct virtio_pcm_msg *msg)
> +{
> + struct virtio_pcm_substream *vss = msg->substream;
> + bool wake;
> +
> + {
> + guard(spinlock)(&vss->lock);
> + msg->length = 0;
> + vss->xfer_enabled = false;
> + if (WARN_ON_ONCE(!vss->msg_count))
> + return;
> + vss->msg_count--;
> + wake = !vss->msg_count;
> + }
> +
> + if (wake)
> + wake_up_all(&vss->msg_empty);
> +}
[Severity: High]
Should virtsnd_reset_vqs() / virtsnd_pcm_msg_cancel() also reconcile
vss->stopped and vss->suspended when resetting the device during
virtsnd_freeze()?
First, if virtsnd_pcm_sync_stop() fails or times out during
snd_pcm_suspend_all() before virtsnd_freeze() (or if a prior stop/start
failed before suspend), vss->stopped remains true while buffers remain
queued. virtsnd_freeze() -> virtsnd_reset_vqs() then resets the VirtIO
device and detaches all pending buffers via virtsnd_pcm_msg_cancel(),
decrementing vss->msg_count to 0, but leaves vss->stopped as true.
After virtsnd_restore(), vss->stopped blocks virtsnd_pcm_prepare() and
virtsnd_pcm_hw_params() with -EBADFD:
sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare() {
scoped_guard(spinlock_irqsave, &vss->lock)
stopped = vss->stopped;
if (stopped) {
dev_err(&vdev->dev, "SID %u: stream release failed\n",
vss->sid);
substream->runtime->stop_operating = true;
return -EBADFD;
}
And if userspace tries to recover via snd_pcm_sync_stop() or by closing and
reopening the substream via virtsnd_pcm_open():
sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_open() {
...
scoped_guard(spinlock_irqsave, &vss->lock)
vss->stopped |= !!vss->msg_count;
vss->suspended = false;
return virtsnd_pcm_sync_stop(substream);
}
virtsnd_pcm_sync_stop() sees vss->stopped == true and sends
VIRTIO_SND_R_PCM_RELEASE to the freshly reset device before
VIRTIO_SND_R_PCM_SET_PARAMS has been sent. If the device rejects RELEASE for
the unconfigured stream, virtsnd_pcm_sync_stop() fails and leaves
vss->stopped set, permanently wedging the substream.
Second, if a substream has queued buffers while in a state where
snd_pcm_running() is false at suspend time (such as SNDRV_PCM_STATE_PAUSED,
capture SNDRV_PCM_STATE_DRAINING, or playback SNDRV_PCM_STATE_PREPARED with
pre-queued buffers), snd_pcm_do_suspend() skips
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_SUSPEND) while snd_pcm_post_suspend()
places the substream into SNDRV_PCM_STATE_SUSPENDED with vss->suspended ==
false. When virtsnd_pcm_msg_cancel() drains vss->msg_count to 0 without
setting vss->suspended = true, a subsequent virtsnd_pcm_prepare() after
virtsnd_restore() sees !vss->suspended and virtsnd_pcm_msg_pending_num(vss)
== 0, skips virtsnd_pcm_dev_set_params(), and sends VIRTIO_SND_R_PCM_PREPARE
to a reset device that has no stream parameters configured.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=3
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (2 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-27 23:59 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 05/11] ALSA: virtio: Keep control payloads alive through DMA completion Joonho Lee
` (6 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
Retain references to the pages backing ALSA's vmalloc buffer until its
queued I/O messages are freed. On NOMMU systems, use driver-owned memory
for DMA and copy playback data before submission and capture data after
completion, so ALSA buffer release cannot invalidate an active request.
Validate the PCM buffer and each period before building scatterlists.
Clamp device-reported capture lengths to the message capacity before
advancing the hardware pointer, preventing an oversized completion from
accounting bytes beyond the buffer actually submitted.
Fixes: f40a28679e0b ("ALSA: virtio: handling control and I/O messages for the PCM device")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Changes in v3:
- No code or commit-message changes from v2.
sound/virtio/virtio_pcm.h | 11 ++
sound/virtio/virtio_pcm_msg.c | 252 +++++++++++++++++++++++++++++++---
sound/virtio/virtio_pcm_ops.c | 2 +
3 files changed, 243 insertions(+), 22 deletions(-)
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index e31f090a0124..61faf7b13cbc 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -13,6 +13,7 @@
struct virtio_pcm;
struct virtio_pcm_msg;
+struct page;
/**
* struct virtio_pcm_substream - VirtIO PCM substream.
@@ -38,6 +39,10 @@ struct virtio_pcm_msg;
* @msgs: Allocated I/O messages.
* @nmsgs: Number of allocated I/O messages.
* @msg_count: Number of pending I/O messages in the virtqueue.
+ * @dma_pages: Pages backing DMA messages, retained until messages are freed.
+ * @ndma_pages: Number of retained pages.
+ * @owned_dma_area: Driver-owned PCM buffer backing asynchronous messages.
+ * @owned_dma_bytes: Size of @owned_dma_area.
* @msg_empty: Notify when msg_count is zero.
*/
struct virtio_pcm_substream {
@@ -60,6 +65,12 @@ struct virtio_pcm_substream {
struct virtio_pcm_msg **msgs;
unsigned int nmsgs;
unsigned int msg_count;
+ struct page **dma_pages;
+ unsigned int ndma_pages;
+#ifndef CONFIG_MMU
+ u8 *owned_dma_area;
+ size_t owned_dma_bytes;
+#endif
wait_queue_head_t msg_empty;
};
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 762480363b86..b7f2334bb221 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -3,6 +3,9 @@
* virtio-snd: Virtio sound device
* Copyright (C) 2021 OpenSynergy GmbH
*/
+#include <linux/limits.h>
+#include <linux/mm.h>
+#include <linux/overflow.h>
#include <sound/pcm_params.h>
#include "virtio_card.h"
@@ -13,6 +16,7 @@
* @xfer: Request header payload.
* @status: Response header payload.
* @length: Data length in bytes.
+ * @data_offset: Offset in the driver-owned PCM buffer on NOMMU systems.
* @sgs: Payload scatter-gather table.
*/
struct virtio_pcm_msg {
@@ -20,6 +24,9 @@ struct virtio_pcm_msg {
struct virtio_snd_pcm_xfer xfer;
struct virtio_snd_pcm_status status;
size_t length;
+#ifndef CONFIG_MMU
+ size_t data_offset;
+#endif
struct scatterlist sgs[];
};
@@ -37,9 +44,8 @@ enum pcm_msg_sg_index {
};
/**
- * virtsnd_pcm_sg_num() - Count the number of sg-elements required to represent
- * vmalloc'ed buffer.
- * @data: Pointer to vmalloc'ed buffer.
+ * virtsnd_pcm_sg_num() - Count the sg-elements for a page-backed buffer.
+ * @data: Pointer to the page-backed buffer.
* @length: Buffer size.
*
* Context: Any context.
@@ -47,15 +53,19 @@ enum pcm_msg_sg_index {
*/
static int virtsnd_pcm_sg_num(u8 *data, unsigned int length)
{
- phys_addr_t sg_address;
- unsigned int sg_length;
+ phys_addr_t sg_address = 0;
+ unsigned int sg_length = 0;
int num = 0;
while (length) {
struct page *pg = vmalloc_to_page(data);
- phys_addr_t pg_address = page_to_phys(pg);
+ phys_addr_t pg_address;
size_t pg_length;
+ if (!pg)
+ return -EINVAL;
+ pg_address = page_to_phys(pg);
+
pg_length = PAGE_SIZE - offset_in_page(data);
if (pg_length > length)
pg_length = length;
@@ -75,11 +85,80 @@ static int virtsnd_pcm_sg_num(u8 *data, unsigned int length)
return num;
}
+#ifdef CONFIG_MMU
+/**
+ * virtsnd_pcm_dma_pages_put() - Release retained PCM buffer pages.
+ * @vss: VirtIO PCM substream.
+ *
+ * Context: Process context.
+ */
+static void virtsnd_pcm_dma_pages_put(struct virtio_pcm_substream *vss)
+{
+ while (vss->ndma_pages)
+ put_page(vss->dma_pages[--vss->ndma_pages]);
+ kfree(vss->dma_pages);
+ vss->dma_pages = NULL;
+}
+
/**
- * virtsnd_pcm_sg_from() - Build sg-list from vmalloc'ed buffer.
+ * virtsnd_pcm_dma_pages_get() - Retain the pages backing the PCM buffer.
+ * @vss: VirtIO PCM substream.
+ * @data: PCM buffer address.
+ * @length: PCM buffer size.
+ *
+ * Keep the backing pages alive while virtqueue descriptors can still refer to
+ * them, even if ALSA releases the vmalloc mapping after a failed drain.
+ *
+ * Context: Process context.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_dma_pages_get(struct virtio_pcm_substream *vss,
+ u8 *data, size_t length)
+{
+ size_t i;
+ size_t npages;
+ size_t span;
+
+ if (!data || !length || !is_vmalloc_addr(data))
+ return -EINVAL;
+ if (vss->dma_pages || vss->ndma_pages)
+ return -EBUSY;
+
+ if (check_add_overflow(length, (size_t)offset_in_page(data), &span))
+ return -EOVERFLOW;
+
+ npages = span / PAGE_SIZE + !!(span % PAGE_SIZE);
+ if (npages > UINT_MAX)
+ return -EOVERFLOW;
+
+ vss->dma_pages = kcalloc(npages, sizeof(*vss->dma_pages), GFP_KERNEL);
+ if (!vss->dma_pages)
+ return -ENOMEM;
+
+ for (i = 0; i < npages; i++) {
+ struct page *page = vmalloc_to_page(data + i * PAGE_SIZE);
+
+ if (!page)
+ goto err_put_pages;
+
+ get_page(page);
+ vss->dma_pages[vss->ndma_pages++] = page;
+ }
+
+ return 0;
+
+err_put_pages:
+ virtsnd_pcm_dma_pages_put(vss);
+
+ return -EINVAL;
+}
+#endif
+
+/**
+ * virtsnd_pcm_sg_from() - Build an sg-list from a page-backed buffer.
* @sgs: Preallocated sg-list to populate.
* @nsgs: The maximum number of elements in the @sgs.
- * @data: Pointer to vmalloc'ed buffer.
+ * @data: Pointer to the page-backed buffer.
* @length: Buffer size.
*
* Splits the buffer into physically contiguous parts and makes an sg-list of
@@ -132,36 +211,99 @@ static void virtsnd_pcm_sg_from(struct scatterlist *sgs, int nsgs, u8 *data,
int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
unsigned int periods, unsigned int period_bytes)
{
- struct snd_pcm_runtime *runtime = vss->substream->runtime;
+ struct snd_pcm_runtime *runtime;
+ size_t buffer_bytes;
unsigned int i;
+ int rc;
- vss->msgs = kzalloc_objs(*vss->msgs, periods);
- if (!vss->msgs)
+ if (vss->msgs || vss->nmsgs || vss->dma_pages || vss->ndma_pages)
+ return -EBUSY;
+#ifndef CONFIG_MMU
+ if (vss->owned_dma_area || vss->owned_dma_bytes)
+ return -EBUSY;
+#endif
+ if (!periods || !period_bytes)
+ return -EINVAL;
+
+ if (check_mul_overflow((size_t)periods, (size_t)period_bytes,
+ &buffer_bytes))
+ return -EOVERFLOW;
+
+ if (!vss->substream || !vss->substream->runtime)
+ return -EINVAL;
+ runtime = vss->substream->runtime;
+ if (!runtime->dma_area || buffer_bytes > runtime->dma_bytes)
+ return -EINVAL;
+
+#ifdef CONFIG_MMU
+ rc = virtsnd_pcm_dma_pages_get(vss, runtime->dma_area, buffer_bytes);
+ if (rc)
+ return rc;
+#else
+ vss->owned_dma_area = kzalloc(buffer_bytes, GFP_KERNEL);
+ if (!vss->owned_dma_area)
return -ENOMEM;
+ vss->owned_dma_bytes = buffer_bytes;
+#endif
+
+ vss->msgs = kzalloc_objs(*vss->msgs, periods);
+ if (!vss->msgs) {
+ rc = -ENOMEM;
+ goto err_free_pages;
+ }
vss->nmsgs = periods;
for (i = 0; i < periods; ++i) {
- u8 *data = runtime->dma_area + period_bytes * i;
- int sg_num = virtsnd_pcm_sg_num(data, period_bytes);
+ int sg_num;
struct virtio_pcm_msg *msg;
+ u8 *data;
+#ifdef CONFIG_MMU
+ data = runtime->dma_area + (size_t)period_bytes * i;
+#else
+ data = vss->owned_dma_area + (size_t)period_bytes * i;
+#endif
+ sg_num = virtsnd_pcm_sg_num(data, period_bytes);
+ if (sg_num < 0) {
+ rc = sg_num;
+ goto err_free_msgs;
+ }
msg = kzalloc_flex(*msg, sgs, sg_num + 2);
- if (!msg)
- return -ENOMEM;
+ if (!msg) {
+ rc = -ENOMEM;
+ goto err_free_msgs;
+ }
msg->substream = vss;
+ vss->msgs[i] = msg;
sg_init_one(&msg->sgs[PCM_MSG_SG_XFER], &msg->xfer,
sizeof(msg->xfer));
sg_init_one(&msg->sgs[PCM_MSG_SG_STATUS], &msg->status,
sizeof(msg->status));
+#ifndef CONFIG_MMU
+ msg->data_offset = (size_t)period_bytes * i;
+#endif
virtsnd_pcm_sg_from(&msg->sgs[PCM_MSG_SG_DATA], sg_num, data,
period_bytes);
-
- vss->msgs[i] = msg;
}
return 0;
+
+err_free_msgs:
+ virtsnd_pcm_msg_free(vss);
+ return rc;
+
+err_free_pages:
+#ifdef CONFIG_MMU
+ virtsnd_pcm_dma_pages_put(vss);
+#else
+ kfree(vss->owned_dma_area);
+ vss->owned_dma_area = NULL;
+ vss->owned_dma_bytes = 0;
+#endif
+
+ return rc;
}
/**
@@ -177,6 +319,13 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
for (i = 0; vss->msgs && i < vss->nmsgs; ++i)
kfree(vss->msgs[i]);
kfree(vss->msgs);
+#ifdef CONFIG_MMU
+ virtsnd_pcm_dma_pages_put(vss);
+#else
+ kfree(vss->owned_dma_area);
+ vss->owned_dma_area = NULL;
+ vss->owned_dma_bytes = 0;
+#endif
vss->msgs = NULL;
vss->nmsgs = 0;
@@ -249,6 +398,33 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
msg->xfer.stream_id = cpu_to_le32(vss->sid);
memset(&msg->status, 0, sizeof(msg->status));
+#ifndef CONFIG_MMU
+ if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK) {
+ struct snd_pcm_runtime *runtime =
+ vss->substream ? vss->substream->runtime : NULL;
+ size_t data_bytes = msg->length;
+
+ if (!runtime || !runtime->dma_area || !vss->owned_dma_area ||
+ msg->data_offset > runtime->dma_bytes ||
+ data_bytes > runtime->dma_bytes - msg->data_offset ||
+ msg->data_offset > vss->owned_dma_bytes ||
+ data_bytes > vss->owned_dma_bytes - msg->data_offset) {
+ dev_err_ratelimited(&vdev->dev,
+ "SID %u: invalid playback buffer for I/O message\n",
+ vss->sid);
+ msg->length -= n;
+ if (msg_count != vss->msg_count &&
+ !(vss->features &
+ (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
+ virtqueue_kick_prepare(vqueue))
+ virtqueue_notify(vqueue);
+ return -EINVAL;
+ }
+ memcpy(vss->owned_dma_area + msg->data_offset,
+ runtime->dma_area + msg->data_offset, data_bytes);
+ }
+#endif
+
if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK)
rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg,
GFP_ATOMIC);
@@ -363,15 +539,48 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
* the hw_ptr value is unconditionally advanced.
*/
guard(spinlock)(&vss->lock);
+#ifndef CONFIG_MMU
+ if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ vss->xfer_enabled && written_bytes > sizeof(msg->status)) {
+ struct snd_pcm_runtime *runtime =
+ vss->substream ? vss->substream->runtime : NULL;
+ size_t reported_bytes = written_bytes - sizeof(msg->status);
+ size_t data_bytes = min_t(size_t, reported_bytes, msg->length);
+
+ if (!runtime || !runtime->dma_area || !vss->owned_dma_area ||
+ msg->data_offset > runtime->dma_bytes ||
+ data_bytes > runtime->dma_bytes - msg->data_offset ||
+ msg->data_offset > vss->owned_dma_bytes ||
+ data_bytes > vss->owned_dma_bytes - msg->data_offset)
+ dev_err_ratelimited(&vss->snd->vdev->dev,
+ "SID %u: invalid capture buffer for I/O message\n",
+ vss->sid);
+ else
+ memcpy(runtime->dma_area + msg->data_offset,
+ vss->owned_dma_area + msg->data_offset, data_bytes);
+ }
+#endif
+ if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ written_bytes > sizeof(msg->status) &&
+ written_bytes - sizeof(msg->status) > msg->length)
+ dev_err_ratelimited(&vss->snd->vdev->dev,
+ "SID %u: capture length exceeds I/O message capacity\n",
+ vss->sid);
/*
* If the capture substream returned an incorrect status, then just
- * increase the hw_ptr by the message size.
+ * increase the hw_ptr by the message size. Clamp valid capture lengths to
+ * the actual SG payload capacity before accounting them.
*/
if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
- written_bytes <= sizeof(msg->status))
+ written_bytes <= sizeof(msg->status)) {
vss->hw_ptr += msg->length;
- else
- vss->hw_ptr += written_bytes - sizeof(msg->status);
+ } else {
+ size_t data_bytes = min_t(size_t,
+ written_bytes - sizeof(msg->status),
+ msg->length);
+
+ vss->hw_ptr += data_bytes;
+ }
if (vss->hw_ptr >= vss->buffer_bytes)
vss->hw_ptr -= vss->buffer_bytes;
@@ -387,7 +596,6 @@ 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);
} else 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 970499fe419b..f74bf365065b 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -25,6 +25,8 @@
* signal or due to a timeout. In this case, the device can still access
* messages even after calling ops->hw_free(). It can also issue an interrupt,
* and the interrupt handler will also try to access message structures.
+ * The messages retain references to the PCM buffer pages until they are
+ * freed, so late DMA does not target pages released by ALSA.
*
* Therefore, freeing of already allocated messages occurs:
*
--
2.54.0 (Apple Git-157)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership
2026-10-27 23:43 ` [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership Joonho Lee
@ 2026-10-27 23:59 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-27 23:59 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_msg_complete() drops NOMMU capture data on pause
--
commit a5392b9f568d1dcfcb1d686f254cd14115ceec53
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Keep PCM DMA buffers alive through device ownership
This commit retains references to the pages backing ALSA's vmalloc buffer
until its queued I/O messages are freed, uses a driver-owned bounce buffer on
NOMMU systems, and clamps device-reported capture lengths to the message
capacity.
> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 762480363b861..b7f2334bb221c 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -363,15 +539,48 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
> * the hw_ptr value is unconditionally advanced.
> */
> guard(spinlock)(&vss->lock);
> +#ifndef CONFIG_MMU
> + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> + vss->xfer_enabled && written_bytes > sizeof(msg->status)) {
> + struct snd_pcm_runtime *runtime =
> + vss->substream ? vss->substream->runtime : NULL;
> + size_t reported_bytes = written_bytes - sizeof(msg->status);
> + size_t data_bytes = min_t(size_t, reported_bytes, msg->length);
> +
> + if (!runtime || !runtime->dma_area || !vss->owned_dma_area ||
> + msg->data_offset > runtime->dma_bytes ||
> + data_bytes > runtime->dma_bytes - msg->data_offset ||
> + msg->data_offset > vss->owned_dma_bytes ||
> + data_bytes > vss->owned_dma_bytes - msg->data_offset)
> + dev_err_ratelimited(&vss->snd->vdev->dev,
> + "SID %u: invalid capture buffer for I/O message\n",
> + vss->sid);
> + else
> + memcpy(runtime->dma_area + msg->data_offset,
> + vss->owned_dma_area + msg->data_offset, data_bytes);
> + }
> +#endif
> + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> + written_bytes > sizeof(msg->status) &&
> + written_bytes - sizeof(msg->status) > msg->length)
> + dev_err_ratelimited(&vss->snd->vdev->dev,
> + "SID %u: capture length exceeds I/O message capacity\n",
> + vss->sid);
> /*
> * If the capture substream returned an incorrect status, then just
> - * increase the hw_ptr by the message size.
> + * increase the hw_ptr by the message size. Clamp valid capture lengths to
> + * the actual SG payload capacity before accounting them.
> */
> if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
> - written_bytes <= sizeof(msg->status))
> + written_bytes <= sizeof(msg->status)) {
> vss->hw_ptr += msg->length;
> - else
> - vss->hw_ptr += written_bytes - sizeof(msg->status);
> + } else {
> + size_t data_bytes = min_t(size_t,
> + written_bytes - sizeof(msg->status),
> + msg->length);
> +
> + vss->hw_ptr += data_bytes;
> + }
[Severity: High]
Can gating the NOMMU capture bounce copy on vss->xfer_enabled drop captured
audio data when a stream is paused?
In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_trigger() handles
SNDRV_PCM_TRIGGER_PAUSE_PUSH by clearing vss->xfer_enabled while leaving
vss->stopped false before synchronously sending VIRTIO_SND_R_PCM_STOP:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
scoped_guard(spinlock_irqsave, &vss->lock) {
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);
If an in-flight capture I/O message completes while vss->xfer_enabled is
false during pause, virtsnd_pcm_msg_complete() skips the memcpy() from
vss->owned_dma_area to runtime->dma_area, but still advances vss->hw_ptr by
data_bytes and resets msg->length to 0.
When the stream is later resumed with SNDRV_PCM_TRIGGER_PAUSE_RELEASE, will
userspace read stale or zeroed bytes from runtime->dma_area for that
completed period?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 05/11] ALSA: virtio: Keep control payloads alive through DMA completion
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (3 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-27 23:55 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown Joonho Lee
` (5 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
Control requests can expose caller-owned payloads to the device after a
synchronous send times out. Keep request and response headers and payloads
in the reference-counted message so caller cleanup cannot invalidate DMA.
Keep the queue reference until device completion or post-reset cleanup,
even when the caller times out or is interrupted. Consume the caller's
reference on every send path, and let response readers retain an extra
reference through copyout. Check payload sizes and allocation arithmetic
before building the scatterlists.
A control value payload is also read by the CPU as a typed structure. DMA
cacheline alignment alone does not guarantee the structure's CPU alignment.
Align regions to the greater of the DMA cacheline and control-value type
alignment, and reserve prefix slack from the buffer base so typed accesses
remain aligned while scatterlist regions stay on cacheline boundaries.
Fixes: 9d45e514da88 ("ALSA: virtio: handling control messages")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Changes in v3:
- Respect both DMA and CPU type alignment, remove the +1 offset,
and retain alignment for every region, including odd-sized payloads.
sound/virtio/virtio_ctl_msg.c | 275 ++++++++++++++++++++++++++--------
sound/virtio/virtio_ctl_msg.h | 24 ++-
sound/virtio/virtio_kctl.c | 104 +++++++------
sound/virtio/virtio_pcm_msg.c | 2 +-
4 files changed, 285 insertions(+), 120 deletions(-)
diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
index 7114bea4033a..2e0ff15e729d 100644
--- a/sound/virtio/virtio_ctl_msg.c
+++ b/sound/virtio/virtio_ctl_msg.c
@@ -3,7 +3,10 @@
* virtio-snd: Virtio sound device
* Copyright (C) 2021 OpenSynergy GmbH
*/
+#include <linux/dma-mapping.h>
+#include <linux/limits.h>
#include <linux/moduleparam.h>
+#include <linux/overflow.h>
#include <linux/virtio_config.h>
#include "virtio_card.h"
@@ -11,7 +14,12 @@
/**
* struct virtio_snd_msg - Control message.
* @sg_request: Scattergather list containing a device request (header).
+ * @sg_request_data: Scattergather list containing request data.
* @sg_response: Scattergather list containing a device response (status).
+ * @sg_response_data: Scattergather list containing response data.
+ * @dma_buffer: Message-owned storage for all DMA-visible SG regions.
+ * @request_data_size: Size of the request data buffer.
+ * @response_data_size: Size of the response data buffer.
* @list: Pending message list entry.
* @notify: Request completed notification.
* @ref_count: Reference count used to manage a message lifetime.
@@ -19,7 +27,12 @@
*/
struct virtio_snd_msg {
struct scatterlist sg_request;
+ struct scatterlist sg_request_data;
struct scatterlist sg_response;
+ struct scatterlist sg_response_data;
+ void *dma_buffer;
+ size_t request_data_size;
+ size_t response_data_size;
struct list_head list;
struct completion notify;
refcount_t ref_count;
@@ -47,8 +60,10 @@ void virtsnd_ctl_msg_ref(struct virtio_snd_msg *msg)
*/
void virtsnd_ctl_msg_unref(struct virtio_snd_msg *msg)
{
- if (refcount_dec_and_test(&msg->ref_count))
+ if (refcount_dec_and_test(&msg->ref_count)) {
+ kfree(msg->dma_buffer);
kfree(msg);
+ }
}
/**
@@ -73,10 +88,71 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
return sg_virt(&msg->sg_response);
}
+/**
+ * virtsnd_ctl_msg_request_data() - Get the request data buffer.
+ * @msg: Control message.
+ *
+ * Context: Any context.
+ */
+void *virtsnd_ctl_msg_request_data(struct virtio_snd_msg *msg)
+{
+ return msg->request_data_size ? sg_virt(&msg->sg_request_data) : NULL;
+}
+
+/**
+ * virtsnd_ctl_msg_response_data() - Get the response data buffer.
+ * @msg: Control message.
+ *
+ * Context: Any context.
+ */
+void *virtsnd_ctl_msg_response_data(struct virtio_snd_msg *msg)
+{
+ return msg->response_data_size ? sg_virt(&msg->sg_response_data) : NULL;
+}
+
+/**
+ * virtsnd_ctl_msg_sg_init() - Place one DMA region on cacheline boundaries.
+ * @sg: Scattergather entry for the region.
+ * @cursor: Current location in the message-owned DMA buffer.
+ * @size: Region length.
+ * @alignment: Alignment for DMA cachelines and typed CPU accesses.
+ *
+ * Context: Any context during message allocation.
+ * Return: 0 on success, -EOVERFLOW if the address cannot be represented.
+ */
+static int virtsnd_ctl_msg_sg_init(struct scatterlist *sg, u8 **cursor,
+ size_t size, size_t alignment)
+{
+ uintptr_t address = (uintptr_t)*cursor;
+ uintptr_t end;
+ size_t padding;
+
+ padding = address % alignment;
+ if (padding &&
+ check_add_overflow(address, (uintptr_t)(alignment - padding),
+ &address))
+ return -EOVERFLOW;
+
+ if (check_add_overflow(address, (uintptr_t)size, &end))
+ return -EOVERFLOW;
+
+ padding = end % alignment;
+ if (padding &&
+ check_add_overflow(end, (uintptr_t)(alignment - padding), &end))
+ return -EOVERFLOW;
+
+ sg_init_one(sg, (void *)address, (unsigned int)size);
+ *cursor = (u8 *)end;
+
+ return 0;
+}
+
/**
* virtsnd_ctl_msg_alloc() - Allocate and initialize a control message.
* @request_size: Size of request header.
* @response_size: Size of response header.
+ * @request_data_size: Size of request data buffer.
+ * @response_data_size: Size of response data buffer.
* @gfp: Kernel flags for memory allocation.
*
* The message will be automatically freed when the ref_count value is 0.
@@ -85,20 +161,85 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
* Return: Allocated message on success, NULL on failure.
*/
struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
- size_t response_size, gfp_t gfp)
+ size_t response_size,
+ size_t request_data_size,
+ size_t response_data_size,
+ gfp_t gfp)
{
struct virtio_snd_msg *msg;
+ struct scatterlist *sgs[4];
+ size_t sizes[4] = {
+ request_size,
+ response_size,
+ request_data_size,
+ response_data_size,
+ };
+ size_t alignment;
+ size_t dma_buffer_size;
+ size_t aligned_size;
+ size_t padding;
+ u8 *data;
+ unsigned int i;
+ int cache_alignment;
+ int rc;
- if (!request_size || !response_size)
+ if (!request_size || !response_size || request_size > UINT_MAX ||
+ response_size > UINT_MAX || request_data_size > UINT_MAX ||
+ response_data_size > UINT_MAX)
return NULL;
+ cache_alignment = dma_get_cache_alignment();
+ if (cache_alignment <= 0)
+ return NULL;
+ /* DMA coherence does not remove the CPU's type alignment requirement. */
+ alignment = max_t(size_t, cache_alignment,
+ __alignof__(struct virtio_snd_ctl_value));
+
+ /* Reserve prefix alignment slack before the first DMA region. */
+ dma_buffer_size = alignment - 1;
+ for (i = 0; i < ARRAY_SIZE(sizes); ++i) {
+ if (!sizes[i])
+ continue;
+
+ padding = sizes[i] % alignment;
+ aligned_size = sizes[i];
+ if (padding &&
+ check_add_overflow(aligned_size, alignment - padding,
+ &aligned_size))
+ return NULL;
+
+ if (check_add_overflow(dma_buffer_size, aligned_size,
+ &dma_buffer_size))
+ return NULL;
+ }
- msg = kzalloc(sizeof(*msg) + request_size + response_size, gfp);
+ msg = kzalloc(sizeof(*msg), gfp);
if (!msg)
return NULL;
- sg_init_one(&msg->sg_request, (u8 *)msg + sizeof(*msg), request_size);
- sg_init_one(&msg->sg_response, (u8 *)msg + sizeof(*msg) + request_size,
- response_size);
+ msg->dma_buffer = kzalloc(dma_buffer_size, gfp);
+ if (!msg->dma_buffer) {
+ kfree(msg);
+ return NULL;
+ }
+ data = msg->dma_buffer;
+ sgs[0] = &msg->sg_request;
+ sgs[1] = &msg->sg_response;
+ sgs[2] = &msg->sg_request_data;
+ sgs[3] = &msg->sg_response_data;
+ for (i = 0; i < ARRAY_SIZE(sizes); ++i) {
+ if (!sizes[i])
+ continue;
+
+ rc = virtsnd_ctl_msg_sg_init(sgs[i], &data, sizes[i], alignment);
+ if (rc) {
+ kfree(msg->dma_buffer);
+ kfree(msg);
+ return NULL;
+ }
+ }
+
+ msg->request_data_size = request_data_size;
+ msg->response_data_size = response_data_size;
INIT_LIST_HEAD(&msg->list);
init_completion(&msg->notify);
@@ -112,17 +253,15 @@ struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
* virtsnd_ctl_msg_send() - Send a control message.
* @snd: VirtIO sound device.
* @msg: Control message.
- * @out_sgs: Additional sg-list to attach to the request header (may be NULL).
- * @in_sgs: Additional sg-list to attach to the response header (may be NULL).
* @nowait: Flag indicating whether to wait for completion.
*
* Context: Any context. Takes and releases the control queue spinlock.
* May sleep if @nowait is false.
+ * Consumes the caller's reference on every return path.
* Return: 0 on success, -errno on failure.
*/
int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
- struct scatterlist *out_sgs,
- struct scatterlist *in_sgs, bool nowait)
+ bool nowait)
{
struct virtio_device *vdev = snd->vdev;
struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
@@ -133,6 +272,7 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
unsigned int nins = 0;
struct scatterlist *psgs[4];
bool notify = false;
+ unsigned long flags;
int rc;
virtsnd_ctl_msg_ref(msg);
@@ -142,27 +282,27 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
response->code = cpu_to_le32(VIRTIO_SND_S_IO_ERR);
psgs[nouts++] = &msg->sg_request;
- if (out_sgs)
- psgs[nouts++] = out_sgs;
+ if (msg->request_data_size)
+ psgs[nouts++] = &msg->sg_request_data;
psgs[nouts + nins++] = &msg->sg_response;
- if (in_sgs)
- psgs[nouts + nins++] = in_sgs;
-
- scoped_guard(spinlock_irqsave, &queue->lock) {
- if (snd->ctl_msgs_blocked)
- rc = -ENODEV;
- else
- rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins,
- msg, GFP_ATOMIC);
- if (!rc) {
- notify = virtqueue_kick_prepare(queue->vqueue);
+ if (msg->response_data_size)
+ psgs[nouts + nins++] = &msg->sg_response_data;
- list_add_tail(&msg->list, &snd->ctl_msgs);
- if (notify)
- virtqueue_notify(queue->vqueue);
- }
+ spin_lock_irqsave(&queue->lock, flags);
+ if (snd->ctl_msgs_blocked)
+ rc = -ENODEV;
+ else
+ rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
+ GFP_ATOMIC);
+ if (!rc) {
+ notify = virtqueue_kick_prepare(queue->vqueue);
+
+ list_add_tail(&msg->list, &snd->ctl_msgs);
+ if (notify)
+ virtqueue_notify(queue->vqueue);
}
+ spin_unlock_irqrestore(&queue->lock, flags);
if (rc) {
dev_err(&vdev->dev, "failed to send control message (0x%08x)\n",
@@ -173,46 +313,36 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
* called, it is necessary to decrement the reference count.
*/
virtsnd_ctl_msg_unref(msg);
-
- goto on_exit;
- }
-
- if (nowait)
- goto on_exit;
-
- rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
- if (rc <= 0) {
+ } else if (!nowait) {
+ rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
if (!rc) {
dev_err(&vdev->dev,
"control message (0x%08x) timeout\n",
le32_to_cpu(request->code));
rc = -ETIMEDOUT;
+ } else if (rc > 0) {
+ if (READ_ONCE(msg->cancelled)) {
+ rc = -ENODEV;
+ } else {
+ switch (le32_to_cpu(response->code)) {
+ case VIRTIO_SND_S_OK:
+ rc = 0;
+ break;
+ case VIRTIO_SND_S_NOT_SUPP:
+ rc = -EOPNOTSUPP;
+ break;
+ case VIRTIO_SND_S_IO_ERR:
+ rc = -EIO;
+ break;
+ default:
+ rc = -EINVAL;
+ break;
+ }
+ }
}
-
- goto on_exit;
- }
-
- if (READ_ONCE(msg->cancelled)) {
- rc = -ENODEV;
- goto on_exit;
- }
-
- switch (le32_to_cpu(response->code)) {
- case VIRTIO_SND_S_OK:
- rc = 0;
- break;
- case VIRTIO_SND_S_NOT_SUPP:
- rc = -EOPNOTSUPP;
- break;
- case VIRTIO_SND_S_IO_ERR:
- rc = -EIO;
- break;
- default:
- rc = -EINVAL;
- break;
}
-on_exit:
+ /* Drop the reference consumed by this send attempt. */
virtsnd_ctl_msg_unref(msg);
return rc;
@@ -308,10 +438,23 @@ int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
{
struct virtio_snd_msg *msg;
struct virtio_snd_query_info *query;
- struct scatterlist sg;
+ size_t info_size;
+ int rc;
+
+ if (count < 0)
+ return -EINVAL;
+
+ if (size > UINT_MAX ||
+ check_mul_overflow((size_t)count, size, &info_size) ||
+ info_size > UINT_MAX)
+ return -EOVERFLOW;
+
+ if (info_size && !info)
+ return -EINVAL;
msg = virtsnd_ctl_msg_alloc(sizeof(*query),
- sizeof(struct virtio_snd_hdr), GFP_KERNEL);
+ sizeof(struct virtio_snd_hdr), 0, info_size,
+ GFP_KERNEL);
if (!msg)
return -ENOMEM;
@@ -321,9 +464,13 @@ int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
query->count = cpu_to_le32(count);
query->size = cpu_to_le32(size);
- sg_init_one(&sg, info, count * size);
+ virtsnd_ctl_msg_ref(msg);
+ rc = virtsnd_ctl_msg_send(snd, msg, false);
+ if (!rc && info_size)
+ memcpy(info, virtsnd_ctl_msg_response_data(msg), info_size);
+ virtsnd_ctl_msg_unref(msg);
- return virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
+ return rc;
}
/**
diff --git a/sound/virtio/virtio_ctl_msg.h b/sound/virtio/virtio_ctl_msg.h
index d917435409d0..f867b9b9233d 100644
--- a/sound/virtio/virtio_ctl_msg.h
+++ b/sound/virtio/virtio_ctl_msg.h
@@ -21,20 +21,28 @@ void *virtsnd_ctl_msg_request(struct virtio_snd_msg *msg);
void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg);
struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
- size_t response_size, gfp_t gfp);
+ size_t response_size,
+ size_t request_data_size,
+ size_t response_data_size,
+ gfp_t gfp);
+
+void *virtsnd_ctl_msg_request_data(struct virtio_snd_msg *msg);
+
+void *virtsnd_ctl_msg_response_data(struct virtio_snd_msg *msg);
int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
- struct scatterlist *out_sgs,
- struct scatterlist *in_sgs, bool nowait);
+ bool nowait);
/**
* virtsnd_ctl_msg_send_sync() - Simplified sending of synchronous message.
* @snd: VirtIO sound device.
* @msg: Control message.
*
- * After returning from this function, the message will be deleted. If message
- * content is still needed, the caller must additionally to
- * virtsnd_ctl_msg_ref/unref() it.
+ * This function consumes the caller's message reference on return. A message
+ * accepted by the control queue keeps its queue reference until completion or
+ * post-reset cleanup, including when this function returns on timeout or
+ * interruption. Callers that read the response after success must retain an
+ * additional reference through that access.
*
* The msg_timeout_ms module parameter defines the message completion timeout.
* If the message is not completed within this time, the function will return an
@@ -49,7 +57,7 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
static inline int virtsnd_ctl_msg_send_sync(struct virtio_snd *snd,
struct virtio_snd_msg *msg)
{
- return virtsnd_ctl_msg_send(snd, msg, NULL, NULL, false);
+ return virtsnd_ctl_msg_send(snd, msg, false);
}
/**
@@ -63,7 +71,7 @@ static inline int virtsnd_ctl_msg_send_sync(struct virtio_snd *snd,
static inline int virtsnd_ctl_msg_send_async(struct virtio_snd *snd,
struct virtio_snd_msg *msg)
{
- return virtsnd_ctl_msg_send(snd, msg, NULL, NULL, true);
+ return virtsnd_ctl_msg_send(snd, msg, true);
}
void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd);
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
index 45f7b6a5b308..4cf75dedbe9a 100644
--- a/sound/virtio/virtio_kctl.c
+++ b/sound/virtio/virtio_kctl.c
@@ -3,8 +3,10 @@
* virtio-snd: Virtio sound device
* Copyright (C) 2022 OpenSynergy GmbH
*/
-#include <sound/control.h>
+#include <linux/limits.h>
+#include <linux/overflow.h>
#include <linux/virtio_config.h>
+#include <sound/control.h>
#include "virtio_card.h"
@@ -161,10 +163,12 @@ static int virtsnd_kctl_get(struct snd_kcontrol *kcontrol,
unsigned int i;
int rc;
- msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL);
+ msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0,
+ GFP_KERNEL);
if (!msg)
return -ENOMEM;
+ /* Response lifetime through ALSA value copyout. */
virtsnd_ctl_msg_ref(msg);
hdr = virtsnd_ctl_msg_request(msg);
@@ -233,7 +237,8 @@ static int virtsnd_kctl_put(struct snd_kcontrol *kcontrol,
size_t response_size = sizeof(struct virtio_snd_hdr);
unsigned int i;
- msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL);
+ msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0,
+ GFP_KERNEL);
if (!msg)
return -ENOMEM;
@@ -288,23 +293,28 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
struct virtio_snd *snd = snd_kcontrol_chip(kcontrol);
struct virtio_snd_msg *msg;
struct virtio_snd_ctl_hdr *hdr;
- unsigned int *tlv;
- struct scatterlist sg;
+ size_t request_data_size = 0;
+ size_t response_data_size = 0;
int rc;
+ switch (op_flag) {
+ case SNDRV_CTL_TLV_OP_READ:
+ response_data_size = size;
+ break;
+ case SNDRV_CTL_TLV_OP_WRITE:
+ case SNDRV_CTL_TLV_OP_CMD:
+ request_data_size = size;
+ break;
+ default:
+ return -EINVAL;
+ }
+
msg = virtsnd_ctl_msg_alloc(sizeof(*hdr), sizeof(struct virtio_snd_hdr),
+ request_data_size, response_data_size,
GFP_KERNEL);
if (!msg)
return -ENOMEM;
- tlv = kzalloc(size, GFP_KERNEL);
- if (!tlv) {
- rc = -ENOMEM;
- goto on_msg_unref;
- }
-
- sg_init_one(&sg, tlv, size);
-
hdr = virtsnd_ctl_msg_request(msg);
hdr->control_id = cpu_to_le32(kcontrol->private_value);
@@ -312,13 +322,16 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
case SNDRV_CTL_TLV_OP_READ:
hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);
- rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
- if (!rc) {
- if (copy_to_user(utlv, tlv, size))
- rc = -EFAULT;
- }
+ /* Response lifetime through user copyout. */
+ virtsnd_ctl_msg_ref(msg);
+ rc = virtsnd_ctl_msg_send(snd, msg, false);
+ if (!rc && size &&
+ copy_to_user(utlv, virtsnd_ctl_msg_response_data(msg),
+ size))
+ rc = -EFAULT;
+ virtsnd_ctl_msg_unref(msg);
- break;
+ return rc;
case SNDRV_CTL_TLV_OP_WRITE:
case SNDRV_CTL_TLV_OP_CMD:
if (op_flag == SNDRV_CTL_TLV_OP_WRITE)
@@ -327,28 +340,16 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
hdr->hdr.code =
cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
- if (copy_from_user(tlv, utlv, size)) {
- rc = -EFAULT;
- goto on_msg_unref;
- } else {
- rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
+ if (size && copy_from_user(virtsnd_ctl_msg_request_data(msg), utlv,
+ size)) {
+ virtsnd_ctl_msg_unref(msg);
+ return -EFAULT;
}
- break;
- default:
- rc = -EINVAL;
- /* We never get here - we listed all values for op_flag */
- WARN_ON(1);
- goto on_msg_unref;
+ return virtsnd_ctl_msg_send(snd, msg, false);
}
- kfree(tlv);
- return rc;
-
-on_msg_unref:
- virtsnd_ctl_msg_unref(msg);
- kfree(tlv);
- return rc;
+ return -EINVAL;
}
/**
@@ -369,27 +370,36 @@ static int virtsnd_kctl_get_enum_items(struct virtio_snd *snd, unsigned int cid)
struct virtio_snd_msg *msg;
struct virtio_snd_ctl_hdr *hdr;
unsigned int n = le32_to_cpu(kinfo->value.enumerated.items);
- struct scatterlist sg;
+ size_t items_size;
+ int rc;
- msg = virtsnd_ctl_msg_alloc(sizeof(*hdr),
- sizeof(struct virtio_snd_hdr), GFP_KERNEL);
- if (!msg)
- return -ENOMEM;
+ if (check_mul_overflow((size_t)n, sizeof(*kctl->items), &items_size) ||
+ items_size > UINT_MAX)
+ return -EOVERFLOW;
kctl->items = devm_kcalloc(&vdev->dev, n, sizeof(*kctl->items),
GFP_KERNEL);
- if (!kctl->items) {
- virtsnd_ctl_msg_unref(msg);
+ if (!kctl->items)
return -ENOMEM;
- }
- sg_init_one(&sg, kctl->items, n * sizeof(*kctl->items));
+ msg = virtsnd_ctl_msg_alloc(sizeof(*hdr),
+ sizeof(struct virtio_snd_hdr), 0, items_size,
+ GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
hdr = virtsnd_ctl_msg_request(msg);
hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_ENUM_ITEMS);
hdr->control_id = cpu_to_le32(cid);
- return virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
+ /* Item-buffer lifetime through the control-list copyout. */
+ virtsnd_ctl_msg_ref(msg);
+ rc = virtsnd_ctl_msg_send(snd, msg, false);
+ if (!rc && items_size)
+ memcpy(kctl->items, virtsnd_ctl_msg_response_data(msg), items_size);
+ virtsnd_ctl_msg_unref(msg);
+
+ return rc;
}
/**
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index b7f2334bb221..b54642a9521d 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -671,7 +671,7 @@ virtsnd_pcm_ctl_msg_alloc(struct virtio_pcm_substream *vss,
break;
}
- msg = virtsnd_ctl_msg_alloc(request_size, response_size, gfp);
+ msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0, gfp);
if (msg) {
struct virtio_snd_pcm_hdr *hdr = virtsnd_ctl_msg_request(msg);
--
2.54.0 (Apple Git-157)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 05/11] ALSA: virtio: Keep control payloads alive through DMA completion
2026-10-27 23:43 ` [PATCH v5 05/11] ALSA: virtio: Keep control payloads alive through DMA completion Joonho Lee
@ 2026-10-27 23:55 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-27 23:55 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio: misaligned control value access in virtsnd_kctl_get()
--
commit 15e13c28bcb1079484dfa947a5ae6109ebb5b3c9
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Keep control payloads alive through DMA completion
This commit moves control request and response payloads into a
reference-counted DMA buffer owned by struct virtio_snd_msg so caller
buffers are not exposed to DMA after a timeout or interruption. It also
aligns each scatterlist region to the greater of the DMA cacheline size
and the alignment of struct virtio_snd_ctl_value.
> diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
> index 7114bea4033a5..2e0ff15e729da 100644
> --- a/sound/virtio/virtio_ctl_msg.c
> +++ b/sound/virtio/virtio_ctl_msg.c
[ ... ]
> @@ -85,20 +161,85 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
> struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
> - size_t response_size, gfp_t gfp)
> + size_t response_size,
> + size_t request_data_size,
> + size_t response_data_size,
> + gfp_t gfp)
> {
[ ... ]
> + cache_alignment = dma_get_cache_alignment();
> + if (cache_alignment <= 0)
> + return NULL;
> + /* DMA coherence does not remove the CPU's type alignment requirement. */
> + alignment = max_t(size_t, cache_alignment,
> + __alignof__(struct virtio_snd_ctl_value));
[ ... ]
> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> index 45f7b6a5b3080..4cf75dedbe9a3 100644
> --- a/sound/virtio/virtio_kctl.c
> +++ b/sound/virtio/virtio_kctl.c
[ ... ]
> @@ -161,10 +163,12 @@ static int virtsnd_kctl_get(struct snd_kcontrol *kcontrol,
> unsigned int i;
> int rc;
>
> - msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL);
> + msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0,
> + GFP_KERNEL);
> if (!msg)
> return -ENOMEM;
[Severity: Medium]
Does packing both the response header and struct virtio_snd_ctl_value into
sg_response leave kvalue misaligned in virtsnd_kctl_get()?
In virtsnd_ctl_msg_alloc(), each scatterlist region is aligned to at least
__alignof__(struct virtio_snd_ctl_value), which is 8 bytes because of the
__le64 integer64[64] array.
In virtsnd_kctl_put(), the request header is struct virtio_snd_ctl_hdr (8
bytes), so placing kvalue immediately after it preserves 8-byte alignment.
However, in virtsnd_kctl_get(), response_data_size is passed as 0 and
response_size is sizeof(struct virtio_snd_hdr) + sizeof(*kvalue), with
kvalue computed right after the 4-byte struct virtio_snd_hdr:
sound/virtio/virtio_kctl.c:virtsnd_kctl_get() {
...
kvalue = (void *)((u8 *)virtsnd_ctl_msg_response(msg) +
sizeof(struct virtio_snd_hdr));
switch (type) {
...
case VIRTIO_SND_CTL_TYPE_INTEGER64:
for (i = 0; i < count; ++i)
uvalue->value.integer64.value[i] =
le64_to_cpu(kvalue->value.integer64[i]);
break;
...
}
Since virtsnd_ctl_msg_response(msg) is aligned to a multiple of 8 bytes and
sizeof(struct virtio_snd_hdr) is 4 bytes, kvalue is guaranteed to land at an
8n + 4 address, causing unaligned 64-bit loads when reading a
VIRTIO_SND_CTL_TYPE_INTEGER64 control.
Should the control value payload be placed in response_data (or the
request/response payloads placed in sg_request_data and sg_response_data) so
that kvalue starts on an aligned boundary?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (4 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 05/11] ALSA: virtio: Keep control payloads alive through DMA completion Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:07 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 07/11] ALSA: virtio: stage VZ capture and restart paused streams Joonho Lee
` (4 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
A signal can end the interruptible wait while a STOP or RELEASE request is
still pending. A later teardown may then submit another RELEASE before the
first request completes.
Keep both waits non-interruptible and use the existing message timeout.
Other control requests remain interruptible. Device reset still cancels the
pending request and wakes the waiter.
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
sound/virtio/virtio_ctl_msg.c | 15 ++++++++++++++-
sound/virtio/virtio_ctl_msg.h | 3 +++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
index 2e0ff15e729d..e50e6c7f3b8d 100644
--- a/sound/virtio/virtio_ctl_msg.c
+++ b/sound/virtio/virtio_ctl_msg.c
@@ -314,7 +314,20 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
*/
virtsnd_ctl_msg_unref(msg);
} else if (!nowait) {
- rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
+ /*
+ * Teardown must observe completion even with a pending signal.
+ * Otherwise a subsequent open can retry RELEASE while the device
+ * is still processing the previous teardown request.
+ */
+ switch (le32_to_cpu(request->code)) {
+ case VIRTIO_SND_R_PCM_STOP:
+ case VIRTIO_SND_R_PCM_RELEASE:
+ rc = wait_for_completion_timeout(&msg->notify, js);
+ break;
+ default:
+ rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
+ break;
+ }
if (!rc) {
dev_err(&vdev->dev,
"control message (0x%08x) timeout\n",
diff --git a/sound/virtio/virtio_ctl_msg.h b/sound/virtio/virtio_ctl_msg.h
index f867b9b9233d..d84f0740fcb0 100644
--- a/sound/virtio/virtio_ctl_msg.h
+++ b/sound/virtio/virtio_ctl_msg.h
@@ -47,6 +47,9 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
* The msg_timeout_ms module parameter defines the message completion timeout.
* If the message is not completed within this time, the function will return an
* error.
+ * PCM STOP and RELEASE wait through pending signals within this timeout.
+ * Other requests remain interruptible. Device reset cancels and wakes either
+ * kind of waiter.
*
* Context: Any context that permits to sleep.
* Return: 0 on success, -errno on failure.
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown
2026-10-27 23:43 ` [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown Joonho Lee
@ 2026-10-28 0:07 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:07 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: interruptible I/O flush wait in virtsnd_pcm_sync_stop()
- [Medium] ALSA: virtio: interrupted PCM_START in virtsnd_ctl_msg_send() breaks teardown
--
commit cb2fb94625971d45e77abbb9e6f9bcb8abb321ee
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: wait through signals for PCM teardown
This commit changes virtsnd_ctl_msg_send() to use an uninterruptible timeout
wait for VIRTIO_SND_R_PCM_STOP and VIRTIO_SND_R_PCM_RELEASE control requests so
that pending signals do not abort PCM teardown before completion.
> diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
> index 2e0ff15e729da..e50e6c7f3b8d1 100644
> --- a/sound/virtio/virtio_ctl_msg.c
> +++ b/sound/virtio/virtio_ctl_msg.c
> @@ -314,7 +314,20 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
> */
> virtsnd_ctl_msg_unref(msg);
> } else if (!nowait) {
> - rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
> + /*
> + * Teardown must observe completion even with a pending signal.
> + * Otherwise a subsequent open can retry RELEASE while the device
> + * is still processing the previous teardown request.
> + */
> + switch (le32_to_cpu(request->code)) {
> + case VIRTIO_SND_R_PCM_STOP:
> + case VIRTIO_SND_R_PCM_RELEASE:
> + rc = wait_for_completion_timeout(&msg->notify, js);
> + break;
[Severity: High]
When a signal is pending during PCM teardown, virtsnd_ctl_msg_send() now
waits uninterruptibly for VIRTIO_SND_R_PCM_RELEASE to complete on the control
virtqueue and returns 0 to virtsnd_pcm_sync_stop() with the signal still
pending on current.
However, right after virtsnd_ctl_msg_send_sync() returns in
sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop(), the wait for pending
TX/RX I/O buffers to drain is still interruptible:
rc = virtsnd_ctl_msg_send_sync(snd, msg);
if (rc) {
substream->runtime->stop_operating = true;
return rc;
}
rc = wait_event_interruptible_timeout(vss->msg_empty,
!virtsnd_pcm_msg_pending_num(vss),
js);
if (rc <= 0) {
dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n",
vss->sid);
substream->runtime->stop_operating = true;
return !rc ? -ETIMEDOUT : rc;
}
scoped_guard(spinlock_irqsave, &vss->lock)
vss->stopped = false;
Because TX/RX buffer completions arrive asynchronously on separate virtqueues
(VIRTIO_SND_VQ_TX / VIRTIO_SND_VQ_RX), vss->msg_count can still be non-zero
when the control message completes. With the signal still pending on current,
wait_event_interruptible_timeout() immediately returns -ERESTARTSYS, leaving
vss->stopped = true and setting substream->runtime->stop_operating = true.
In snd_pcm_release_substream(), do_hw_free() runs after snd_pcm_drop(), sees
runtime->stop_operating == true, and calls virtsnd_pcm_sync_stop() a second
time (or a subsequent virtsnd_pcm_open() preserves vss->stopped via
vss->stopped |= !!vss->msg_count and calls virtsnd_pcm_sync_stop()). Both
paths see need_release = vss->stopped || vss->msg_count as true and send a
duplicate VIRTIO_SND_R_PCM_RELEASE while the first RELEASE is still flushing
I/O or after the stream is already released. In addition,
virtsnd_pcm_hw_free() skips virtsnd_pcm_msg_free(vss) while
virtsnd_pcm_msg_pending_num(vss) is non-zero, leaving vss->msgs and retained
vss->dma_pages (or vss->owned_dma_area on !CONFIG_MMU) allocated after close.
Should the vss->msg_empty wait in virtsnd_pcm_sync_stop() also use
wait_event_timeout() so that the entire RELEASE teardown completes through
pending signals?
> + default:
> + rc = wait_for_completion_interruptible_timeout(&msg->notify, js);
> + break;
> + }
[Severity: Medium]
What happens if a signal interrupts VIRTIO_SND_R_PCM_START in this default
branch after the request has already been queued to the control virtqueue?
In sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger(), both
SNDRV_PCM_TRIGGER_START and SNDRV_PCM_TRIGGER_PAUSE_RELEASE send
VIRTIO_SND_R_PCM_START synchronously:
rc = virtsnd_ctl_msg_send_sync(snd, msg);
if (rc)
virtsnd_pcm_start_failed(vss);
return rc;
Notice that snd_pcm_drop() also triggers SNDRV_PCM_TRIGGER_PAUSE_RELEASE via
snd_pcm_pause(substream, false) during teardown when runtime->state is
SNDRV_PCM_STATE_PAUSED.
If a signal is pending or arrives while waiting for VIRTIO_SND_R_PCM_START,
virtsnd_ctl_msg_send_sync() returns -ERESTARTSYS while the device still
processes the queued VIRTIO_SND_R_PCM_START and transitions the stream to
STARTED. Meanwhile, virtsnd_pcm_start_failed() sets vss->stopped = true and
substream->runtime->stop_operating = true, and ALSA keeps runtime->state in
SNDRV_PCM_STATE_PREPARED or SNDRV_PCM_STATE_PAUSED:
1. For linked streams in snd_pcm_action_group(), ops->undo_action is only
called on streams iterated before the failing stream, so
SNDRV_PCM_TRIGGER_STOP is not sent for the interrupted stream. Subsequent
snd_pcm_stop() sees snd_pcm_running(substream) == false and skips
SNDRV_PCM_TRIGGER_STOP, after which virtsnd_pcm_sync_stop() sees
vss->stopped == true and sends VIRTIO_SND_R_PCM_RELEASE while the device
stream is still STARTED.
2. For unlinked streams where snd_pcm_undo_start() or snd_pcm_undo_pause()
sends VIRTIO_SND_R_PCM_STOP, vss->stopped remains true while
runtime->state stays PREPARED or PAUSED without snd_pcm_sync_stop()
running, so a syscall restart or retry of START/PAUSE_RELEASE fails with
-EBADFD in virtsnd_pcm_trigger(). In addition, when pause-restart mode is
active (from commit "ALSA: virtio: stage VZ capture and restart paused
streams"), an interrupted PAUSE_RELEASE during snd_pcm_drop() transitions
vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED,
skipping STOP and RELEASE and wedging the substream.
Should VIRTIO_SND_R_PCM_START also wait uninterruptibly once queued so the
driver and device stream states stay synchronized?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=6
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 07/11] ALSA: virtio: stage VZ capture and restart paused streams
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (5 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:19 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 08/11] ALSA: virtio: account staged capture reads before RX refill Joonho Lee
` (3 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
Add opt-in capture staging with response validation and accounting.
Add a pause release/reprepare state machine for the VZ profile.
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
sound/virtio/virtio_pcm.c | 93 ++++-
sound/virtio/virtio_pcm.h | 68 ++++
sound/virtio/virtio_pcm_msg.c | 286 ++++++++++++-
sound/virtio/virtio_pcm_ops.c | 734 +++++++++++++++++++++++++++++++++-
4 files changed, 1151 insertions(+), 30 deletions(-)
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index be3893de40a5..a3478fc8848d 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -310,8 +310,94 @@ 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) {
+ staged_capture = vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ vss->capture_staging;
+ }
+
+ if (!staged_capture) {
+ snd_pcm_period_elapsed(substream);
+ return;
+ }
+
+ /* Serialize the pointer snapshot with ALSA's pointer and ACK callbacks. */
+ guard(pcm_stream_lock_irqsave)(substream);
+ if (!snd_pcm_running(substream)) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ runtime = substream->runtime;
+ paused_xrun = runtime &&
+ runtime->state == SNDRV_PCM_STATE_PAUSED &&
+ vss->runtime_active && vss->xfer_xrun;
+ }
+ if (paused_xrun) {
+ /* Wake readers without sending STOP for a paused stream. */
+ snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
+ }
+ return;
+ }
+
+ 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 >
+ vss->capture_copied_total / period_bytes) {
+ vss->xfer_xrun = true;
+ notify = true;
+ } else {
+ period_floor = 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;
+ }
+ }
+
+ if (!notify)
+ return;
+
+ /* Pin the callback position while completions keep updating actual totals. */
+ snd_pcm_period_elapsed_under_stream_lock(substream);
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->capture_callback_snapshot_active = false;
+ vss->capture_callback_snapshot = 0;
+ if (vss->runtime_active && vss->xfer_enabled &&
+ !vss->xfer_xrun && substream->runtime) {
+ period_bytes = snd_pcm_lib_period_bytes(substream);
+ if (period_bytes &&
+ vss->capture_copied_total / period_bytes >
+ vss->capture_notified_periods)
+ requeue = true;
+ }
+ }
- snd_pcm_period_elapsed(vss->substream);
+ if (requeue)
+ schedule_work(&vss->elapsed_period);
}
/**
@@ -517,8 +603,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)
+ schedule_work(&vss->elapsed_period);
+ }
}
break;
}
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 61faf7b13cbc..e143a15592f0 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -15,6 +15,22 @@ struct virtio_pcm;
struct virtio_pcm_msg;
struct page;
+enum virtio_pcm_pause_restart_state {
+ VIRTSND_PCM_PAUSE_RESTART_NONE,
+ VIRTSND_PCM_PAUSE_RESTART_CONFIGURED,
+ VIRTSND_PCM_PAUSE_RESTART_SET_PARAMS_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_PREPARE_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_PREPARED,
+ VIRTSND_PCM_PAUSE_RESTART_START_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_RUNNING,
+ VIRTSND_PCM_PAUSE_RESTART_STOP_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_STOPPED,
+ VIRTSND_PCM_PAUSE_RESTART_RELEASE_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT,
+ VIRTSND_PCM_PAUSE_RESTART_RELEASED,
+ VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED,
+};
+
/**
* struct virtio_pcm_substream - VirtIO PCM substream.
* @snd: VirtIO sound device.
@@ -30,12 +46,24 @@ struct page;
* substream operators, including @stopped.
* @buffer_bytes: Current buffer size in bytes.
* @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
+ * @capture_ready: Captured bytes copied but not yet acknowledged by ALSA.
+ * @capture_copied_total: Monotonic bytes copied into the capture ring.
+ * @capture_acked_total: Monotonic bytes acknowledged by the capture client.
+ * @capture_pointer_seen_total: Last capture position sampled by ALSA.
+ * @capture_notified_periods: Highest full-period boundary reserved for notify.
+ * @capture_callback_snapshot: Capture position fixed for the period callback.
+ * @capture_callback_snapshot_active: The callback must use its fixed position.
+ * @runtime_active: ALSA runtime buffer may be accessed by the staged path.
+ * @capture_staging: Capture uses request-owned payload buffers.
+ * @capture_started: Non-empty capture data was received since prepare.
+ * @capture_empty: Consecutive empty startup completions.
* @xfer_enabled: Data transfer state (0 - off, 1 - on).
* @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
* @stopped: True if the substream is stopped and must be released on the device
* side; protected by @lock.
* @suspended: True if the substream is suspended and must be reconfigured on
* the device side at resume.
+ * @pause_restart_state: Device-side state for opt-in pause compatibility.
* @msgs: Allocated I/O messages.
* @nmsgs: Number of allocated I/O messages.
* @msg_count: Number of pending I/O messages in the virtqueue.
@@ -58,10 +86,22 @@ struct virtio_pcm_substream {
spinlock_t lock;
size_t buffer_bytes;
size_t hw_ptr;
+ size_t capture_ready;
+ u64 capture_copied_total;
+ u64 capture_acked_total;
+ u64 capture_pointer_seen_total;
+ u64 capture_notified_periods;
+ u64 capture_callback_snapshot;
+ bool capture_callback_snapshot_active;
+ bool runtime_active;
+ bool capture_staging;
+ bool capture_started;
+ unsigned int capture_empty;
bool xfer_enabled;
bool xfer_xrun;
bool stopped;
bool suspended;
+ enum virtio_pcm_pause_restart_state pause_restart_state;
struct virtio_pcm_msg **msgs;
unsigned int nmsgs;
unsigned int msg_count;
@@ -74,6 +114,27 @@ struct virtio_pcm_substream {
wait_queue_head_t msg_empty;
};
+/*
+ * ALSA may observe a subperiod tail before its period callback. Keep actual
+ * unread occupancy separate from pointer observations and notification state.
+ */
+static inline bool
+virtsnd_pcm_capture_counters_valid(const struct virtio_pcm_substream *vss)
+{
+ return vss->buffer_bytes &&
+ vss->capture_copied_total >= vss->capture_acked_total &&
+ vss->capture_copied_total - vss->capture_acked_total ==
+ (u64)vss->capture_ready &&
+ vss->capture_ready < vss->buffer_bytes &&
+ vss->capture_pointer_seen_total <= vss->capture_copied_total &&
+ vss->hw_ptr == vss->capture_copied_total % vss->buffer_bytes &&
+ (!vss->capture_callback_snapshot_active ||
+ (vss->capture_callback_snapshot >=
+ vss->capture_pointer_seen_total &&
+ vss->capture_callback_snapshot <=
+ vss->capture_copied_total));
+}
+
/**
* struct virtio_pcm_stream - VirtIO PCM stream.
* @substreams: VirtIO substreams belonging to the stream.
@@ -104,6 +165,11 @@ struct virtio_pcm {
extern const struct snd_pcm_ops virtsnd_pcm_ops[];
+extern bool virtsnd_capture_staging;
+extern bool virtsnd_capture_packed_status;
+extern bool virtsnd_capture_allow_short;
+extern bool virtsnd_capture_allow_empty;
+
int virtsnd_pcm_validate(struct virtio_device *vdev);
int virtsnd_pcm_parse_cfg(struct virtio_snd *snd);
@@ -127,6 +193,8 @@ virtsnd_pcm_ctl_msg_alloc(struct virtio_pcm_substream *vss,
int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
unsigned int periods, unsigned int period_bytes);
+int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss);
+
void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss);
void virtsnd_pcm_msg_reset(struct virtio_pcm_substream *vss);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index b54642a9521d..795e52e09564 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -6,6 +6,7 @@
#include <linux/limits.h>
#include <linux/mm.h>
#include <linux/overflow.h>
+#include <linux/vmalloc.h>
#include <sound/pcm_params.h>
#include "virtio_card.h"
@@ -16,6 +17,7 @@
* @xfer: Request header payload.
* @status: Response header payload.
* @length: Data length in bytes.
+ * @data: Request-owned payload for staged capture.
* @data_offset: Offset in the driver-owned PCM buffer on NOMMU systems.
* @sgs: Payload scatter-gather table.
*/
@@ -24,6 +26,7 @@ struct virtio_pcm_msg {
struct virtio_snd_pcm_xfer xfer;
struct virtio_snd_pcm_status status;
size_t length;
+ u8 *data;
#ifndef CONFIG_MMU
size_t data_offset;
#endif
@@ -228,6 +231,8 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
if (check_mul_overflow((size_t)periods, (size_t)period_bytes,
&buffer_bytes))
return -EOVERFLOW;
+ if (vss->capture_staging && periods < 2)
+ return -EINVAL;
if (!vss->substream || !vss->substream->runtime)
return -EINVAL;
@@ -236,14 +241,19 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
return -EINVAL;
#ifdef CONFIG_MMU
- rc = virtsnd_pcm_dma_pages_get(vss, runtime->dma_area, buffer_bytes);
- if (rc)
- return rc;
+ if (!vss->capture_staging) {
+ rc = virtsnd_pcm_dma_pages_get(vss, runtime->dma_area,
+ buffer_bytes);
+ if (rc)
+ return rc;
+ }
#else
- vss->owned_dma_area = kzalloc(buffer_bytes, GFP_KERNEL);
- if (!vss->owned_dma_area)
- return -ENOMEM;
- vss->owned_dma_bytes = buffer_bytes;
+ if (!vss->capture_staging) {
+ vss->owned_dma_area = kzalloc(buffer_bytes, GFP_KERNEL);
+ if (!vss->owned_dma_area)
+ return -ENOMEM;
+ vss->owned_dma_bytes = buffer_bytes;
+ }
#endif
vss->msgs = kzalloc_objs(*vss->msgs, periods);
@@ -255,27 +265,42 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
vss->nmsgs = periods;
for (i = 0; i < periods; ++i) {
- int sg_num;
struct virtio_pcm_msg *msg;
u8 *data;
+ int sg_num;
+
+ if (vss->capture_staging) {
+ data = vzalloc(period_bytes);
+ } else {
#ifdef CONFIG_MMU
- data = runtime->dma_area + (size_t)period_bytes * i;
+ data = runtime->dma_area + (size_t)period_bytes * i;
#else
- data = vss->owned_dma_area + (size_t)period_bytes * i;
+ data = vss->owned_dma_area + (size_t)period_bytes * i;
#endif
+ }
+ if (!data) {
+ rc = -ENOMEM;
+ goto err_free_msgs;
+ }
+
sg_num = virtsnd_pcm_sg_num(data, period_bytes);
if (sg_num < 0) {
+ if (vss->capture_staging)
+ vfree(data);
rc = sg_num;
goto err_free_msgs;
}
msg = kzalloc_flex(*msg, sgs, sg_num + 2);
if (!msg) {
+ if (vss->capture_staging)
+ vfree(data);
rc = -ENOMEM;
goto err_free_msgs;
}
msg->substream = vss;
+ msg->data = vss->capture_staging ? data : NULL;
vss->msgs[i] = msg;
sg_init_one(&msg->sgs[PCM_MSG_SG_XFER], &msg->xfer,
sizeof(msg->xfer));
@@ -316,8 +341,12 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
{
unsigned int i;
- for (i = 0; vss->msgs && i < vss->nmsgs; ++i)
- kfree(vss->msgs[i]);
+ for (i = 0; vss->msgs && i < vss->nmsgs; ++i) {
+ if (vss->msgs[i]) {
+ vfree(vss->msgs[i]->data);
+ kfree(vss->msgs[i]);
+ }
+ }
kfree(vss->msgs);
#ifdef CONFIG_MMU
virtsnd_pcm_dma_pages_put(vss);
@@ -348,6 +377,15 @@ void virtsnd_pcm_msg_reset(struct virtio_pcm_substream *vss)
guard(spinlock_irqsave)(&vss->lock);
for (i = 0; vss->msgs && i < vss->nmsgs; ++i)
vss->msgs[i]->length = 0;
+ vss->capture_ready = 0;
+ vss->capture_copied_total = 0;
+ vss->capture_acked_total = 0;
+ vss->capture_pointer_seen_total = 0;
+ vss->capture_notified_periods = 0;
+ vss->capture_callback_snapshot = 0;
+ vss->capture_callback_snapshot_active = false;
+ vss->capture_empty = 0;
+ vss->capture_started = false;
}
/**
@@ -470,6 +508,224 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
return 0;
}
+/**
+ * virtsnd_pcm_capture_send() - Queue staged capture requests.
+ * @vss: VirtIO PCM substream.
+ *
+ * Queue full-period requests while the outstanding requests fit the remaining
+ * ALSA ring capacity. The final request may exceed that capacity by less than
+ * one period; completion validates the returned payload before copying it.
+ *
+ * Context: Expects the RX queue and substream spinlocks to be held by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss)
+{
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct virtqueue *vqueue = queue->vqueue;
+ unsigned long period_bytes;
+ size_t free_bytes;
+ size_t request_limit;
+ unsigned int i;
+ bool added = false;
+
+ if (!vss->capture_staging || !vss->runtime_active)
+ return -EBADFD;
+
+ period_bytes = snd_pcm_lib_period_bytes(vss->substream);
+ if (!period_bytes || !vss->buffer_bytes ||
+ period_bytes >= vss->buffer_bytes ||
+ !virtsnd_pcm_capture_counters_valid(vss) ||
+ vss->msg_count > vss->nmsgs)
+ return -EIO;
+
+ free_bytes = vss->buffer_bytes - vss->capture_ready;
+ request_limit = free_bytes / period_bytes +
+ !!(free_bytes % period_bytes);
+ request_limit = min_t(size_t, request_limit, vss->nmsgs);
+ if (vss->msg_count > request_limit)
+ return -EIO;
+
+ for (i = 0; i < vss->nmsgs && vss->msg_count < request_limit; ++i) {
+ struct virtio_pcm_msg *msg = vss->msgs[i];
+ struct scatterlist *sgs[] = {
+ &msg->sgs[PCM_MSG_SG_XFER],
+ &msg->sgs[PCM_MSG_SG_DATA],
+ &msg->sgs[PCM_MSG_SG_STATUS]
+ };
+ int rc;
+
+ if (msg->length)
+ continue;
+
+ msg->xfer.stream_id = cpu_to_le32(vss->sid);
+ /* Do not let an absent packed trailer reuse old PCM bytes. */
+ memset(msg->data, 0, period_bytes);
+ memset(&msg->status, 0, sizeof(msg->status));
+ msg->status.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+ rc = virtqueue_add_sgs(vqueue, sgs, 1, 2, msg, GFP_ATOMIC);
+ if (rc) {
+ if (added && !(vss->features &
+ (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
+ virtqueue_kick_prepare(vqueue))
+ virtqueue_notify(vqueue);
+ return rc;
+ }
+
+ msg->length = period_bytes;
+ vss->msg_count++;
+ added = true;
+ }
+
+ if (added && !(vss->features &
+ (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
+ virtqueue_kick_prepare(vqueue))
+ virtqueue_notify(vqueue);
+
+ return 0;
+}
+
+static int virtsnd_pcm_capture_status(const struct virtio_pcm_msg *msg,
+ size_t written_bytes,
+ size_t frame_bytes,
+ size_t *audio_bytes,
+ u32 *latency_bytes)
+{
+ struct virtio_snd_pcm_status status;
+ size_t max_written;
+ size_t first;
+
+ if (!frame_bytes ||
+ check_add_overflow(msg->length, sizeof(status), &max_written) ||
+ written_bytes < sizeof(status) || written_bytes > max_written)
+ return -EIO;
+
+ *audio_bytes = written_bytes - sizeof(status);
+ if (*audio_bytes > msg->length || *audio_bytes % frame_bytes)
+ return -EIO;
+
+ if (virtsnd_capture_packed_status) {
+ first = min_t(size_t, sizeof(status),
+ msg->length - *audio_bytes);
+ if (first)
+ memcpy(&status, msg->data + *audio_bytes, first);
+ if (first < sizeof(status))
+ memcpy((u8 *)&status + first, &msg->status,
+ sizeof(status) - first);
+ } else {
+ status = msg->status;
+ }
+
+ if (le32_to_cpu(status.status) != VIRTIO_SND_S_OK)
+ return -EIO;
+
+ *latency_bytes = le32_to_cpu(status.latency_bytes);
+ return 0;
+}
+
+static void virtsnd_pcm_capture_xrun_locked(struct virtio_pcm_substream *vss)
+{
+ if (!vss->xfer_xrun) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ }
+}
+
+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 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;
+ size_t frame_bytes = runtime ? frames_to_bytes(runtime, 1) : 0;
+ u32 latency_bytes;
+ const char *invalid_reason = NULL;
+
+ /* A full ring is modulo-ambiguous, so fail closed before copying. */
+ if (!runtime || !runtime->dma_area)
+ invalid_reason = "runtime";
+ else if (!virtsnd_pcm_capture_counters_valid(vss))
+ invalid_reason = "counters";
+ else if (vss->hw_ptr !=
+ vss->capture_copied_total % vss->buffer_bytes)
+ invalid_reason = "hw_ptr";
+ else if (virtsnd_pcm_capture_status(msg, written_bytes,
+ frame_bytes, &audio_bytes,
+ &latency_bytes))
+ invalid_reason = "status";
+ else if (audio_bytes >=
+ vss->buffer_bytes - vss->capture_ready)
+ invalid_reason = "ring_capacity";
+ else if (check_add_overflow(vss->capture_ready, audio_bytes,
+ &next_ready))
+ invalid_reason = "ready_overflow";
+ else if (next_ready >= vss->buffer_bytes)
+ invalid_reason = "ring_full";
+ else if (check_add_overflow(vss->capture_copied_total,
+ (u64)audio_bytes,
+ &next_copied_total))
+ invalid_reason = "total_overflow";
+
+ if (invalid_reason) {
+ virtsnd_pcm_capture_xrun_locked(vss);
+ dev_warn_ratelimited(&vss->snd->vdev->dev,
+ "SID %u: staged capture completion rejected (%s)\n",
+ vss->sid, invalid_reason);
+ } else if (!audio_bytes) {
+ unsigned int limit = min_t(unsigned int, vss->nmsgs,
+ UINT_MAX / 2) * 2;
+
+ if (!virtsnd_capture_allow_empty || vss->capture_started ||
+ vss->capture_empty >= limit) {
+ virtsnd_pcm_capture_xrun_locked(vss);
+ } else {
+ vss->capture_empty++;
+ }
+ } else if (audio_bytes < msg->length &&
+ !virtsnd_capture_allow_short) {
+ virtsnd_pcm_capture_xrun_locked(vss);
+ } else {
+ size_t ring_ptr = vss->capture_copied_total %
+ vss->buffer_bytes;
+ size_t first = min(audio_bytes,
+ vss->buffer_bytes - ring_ptr);
+
+ memcpy(runtime->dma_area + ring_ptr, msg->data, first);
+ if (audio_bytes > first)
+ memcpy(runtime->dma_area, msg->data + first,
+ audio_bytes - first);
+ vss->capture_copied_total = next_copied_total;
+ vss->hw_ptr = next_copied_total % vss->buffer_bytes;
+ vss->capture_ready = next_ready;
+ vss->capture_started = true;
+ vss->capture_empty = 0;
+ runtime->delay = bytes_to_frames(runtime, latency_bytes);
+ got_audio = true;
+ }
+ }
+
+ msg->length = 0;
+ if (vss->runtime_active && vss->xfer_enabled) {
+ if (!vss->xfer_xrun && virtsnd_pcm_capture_send(vss)) {
+ dev_warn_ratelimited(&vss->snd->vdev->dev,
+ "SID %u: failed to refill capture queue\n",
+ vss->sid);
+ virtsnd_pcm_capture_xrun_locked(vss);
+ }
+ if (got_audio && !vss->xfer_xrun)
+ schedule_work(&vss->elapsed_period);
+ } else if (!vss->msg_count) {
+ wake_up_all(&vss->msg_empty);
+ }
+}
+
/**
* virtsnd_pcm_msg_pending_num() - Returns the number of pending I/O messages.
* @vss: VirtIO substream.
@@ -533,6 +789,12 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
{
struct virtio_pcm_substream *vss = msg->substream;
+ if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ vss->capture_staging) {
+ virtsnd_pcm_capture_complete(msg, written_bytes);
+ return;
+ }
+
/*
* hw_ptr always indicates the buffer position of the first I/O message
* in the virtqueue. Therefore, on each completion of an I/O message,
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index f74bf365065b..003832d11255 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -3,10 +3,41 @@
* virtio-snd: Virtio sound device
* Copyright (C) 2021 OpenSynergy GmbH
*/
+#include <linux/moduleparam.h>
+#include <linux/overflow.h>
#include <sound/pcm_params.h>
#include "virtio_card.h"
+bool virtsnd_capture_staging;
+module_param_named(capture_staging, virtsnd_capture_staging, bool, 0444);
+MODULE_PARM_DESC(capture_staging,
+ "Use request-owned buffers for experimental PCM capture");
+
+bool virtsnd_capture_packed_status;
+module_param_named(capture_packed_status, virtsnd_capture_packed_status,
+ bool, 0444);
+MODULE_PARM_DESC(capture_packed_status,
+ "Read staged capture status after the actual audio payload");
+
+bool virtsnd_capture_allow_short;
+module_param_named(capture_allow_short, virtsnd_capture_allow_short,
+ bool, 0444);
+MODULE_PARM_DESC(capture_allow_short,
+ "Accept non-empty short staged capture responses");
+
+bool virtsnd_capture_allow_empty;
+module_param_named(capture_allow_empty, virtsnd_capture_allow_empty,
+ bool, 0444);
+MODULE_PARM_DESC(capture_allow_empty,
+ "Retry bounded empty startup capture responses");
+
+static bool virtsnd_capture_pause_restart;
+module_param_named(capture_pause_restart, virtsnd_capture_pause_restart,
+ bool, 0444);
+MODULE_PARM_DESC(capture_pause_restart,
+ "Release and reprepare staged capture streams across pause");
+
/*
* I/O messages lifetime
* ---------------------
@@ -98,15 +129,195 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
static int virtsnd_pcm_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,
+ bool pause_restart)
{
scoped_guard(spinlock_irqsave, &vss->lock) {
vss->xfer_enabled = false;
vss->stopped = true;
+ if (pause_restart)
+ vss->xfer_xrun = true;
}
vss->substream->runtime->stop_operating = true;
}
+static bool
+virtsnd_pcm_pause_restart_enabled(struct virtio_pcm_substream *vss)
+{
+ return virtsnd_capture_pause_restart && vss->capture_staging &&
+ vss->direction == SNDRV_PCM_STREAM_CAPTURE;
+}
+
+static bool virtsnd_pcm_pause_restart_transitioning(enum virtio_pcm_pause_restart_state state)
+{
+ switch (state) {
+ case VIRTSND_PCM_PAUSE_RESTART_SET_PARAMS_SUBMITTED:
+ case VIRTSND_PCM_PAUSE_RESTART_PREPARE_SUBMITTED:
+ case VIRTSND_PCM_PAUSE_RESTART_START_SUBMITTED:
+ case VIRTSND_PCM_PAUSE_RESTART_STOP_SUBMITTED:
+ case VIRTSND_PCM_PAUSE_RESTART_RELEASE_SUBMITTED:
+ return true;
+ default:
+ return false;
+ }
+}
+
+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;
+ int rc;
+
+ if (!virtsnd_pcm_pause_restart_enabled(vss))
+ return virtsnd_ctl_msg_send_sync(vss->snd, msg);
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ state = vss->pause_restart_state;
+ 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;
+ }
+ }
+
+ if (rc) {
+ virtsnd_ctl_msg_unref(msg);
+ return rc;
+ }
+
+ rc = virtsnd_ctl_msg_send_sync(vss->snd, msg);
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ if (rc || vss->pause_restart_state != submitted_state) {
+ vss->pause_restart_state =
+ VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
+ if (!rc)
+ rc = -EIO;
+ } else {
+ vss->pause_restart_state = success_state;
+ }
+ }
+
+ if (rc)
+ dev_err(&vss->snd->vdev->dev,
+ "SID %u: PCM control state is unknown; device reprobe required\n",
+ vss->sid);
+
+ return rc;
+}
+
+static void
+virtsnd_pcm_pause_restart_fail(struct virtio_pcm_substream *vss,
+ bool reset_required)
+{
+ bool notify_xrun;
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ notify_xrun = !vss->xfer_xrun;
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ vss->xfer_xrun = true;
+ if (reset_required)
+ vss->pause_restart_state =
+ VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
+ }
+ if (notify_xrun)
+ schedule_work(&vss->elapsed_period);
+ vss->substream->runtime->stop_operating = true;
+}
+
+static int virtsnd_pcm_dev_prepare(struct virtio_pcm_substream *vss)
+{
+ struct virtio_snd_msg *msg;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
+ GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ return virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_CONFIGURED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RELEASED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_STOPPED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED),
+ VIRTSND_PCM_PAUSE_RESTART_PREPARE_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_PREPARED);
+}
+
+static int virtsnd_pcm_pause_prepare(struct virtio_pcm_substream *vss)
+{
+ struct virtio_snd_msg *msg;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
+ GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ return virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RELEASED),
+ VIRTSND_PCM_PAUSE_RESTART_PREPARE_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_PREPARED);
+}
+
+static int
+virtsnd_pcm_pause_release_flush(struct virtio_pcm_substream *vss,
+ unsigned long allowed_states)
+{
+ struct virtio_snd_msg *msg;
+ unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
+ long waited;
+ int rc;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE,
+ GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ rc = virtsnd_pcm_pause_restart_send(vss, msg,
+ allowed_states,
+ VIRTSND_PCM_PAUSE_RESTART_RELEASE_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT);
+ if (rc)
+ return rc;
+
+ waited = wait_event_timeout(vss->msg_empty,
+ !virtsnd_pcm_msg_pending_num(vss), js);
+ if (waited <= 0) {
+ dev_warn(&vss->snd->vdev->dev,
+ "SID %u: pause RELEASE did not flush I/O queue\n",
+ vss->sid);
+ return waited ? (int)waited : -ETIMEDOUT;
+ }
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ if (vss->msg_count ||
+ vss->pause_restart_state !=
+ VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT) {
+ rc = -ETIMEDOUT;
+ } else {
+ vss->pause_restart_state =
+ VIRTSND_PCM_PAUSE_RESTART_RELEASED;
+ }
+ }
+
+ return rc;
+}
+
+static void virtsnd_pcm_pause_restart_failed(struct virtio_pcm_substream *vss)
+{
+ virtsnd_pcm_pause_restart_fail(vss, false);
+}
+
/**
* virtsnd_pcm_open() - Open the PCM substream.
* @substream: Kernel ALSA substream.
@@ -122,6 +333,14 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
substream->runtime->hw = vss->hw;
substream->private_data = vss;
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->capture_staging = virtsnd_capture_staging &&
+ vss->direction == SNDRV_PCM_STREAM_CAPTURE;
+ if (vss->capture_staging) {
+ vss->runtime_active = false;
+ vss->xfer_enabled = false;
+ }
+ }
snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS);
@@ -147,6 +366,16 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
*/
static int virtsnd_pcm_close(struct snd_pcm_substream *substream)
{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+
+ if (vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->runtime_active = false;
+ vss->xfer_enabled = false;
+ }
+ cancel_work_sync(&vss->elapsed_period);
+ }
+
return 0;
}
@@ -213,7 +442,14 @@ static int virtsnd_pcm_dev_set_params(struct virtio_pcm_substream *vss,
request->features |=
cpu_to_le32(1U << VIRTIO_SND_PCM_F_EVT_XRUNS);
- return virtsnd_ctl_msg_send_sync(vss->snd, msg);
+ return virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_NONE) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_CONFIGURED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_STOPPED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RELEASED),
+ VIRTSND_PCM_PAUSE_RESTART_SET_PARAMS_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_CONFIGURED);
}
/**
@@ -233,6 +469,14 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
bool stopped;
int rc;
+ if (vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->runtime_active = false;
+ vss->xfer_enabled = false;
+ }
+ cancel_work_sync(&vss->elapsed_period);
+ }
+
scoped_guard(spinlock_irqsave, &vss->lock) {
msg_count = vss->msg_count;
stopped = vss->stopped;
@@ -263,8 +507,14 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
*/
virtsnd_pcm_msg_free(vss);
- return virtsnd_pcm_msg_alloc(vss, params_periods(hw_params),
- params_period_bytes(hw_params));
+ rc = virtsnd_pcm_msg_alloc(vss, params_periods(hw_params),
+ params_period_bytes(hw_params));
+ if (!rc && vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->runtime_active = true;
+ }
+
+ return rc;
}
/**
@@ -278,6 +528,24 @@ static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ if (vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->runtime_active = false;
+ vss->xfer_enabled = false;
+ }
+ cancel_work_sync(&vss->elapsed_period);
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->hw_ptr = 0;
+ vss->capture_ready = 0;
+ vss->capture_copied_total = 0;
+ vss->capture_acked_total = 0;
+ vss->capture_pointer_seen_total = 0;
+ vss->capture_notified_periods = 0;
+ vss->capture_callback_snapshot = 0;
+ vss->capture_callback_snapshot_active = false;
+ }
+ }
+
/* If the queue is flushed, we can safely free the messages here. */
if (!virtsnd_pcm_msg_pending_num(vss))
virtsnd_pcm_msg_free(vss);
@@ -296,9 +564,22 @@ static int virtsnd_pcm_prepare(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);
+ int rc;
bool stopped;
+ 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);
+ return -EIO;
+ }
+ }
+
scoped_guard(spinlock_irqsave, &vss->lock)
stopped = vss->stopped;
if (stopped) {
@@ -321,16 +602,23 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
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)
+ virtsnd_pcm_pause_restart_fail(vss, false);
return rc;
+ }
}
- vss->xfer_xrun = false;
+ /* ALSA prepare resets its pointers even after a suspended stream. */
+ if (vss->capture_staging)
+ vss->hw_ptr = 0;
+
+ if (!pause_restart)
+ vss->xfer_xrun = false;
vss->suspended = false;
vss->msg_count = 0;
virtsnd_pcm_msg_reset(vss);
@@ -340,12 +628,210 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
vss->pcm_indirect.hw_buffer_size =
snd_pcm_lib_buffer_bytes(substream);
- msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
- GFP_KERNEL);
- if (!msg)
+ rc = virtsnd_pcm_dev_prepare(vss);
+ if (pause_restart) {
+ if (rc) {
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ } else {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->stopped = false;
+ vss->xfer_xrun = false;
+ }
+ }
+ }
+
+ return rc;
+}
+
+static int virtsnd_pcm_pause_restart_start(struct virtio_pcm_substream *vss,
+ bool resume)
+{
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct virtio_snd_msg *msg;
+ enum virtio_pcm_pause_restart_state state;
+ bool reset_required;
+ bool invalid_state;
+ int rc = 0;
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ state = vss->pause_restart_state;
+ reset_required =
+ state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
+ virtsnd_pcm_pause_restart_transitioning(state);
+ invalid_state =
+ vss->xfer_xrun ||
+ (resume &&
+ (state != VIRTSND_PCM_PAUSE_RESTART_RELEASED ||
+ !vss->stopped || vss->msg_count ||
+ !vss->runtime_active)) ||
+ (!resume &&
+ (state != VIRTSND_PCM_PAUSE_RESTART_PREPARED ||
+ vss->stopped));
+ }
+ if (reset_required) {
+ virtsnd_pcm_pause_restart_fail(vss, true);
+ return -EIO;
+ }
+ if (invalid_state) {
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return -EBADFD;
+ }
+
+ if (resume) {
+ rc = virtsnd_pcm_pause_prepare(vss);
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ if (vss->pause_restart_state !=
+ VIRTSND_PCM_PAUSE_RESTART_PREPARED ||
+ vss->msg_count || !vss->runtime_active) {
+ rc = -EBADFD;
+ } else {
+ /* Preserve ring accounting, restarting only startup grace. */
+ vss->capture_started = false;
+ vss->capture_empty = 0;
+ }
+ }
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
+ }
+
+ scoped_guard(spinlock_irqsave, &queue->lock) {
+ guard(spinlock)(&vss->lock);
+ if (vss->pause_restart_state !=
+ VIRTSND_PCM_PAUSE_RESTART_PREPARED ||
+ vss->msg_count || !vss->runtime_active || vss->xfer_xrun) {
+ rc = -EBADFD;
+ } else {
+ rc = virtsnd_pcm_capture_send(vss);
+ if (!rc)
+ vss->xfer_enabled = true;
+ }
+ }
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
+
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, GFP_KERNEL);
+ if (!msg) {
+ virtsnd_pcm_pause_restart_failed(vss);
return -ENOMEM;
+ }
+
+ rc = virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED),
+ VIRTSND_PCM_PAUSE_RESTART_START_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_RUNNING);
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
- return virtsnd_ctl_msg_send_sync(vss->snd, msg);
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->stopped = false;
+
+ return 0;
+}
+
+static int virtsnd_pcm_pause_restart_pause(struct virtio_pcm_substream *vss)
+{
+ struct virtio_snd_msg *msg;
+ enum virtio_pcm_pause_restart_state state;
+ int rc;
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ state = vss->pause_restart_state;
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT ||
+ state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)
+ return 0;
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ }
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
+ virtsnd_pcm_pause_restart_transitioning(state)) {
+ virtsnd_pcm_pause_restart_fail(vss, true);
+ return -EIO;
+ }
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RUNNING) {
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
+ GFP_KERNEL);
+ if (!msg) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return -ENOMEM;
+ }
+
+ rc = virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RUNNING),
+ VIRTSND_PCM_PAUSE_RESTART_STOP_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_STOPPED);
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
+ state = VIRTSND_PCM_PAUSE_RESTART_STOPPED;
+ } else if (state != VIRTSND_PCM_PAUSE_RESTART_STOPPED &&
+ state != VIRTSND_PCM_PAUSE_RESTART_PREPARED) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return -EBADFD;
+ }
+
+ rc = virtsnd_pcm_pause_release_flush(vss,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_STOPPED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED));
+ if (rc) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return rc;
+ }
+
+ return 0;
+}
+
+static int virtsnd_pcm_pause_restart_stop(struct virtio_pcm_substream *vss,
+ bool suspend)
+{
+ struct virtio_snd_msg *msg;
+ enum virtio_pcm_pause_restart_state state;
+ int rc;
+
+ if (suspend)
+ vss->suspended = true;
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ 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);
+ return -EIO;
+ }
+ if (state != VIRTSND_PCM_PAUSE_RESTART_RUNNING)
+ return 0;
+
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, GFP_KERNEL);
+ if (!msg) {
+ virtsnd_pcm_pause_restart_failed(vss);
+ return -ENOMEM;
+ }
+
+ rc = virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RUNNING),
+ VIRTSND_PCM_PAUSE_RESTART_STOP_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_STOPPED);
+ if (rc)
+ virtsnd_pcm_pause_restart_failed(vss);
+
+ return rc;
}
/**
@@ -365,6 +851,23 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
struct virtio_snd_msg *msg;
int rc = 0;
+ 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:
@@ -376,8 +879,17 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
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 (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
+ if (vss->capture_staging) {
+ if (!vss->runtime_active)
+ rc = -EBADFD;
+ else
+ rc = virtsnd_pcm_capture_send(vss);
+ } else {
+ rc = virtsnd_pcm_msg_send(vss, 0,
+ vss->buffer_bytes);
+ }
+ }
if (rc) {
vss->stopped = true;
substream->runtime->stop_operating = true;
@@ -389,13 +901,13 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
GFP_KERNEL);
if (!msg) {
- virtsnd_pcm_start_failed(vss);
+ virtsnd_pcm_start_failed(vss, false);
return -ENOMEM;
}
rc = virtsnd_ctl_msg_send_sync(snd, msg);
if (rc)
- virtsnd_pcm_start_failed(vss);
+ virtsnd_pcm_start_failed(vss, false);
return rc;
case SNDRV_PCM_TRIGGER_SUSPEND:
@@ -420,6 +932,106 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
}
}
+static int
+virtsnd_pcm_pause_restart_sync_stop(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
+ enum virtio_pcm_pause_restart_state state;
+ struct virtio_snd_msg *msg;
+ long waited;
+ int rc;
+
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ state = vss->pause_restart_state;
+ }
+ cancel_work_sync(&vss->elapsed_period);
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
+ virtsnd_pcm_pause_restart_transitioning(state)) {
+ virtsnd_pcm_pause_restart_fail(vss, true);
+ dev_err(&vss->snd->vdev->dev,
+ "SID %u: PCM state is unknown; device reprobe required\n",
+ vss->sid);
+ return -EIO;
+ }
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_NONE ||
+ state == VIRTSND_PCM_PAUSE_RESTART_CONFIGURED) {
+ if (virtsnd_pcm_msg_pending_num(vss)) {
+ virtsnd_pcm_pause_restart_fail(vss, true);
+ return -EIO;
+ }
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->stopped = false;
+ return 0;
+ }
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RUNNING) {
+ msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
+ GFP_KERNEL);
+ if (!msg) {
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return -ENOMEM;
+ }
+
+ rc = virtsnd_pcm_pause_restart_send(vss, msg,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_RUNNING),
+ VIRTSND_PCM_PAUSE_RESTART_STOP_SUBMITTED,
+ VIRTSND_PCM_PAUSE_RESTART_STOPPED);
+ if (rc) {
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return rc;
+ }
+ state = VIRTSND_PCM_PAUSE_RESTART_STOPPED;
+ }
+
+ if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT ||
+ state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) {
+ waited = wait_event_timeout(vss->msg_empty,
+ !virtsnd_pcm_msg_pending_num(vss), js);
+ if (waited <= 0) {
+ rc = waited ? (int)waited : -ETIMEDOUT;
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return rc;
+ }
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ if (vss->msg_count) {
+ rc = -ETIMEDOUT;
+ } else {
+ vss->pause_restart_state =
+ VIRTSND_PCM_PAUSE_RESTART_RELEASED;
+ vss->stopped = false;
+ rc = 0;
+ }
+ }
+ if (rc)
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return rc;
+ }
+
+ if (state != VIRTSND_PCM_PAUSE_RESTART_PREPARED &&
+ state != VIRTSND_PCM_PAUSE_RESTART_STOPPED) {
+ virtsnd_pcm_pause_restart_fail(vss, true);
+ return -EIO;
+ }
+
+ rc = virtsnd_pcm_pause_release_flush(vss,
+ BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED) |
+ BIT(VIRTSND_PCM_PAUSE_RESTART_STOPPED));
+ if (rc) {
+ virtsnd_pcm_pause_restart_fail(vss, false);
+ return rc;
+ }
+
+ scoped_guard(spinlock_irqsave, &vss->lock)
+ vss->stopped = false;
+
+ return 0;
+}
+
/**
* virtsnd_pcm_sync_stop() - Synchronous PCM substream stop.
* @substream: Kernel ALSA substream.
@@ -439,6 +1051,9 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
bool need_release;
int rc;
+ if (virtsnd_pcm_pause_restart_enabled(vss))
+ return virtsnd_pcm_pause_restart_sync_stop(substream);
+
scoped_guard(spinlock_irqsave, &vss->lock) {
vss->xfer_enabled = false;
need_release = vss->stopped || vss->msg_count;
@@ -515,6 +1130,38 @@ static snd_pcm_uframes_t
virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ size_t hw_ptr = 0;
+ u64 total;
+ bool xrun;
+
+ if (vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ xrun = vss->xfer_xrun ||
+ !virtsnd_pcm_capture_counters_valid(vss);
+ if (!xrun) {
+ total = vss->capture_callback_snapshot_active ?
+ vss->capture_callback_snapshot :
+ vss->capture_copied_total;
+ /* A full-ring observation gap is modulo-ambiguous. */
+ if (total < vss->capture_pointer_seen_total ||
+ total - vss->capture_pointer_seen_total >=
+ vss->buffer_bytes) {
+ xrun = true;
+ vss->xfer_xrun = true;
+ } else {
+ vss->capture_pointer_seen_total = total;
+ hw_ptr = total % vss->buffer_bytes;
+ }
+ }
+ if (xrun)
+ vss->xfer_xrun = true;
+ }
+
+ if (xrun)
+ return SNDRV_PCM_POS_XRUN;
+
+ return bytes_to_frames(substream->runtime, hw_ptr);
+ }
return snd_pcm_indirect_capture_pointer(substream,
&vss->pcm_indirect,
@@ -586,11 +1233,66 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
{
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct snd_pcm_runtime *runtime = substream->runtime;
struct virtsnd_pcm_transfer transfer = {};
int rc;
scoped_guard(spinlock_irqsave, &queue->lock) {
guard(spinlock)(&vss->lock);
+ if (vss->capture_staging) {
+ snd_pcm_uframes_t appl_ptr;
+ snd_pcm_sframes_t diff;
+ size_t bytes;
+ u64 next_acked_total;
+
+ if (!vss->runtime_active || !runtime || !runtime->control ||
+ !runtime->dma_area)
+ return -EBADFD;
+ if (vss->xfer_xrun)
+ return -EPIPE;
+ if (!virtsnd_pcm_capture_counters_valid(vss)) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+ 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 ||
+ (snd_pcm_uframes_t)diff > runtime->buffer_size) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+
+ /* ACK includes any subperiod tail already copied to the ring. */
+ bytes = frames_to_bytes(runtime, diff);
+ if (bytes > vss->capture_ready ||
+ check_add_overflow(vss->capture_acked_total, (u64)bytes,
+ &next_acked_total) ||
+ next_acked_total > vss->capture_copied_total) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+
+ vss->capture_acked_total = next_acked_total;
+ vss->capture_ready -= bytes;
+ vss->pcm_indirect.appl_ptr = appl_ptr;
+ if (vss->xfer_enabled) {
+ rc = virtsnd_pcm_capture_send(vss);
+ if (rc) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ }
+ return rc;
+ }
+
+ return 0;
+ }
+
transfer.indirect = vss->pcm_indirect;
if (vss->stopped) {
rc = -EPIPE;
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 07/11] ALSA: virtio: stage VZ capture and restart paused streams
2026-10-27 23:43 ` [PATCH v5 07/11] ALSA: virtio: stage VZ capture and restart paused streams Joonho Lee
@ 2026-10-28 0:19 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:19 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] ALSA: virtio: UAF and NULL deref of runtime in virtsnd_pcm_period_elapsed()
- [High] ALSA: virtio: virtsnd_pcm_pause_restart_send() bricks stream on signal or error
- [Medium] ALSA: virtio: pause_restart_state not reconciled across suspend and resume
- [Medium] ALSA: virtio: virtsnd_pcm_pause_restart_pause() skips flush on RELEASE_SENT
- [Medium] ALSA: virtio: unlocked state reset in virtsnd_pcm_prepare() races with ack
- [Medium] ALSA: virtio: virtsnd_pcm_cp_ack() checks acked bytes against copied_total
- [Low] ALSA: virtio: undocumented module parameters in virtio_pcm_ops.c
- [Low] ALSA: virtio: unused pause_restart parameter in virtsnd_pcm_start_failed()
--
commit d608f6a99cc3b67a96be954c9e03a4e0866d62bf
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: stage VZ capture and restart paused streams
This commit adds opt-in capture staging with response validation and
accounting, along with a pause release and reprepare state machine for
capture streams.
> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index be3893de40a55..a3478fc8848db 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -310,8 +310,94 @@ 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) {
> + staged_capture = vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> + vss->capture_staging;
> + }
> +
> + if (!staged_capture) {
> + snd_pcm_period_elapsed(substream);
> + return;
> + }
> +
> + /* Serialize the pointer snapshot with ALSA's pointer and ACK callbacks. */
> + guard(pcm_stream_lock_irqsave)(substream);
> + if (!snd_pcm_running(substream)) {
> + scoped_guard(spinlock_irqsave, &vss->lock) {
> + runtime = substream->runtime;
> + paused_xrun = runtime &&
> + runtime->state == SNDRV_PCM_STATE_PAUSED &&
> + vss->runtime_active && vss->xfer_xrun;
> + }
[Severity: High]
Can virtsnd_pcm_period_elapsed() dereference a null or freed
substream->runtime when calling snd_pcm_running(substream) here?
In virtsnd_pcm_pause_restart_sync_stop() in sound/virtio/virtio_pcm_ops.c,
cancel_work_sync(&vss->elapsed_period) is called at entry, but every
subsequent error branch calls virtsnd_pcm_pause_restart_fail():
cancel_work_sync(&vss->elapsed_period);
if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
virtsnd_pcm_pause_restart_transitioning(state)) {
virtsnd_pcm_pause_restart_fail(vss, true);
...
return -EIO;
}
In turn, virtsnd_pcm_pause_restart_fail() checks !vss->xfer_xrun and
schedules vss->elapsed_period even when vss->runtime_active and
vss->xfer_enabled are false:
scoped_guard(spinlock_irqsave, &vss->lock) {
notify_xrun = !vss->xfer_xrun;
...
}
if (notify_xrun)
schedule_work(&vss->elapsed_period);
When virtsnd_pcm_open() calls virtsnd_pcm_sync_stop() ->
virtsnd_pcm_pause_restart_sync_stop() and it fails (for example, after a
prior failed control message or a RELEASE flush timeout), vss->elapsed_period
is re-queued and virtsnd_pcm_open() returns an error to
snd_pcm_open_substream().
Because substream->ops->open() failed before substream->hw_opened is set,
snd_pcm_release_substream() skips virtsnd_pcm_close() and frees
substream->runtime via snd_pcm_detach_substream() without holding
pcm_stream_lock. When virtsnd_pcm_period_elapsed() then runs,
snd_pcm_running(substream) dereferences substream->runtime->state before
substream->runtime is checked for null.
Should virtsnd_pcm_pause_restart_fail() avoid scheduling vss->elapsed_period
when the stream is not active, and should virtsnd_pcm_period_elapsed() also
verify substream->runtime before calling snd_pcm_running(substream)?
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index f74bf365065b6..003832d112551 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -3,10 +3,41 @@
> * virtio-snd: Virtio sound device
> * Copyright (C) 2021 OpenSynergy GmbH
> */
> +#include <linux/moduleparam.h>
> +#include <linux/overflow.h>
> #include <sound/pcm_params.h>
>
> #include "virtio_card.h"
>
> +bool virtsnd_capture_staging;
> +module_param_named(capture_staging, virtsnd_capture_staging, bool, 0444);
> +MODULE_PARM_DESC(capture_staging,
> + "Use request-owned buffers for experimental PCM capture");
> +
> +bool virtsnd_capture_packed_status;
> +module_param_named(capture_packed_status, virtsnd_capture_packed_status,
> + bool, 0444);
> +MODULE_PARM_DESC(capture_packed_status,
> + "Read staged capture status after the actual audio payload");
> +
> +bool virtsnd_capture_allow_short;
> +module_param_named(capture_allow_short, virtsnd_capture_allow_short,
> + bool, 0444);
> +MODULE_PARM_DESC(capture_allow_short,
> + "Accept non-empty short staged capture responses");
> +
> +bool virtsnd_capture_allow_empty;
> +module_param_named(capture_allow_empty, virtsnd_capture_allow_empty,
> + bool, 0444);
> +MODULE_PARM_DESC(capture_allow_empty,
> + "Retry bounded empty startup capture responses");
> +
> +static bool virtsnd_capture_pause_restart;
> +module_param_named(capture_pause_restart, virtsnd_capture_pause_restart,
> + bool, 0444);
> +MODULE_PARM_DESC(capture_pause_restart,
> + "Release and reprepare staged capture streams across pause");
[Severity: Low]
Should these five new module parameters (capture_staging,
capture_packed_status, capture_allow_short, capture_allow_empty, and
capture_pause_restart) be documented in
Documentation/admin-guide/kernel-parameters.txt?
> @@ -98,15 +129,195 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
>
> static int virtsnd_pcm_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,
> + bool pause_restart)
> {
> scoped_guard(spinlock_irqsave, &vss->lock) {
> vss->xfer_enabled = false;
> vss->stopped = true;
> + if (pause_restart)
> + vss->xfer_xrun = true;
> }
> vss->substream->runtime->stop_operating = true;
> }
[Severity: Low]
Is the pause_restart parameter in virtsnd_pcm_start_failed() still needed?
Both call sites in virtsnd_pcm_trigger() pass false because the pause_restart
path returns early via virtsnd_pcm_pause_restart_start().
[ ... ]
> +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;
> + int rc;
> +
> + if (!virtsnd_pcm_pause_restart_enabled(vss))
> + return virtsnd_ctl_msg_send_sync(vss->snd, msg);
> +
> + scoped_guard(spinlock_irqsave, &vss->lock) {
> + state = vss->pause_restart_state;
> + 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;
> + }
> + }
> +
> + if (rc) {
> + virtsnd_ctl_msg_unref(msg);
> + return rc;
> + }
> +
> + rc = virtsnd_ctl_msg_send_sync(vss->snd, msg);
> + scoped_guard(spinlock_irqsave, &vss->lock) {
> + if (rc || vss->pause_restart_state != submitted_state) {
> + vss->pause_restart_state =
> + VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
> + if (!rc)
> + rc = -EIO;
> + } else {
> + vss->pause_restart_state = success_state;
> + }
> + }
[Severity: High]
What happens if virtsnd_ctl_msg_send_sync() returns -ERESTARTSYS due to a
pending signal (or fails with a control error) during SET_PARAMS, PREPARE, or
START?
In virtsnd_ctl_msg_send(), only VIRTIO_SND_R_PCM_STOP and
VIRTIO_SND_R_PCM_RELEASE use uninterruptible waits; SET_PARAMS, PREPARE, and
START use wait_for_completion_interruptible_timeout(), which returns
-ERESTARTSYS immediately when a signal is pending (including during close or
snd_pcm_drop() of a paused stream, where ALSA core first unpauses via
SNDRV_PCM_TRIGGER_PAUSE_RELEASE -> virtsnd_pcm_pause_restart_start()).
Furthermore, in virtsnd_pcm_pause_restart_start(), virtsnd_pcm_capture_send()
queues RX capture buffers into the virtqueue (incrementing vss->msg_count)
before calling virtsnd_pcm_pause_restart_send() for VIRTIO_SND_R_PCM_START:
scoped_guard(spinlock_irqsave, &queue->lock) {
guard(spinlock)(&vss->lock);
...
rc = virtsnd_pcm_capture_send(vss);
if (!rc)
vss->xfer_enabled = true;
}
...
rc = virtsnd_pcm_pause_restart_send(vss, msg,
BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED),
VIRTSND_PCM_PAUSE_RESTART_START_SUBMITTED,
VIRTSND_PCM_PAUSE_RESTART_RUNNING);
When virtsnd_pcm_pause_restart_send() sets vss->pause_restart_state to
VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED on any non-zero rc,
virtsnd_pcm_pause_restart_sync_stop() immediately aborts with -EIO without
sending VIRTIO_SND_R_PCM_RELEASE or waiting for vss->msg_empty.
Doesn't this permanently wedge the substream until device reprobe, while also
leaving the queued RX buffers in the virtqueue so virtsnd_pcm_hw_free()
cannot free vss->msgs and msg->data on close?
[ ... ]
> @@ -296,9 +564,22 @@ static int virtsnd_pcm_prepare(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);
> + int rc;
> bool stopped;
>
> + 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);
> + return -EIO;
> + }
> + }
> +
> scoped_guard(spinlock_irqsave, &vss->lock)
> stopped = vss->stopped;
> if (stopped) {
[Severity: Medium]
How are vss->pause_restart_state and vss->stopped reconciled across system
suspend and resume?
During virtsnd_freeze(), virtsnd_ctl_msg_cancel_all() cancels any in-flight
control messages (which sets vss->pause_restart_state to
VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED in virtsnd_pcm_pause_restart_send())
and virtsnd_reset_vqs() resets the VirtIO device, returning all streams on
the device to their initial unconfigured state. However, neither
virtsnd_freeze() nor virtsnd_restore() resets vss->pause_restart_state to
VIRTSND_PCM_PAUSE_RESTART_NONE.
In addition, if snd_pcm_suspend_all() suspends a stream that is currently in
SNDRV_PCM_STATE_PAUSED with capture_pause_restart enabled, snd_pcm_do_suspend()
skips ops->trigger(SNDRV_PCM_TRIGGER_SUSPEND) and does not set
runtime->stop_operating because snd_pcm_running(substream) is false. That
leaves vss->stopped set to true and vss->suspended set to false in
SNDRV_PCM_STATE_SUSPENDED, so a subsequent snd_pcm_prepare() skips
virtsnd_pcm_sync_stop() and fails at the if (stopped) check in
virtsnd_pcm_prepare() with -EBADFD.
Should virtsnd_freeze() / virtsnd_restore() and virtsnd_pcm_prepare() reconcile
vss->pause_restart_state and vss->stopped after suspend?
> @@ -321,16 +602,23 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
> 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)
> + virtsnd_pcm_pause_restart_fail(vss, false);
> return rc;
> + }
> }
>
> - vss->xfer_xrun = false;
> + /* ALSA prepare resets its pointers even after a suspended stream. */
> + if (vss->capture_staging)
> + vss->hw_ptr = 0;
> +
> + if (!pause_restart)
> + vss->xfer_xrun = false;
> vss->suspended = false;
> vss->msg_count = 0;
> virtsnd_pcm_msg_reset(vss);
> @@ -340,12 +628,210 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
> vss->pcm_indirect.hw_buffer_size =
> snd_pcm_lib_buffer_bytes(substream);
[Severity: Medium]
Can these unlocked updates to vss->buffer_bytes, vss->hw_ptr, vss->xfer_xrun,
vss->msg_count, and vss->pcm_indirect in virtsnd_pcm_prepare() race with a
concurrent virtsnd_pcm_cp_ack() call?
Because vpcm->pcm->nonatomic is true, snd_pcm_prepare() calls
virtsnd_pcm_prepare() under runtime->buffer_mutex without holding
pcm_stream_lock. Meanwhile, SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_sync_ptr()
acquires pcm_stream_lock and invokes pcm_lib_apply_appl_ptr() ->
virtsnd_pcm_cp_ack(), which enters the staged capture path whenever
vss->runtime_active is true (set in virtsnd_pcm_hw_params()).
If virtsnd_pcm_cp_ack() runs after virtsnd_pcm_prepare() sets vss->hw_ptr = 0
but before virtsnd_pcm_msg_reset() zeroes vss->capture_copied_total under
vss->lock, virtsnd_pcm_capture_counters_valid(vss) in virtsnd_pcm_cp_ack() can
see vss->hw_ptr != vss->capture_copied_total % vss->buffer_bytes, set
vss->xfer_xrun = true, and return -EPIPE, while memset(&vss->pcm_indirect, 0,
sizeof(vss->pcm_indirect)) also races with virtsnd_pcm_cp_ack() reading and
updating vss->pcm_indirect.appl_ptr.
Should all of these state resets in virtsnd_pcm_prepare() be performed
together under vss->lock?
[ ... ]
> +static int virtsnd_pcm_pause_restart_pause(struct virtio_pcm_substream *vss)
> +{
> + struct virtio_snd_msg *msg;
> + enum virtio_pcm_pause_restart_state state;
> + int rc;
> +
> + scoped_guard(spinlock_irqsave, &vss->lock) {
> + state = vss->pause_restart_state;
> + if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT ||
> + state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)
> + return 0;
> + vss->xfer_enabled = false;
> + vss->stopped = true;
> + }
[Severity: Medium]
Should virtsnd_pcm_pause_restart_pause() return 0 immediately when state is
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT?
In virtsnd_pcm_pause_release_flush(), sending VIRTIO_SND_R_PCM_RELEASE sets
vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT before
waiting on vss->msg_empty:
rc = virtsnd_pcm_pause_restart_send(vss, msg,
allowed_states,
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SUBMITTED,
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT);
if (rc)
return rc;
waited = wait_event_timeout(vss->msg_empty,
!virtsnd_pcm_msg_pending_num(vss), js);
If that wait times out, vss->pause_restart_state remains
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT while RX messages may still be pending,
and ALSA keeps the substream in SNDRV_PCM_STATE_RUNNING.
Unlike virtsnd_pcm_pause_restart_sync_stop(), which waits on vss->msg_empty
when in VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT and transitions to
VIRTSND_PCM_PAUSE_RESTART_RELEASED, virtsnd_pcm_pause_restart_pause() returns
0 immediately without waiting for the queue to drain or updating the state to
VIRTSND_PCM_PAUSE_RESTART_RELEASED. A subsequent SNDRV_PCM_TRIGGER_PAUSE_RELEASE
in virtsnd_pcm_pause_restart_start(vss, true) will then fail with -EBADFD
because it requires state == VIRTSND_PCM_PAUSE_RESTART_RELEASED.
[ ... ]
> @@ -586,11 +1233,66 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
> {
> struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> + struct snd_pcm_runtime *runtime = substream->runtime;
> struct virtsnd_pcm_transfer transfer = {};
> int rc;
>
> scoped_guard(spinlock_irqsave, &queue->lock) {
> guard(spinlock)(&vss->lock);
> + if (vss->capture_staging) {
> + snd_pcm_uframes_t appl_ptr;
> + snd_pcm_sframes_t diff;
> + size_t bytes;
> + u64 next_acked_total;
> +
> + if (!vss->runtime_active || !runtime || !runtime->control ||
> + !runtime->dma_area)
> + return -EBADFD;
> + if (vss->xfer_xrun)
> + return -EPIPE;
> + if (!virtsnd_pcm_capture_counters_valid(vss)) {
> + vss->xfer_xrun = true;
> + schedule_work(&vss->elapsed_period);
> + return -EPIPE;
> + }
> + 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 ||
> + (snd_pcm_uframes_t)diff > runtime->buffer_size) {
> + vss->xfer_xrun = true;
> + schedule_work(&vss->elapsed_period);
> + return -EPIPE;
> + }
> +
> + /* ACK includes any subperiod tail already copied to the ring. */
> + bytes = frames_to_bytes(runtime, diff);
> + if (bytes > vss->capture_ready ||
> + check_add_overflow(vss->capture_acked_total, (u64)bytes,
> + &next_acked_total) ||
> + next_acked_total > vss->capture_copied_total) {
> + vss->xfer_xrun = true;
> + schedule_work(&vss->elapsed_period);
> + return -EPIPE;
> + }
[Severity: Medium]
Should virtsnd_pcm_cp_ack() validate next_acked_total against
vss->capture_pointer_seen_total rather than vss->capture_copied_total?
When virtsnd_pcm_capture_complete() copies completed RX payloads into
runtime->dma_area, it advances vss->capture_copied_total and
vss->capture_ready immediately, whereas runtime->status->hw_ptr and
vss->capture_pointer_seen_total only advance when virtsnd_pcm_cp_pointer()
runs.
If userspace invokes SNDRV_PCM_IOCTL_SYNC_PTR without
SNDRV_PCM_SYNC_PTR_HWSYNC (or while in SNDRV_PCM_STATE_PAUSED, where
do_pcm_hwsync() is a no-op) with appl_ptr advancing into the range between
vss->capture_pointer_seen_total and vss->capture_copied_total,
snd_pcm_sync_ptr() calls pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack()
without calling virtsnd_pcm_cp_pointer().
Because virtsnd_pcm_cp_ack() checks next_acked_total >
vss->capture_copied_total, it accepts the update and allows
runtime->control->appl_ptr to move ahead of runtime->status->hw_ptr, causing
snd_pcm_capture_avail() to underflow and wrap around runtime->boundary.
(Note: a separate issue here where a virtsnd_pcm_capture_send() failure
returns an error after committing vss->capture_acked_total is addressed in a
later commit in this series, "ALSA: virtio: account staged capture reads
before RX refill".)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=7
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 08/11] ALSA: virtio: account staged capture reads before RX refill
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (6 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 07/11] ALSA: virtio: stage VZ capture and restart paused streams Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-27 23:53 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work Joonho Lee
` (2 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
The RX callback holds the queue lock across a batch of completions. A
reader that has copied samples can wait for that lock in ACK before its
consumed bytes are removed from capture_ready. A later completion in the
same batch can then reject data using stale ring occupancy.
Account the staged read under the substream lock before acquiring the RX
queue lock. Release that lock before taking the queue and substream locks
in their established order to refill requests.
Once accounting is committed, latch refill errors as XRUN and return
success from ACK. A negative return would make ALSA restore appl_ptr while
the driver retained the consumed-byte accounting. Keep pre-commit errors,
ring-capacity checks, and normal period-work scheduling unchanged.
Source-extracted host and kernel fixtures reproduce the stale-capacity
race and post-commit ALSA pointer rollback. The candidate preserves wrapped
data and pointer accounting. KASAN/lockdep QEMU lifecycle checks passed.
The fixtures substitute selected backend helpers and do not independently
establish physical-device acceptance.
This follows pending staged capture support, whose introducing patch has
no upstream commit ID for a Fixes tag yet.
Link: https://lore.kernel.org/all/rfc-v2-vz-capture-compat-1.b9ab4c1540dc4691@gmail.com/
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
sound/virtio/virtio_pcm_ops.c | 128 ++++++++++++++++++++--------------
1 file changed, 77 insertions(+), 51 deletions(-)
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index 003832d112551..85e7576903d56 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -1229,70 +1229,96 @@ static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
return 0;
}
-static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
+static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
+ struct snd_pcm_runtime *runtime)
{
- struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct virtsnd_pcm_transfer transfer = {};
- int rc;
+ 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 next_acked_total;
+
+ if (!vss->runtime_active || !runtime || !runtime->control ||
+ !runtime->dma_area)
+ return -EBADFD;
+ if (vss->xfer_xrun)
+ return -EPIPE;
+ if (!virtsnd_pcm_capture_counters_valid(vss)) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+ 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 ||
+ (snd_pcm_uframes_t)diff > runtime->buffer_size) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+
+ /* ACK includes any subperiod tail already copied to the ring. */
+ bytes = frames_to_bytes(runtime, diff);
+ if (bytes > vss->capture_ready ||
+ check_add_overflow(vss->capture_acked_total, (u64)bytes,
+ &next_acked_total) ||
+ next_acked_total > vss->capture_copied_total) {
+ vss->xfer_xrun = true;
+ schedule_work(&vss->elapsed_period);
+ return -EPIPE;
+ }
+
+ vss->capture_acked_total = next_acked_total;
+ vss->capture_ready -= bytes;
+ vss->pcm_indirect.appl_ptr = appl_ptr;
+ }
+
+ /* Preserve the queue -> substream lock order while refilling requests. */
scoped_guard(spinlock_irqsave, &queue->lock) {
guard(spinlock)(&vss->lock);
- if (vss->capture_staging) {
- snd_pcm_uframes_t appl_ptr;
- snd_pcm_sframes_t diff;
- size_t bytes;
- u64 next_acked_total;
-
- if (!vss->runtime_active || !runtime || !runtime->control ||
- !runtime->dma_area)
- return -EBADFD;
- if (vss->xfer_xrun)
- return -EPIPE;
- if (!virtsnd_pcm_capture_counters_valid(vss)) {
+ if (vss->runtime_active && vss->xfer_enabled &&
+ !vss->xfer_xrun) {
+ rc = virtsnd_pcm_capture_send(vss);
+ if (rc) {
vss->xfer_xrun = true;
schedule_work(&vss->elapsed_period);
- return -EPIPE;
}
- 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 ||
- (snd_pcm_uframes_t)diff > runtime->buffer_size) {
- vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
- return -EPIPE;
- }
+ if (rc)
+ dev_warn_ratelimited(&vss->snd->vdev->dev,
+ "SID %u: failed to refill capture queue after ACK: %d\n",
+ vss->sid, rc);
- /* ACK includes any subperiod tail already copied to the ring. */
- bytes = frames_to_bytes(runtime, diff);
- if (bytes > vss->capture_ready ||
- check_add_overflow(vss->capture_acked_total, (u64)bytes,
- &next_acked_total) ||
- next_acked_total > vss->capture_copied_total) {
- vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
- return -EPIPE;
- }
+ /* The ACK is committed; a refill failure is reported by the next XRUN. */
+ return 0;
+}
- vss->capture_acked_total = next_acked_total;
- vss->capture_ready -= bytes;
- vss->pcm_indirect.appl_ptr = appl_ptr;
- if (vss->xfer_enabled) {
- rc = virtsnd_pcm_capture_send(vss);
- if (rc) {
- vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
- }
- return rc;
- }
+static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
+{
+ struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
+ struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
+ struct snd_pcm_runtime *runtime = substream->runtime;
+ struct virtsnd_pcm_transfer transfer = {};
+ int rc;
- return 0;
- }
+ if (vss->capture_staging)
+ return virtsnd_pcm_capture_staged_ack(vss, runtime);
+ scoped_guard(spinlock_irqsave, &queue->lock) {
+ guard(spinlock)(&vss->lock);
transfer.indirect = vss->pcm_indirect;
if (vss->stopped) {
rc = -EPIPE;
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 08/11] ALSA: virtio: account staged capture reads before RX refill
2026-10-27 23:43 ` [PATCH v5 08/11] ALSA: virtio: account staged capture reads before RX refill Joonho Lee
@ 2026-10-27 23:53 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-27 23:53 UTC (permalink / raw)
To: Joonho Lee; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
> The RX callback holds the queue lock across a batch of completions. A
> reader that has copied samples can wait for that lock in ACK before its
> consumed bytes are removed from capture_ready. A later completion in the
> same batch can then reject data using stale ring occupancy.
>
> Account the staged read under the substream lock before acquiring the RX
> queue lock. Release that lock before taking the queue and substream locks
> in their established order to refill requests.
>
> Once accounting is committed, latch refill errors as XRUN and return
> success from ACK. A negative return would make ALSA restore appl_ptr while
> the driver retained the consumed-byte accounting. Keep pre-commit errors,
> ring-capacity checks, and normal period-work scheduling unchanged.
>
> Source-extracted host and kernel fixtures reproduce the stale-capacity
> race and post-commit ALSA pointer rollback. The candidate preserves wrapped
> data and pointer accounting. KASAN/lockdep QEMU lifecycle checks passed.
> The fixtures substitute selected backend helpers and do not independently
> establish physical-device acceptance.
>
> This follows pending staged capture support, whose introducing patch has
> no upstream commit ID for a Fixes tag yet.
>
> Link: https://lore.kernel.org/all/rfc-v2-vz-capture-compat-1.b9ab4c1540dc4691@gmail.com/
> Assisted-by: LLM
> Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=8
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (7 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 08/11] ALSA: virtio: account staged capture reads before RX refill Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:03 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 10/11] ALSA: pcm: mark stop-threshold accesses for capture policy sampling Joonho Lee
2026-10-27 23:43 ` [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers Joonho Lee
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
CPU contention can delay staged capture period notifications while RX
completions continue filling the unread ring. In a two-vCPU VZ guest with
16 nice-0 CPU workers, the ACK-fixed baseline stopped with ring_capacity
and EPIPE in three runs, after 11.74, 30.77 and 30.33 seconds of audio.
Route staged capture period work through system_highpri_wq with a common
helper at all ten producers, including ACK errors and self-requeue. Keep
playback and non-staged streams on the normal workqueue. Initialization,
cancellation, buffer geometry and capture accounting remain unchanged.
With the same kernel, input, geometry and userspace priority, the candidate
completed three 70-second captures and immediate reopens. Its maximum
matched queue-to-start delay was 20.366 ms, versus 108.112 ms in the first
baseline failure. The other baseline failures had pending work without
matching execution, so matched-only statistics do not bound that delay.
Reader scheduling delays were also present. Both arms passed a separate
nice -10 reader control, and host load varied between runs. These results
support the change for the tested workload without isolating a kernel-only
cause or guaranteeing lossless capture under arbitrary CPU load.
KASAN/lockdep QEMU regressions passed with the unchanged candidate source.
This follows the staged ACK fix and pending VZ capture support. The
introducing staged-capture patch has no upstream Fixes commit ID yet.
Link: https://lore.kernel.org/all/rfc-v2-vz-capture-compat-1.b9ab4c1540dc4691@gmail.com/
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
sound/virtio/virtio_pcm.c | 4 ++--
sound/virtio/virtio_pcm.h | 12 ++++++++++++
sound/virtio/virtio_pcm_msg.c | 6 +++---
sound/virtio/virtio_pcm_ops.c | 10 +++++-----
4 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index a3478fc8848db..d8515b2144a14 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -397,7 +397,7 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
}
if (requeue)
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
}
/**
@@ -606,7 +606,7 @@ void virtsnd_pcm_event(struct virtio_snd *snd, struct virtio_snd_event *event)
if (vss->xfer_enabled) {
vss->xfer_xrun = true;
if (vss->capture_staging)
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
}
}
break;
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index e143a15592f05..5d1d88f2823b2 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -8,6 +8,7 @@
#include <linux/atomic.h>
#include <linux/virtio_config.h>
+#include <linux/workqueue.h>
#include <sound/pcm.h>
#include <sound/pcm-indirect.h>
@@ -114,6 +115,17 @@ struct virtio_pcm_substream {
wait_queue_head_t msg_empty;
};
+/* Keep non-staged PCM notifications on schedule_work()'s normal queue. */
+static inline bool
+virtsnd_pcm_schedule_period_elapsed(struct virtio_pcm_substream *vss)
+{
+ if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ vss->capture_staging)
+ return queue_work(system_highpri_wq, &vss->elapsed_period);
+
+ return schedule_work(&vss->elapsed_period);
+}
+
/*
* ALSA may observe a subperiod tail before its period callback. Keep actual
* unread occupancy separate from pointer observations and notification state.
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 795e52e09564b..663f7695b5be8 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -627,7 +627,7 @@ static void virtsnd_pcm_capture_xrun_locked(struct virtio_pcm_substream *vss)
{
if (!vss->xfer_xrun) {
vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
}
}
@@ -720,7 +720,7 @@ static void virtsnd_pcm_capture_complete(struct virtio_pcm_msg *msg,
virtsnd_pcm_capture_xrun_locked(vss);
}
if (got_audio && !vss->xfer_xrun)
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
} else if (!vss->msg_count) {
wake_up_all(&vss->msg_empty);
}
@@ -858,7 +858,7 @@ 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);
+ virtsnd_pcm_schedule_period_elapsed(vss);
} else 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 85e7576903d56..d50924b792f65 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -232,7 +232,7 @@ virtsnd_pcm_pause_restart_fail(struct virtio_pcm_substream *vss,
VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
}
if (notify_xrun)
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
vss->substream->runtime->stop_operating = true;
}
@@ -1253,7 +1253,7 @@ static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
return -EPIPE;
if (!virtsnd_pcm_capture_counters_valid(vss)) {
vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
return -EPIPE;
}
@@ -1264,7 +1264,7 @@ static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
if (diff < 0 ||
(snd_pcm_uframes_t)diff > runtime->buffer_size) {
vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
return -EPIPE;
}
@@ -1275,7 +1275,7 @@ static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
&next_acked_total) ||
next_acked_total > vss->capture_copied_total) {
vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
return -EPIPE;
}
@@ -1292,7 +1292,7 @@ static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
rc = virtsnd_pcm_capture_send(vss);
if (rc) {
vss->xfer_xrun = true;
- schedule_work(&vss->elapsed_period);
+ virtsnd_pcm_schedule_period_elapsed(vss);
}
}
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work
2026-10-27 23:43 ` [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work Joonho Lee
@ 2026-10-28 0:03 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:03 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: UAF/NULL deref in virtsnd_pcm_period_elapsed() after open fail
--
commit fc06cf9505ce3c738b1bc2bf02c081ba34868b30
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: prioritize staged capture period work
This commit introduces virtsnd_pcm_schedule_period_elapsed() to queue
elapsed_period work onto system_highpri_wq for staged capture streams while
keeping playback and non-staged streams on the normal workqueue. All ten
producers of elapsed_period work are updated to use this helper.
Link: https://lore.kernel.org/all/rfc-v2-vz-capture-compat-1.b9ab4c1540dc4691@gmail.com/
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 85e7576903d56..d50924b792f65 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -232,7 +232,7 @@ virtsnd_pcm_pause_restart_fail(struct virtio_pcm_substream *vss,
> VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
> }
> if (notify_xrun)
> - schedule_work(&vss->elapsed_period);
> + virtsnd_pcm_schedule_period_elapsed(vss);
> vss->substream->runtime->stop_operating = true;
> }
[Severity: High]
Can queuing vss->elapsed_period onto system_highpri_wq from
virtsnd_pcm_pause_restart_fail() cause a NULL pointer dereference or
use-after-free of substream->runtime if virtsnd_pcm_open() fails?
During virtsnd_pcm_open(), the driver calls virtsnd_pcm_sync_stop(), which
dispatches to virtsnd_pcm_pause_restart_sync_stop() when pause-restart is
enabled:
sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pause_restart_sync_stop() {
scoped_guard(spinlock_irqsave, &vss->lock) {
vss->xfer_enabled = false;
vss->stopped = true;
state = vss->pause_restart_state;
}
cancel_work_sync(&vss->elapsed_period);
if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
virtsnd_pcm_pause_restart_transitioning(state)) {
virtsnd_pcm_pause_restart_fail(vss, true);
...
return -EIO;
}
...
}
Although virtsnd_pcm_pause_restart_sync_stop() cancels vss->elapsed_period
at the beginning, all of its error paths call
virtsnd_pcm_pause_restart_fail(). Whenever vss->xfer_xrun was false (for
example, after a prior session failed in virtsnd_pcm_dev_set_params() ->
virtsnd_pcm_pause_restart_send() or left the stream in
VIRTSND_PCM_PAUSE_RESTART_PREPARED without starting), notify_xrun is true
and virtsnd_pcm_pause_restart_fail() re-queues vss->elapsed_period onto
system_highpri_wq after cancel_work_sync() has already completed.
When virtsnd_pcm_open() returns this error to snd_pcm_open_substream(),
substream->hw_opened is still 0, so snd_pcm_release_substream() skips
virtsnd_pcm_close() and immediately calls snd_pcm_detach_substream(),
freeing substream->runtime and setting substream->runtime = NULL without
holding pcm_stream_lock.
When virtsnd_pcm_period_elapsed() then runs on system_highpri_wq for a
staged capture stream, it calls snd_pcm_running(substream) before checking
whether substream->runtime is NULL:
sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() {
...
guard(pcm_stream_lock_irqsave)(substream);
if (!snd_pcm_running(substream)) {
scoped_guard(spinlock_irqsave, &vss->lock) {
runtime = substream->runtime;
paused_xrun = runtime &&
...
}
Since snd_pcm_running(substream) unconditionally reads
substream->runtime->state, can this dereference a freed or NULL
substream->runtime, or leave vss->elapsed_period pending after
virtsnd_pcm_sync_stop() returns?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=9
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 10/11] ALSA: pcm: mark stop-threshold accesses for capture policy sampling
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (8 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-27 23:48 ` sashiko-bot
2026-10-27 23:43 ` [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers Joonho Lee
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
The staged virtio-snd capture path needs to sample stop_threshold from
completion context to distinguish free-running capture from bounded
capture. Software parameters can update this field while capture runs.
Mark the runtime stop-threshold assignments with WRITE_ONCE and the proc
status read with READ_ONCE. This pairs the assignments with the driver's
READ_ONCE policy sampling in the following patch.
These annotations do not provide a transaction for the entire sw_params
update. The driver serializes its cached policy and accounting with its
own substream lock.
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -2958,7 +2958,7 @@
runtime->oss.mmap_bytes);
#endif
/* In mmap mode we never stop */
- runtime->stop_threshold = runtime->boundary;
+ WRITE_ONCE(runtime->stop_threshold, runtime->boundary);
return 0;
}
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -422,7 +422,8 @@
snd_iprintf(buffer, "period_step: %u\n", runtime->period_step);
snd_iprintf(buffer, "avail_min: %lu\n", runtime->control->avail_min);
snd_iprintf(buffer, "start_threshold: %lu\n", runtime->start_threshold);
- snd_iprintf(buffer, "stop_threshold: %lu\n", runtime->stop_threshold);
+ snd_iprintf(buffer, "stop_threshold: %lu\n",
+ READ_ONCE(runtime->stop_threshold));
snd_iprintf(buffer, "silence_threshold: %lu\n", runtime->silence_threshold);
snd_iprintf(buffer, "silence_size: %lu\n", runtime->silence_size);
snd_iprintf(buffer, "boundary: %lu\n", runtime->boundary);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -871,7 +871,7 @@
runtime->period_step = 1;
runtime->control->avail_min = runtime->period_size;
runtime->start_threshold = 1;
- runtime->stop_threshold = runtime->buffer_size;
+ WRITE_ONCE(runtime->stop_threshold, runtime->buffer_size);
runtime->silence_threshold = 0;
runtime->silence_size = 0;
runtime->boundary = runtime->buffer_size;
@@ -1062,7 +1062,8 @@
runtime->period_step = params->period_step;
runtime->control->avail_min = params->avail_min;
runtime->start_threshold = params->start_threshold;
- runtime->stop_threshold = params->stop_threshold;
+ /* Drivers may sample the stop policy from completion context. */
+ WRITE_ONCE(runtime->stop_threshold, params->stop_threshold);
runtime->silence_threshold = params->silence_threshold;
runtime->silence_size = params->silence_size;
params->boundary = runtime->boundary;
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers
2026-10-27 23:43 [PATCH v5 00/11] ALSA: virtio: staged capture and dsnoop recovery Joonho Lee
` (9 preceding siblings ...)
2026-10-27 23:43 ` [PATCH v5 10/11] ALSA: pcm: mark stop-threshold accesses for capture policy sampling Joonho Lee
@ 2026-10-27 23:43 ` Joonho Lee
2026-10-28 0:13 ` sashiko-bot
10 siblings, 1 reply; 23+ messages in thread
From: Joonho Lee @ 2026-10-27 23:43 UTC (permalink / raw)
To: linux-sound
Cc: tiwai, perex, anton.yakovlev, mst, virtualization, linux-kernel,
Joonho Lee
dsnoop runs its hardware slave with stop_threshold == boundary and can
consume the mmap capture ring without advancing the hardware application
pointer. Staged capture currently treats every stream as a bounded
consumer: capture_ready grows until the completion capacity check fails.
With a 48 kHz mono slave, 960-frame periods and a 7680-frame buffer, the
baseline dsnoop reproduction stops after 6720 frames with -EPIPE and a
ring_capacity rejection. Direct capture continues to work.
Cache the prepared boundary and recognize the free-running policy under
the substream lock. Keep RX requests supplied in that mode, while retaining
payload, frame alignment, counter and pointer-window checks. Sample the
policy once per completion so validation, copying and refill agree.
Refresh it separately in the ACK accounting and refill critical sections.
On a return to bounded capture, reconstruct unread occupancy from the
copied and acknowledged totals. If outstanding data fills the ring, the
policy helper returns -EPIPE and preserves the cached mode and counters.
The detecting RX, pointer or ACK path reports XRUN through its existing
error path; this does not make sw_params fail synchronously.
A native pause release can also detect this overflow during queue refill.
Its trigger error leaves ALSA in PAUSED, so mark the staged capture as XRUN
and schedule the period worker before returning the error. The worker takes
the PCM stream lock after the pause action and wakes readers from XRUN.
A paused ACK can reach XRUN before that worker runs. Arm synchronous stop
before returning -EPIPE from staged ACK, and before the worker stops a
paused stream. ALSA skips trigger(STOP) for paused streams; these paths
must preserve the RELEASE and pending-RX cleanup needed by prepare.
Reset cached policy state during prepare and teardown.
Before the pause revisions, MOTU/VZ tests with one and two vCPUs failed all
32 shared-client captures. The candidate passed 48 capture cases and eight
recovery diagnostics, including 70-second CPU-saturated shared captures.
Synthetic QEMU dsnoop data matched 2160000 reference frames across 12
files. Forced policy transitions passed KASAN/KCSAN checks. Aggressive
KCSAN still triggered dsnoop client-threshold XRUNs with a 160 ms buffer.
The physical tests checked frame counts and nonzero mono input; exact
sample continuity was checked with the synthetic source.
The native-pause regression reproduces -EPIPE followed by a persistent
PAUSED state and a poll timeout with v2. With this revision, the same case
reaches XRUN, wakes poll with POLLERR and returns -EPIPE from read.
Prepare recovery and reopening also pass under ARM64 QEMU with KASAN
and lockdep.
Paused read and forward ACK errors reproduced a failing first prepare
with v3. This revision passes 16 recovery cases across native pause and
pause-restart with one and two vCPUs. Each prepare recovery and reopen
captures 48,000 frames. The existing 13-case mode suite and native-pause
regression also pass with KASAN and lockdep. No new physical-device or
KCSAN results are claimed for this revision.
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -338,6 +338,8 @@
paused_xrun = runtime &&
runtime->state == SNDRV_PCM_STATE_PAUSED &&
vss->runtime_active && vss->xfer_xrun;
+ if (paused_xrun)
+ virtsnd_pcm_capture_arm_paused_stop(vss);
}
if (paused_xrun) {
/* Wake readers without sending STOP for a paused stream. */
diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -47,7 +47,9 @@
* substream operators, including @stopped.
* @buffer_bytes: Current buffer size in bytes.
* @hw_ptr: Substream hardware pointer value in bytes [0 ... buffer_bytes).
- * @capture_ready: Captured bytes copied but not yet acknowledged by ALSA.
+ * @capture_ready: Managed-mode bytes copied but not yet acknowledged by ALSA;
+ * always zero in free-running mode.
+ * @capture_boundary: ALSA boundary captured for this prepared epoch.
* @capture_copied_total: Monotonic bytes copied into the capture ring.
* @capture_acked_total: Monotonic bytes acknowledged by the capture client.
* @capture_pointer_seen_total: Last capture position sampled by ALSA.
@@ -58,6 +60,7 @@
* @capture_staging: Capture uses request-owned payload buffers.
* @capture_started: Non-empty capture data was received since prepare.
* @capture_empty: Consecutive empty startup completions.
+ * @capture_free_running: Cached staged-capture policy, protected by @lock.
* @xfer_enabled: Data transfer state (0 - off, 1 - on).
* @xfer_xrun: Data underflow/overflow state (0 - no xrun, 1 - xrun).
* @stopped: True if the substream is stopped and must be released on the device
@@ -88,6 +91,7 @@
size_t buffer_bytes;
size_t hw_ptr;
size_t capture_ready;
+ snd_pcm_uframes_t capture_boundary;
u64 capture_copied_total;
u64 capture_acked_total;
u64 capture_pointer_seen_total;
@@ -97,6 +101,7 @@
bool runtime_active;
bool capture_staging;
bool capture_started;
+ bool capture_free_running;
unsigned int capture_empty;
bool xfer_enabled;
bool xfer_xrun;
@@ -115,6 +120,22 @@
wait_queue_head_t msg_empty;
};
+/* Caller holds the PCM stream lock and vss->lock. */
+static inline void
+virtsnd_pcm_capture_arm_paused_stop(struct virtio_pcm_substream *vss)
+{
+ struct snd_pcm_runtime *runtime = vss->substream->runtime;
+
+ lockdep_assert_held(&vss->lock);
+ if (!runtime || runtime->state != SNDRV_PCM_STATE_PAUSED)
+ return;
+
+ /* ALSA skips trigger(STOP) when moving a paused stream to XRUN. */
+ vss->xfer_enabled = false;
+ vss->stopped = true;
+ runtime->stop_operating = true;
+}
+
/* Keep non-staged PCM notifications on schedule_work()'s normal queue. */
static inline bool
virtsnd_pcm_schedule_period_elapsed(struct virtio_pcm_substream *vss)
@@ -128,23 +149,29 @@
/*
* ALSA may observe a subperiod tail before its period callback. Keep actual
- * unread occupancy separate from pointer observations and notification state.
+ * unread occupancy separate from pointer observations and notification state;
+ * free-running streams account occupancy from monotonic totals instead.
*/
static inline bool
virtsnd_pcm_capture_counters_valid(const struct virtio_pcm_substream *vss)
{
- return vss->buffer_bytes &&
- vss->capture_copied_total >= vss->capture_acked_total &&
- vss->capture_copied_total - vss->capture_acked_total ==
- (u64)vss->capture_ready &&
- vss->capture_ready < vss->buffer_bytes &&
- vss->capture_pointer_seen_total <= vss->capture_copied_total &&
- vss->hw_ptr == vss->capture_copied_total % vss->buffer_bytes &&
- (!vss->capture_callback_snapshot_active ||
- (vss->capture_callback_snapshot >=
- vss->capture_pointer_seen_total &&
- vss->capture_callback_snapshot <=
- vss->capture_copied_total));
+ u64 outstanding;
+
+ if (!vss->buffer_bytes ||
+ vss->capture_copied_total < vss->capture_acked_total ||
+ vss->capture_pointer_seen_total > vss->capture_copied_total ||
+ vss->hw_ptr != vss->capture_copied_total % vss->buffer_bytes ||
+ (vss->capture_callback_snapshot_active &&
+ (vss->capture_callback_snapshot < vss->capture_pointer_seen_total ||
+ vss->capture_callback_snapshot > vss->capture_copied_total)))
+ return false;
+
+ if (vss->capture_free_running)
+ return !vss->capture_ready;
+
+ outstanding = vss->capture_copied_total - vss->capture_acked_total;
+ return outstanding == (u64)vss->capture_ready &&
+ vss->capture_ready < vss->buffer_bytes;
}
/**
@@ -207,6 +234,9 @@
int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss);
+int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss,
+ bool *free_running);
+
void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss);
void virtsnd_pcm_msg_reset(struct virtio_pcm_substream *vss);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -3,6 +3,7 @@
* virtio-snd: Virtio sound device
* Copyright (C) 2021 OpenSynergy GmbH
*/
+#include <linux/compiler.h>
#include <linux/limits.h>
#include <linux/mm.h>
#include <linux/overflow.h>
@@ -384,6 +385,8 @@
vss->capture_notified_periods = 0;
vss->capture_callback_snapshot = 0;
vss->capture_callback_snapshot_active = false;
+ vss->capture_boundary = 0;
+ vss->capture_free_running = false;
vss->capture_empty = 0;
vss->capture_started = false;
}
@@ -508,24 +511,54 @@
return 0;
}
-/**
- * virtsnd_pcm_capture_send() - Queue staged capture requests.
- * @vss: VirtIO PCM substream.
- *
- * Queue full-period requests while the outstanding requests fit the remaining
- * ALSA ring capacity. The final request may exceed that capacity by less than
- * one period; completion validates the returned payload before copying it.
- *
- * Context: Expects the RX queue and substream spinlocks to be held by caller.
- * Return: 0 on success, -errno on failure.
- */
-int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss)
+/* Caller holds vss->lock. */
+int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss,
+ bool *free_running)
+{
+ struct snd_pcm_runtime *runtime;
+ snd_pcm_uframes_t stop_threshold;
+ u64 outstanding;
+ bool new_free_running;
+
+ if (!vss->capture_staging || !vss->runtime_active || !vss->substream)
+ return -EBADFD;
+
+ runtime = vss->substream->runtime;
+ if (!runtime || !vss->buffer_bytes || !vss->capture_boundary)
+ return -EBADFD;
+
+ /* Validate the currently committed policy before observing a transition. */
+ if (!virtsnd_pcm_capture_counters_valid(vss))
+ return -EIO;
+
+ stop_threshold = READ_ONCE(runtime->stop_threshold);
+ new_free_running = stop_threshold == vss->capture_boundary;
+ if (new_free_running != vss->capture_free_running) {
+ outstanding = vss->capture_copied_total -
+ vss->capture_acked_total;
+ if (new_free_running) {
+ vss->capture_ready = 0;
+ } else {
+ if (outstanding >= vss->buffer_bytes)
+ return -EPIPE;
+ vss->capture_ready = outstanding;
+ }
+ vss->capture_free_running = new_free_running;
+ }
+
+ *free_running = new_free_running;
+ return 0;
+}
+
+/* Caller holds the RX queue and vss locks and has sampled the policy. */
+static int virtsnd_pcm_capture_enqueue(struct virtio_pcm_substream *vss,
+ bool free_running)
{
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
struct virtqueue *vqueue = queue->vqueue;
unsigned long period_bytes;
size_t free_bytes;
- size_t request_limit;
+ size_t request_limit = vss->nmsgs;
unsigned int i;
bool added = false;
@@ -535,16 +568,17 @@
period_bytes = snd_pcm_lib_period_bytes(vss->substream);
if (!period_bytes || !vss->buffer_bytes ||
period_bytes >= vss->buffer_bytes ||
+ free_running != vss->capture_free_running ||
!virtsnd_pcm_capture_counters_valid(vss) ||
vss->msg_count > vss->nmsgs)
return -EIO;
- free_bytes = vss->buffer_bytes - vss->capture_ready;
- request_limit = free_bytes / period_bytes +
- !!(free_bytes % period_bytes);
- request_limit = min_t(size_t, request_limit, vss->nmsgs);
- if (vss->msg_count > request_limit)
- return -EIO;
+ if (!free_running) {
+ free_bytes = vss->buffer_bytes - vss->capture_ready;
+ request_limit = free_bytes / period_bytes +
+ !!(free_bytes % period_bytes);
+ request_limit = min_t(size_t, request_limit, vss->nmsgs);
+ }
for (i = 0; i < vss->nmsgs && vss->msg_count < request_limit; ++i) {
struct virtio_pcm_msg *msg = vss->msgs[i];
@@ -585,6 +619,25 @@
return 0;
}
+/**
+ * virtsnd_pcm_capture_send() - Refresh policy and queue staged requests.
+ * @vss: VirtIO PCM substream.
+ *
+ * Context: Expects the RX queue and substream spinlocks to be held by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+int virtsnd_pcm_capture_send(struct virtio_pcm_substream *vss)
+{
+ bool free_running;
+ int rc;
+
+ rc = virtsnd_pcm_capture_refresh_mode_locked(vss, &free_running);
+ if (rc)
+ return rc;
+
+ return virtsnd_pcm_capture_enqueue(vss, free_running);
+}
+
static int virtsnd_pcm_capture_status(const struct virtio_pcm_msg *msg,
size_t written_bytes,
size_t frame_bytes,
@@ -638,6 +691,7 @@
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);
@@ -651,6 +705,9 @@
/* A full ring is modulo-ambiguous, so fail closed before copying. */
if (!runtime || !runtime->dma_area)
invalid_reason = "runtime";
+ else if (virtsnd_pcm_capture_refresh_mode_locked(vss,
+ &free_running))
+ invalid_reason = "policy";
else if (!virtsnd_pcm_capture_counters_valid(vss))
invalid_reason = "counters";
else if (vss->hw_ptr !=
@@ -660,18 +717,28 @@
frame_bytes, &audio_bytes,
&latency_bytes))
invalid_reason = "status";
- else if (audio_bytes >=
- vss->buffer_bytes - vss->capture_ready)
- invalid_reason = "ring_capacity";
- else if (check_add_overflow(vss->capture_ready, audio_bytes,
- &next_ready))
- invalid_reason = "ready_overflow";
- else if (next_ready >= vss->buffer_bytes)
- invalid_reason = "ring_full";
else if (check_add_overflow(vss->capture_copied_total,
(u64)audio_bytes,
&next_copied_total))
invalid_reason = "total_overflow";
+ else if (free_running && audio_bytes >= vss->buffer_bytes)
+ invalid_reason = "payload_exceeds_ring";
+ else if (!free_running &&
+ audio_bytes >= vss->buffer_bytes - vss->capture_ready)
+ invalid_reason = "ring_capacity";
+ else if (!free_running &&
+ check_add_overflow(vss->capture_ready, audio_bytes,
+ &next_ready))
+ invalid_reason = "ready_overflow";
+ else if (!free_running && next_ready >= vss->buffer_bytes)
+ invalid_reason = "ring_full";
+ else if (free_running &&
+ (next_copied_total < vss->capture_pointer_seen_total ||
+ next_copied_total - vss->capture_pointer_seen_total >=
+ vss->buffer_bytes))
+ invalid_reason = "pointer_window";
+ else if (free_running)
+ next_ready = 0;
if (invalid_reason) {
virtsnd_pcm_capture_xrun_locked(vss);
@@ -713,7 +780,8 @@
msg->length = 0;
if (vss->runtime_active && vss->xfer_enabled) {
- if (!vss->xfer_xrun && virtsnd_pcm_capture_send(vss)) {
+ if (!vss->xfer_xrun &&
+ virtsnd_pcm_capture_enqueue(vss, free_running)) {
dev_warn_ratelimited(&vss->snd->vdev->dev,
"SID %u: failed to refill capture queue\n",
vss->sid);
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -543,6 +543,8 @@
vss->capture_notified_periods = 0;
vss->capture_callback_snapshot = 0;
vss->capture_callback_snapshot_active = false;
+ vss->capture_boundary = 0;
+ vss->capture_free_running = false;
}
}
@@ -565,6 +567,7 @@
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_device *vdev = vss->snd->vdev;
bool pause_restart = virtsnd_pcm_pause_restart_enabled(vss);
+ bool free_running;
int rc;
bool stopped;
@@ -622,6 +625,15 @@
vss->suspended = false;
vss->msg_count = 0;
virtsnd_pcm_msg_reset(vss);
+ if (vss->capture_staging) {
+ scoped_guard(spinlock_irqsave, &vss->lock) {
+ 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 =
@@ -893,6 +905,13 @@
if (rc) {
vss->stopped = true;
substream->runtime->stop_operating = true;
+ if (vss->capture_staging &&
+ vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
+ command == SNDRV_PCM_TRIGGER_PAUSE_RELEASE) {
+ /* Pause undo leaves ALSA paused; wake its readers. */
+ vss->xfer_xrun = true;
+ virtsnd_pcm_schedule_period_elapsed(vss);
+ }
return rc;
}
vss->xfer_enabled = true;
@@ -1132,12 +1151,18 @@
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
size_t hw_ptr = 0;
u64 total;
+ bool free_running;
bool xrun;
if (vss->capture_staging) {
scoped_guard(spinlock_irqsave, &vss->lock) {
- xrun = vss->xfer_xrun ||
- !virtsnd_pcm_capture_counters_valid(vss);
+ xrun = vss->xfer_xrun;
+ if (!xrun &&
+ virtsnd_pcm_capture_refresh_mode_locked(vss,
+ &free_running))
+ xrun = true;
+ if (!xrun && !virtsnd_pcm_capture_counters_valid(vss))
+ xrun = true;
if (!xrun) {
total = vss->capture_callback_snapshot_active ?
vss->capture_callback_snapshot :
@@ -1244,14 +1269,18 @@
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)
return -EBADFD;
if (vss->xfer_xrun)
return -EPIPE;
- if (!virtsnd_pcm_capture_counters_valid(vss)) {
+ rc = virtsnd_pcm_capture_refresh_mode_locked(vss,
+ &free_running);
+ if (rc || !virtsnd_pcm_capture_counters_valid(vss)) {
vss->xfer_xrun = true;
virtsnd_pcm_schedule_period_elapsed(vss);
return -EPIPE;
@@ -1270,7 +1299,11 @@
/* ACK includes any subperiod tail already copied to the ring. */
bytes = frames_to_bytes(runtime, diff);
- if (bytes > vss->capture_ready ||
+ 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_copied_total) {
@@ -1280,7 +1313,8 @@
}
vss->capture_acked_total = next_acked_total;
- vss->capture_ready -= bytes;
+ if (!free_running)
+ vss->capture_ready -= bytes;
vss->pcm_indirect.appl_ptr = appl_ptr;
}
@@ -1314,8 +1348,14 @@
struct virtsnd_pcm_transfer transfer = {};
int rc;
- if (vss->capture_staging)
- return virtsnd_pcm_capture_staged_ack(vss, runtime);
+ if (vss->capture_staging) {
+ rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
+ if (rc == -EPIPE) {
+ guard(spinlock_irqsave)(&vss->lock);
+ virtsnd_pcm_capture_arm_paused_stop(vss);
+ }
+ return rc;
+ }
scoped_guard(spinlock_irqsave, &queue->lock) {
guard(spinlock)(&vss->lock);
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers
2026-10-27 23:43 ` [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers Joonho Lee
@ 2026-10-28 0:13 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-28 0:13 UTC (permalink / raw)
To: Joonho Lee, linux-sound; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio-snd: un-armed paused stop on read XRUN in virtsnd_pcm_cp_ack()
--
commit ceb71ae8baa14854bd1fb36ae47981ac52e944e5
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio-snd: fix dsnoop capture with staged buffers
This commit adds free-running capture mode support for staged capture buffers
when stop_threshold equals the boundary, allowing dsnoop clients to consume
the mmap ring without triggering capacity rejections. It also handles transitions
back to bounded capture and arms synchronous stop on paused XRUN paths.
> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 663f7695b5be8..e7258bce2e6f0 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -508,24 +511,54 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
> return 0;
> }
>
[ ... ]
> +/* Caller holds vss->lock. */
> +int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss,
> + bool *free_running)
> +{
> + struct snd_pcm_runtime *runtime;
> + snd_pcm_uframes_t stop_threshold;
> + u64 outstanding;
> + bool new_free_running;
> +
> + if (!vss->capture_staging || !vss->runtime_active || !vss->substream)
> + return -EBADFD;
> +
> + runtime = vss->substream->runtime;
> + if (!runtime || !vss->buffer_bytes || !vss->capture_boundary)
> + return -EBADFD;
> +
> + /* Validate the currently committed policy before observing a transition. */
> + if (!virtsnd_pcm_capture_counters_valid(vss))
> + return -EIO;
> +
> + stop_threshold = READ_ONCE(runtime->stop_threshold);
> + new_free_running = stop_threshold == vss->capture_boundary;
> + if (new_free_running != vss->capture_free_running) {
> + outstanding = vss->capture_copied_total -
> + vss->capture_acked_total;
> + if (new_free_running) {
> + vss->capture_ready = 0;
> + } else {
> + if (outstanding >= vss->buffer_bytes)
> + return -EPIPE;
> + vss->capture_ready = outstanding;
> + }
> + vss->capture_free_running = new_free_running;
> + }
> +
> + *free_running = new_free_running;
> + return 0;
> +}
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index d50924b792f65..cbf4e479d295f 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -1314,8 +1348,14 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
> struct virtsnd_pcm_transfer transfer = {};
> int rc;
>
> - if (vss->capture_staging)
> - return virtsnd_pcm_capture_staged_ack(vss, runtime);
> + if (vss->capture_staging) {
> + rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
> + if (rc == -EPIPE) {
> + guard(spinlock_irqsave)(&vss->lock);
> + virtsnd_pcm_capture_arm_paused_stop(vss);
> + }
> + return rc;
> + }
[Severity: Medium]
Can a paused staged-capture stream still enter SNDRV_PCM_STATE_XRUN during a
partial read without arming vss->stopped and runtime->stop_operating?
If a staged-capture stream is paused in SNDRV_PCM_STATE_PAUSED with unread
frames in the ring (where outstanding < vss->buffer_bytes), and userspace
lowers runtime->stop_threshold via snd_pcm_sw_params() from boundary to a
bounded threshold smaller than the frames that will remain after a partial
read, snd_pcm_sw_params() does not call snd_pcm_update_state() because
snd_pcm_running(substream) is false while paused.
When userspace then performs a partial read through __snd_pcm_lib_xfer() that
leaves remaining avail >= runtime->stop_threshold:
__snd_pcm_lib_xfer()
pcm_lib_apply_appl_ptr()
virtsnd_pcm_cp_ack()
virtsnd_pcm_capture_staged_ack()
virtsnd_pcm_capture_refresh_mode_locked()
virtsnd_pcm_capture_refresh_mode_locked() in sound/virtio/virtio_pcm_msg.c
only checks outstanding >= vss->buffer_bytes when transitioning from
free-running to bounded mode, so virtsnd_pcm_capture_staged_ack() succeeds
with rc == 0. Because virtsnd_pcm_cp_ack() in sound/virtio/virtio_pcm_ops.c
only calls virtsnd_pcm_capture_arm_paused_stop(vss) when rc == -EPIPE, it
returns 0 without setting vss->stopped or runtime->stop_operating.
Immediately afterward at the end of __snd_pcm_lib_xfer() in
sound/core/pcm_lib.c:
_end_unlock:
runtime->twake = 0;
if (xfer > 0 && err >= 0)
snd_pcm_update_state(substream, runtime);
snd_pcm_update_state() sees avail >= runtime->stop_threshold and calls
__snd_pcm_xrun() -> snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN). Because
snd_pcm_running() is false in SNDRV_PCM_STATE_PAUSED, snd_pcm_do_stop() skips
trigger(STOP) and does not set runtime->stop_operating = true.
On the subsequent recovery call to snd_pcm_prepare(), snd_pcm_sync_stop() is
skipped and virtsnd_pcm_prepare() in sound/virtio/virtio_pcm_ops.c fails with
-EBADFD on vss->stopped (in pause-restart mode) or
virtsnd_pcm_msg_pending_num(vss) (in native pause mode):
scoped_guard(spinlock_irqsave, &vss->lock)
stopped = vss->stopped;
if (stopped) {
dev_err(&vdev->dev, "SID %u: stream release failed\n",
vss->sid);
substream->runtime->stop_operating = true;
return -EBADFD;
}
if (!vss->suspended) {
if (virtsnd_pcm_msg_pending_num(vss)) {
dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
vss->sid);
return -EBADFD;
}
Should virtsnd_pcm_cp_ack() also check whether a paused stream has remaining
capture avail >= runtime->stop_threshold (or check stop_threshold in
virtsnd_pcm_capture_staged_ack()) so virtsnd_pcm_capture_arm_paused_stop() is
armed before snd_pcm_update_state() moves the paused stream to
SNDRV_PCM_STATE_XRUN?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=11
^ permalink raw reply [flat|nested] 23+ messages in thread