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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: virtio: dropped refill in virtsnd_pcm_pb_pointer()
  stalls playback
--

commit 4311dfc7324b8614c483b2c630bddb6a93007fbd
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 substream state, caps indirect hw_queue_size to one period
below the buffer size, and skips period notifications when no playback
progress is pending.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 6d6c1bcd8601b..6c11885c55104 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -770,6 +770,13 @@ 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;
> +			substream->runtime->delay = 0;
> +		}

[Severity: Medium]
Can capping vss->pcm_indirect.hw_queue_size leave the held-back period in
vss->pcm_indirect.sw_ready unsubmitted if virtsnd_pcm_pb_pointer() runs while
runtime->buffer_accessing is temporarily negative?

With hw_queue_size capped at buffer_bytes - period_bytes, filling the buffer
holds the final period in vss->pcm_indirect.sw_ready until
virtsnd_pcm_pb_pointer() calls snd_pcm_indirect_playback_pointer(), which
updates hw_io and hw_ready before invoking substream->ops->ack()
(virtsnd_pcm_pb_ack()).

If a concurrent non-atomic ioctl such as SNDRV_PCM_IOCTL_RESET or
SNDRV_PCM_IOCTL_PREPARE on a running stream calls
snd_pcm_buffer_access_lock() before acquiring the PCM stream lock and being
rejected with -EBUSY, runtime->buffer_accessing is transiently -1:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_ack_state() {
    ...
	case SNDRV_PCM_STATE_RUNNING:
	case SNDRV_PCM_STATE_PAUSED:
	case SNDRV_PCM_STATE_DRAINING:
		if (atomic_read(&substream->runtime->buffer_accessing) < 0)
			return -EBADFD;
		return 0;
    ...
}

snd_pcm_indirect_playback_pointer() only checks for -EPIPE and ignores
-EBADFD, so vss->pcm_indirect.hw_io advances to vss->hw_ptr without queuing
vss->pcm_indirect.sw_ready.  With a two-period buffer or during
SNDRV_PCM_STATE_DRAINING, vss->msg_count reaches 0 with vss->hw_ptr equal to
vss->pcm_indirect.hw_io, virtsnd_pcm_playback_progress_pending() returns
false, and playback or drain stalls.

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