Re: [PATCH v25 04/16] ALSA: pcm: notify drivers after linked prepare completes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: pcm: state desync on opted-in streams in
  snd_pcm_finish_prepare()
--

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

ALSA: pcm: notify drivers after linked prepare completes

This commit adds an optional prepare_complete callback to snd_pcm_ops and
defers publishing SNDRV_PCM_STATE_PREPARED until all linked prepare and
reset actions finish under the stream locks.

> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index a547ac720b518..f98eb3329f2de 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
[ ... ]
> @@ -2089,20 +2113,108 @@ static int snd_pcm_do_prepare(struct snd_pcm_substream *substream,
>  	return snd_pcm_reset_substream(substream, true);
>  }
>  
> -static void snd_pcm_post_prepare(struct snd_pcm_substream *substream,
> -				 snd_pcm_state_t state)
> +static int snd_pcm_finish_prepare(struct snd_pcm_substream *substream,
> +				  snd_pcm_state_t state, bool success,
> +				  struct snd_pcm_substream *failed_substream,
> +				  bool stream_lock)
>  {
[ ... ]
> +	/* Preserve terminal states instead of publishing a partial group commit. */
> +	snd_pcm_group_for_each_entry(s, substream) {
> +		struct snd_pcm_runtime *runtime = s->runtime;
> +
> +		if (runtime->state == SNDRV_PCM_STATE_DISCONNECTED) {
> +			cancel = true;
> +			cancel_err = -EBADFD;
> +		} else if (runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
> +			cancel = true;
> +			if (!cancel_err)
> +				cancel_err = -ESTRPIPE;
> +		}
> +		if (!success && s == failed_substream)
> +			break;
> +	}
> +
> +	if (success && !cancel) {
> +		snd_pcm_group_for_each_entry(s, substream) {
> +			struct snd_pcm_runtime *runtime = s->runtime;
> +
> +			/* The reset callback path already completed this rebase. */
> +			if (!s->ops->reset)
> +				runtime->control->appl_ptr = runtime->status->hw_ptr;
> +			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_PREPARED);
> +		}
> +	} else if (!success && !cancel) {
> +		snd_pcm_group_for_each_entry(s, substream) {
> +			struct snd_pcm_runtime *runtime = s->runtime;
> +
> +			if (s->ops->prepare_complete &&
> +			    runtime->state != SNDRV_PCM_STATE_SETUP)
> +				__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
> +			if (s == failed_substream)
> +				break;
> +		}
> +	}
> +
> +	if (!success || cancel) {
> +		snd_pcm_group_for_each_entry(s, substream) {
> +			wake_up(&s->runtime->sleep);
> +			wake_up(&s->runtime->tsleep);
> +			if (!success && s == failed_substream)
> +				break;
> +		}
> +	}
> +
> +	/* Publish all states before any driver observes completion. */
> +	snd_pcm_group_for_each_entry(s, substream) {
> +		if (s->ops->prepare_complete)
> +			s->ops->prepare_complete(s, success && !cancel);
> +		if (!success && s == failed_substream)
> +			break;
> +	}

[Severity: Medium]
Can gating the SNDRV_PCM_STATE_SETUP rollback on !cancel leave healthy
opted-in streams in SNDRV_PCM_STATE_PREPARED after calling
prepare_complete(s, false)?

When a linked group is re-prepared from SNDRV_PCM_STATE_PREPARED and one
member's PCM device disconnects during snd_pcm_do_prepare(),
snd_pcm_dev_disconnect() marks only that device's substreams as
SNDRV_PCM_STATE_DISCONNECTED because the streams are not running.
snd_pcm_finish_prepare() then sets cancel = true, skips resetting the other
opted-in members to SNDRV_PCM_STATE_SETUP, and still calls
s->ops->prepare_complete(s, false) on them.

That revokes driver preparation while runtime->state (or
runtime->suspended_state if the stream was suspended during
snd_pcm_do_prepare()) remains SNDRV_PCM_STATE_PREPARED in the ALSA core,
so once the disconnected sibling is unlinked or closed, later START or ACK
calls pass core state checks and fail in the driver.  Should each opted-in
member in the affected group or prefix that is not itself DISCONNECTED or
SUSPENDED be reset to SNDRV_PCM_STATE_SETUP whenever !success || cancel
(and have runtime->suspended_state updated to SNDRV_PCM_STATE_SETUP if
suspended)?

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