Re: [PATCH v22 11/13] ALSA: virtio: gate starts during PCM prepare

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] ALSA: virtio: missing playback XRUN state in
  virtsnd_pcm_start_failed()
- [Medium] ALSA: virtio: PAUSE_PUSH rollback re-enables queue in
  virtsnd_pcm_trigger_inner()
- [Medium] ALSA: virtio: paused ACK validation skips XRUN latch in
  virtsnd_pcm_cp_ack_locked()
- [Medium] ALSA: virtio: unaligned appl_ptr in
  virtsnd_pcm_capture_staged_ack() passes hw_ptr
- [Low] ALSA: virtio: commit message omits staged capture, reset, and
  param changes
--

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

> ALSA: virtio: gate starts during PCM prepare
>
> Use the prepare-completion callback to keep ACK admission closed until
> every linked member finishes device prepare and the core pointer reset.
> Clear the gate after success or failure, and keep failed members
> unprepared so a later ACK cannot use stale accounting.
>
> Keep START and PAUSE_RELEASE admission checks at the virtio trigger and
> deferred restart entry points. Preserve the existing capture lifecycle
> and reset rules.
>
> Assisted-by: LLM
> Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>

[Severity: Low]
Does the commit message match the actual changes in this patch?

While the changelog describes gating ACK and start admission during
prepare and claims to preserve the existing capture lifecycle and reset
rules, the patch adds a request-owned staged capture path, five new
module parameters (capture_staging, capture_packed_status,
capture_allow_short, capture_allow_empty, and capture_pause_restart), a
pause-restart state machine, custom reset callbacks
(virtsnd_pcm_playback_reset(), virtsnd_pcm_indirect_reset(), and
virtsnd_pcm_reset()), and manual DMA buffer management replacing
snd_pcm_set_managed_buffer_all().

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index b9728d04ce608..b2a569866d5c9 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -97,46 +131,265 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
>  };
>  
>  static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream);
> +static int virtsnd_pcm_sync_stop_locked(struct snd_pcm_substream *substream);
> +static int virtsnd_pcm_pause_restart_sync_stop(struct snd_pcm_substream *substream);
>  
> -/**
> - * virtsnd_pcm_close_ack() - Update PCM prepare admission state.
> - * @substream: Kernel ALSA substream.
> - * @preparing: True while driver prepare and the core pointer reset are active.
> - *
> - * Context: Process context.
> - */
> -static void virtsnd_pcm_close_ack(struct snd_pcm_substream *substream,
> -				  bool preparing)
> +static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss, u64 epoch)
>  {
> -	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	bool notify;
>  
> -	scoped_guard(pcm_stream_lock_irqsave, substream) {
> -		guard(spinlock_irqsave)(&vss->lock);
> -		vss->prepared = false;
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->resetting || vss->reset_epoch != epoch)
> +			return;
>  		vss->xfer_enabled = false;
> -		vss->preparing = preparing;
> +		vss->stopped = vss->release_required || vss->msg_count;
> +		notify = vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +			 (!vss->capture_staging || vss->runtime_active);
> +		if (notify)
> +			vss->xfer_xrun = true;
>  	}
> +	vss->substream->runtime->stop_operating = true;
> +	if (notify)
> +		virtsnd_pcm_schedule_period_elapsed(vss);
>  }

[Severity: Medium]
Should virtsnd_pcm_start_failed() also latch vss->xfer_xrun and schedule
vss->elapsed_period for playback streams?

When SNDRV_PCM_TRIGGER_START or SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails on
a playback substream, vss->stopped is set to true while vss->xfer_xrun
remains false.  A subsequent SNDRV_PCM_TRIGGER_PAUSE_RELEASE (such as
during snd_pcm_drop() or snd_pcm_prepare() on a paused stream) then skips
virtsnd_pcm_defer_resume_xrun() and fails in virtsnd_pcm_start_queue()
with -EBADFD, and a start failure during snd_pcm_do_drain_init() or
snd_pcm_undo_pause() leaves the stream in RUNNING or DRAINING without
virtsnd_pcm_pb_pointer() ever reporting SNDRV_PCM_POS_XRUN.

[ ... ]

> +		rc = virtsnd_pcm_stop_device(vss, epoch);
> +		if (rc && command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
> +			guard(spinlock_irqsave)(&vss->lock);
>  
> -		return virtsnd_ctl_msg_send_sync(snd, msg);
> +			if (vss->reset_epoch == epoch && !vss->resetting) {
> +				vss->xfer_xrun = true;
> +				substream->runtime->stop_operating = true;
> +				if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
> +					virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
> +		}
> +		if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
> +			if (virtsnd_pcm_start_queue(vss, epoch)) {
> +				guard(spinlock_irqsave)(&vss->lock);
> +				if (vss->reset_epoch == epoch && !vss->resetting) {
> +					vss->xfer_xrun = true;
> +					vss->stopped = true;
> +					substream->runtime->stop_operating = true;
> +					virtsnd_pcm_schedule_period_elapsed(vss);
> +				}
> +			} else if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
> +		}
> +		return rc;

[Severity: Medium]
Can a failed SNDRV_PCM_TRIGGER_PAUSE_PUSH re-enable the queue while
substream->runtime->state is SNDRV_PCM_STATE_PAUSED?

When resuming a linked group from pause fails on a later member,
snd_pcm_undo_pause() calls trigger(SNDRV_PCM_TRIGGER_PAUSE_PUSH) on
earlier members while their runtime->state is still
SNDRV_PCM_STATE_PAUSED and ignores the return value.  If
virtsnd_pcm_stop_device() fails during that rollback, calling
virtsnd_pcm_start_queue() leaves vss->xfer_enabled set and capture
buffers queued on a paused substream instead of latching vss->xfer_xrun
and vss->stopped.

[ ... ]

> +		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)
> +			return virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		appl_diff = diff;
> +		ack_frames = appl_diff - appl_diff % runtime->min_align;
> +		if (ack_frames > runtime->buffer_size ||
> +		    (appl_diff > runtime->buffer_size &&
> +		     appl_diff - runtime->buffer_size >= runtime->min_align)) {
> +			return virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		}
> +
> +		/* ACK includes any subperiod tail already copied to the ring. */
> +		bytes = frames_to_bytes(runtime, ack_frames);
> +		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_pointer_seen_total) {
> +			return virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		}
> +
> +		vss->capture_acked_total = next_acked_total;
> +		if (!free_running)
> +			vss->capture_ready -= bytes;
> +		/* Keep any unaligned frame tail for the next acknowledgment. */
> +		vss->pcm_indirect.appl_ptr += ack_frames;
> +		if (vss->pcm_indirect.appl_ptr >= runtime->boundary)
> +			vss->pcm_indirect.appl_ptr -= runtime->boundary;
> +	}

[Severity: Medium]
Can an unaligned appl_ptr update in virtsnd_pcm_capture_staged_ack()
advance runtime->control->appl_ptr past runtime->status->hw_ptr?

For sub-byte frame formats where runtime->min_align > 1 (such as
1-channel SNDRV_PCM_FORMAT_IMA_ADPCM), rounding appl_diff down to
ack_frames and returning 0 lets pcm_lib_apply_appl_ptr() commit the
unaligned runtime->control->appl_ptr even when bytes ==
vss->capture_ready and no unread sample covers the tail frame.  That puts
appl_ptr ahead of hw_ptr and causes snd_pcm_capture_avail() to underflow
near runtime->boundary.  Rejecting unaligned appl_diff with -EBADFD, as
virtsnd_pcm_capture_ack_validate() does, would prevent the partial-frame
advance.

[ ... ]

> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		paused = runtime->state == SNDRV_PCM_STATE_PAUSED;
> +		if (vss->preparing)
> +			return -EBADFD;
> +		/* A latched XRUN must not re-arm sync_stop once teardown began. */
> +		if (vss->stopped && vss->xfer_xrun)
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +		} else {
> +			if (!vss->prepared ||
> +			    (vss->stopped &&
> +			     runtime->state != SNDRV_PCM_STATE_DRAINING))
> +				return -EBADFD;
> +			rc = virtsnd_pcm_capture_ack_validate(vss, runtime,
> +							      &skip_transfer);
> +			if (rc == -EBADFD || (rc && paused))
> +				return -EBADFD;
> +			if (!rc && skip_transfer)
> +				return 0;
> +			if (!rc) {
>  				transfer.indirect = vss->pcm_indirect;
>  				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
> +		if (rc) {
> +			if (paused)
> +				rc = virtsnd_pcm_ack_xrun_locked(vss, runtime);
> +			else
> +				virtsnd_pcm_transfer_failed(vss);
> +		} else {
>  			vss->pcm_indirect = transfer.indirect;
> +		}
>  	}

[Severity: Medium]
Does returning -EBADFD on (rc && paused) right after
virtsnd_pcm_capture_ack_validate() skip latching the paused XRUN?

When virtsnd_pcm_capture_ack_validate() returns -EPIPE while paused, the
early return bypasses virtsnd_pcm_ack_xrun_locked() below, so
vss->xfer_xrun is never latched and the XRUN is not deferred to pause
release as in virtsnd_pcm_pb_ack_locked().

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