Re: [PATCH v11 14/14] ALSA: virtio: rebase ordinary capture accounting on RESET

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio: stale pcm_indirect state after RESET in PREPARED state
--

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

ALSA: virtio: rebase ordinary capture accounting on RESET

This commit introduces virtsnd_pcm_indirect_reset() and
virtsnd_pcm_capture_indirect_ops to rebase ordinary capture indirect
positions and return discarded RX space when SNDRV_PCM_IOCTL_RESET is
called. It also suppresses nested ACK refills during the RESET pointer
observation and updates the period notification floor.

Link: https://sashiko.dev/#/patchset/20261030052629.27742-1-mynameisjune111%40gmail.com?part=14

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index f350d0c753763..9af0813acd77a 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -1761,6 +1769,89 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	return virtsnd_pcm_sync_stop_locked(substream);
>  }
>  
> +/*
> + * The PCM lock covers indirect accounting and core completion. Once lib_reset
> + * changes hw_ptr, complete the core rebase even if refilling fails. The latched
> + * XRUN rejects subsequent pointer/ACK operations; the worker publishes XRUN to
> + * paused streams too. RESET success therefore does not imply stream recovery.
> + */
> +static int virtsnd_pcm_indirect_reset(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct virtsnd_pcm_transfer transfer = {};
> +	snd_pcm_uframes_t paused_ptr = 0;
> +	bool paused = runtime->state == SNDRV_PCM_STATE_PAUSED;
> +	int rc;
> +
> +	/* Only PCM-locked callbacks access this observation flag. */
> +	vss->indirect_reset_observing = true;
> +	rc = snd_pcm_lib_reset(substream);
> +	vss->indirect_reset_observing = false;
> +	if (rc)
> +		return rc;
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting)
> +		return 0;
> +	if (vss->xfer_xrun) {
> +		virtsnd_pcm_transfer_failed(vss);
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}
> +	if (!snd_pcm_running(substream) && !paused)
> +		return 0;

[Severity: Medium]
Can returning early here when the substream is in SNDRV_PCM_STATE_PREPARED
leave vss->pcm_indirect.appl_ptr and vss->pcm_indirect.sw_ready out of sync
with runtime->control->appl_ptr?

While in SNDRV_PCM_STATE_PREPARED (where vss->prepared is true and
vss->stopped is false), userspace can invoke SNDRV_PCM_IOCTL_SYNC_PTR
without SNDRV_PCM_SYNC_PTR_APPL to advance appl_ptr to N:

  snd_pcm_sync_ptr()
    pcm_lib_apply_appl_ptr()
      virtsnd_pcm_cp_ack()
        virtsnd_pcm_cp_ack_locked()
          snd_pcm_indirect_capture_transfer()

which sets vss->pcm_indirect.appl_ptr = N and
vss->pcm_indirect.sw_ready = -frames_to_bytes(runtime, N).

If userspace then calls SNDRV_PCM_IOCTL_RESET in SNDRV_PCM_STATE_PREPARED,
snd_pcm_lib_reset() sets runtime->status->hw_ptr = 0,
virtsnd_pcm_indirect_reset() returns 0 early without updating
vss->pcm_indirect, and snd_pcm_reset_substream() resets
runtime->control->appl_ptr = runtime->status->hw_ptr (0).

When the stream is subsequently started and virtsnd_pcm_cp_pointer() or
virtsnd_pcm_cp_ack() runs before appl_ptr reaches N,
snd_pcm_indirect_capture_transfer() sees diff = appl_ptr - rec->appl_ptr < 0:

  if (diff) {
          if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
                  diff += runtime->boundary;
          if (diff < 0)
                  return -EPIPE;

causing a false -EPIPE XRUN and a dev_err() message in
virtsnd_pcm_cp_ack_locked().

Should virtsnd_pcm_indirect_reset() also reset vss->pcm_indirect.sw_ready = 0
and vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr when not running or
paused, similar to how virtsnd_pcm_reset() updates
vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr?

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