Re: [PATCH v32 20/20] ALSA: virtio: reset the device when PCM state stays unknown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] ALSA: virtio: deadlock between virtsnd_pcm_prepare() and
  virtsnd_recover_work()
- [High] ALSA: virtio: missing recovery wait in open, hw_free, and
  virtsnd_pcm_sync_stop()
- [Medium] ALSA: virtio: pending recover_work timer triggers premature
  device reset
- [Medium] ALSA: virtio: virtsnd_recover_work() races with freeze/reset
  and restore
- [Medium] ALSA: virtio: virtsnd_reset_prepare() does not suspend PCM
  streams on reset
- [Medium] ALSA: virtio: snd_pcm_suspend_all() in recovery sends
  blocking requests
- [Medium] ALSA: virtio: recovery ordering and races break
  virtsnd_pcm_prepare() and virtsnd_pcm_hw_params()
- [Low] ALSA: virtio: double volatile read of jiffies in
  virtsnd_recover_work()
--

commit ab36d8f3f343747049667c96b153b488074305f1
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: reset the device when PCM state stays unknown

This commit adds a delayed recovery work item that suspends open PCM
streams and resets the VirtIO sound device when a PCM substream's device
state remains unknown or an I/O queue flush times out, disconnecting the
ALSA card if the transport reset fails.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 3f2242f274404..dd110fd7ad165 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c

[ ... ]

> @@ -528,6 +530,190 @@ static int virtsnd_restore(struct virtio_device *vdev)
>  
>  	return 0;
>  }
> +
> +/* Minimum time between recovery resets of one device. */
> +#define VIRTSND_RECOVER_INTERVAL_MS	30000
> +
> +static bool virtsnd_recover_needed(struct virtio_snd *snd)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
> +		struct virtio_pcm_substream *vss = &snd->substreams[i];
> +
> +		guard(spinlock_irqsave)(&vss->lock);
> +		if (vss->resetting)
> +			continue;
> +		if (vss->control_uncertain ||
> +		    vss->pause_restart_state ==
> +		    VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
> +		    (vss->io_flush_failed && vss->msg_count))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * virtsnd_recover_work() - Reset the device when a PCM state stays unknown.
> + * @work: Recovery work of the sound device.
> + *
> + * A control reply or released I/O request that never returns leaves its PCM
> + * stream unusable. Suspend the open streams so applications prepare them
> + * again, then reset the device with the transport reset sequence.
> + *
> + * Context: Process context.
> + */
> +static void virtsnd_recover_work(struct work_struct *work)
> +{
> +	struct virtio_snd *snd = container_of(to_delayed_work(work),
> +					      struct virtio_snd, recover_work);
> +	struct virtio_device *vdev = snd->vdev;
> +	struct virtio_pcm *vpcm;
> +	unsigned int i;
> +	int rc;
> +
> +	if (!virtsnd_recover_needed(snd))
> +		return;
> +
> +	if (time_before(jiffies, snd->recover_next)) {
> +		queue_delayed_work(system_dfl_wq, &snd->recover_work,
> +				   snd->recover_next - jiffies);
> +		return;
> +	}

[Severity: Low]
Can snd->recover_next - jiffies underflow here?  jiffies is volatile and
read twice, so if it advances to or past snd->recover_next between
time_before() and the subtraction, the delay wraps around near ULONG_MAX
and prevents later queue_delayed_work() calls from scheduling recovery.

> +	snd->recover_next = jiffies +
> +			    msecs_to_jiffies(VIRTSND_RECOVER_INTERVAL_MS);
> +
> +	dev_warn(&vdev->dev, "PCM device state is unknown; resetting device\n");
> +
> +	WRITE_ONCE(snd->recovering, true);
> +	for (i = 0; i < snd->nsubstreams; ++i)
> +		wake_up_all(&snd->substreams[i].control_wait);
> +
> +	/* Applications observe -ESTRPIPE and prepare their streams again. */
> +	list_for_each_entry(vpcm, &snd->pcm_list, list)
> +		snd_pcm_suspend_all(vpcm->pcm);
> +
> +	rc = virtio_device_reset_prepare(vdev);

[Severity: Medium]
Does calling snd_pcm_suspend_all() before virtio_device_reset_prepare()
cause recovery to send new synchronous control requests and repeat flush
waits against the unresponsive device?

At this point vss->resetting is still false and control messages are not
blocked.  In virtsnd_pcm_control_settle(), virtsnd_pcm_control_settled()
is checked before READ_ONCE(vss->snd->recovering), so it returns 0 for any
substream where vss->control_uncertain is false (including streams being
recovered for vss->io_flush_failed or
VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED, as well as any other active
substream on the card).  During snd_pcm_suspend_all():

- snd_pcm_suspend() -> virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_SUSPEND)
  sends a synchronous VIRTIO_SND_R_PCM_STOP request and blocks for
  virtsnd_msg_timeout_ms.
- snd_pcm_sync_stop() -> virtsnd_pcm_release_locked() waits another
  virtsnd_msg_timeout_ms on vss->msg_empty, logs a duplicate warning, sets
  substream->runtime->stop_operating = true again, and calls
  virtsnd_pcm_flush_failed() to re-schedule snd->recover_work for another
  reset (or virtsnd_pcm_pause_restart_sync_stop() logs a duplicate error
  and calls virtsnd_recover_schedule(vss->snd, 0)).

> +	if (!rc)
> +		rc = virtio_device_reset_done(vdev);
> +	WRITE_ONCE(snd->recover_failed, !!rc);
> +	/* Publish the result before releasing configuration waiters. */
> +	smp_store_release(&snd->recovering, false);
> +	wake_up_all(&snd->recover_wait);
> +
> +	if (rc) {
> +		/*
> +		 * Wake configuration waiters before disconnect, which waits for
> +		 * their ALSA power references to be released.
> +		 */
> +		dev_err(&vdev->dev, "device recovery reset failed: %d; disconnecting\n",
> +			rc);
> +		snd_card_disconnect(snd->card);
> +	}
> +}
> +
> +/**
> + * virtsnd_recover_wait() - Wait until a recovery reset has finished.
> + * @snd: VirtIO sound device.
> + *
> + * Applications prepare their streams again as soon as recovery suspends
> + * them. Configuring a stream must wait until the reset has completed.
> + *
> + * Context: Process context. The caller must not hold the PCM open mutex or
> + *          a substream lifecycle lock, which recovery takes.
> + * Return: 0, -ENODEV if recovery failed, or -ERESTARTSYS if killed.
> + */
> +int virtsnd_recover_wait(struct virtio_snd *snd)
> +{
> +	int rc;
> +
> +	rc = wait_event_killable(snd->recover_wait,
> +				 /* Pair with recovery's result publication. */
> +				 !smp_load_acquire(&snd->recovering));
> +	if (rc)
> +		return rc;
> +	return READ_ONCE(snd->recover_failed) ? -ENODEV : 0;
> +}

[Severity: High]
Can virtsnd_recover_wait() called from virtsnd_pcm_prepare() deadlock with
virtsnd_recover_work() and snd_pcm_release()?

snd_pcm_prepare() -> snd_pcm_action_nonatomic() holds
down_read(&snd_pcm_link_rwsem) when it invokes virtsnd_pcm_prepare() ->
virtsnd_recover_wait().  Meanwhile, snd_pcm_release() holds
pcm->open_mutex and calls snd_pcm_release_substream() -> snd_pcm_unlink(),
which unconditionally takes down_write(&snd_pcm_link_rwsem) before
checking whether the substream is linked.  Because virtsnd_recover_work()
sets snd->recovering = true before taking pcm->open_mutex in
snd_pcm_suspend_all(), the three contexts can lock up:

CPU0                          CPU1                          kworker
----                          ----                          -------
                                                            virtsnd_recover_work()
                                                              snd->recovering = true
snd_pcm_prepare()
  down_read(&snd_pcm_link_rwsem)
  virtsnd_pcm_prepare()
    virtsnd_recover_wait()
                              snd_pcm_release()
                                mutex_lock(&pcm->open_mutex)
                                snd_pcm_unlink()
                                  down_write(&snd_pcm_link_rwsem)
                                                            snd_pcm_suspend_all()
                                                              mutex_lock(&pcm->open_mutex)

 *** DEADLOCK ***

This wedges system_dfl_wq and the global snd_pcm_link_rwsem across all
sound cards.

> +
> +/**
> + * virtsnd_recover_schedule() - Request a recovery check.
> + * @snd: VirtIO sound device.
> + * @delay: Time before the check, in jiffies. Zero brings a pending check
> + *         forward.
> + *
> + * Context: Any context.
> + */
> +void virtsnd_recover_schedule(struct virtio_snd *snd, unsigned long delay)
> +{
> +	if (delay)
> +		queue_delayed_work(system_dfl_wq, &snd->recover_work, delay);
> +	else
> +		mod_delayed_work(system_dfl_wq, &snd->recover_work, 0);
> +}

[Severity: Medium]
Can an already-pending snd->recover_work from an earlier event trigger a
premature device reset on a later timeout?

When a late control reply or I/O flush failure occurs at T1,
queue_delayed_work() schedules snd->recover_work for T1 + 5 timeouts.  If
that reply arrives and settles the substream (or the I/O queue drains),
snd->recover_work remains queued for T1 + 5 timeouts.  If another control
reply is late or an I/O flush fails at T2 < T1 + 5 timeouts,
queue_delayed_work() leaves the timer at T1 + 5 timeouts, and when
virtsnd_recover_work() runs at T1 + 5 timeouts, virtsnd_recover_needed()
sees vss->control_uncertain or (vss->io_flush_failed && vss->msg_count) set
and immediately resets the device before the second event's 4-timeout
settle window has elapsed.

> +
> +/*
> + * Transport resets and driver recovery use the freeze and restore sequence.
> + * Recovery calls these callbacks from its own work item.
> + */
> +static int virtsnd_reset_prepare(struct virtio_device *vdev)
> +{
> +	struct virtio_snd *snd = vdev->priv;
> +
> +	if (current_work() != &snd->recover_work.work)
> +		disable_delayed_work_sync(&snd->recover_work);
> +	virtsnd_quiesce(snd);
> +
> +	return 0;
> +}
> +
> +static int virtsnd_reset_done(struct virtio_device *vdev)
> +{
> +	struct virtio_snd *snd = vdev->priv;
> +	int rc;
> +
> +	rc = virtsnd_restart(snd);
> +	if (current_work() != &snd->recover_work.work)
> +		enable_delayed_work(&snd->recover_work);
> +
> +	return rc;
> +}

[Severity: Medium]
On a transport-initiated reset (where current_work() !=
&snd->recover_work.work), does virtsnd_reset_prepare() leave open ALSA PCM
streams in their active states instead of suspending them?

During system sleep, the PM core suspends child PCM devices before calling
virtsnd_freeze(), and virtsnd_recover_work() calls snd_pcm_suspend_all()
before virtio_device_reset_prepare().  A transport-initiated reset calls
virtsnd_reset_prepare() directly without setting snd->recovering or
calling snd_pcm_suspend_all(), so virtsnd_quiesce() detaches all virtqueue
buffers and sets vss->prepared = false and vss->suspended = true while
leaving substream->runtime->state in RUNNING, PREPARED, or PAUSED, causing
blocked readers/writers to hang until timeout (-EIO) instead of observing
-ESTRPIPE and subsequent triggers to fail with -EBADFD.  In addition, if
virtsnd_restart() fails in virtsnd_reset_done(), it still re-enables
snd->recover_work and does not set snd->recover_failed or disconnect the
card.

> +
> +#ifdef CONFIG_PM_SLEEP
> +/**
> + * virtsnd_freeze() - Suspend device.
> + * @vdev: VirtIO parent device.
> + *
> + * Context: Any context.
> + * Return: 0 on success, -errno on failure.
> + */
> +static int virtsnd_freeze(struct virtio_device *vdev)
> +{
> +	struct virtio_snd *snd = vdev->priv;
> +
> +	disable_delayed_work_sync(&snd->recover_work);
> +	virtsnd_quiesce(snd);
> +
> +	return 0;
> +}
> +
> +/**
> + * virtsnd_restore() - Resume device.
> + * @vdev: VirtIO parent device.
> + *
> + * Context: Any context.
> + * Return: 0 on success, -errno on failure.
> + */
> +static int virtsnd_restore(struct virtio_device *vdev)
> +{
> +	struct virtio_snd *snd = vdev->priv;
> +	int rc;
> +
> +	rc = virtsnd_restart(snd);
> +	enable_delayed_work(&snd->recover_work);
> +
> +	return rc;
> +}

[Severity: Medium]
Can virtsnd_recover_work() race with virtio_device_freeze() or a
transport-initiated virtio_device_reset_prepare(), and can
virtsnd_restore() or virtsnd_reset_done() revive a card that was already
disconnected after a failed recovery?

virtio_device_freeze() and virtio_device_reset_prepare() call
virtio_config_core_disable(vdev) before invoking the driver callback:

CPU0                                  kworker
----                                  -------
virtio_device_freeze(vdev)            virtsnd_recover_work()
  virtio_config_core_disable(vdev)
  virtsnd_freeze(vdev)
    disable_delayed_work_sync(...)
                                        virtio_device_reset_done(vdev)
                                          virtio_device_restore_priv(vdev)
                                            virtio_config_core_enable(vdev)
    virtsnd_quiesce(snd)  <-- vdev->config_core_enabled left true

Furthermore, if virtsnd_recover_work() fails its reset, sets
snd->recover_failed = true, and disconnects snd->card, a subsequent
virtsnd_restore() or transport-initiated virtsnd_reset_done() does not
check snd->recover_failed and unconditionally calls virtsnd_restart() and
enable_delayed_work(&snd->recover_work), re-initializing virtqueues and
enabling event dispatch on a disconnected card.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 15d4473f4c0a0..dcce428d704c7 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -661,6 +706,9 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
>  	u64 epoch;
>  	int rc;
>  
> +	rc = virtsnd_recover_wait(vss->snd);
> +	if (rc)
> +		return rc;
>  	guard(mutex)(&vss->lifecycle_lock);

[Severity: High]
Why don't virtsnd_pcm_open(), virtsnd_pcm_hw_free(), and
virtsnd_pcm_sync_stop() also wait for an in-progress recovery reset?

The commit message notes that open and hw_free do not wait because they
run under pcm->open_mutex, which recovery takes to suspend the streams.
However:

1. virtsnd_recover_work() only holds pcm->open_mutex inside
   snd_pcm_suspend_all() and releases it before calling
   virtio_device_reset_prepare() and virtio_device_reset_done(), so
   pcm->open_mutex is not held while vss->resetting and snd->recovering
   are true.
2. SNDRV_PCM_IOCTL_HW_FREE (snd_pcm_hw_free()) and the error path of
   snd_pcm_hw_params() only take runtime->buffer_mutex, never
   pcm->open_mutex.

As a result, during recovery virtsnd_pcm_open() fails with -ECANCELED or
-EIO; virtsnd_pcm_hw_free() on close returns -EIO or -ECANCELED from
virtsnd_pcm_sync_stop_locked() before virtsnd_reset_vqs() detaches pending
buffers, causing !virtsnd_pcm_msg_pending_num(vss) to be false and skipping
virtsnd_pcm_msg_free(vss) so vss->msgs and vss->dma_pages remain allocated;
and when snd_pcm_hw_params() or snd_pcm_do_prepare() calls
snd_pcm_sync_stop() before invoking ops->hw_params() or ops->prepare(),
virtsnd_pcm_sync_stop() sees vss->resetting == true and sets
substream->runtime->stop_operating = true again before
virtsnd_recover_wait() runs.

[ ... ]

> @@ -933,6 +982,10 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  
>  	int rc;
>  
> +	/* Applications prepare again as soon as recovery suspends the stream. */
> +	rc = virtsnd_recover_wait(vss->snd);
> +	if (rc)
> +		return rc;
>  	guard(mutex)(&vss->lifecycle_lock);
>  	rc = virtsnd_pcm_prepare_locked(substream);

[Severity: Medium]
Can ordering races between virtsnd_pcm_control_settle(),
virtsnd_recover_work(), and virtsnd_recover_wait() cause
snd_pcm_prepare() and snd_pcm_hw_params() to fail with -EIO or -ESTRPIPE
instead of waiting for recovery?

Two sequences can happen when recovery starts:

1. In snd_pcm_do_prepare() and snd_pcm_hw_params(), ALSA core calls
   snd_pcm_sync_stop() before ops->prepare() or ops->hw_params().  When
   vss->control_uncertain is true and runtime->stop_operating is set,
   snd_pcm_sync_stop() -> virtsnd_pcm_control_settle() waits 4 timeouts,
   calls virtsnd_recover_schedule(vss->snd, 0), and returns -EIO (which
   snd_pcm_sync_stop() ignores).  Because virtsnd_recover_schedule() only
   queues snd->recover_work without setting snd->recovering = true,
   virtsnd_recover_wait() in virtsnd_pcm_prepare() or
   virtsnd_pcm_hw_params() sees snd->recovering == false and returns 0
   immediately.  virtsnd_pcm_prepare_locked() or virtsnd_pcm_hw_params()
   then enters virtsnd_pcm_control_settle() a second time, which either
   returns -EIO as soon as virtsnd_recover_work() sets snd->recovering =
   true (leaving runtime->state in SNDRV_PCM_STATE_SETUP, which
   snd_pcm_suspend_all() then skips with -EBUSY) or waits another 4
   timeouts if rate-limited by snd->recover_next.
2. In virtsnd_recover_work(), WRITE_ONCE(snd->recovering, true) runs
   before snd_pcm_suspend_all().  If a thread enters snd_pcm_prepare() on
   a stream in PREPARED or XRUN state after snd->recovering becomes true
   and sleeps in virtsnd_recover_wait() before snd_pcm_suspend_all()
   suspends that substream, snd_pcm_suspend_all() sets runtime->state to
   SNDRV_PCM_STATE_SUSPENDED while the thread is sleeping.  After the
   reset completes, virtsnd_pcm_prepare_locked() configures and prepares
   the reset device, but snd_pcm_finish_prepare() then sees
   runtime->state == SNDRV_PCM_STATE_SUSPENDED, cancels the completed
   prepare via virtsnd_pcm_prepare_complete(s, false), and returns
   -ESTRPIPE:

sound/core/pcm_native.c:snd_pcm_finish_prepare() {
	if (runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
		snd_pcm_Substream_ops_prepare_complete(substream, false);
		return -ESTRPIPE;
	}
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fix-virtio-snd-recovery-final-v32-0-4e95979e312c@gmail.com?part=20