Re: [PATCH v24 16/16] ALSA: virtio: preserve playback progress across pause

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ALSA: virtio: spurious playback wrap and XRUN in
  virtsnd_pcm_period_elapsed()
- [Medium] ALSA: virtio: stale runtime->delay after prepare and playback
  reset
--

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

ALSA: virtio: preserve playback progress across pause

This commit preserves playback completion progress across pause, caches
device latency in the substream state, and limits queued playback data to
less than one buffer ring.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 5fdd22de61736..7721376f2ecde 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -1002,12 +1002,10 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  	/* Keep latched errors until prepare establishes a fresh epoch. */
>  	vss->msg_count--;
>  
> -	if (vss->xfer_enabled) {
> -		struct snd_pcm_runtime *runtime = vss->substream->runtime;
> -
> -		runtime->delay = bytes_to_frames(runtime, latency_bytes);
> +	/* A queued request can outlive the ALSA runtime after hw_free. */
> +	vss->playback_latency_bytes = latency_bytes;
> +	if (vss->prepared && !vss->preparing)
>  		virtsnd_pcm_schedule_period_elapsed(vss);
> -	}
>  	if (!vss->msg_count)
>  		wake_up_all(&vss->msg_empty);
>  }

[Severity: High]
Can a stale or duplicate vss->elapsed_period work item run after all
playback progress has already been consumed or reset, causing a false
XRUN?

virtsnd_pcm_msg_complete() now schedules vss->elapsed_period while paused,
and virtsnd_pcm_trigger_inner() schedules it again on resume when
virtsnd_pcm_playback_progress_pending() is true.  However,
virtsnd_pcm_playback_reset() does not cancel pending work when rebasing
vss->hw_ptr and vss->pcm_indirect.hw_io, and virtsnd_pcm_period_elapsed()
calls snd_pcm_period_elapsed_under_stream_lock() unconditionally for
playback:

sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() {
    ...
	if (vss->direction != SNDRV_PCM_STREAM_CAPTURE) {
		snd_pcm_period_elapsed_under_stream_lock(substream);
		return;
	}
}

For example, if a late TX completion arrives while paused and queues
vss->elapsed_period, and the application runs SNDRV_PCM_IOCTL_RESET
followed by SNDRV_PCM_TRIGGER_PAUSE_RELEASE before the worker executes:

CPU0 (vhost / irq)                CPU1 (ioctl / kworker)
virtsnd_pcm_msg_complete()
  msg_count-- (becomes 0)
  schedule_work(&elapsed_period)
                                  virtsnd_pcm_playback_reset()
                                    hw_ptr = 0;
                                    pcm_indirect.hw_io = 0;
                                  snd_pcm_do_pause(substream, 0)
                                    runtime->hw_ptr_jiffies =
                                      jiffies - HZ * 1000;
                                  virtsnd_pcm_period_elapsed()
                                    snd_pcm_period_elapsed_under_stream_lock()
                                      snd_pcm_update_hw_ptr0(..., 1)
                                        <-- false wrap + XRUN

Because virtsnd_pcm_period_elapsed() runs with in_interrupt = 1 when
vss->hw_ptr == vss->pcm_indirect.hw_io, new_hw_ptr equals
runtime->hw_ptr_interrupt while runtime->hw_ptr_jiffies is backdated
from pause, so snd_pcm_update_hw_ptr0() treats the unchanged pointer as a
full-buffer interrupt wrap, adds runtime->buffer_size to hw_base, and
stops the stream with an XRUN.  The same happens if the worker dequeues
elapsed_period and blocks on the stream lock while PAUSE_RELEASE holds it
and queues a second run.

Should virtsnd_pcm_period_elapsed() check
virtsnd_pcm_playback_progress_pending(vss) before calling
snd_pcm_period_elapsed_under_stream_lock()?

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index da4e98836721d..7fbbce1dcbb73 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -770,6 +770,12 @@ static int virtsnd_pcm_prepare_locked(struct snd_pcm_substream *substream)
>  		memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
>  		vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes;
>  		vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes;
> +		if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK) {
> +			/* Keep unobserved completions below one physical ring. */
> +			vss->pcm_indirect.hw_queue_size = vss->buffer_bytes -
> +				snd_pcm_lib_period_bytes(substream);
> +			vss->playback_latency_bytes = 0;
> +		}

[ ... ]

> @@ -1610,6 +1627,7 @@ virtsnd_pcm_pb_pointer(struct snd_pcm_substream *substream)
>  		}
>  		if (!paused_pointer)
>  			hw_ptr = vss->hw_ptr;
> +		runtime->delay = bytes_to_frames(runtime, vss->playback_latency_bytes);
>  	}
>  	if (paused_pointer) {
>  		if (defer_work)
> @@ -2226,6 +2244,7 @@ static int virtsnd_pcm_playback_reset(struct snd_pcm_substream *substream)
>  	vss->pcm_indirect.hw_data = position;
>  	vss->pcm_indirect.sw_io = position;
>  	vss->pcm_indirect.sw_data = position;
> +	vss->playback_latency_bytes = 0;
>  	vss->pcm_indirect.hw_ready = 0;
>  	vss->pcm_indirect.sw_ready = 0;
>  	vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr;

[Severity: Medium]
Does clearing vss->playback_latency_bytes in virtsnd_pcm_prepare_locked()
and virtsnd_pcm_playback_reset() leave a stale runtime->delay behind?

runtime->delay is now updated only when virtsnd_pcm_pb_pointer() runs.
During prepare, or during SNDRV_PCM_IOCTL_RESET inPREPARED or PAUSED
state, snd_pcm_lib_reset() does not call virtsnd_pcm_pb_pointer().  When
SNDRV_PCM_IOCTL_RESET is called in RUNNING state,
virtsnd_pcm_playback_reset() calls snd_pcm_lib_reset() before zeroing
vss->playback_latency_bytes, so virtsnd_pcm_pb_pointer() reloads the old
vss->playback_latency_bytes into runtime->delay during the reset.  In both
cases runtime->delay keeps the previous playback latency until the next
pointer update, and SNDRV_PCM_IOCTL_DELAY or SNDRV_PCM_IOCTL_STATUS
reports stale delay.

Clearing vss->playback_latency_bytes and runtime->delay before
snd_pcm_lib_reset(), and zeroing runtime->delay in
virtsnd_pcm_prepare_locked(), would keep both in sync.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111000543.28550-1-mynameisjune111@gmail.com?part=16