Re: [PATCH v22 04/13] ALSA: pcm: notify drivers after linked prepare completes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: pcm: spurious MSTOP timer notification in
  snd_pcm_complete_prepare()
- [Medium] ALSA: pcm: race window between snd_pcm_post_prepare() and
  prepare_complete()
--

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

ALSA: pcm: notify drivers after linked prepare completes

This commit adds an optional prepare_complete callback to struct
snd_pcm_ops and invokes it under the PCM stream lock after single or
linked prepare actions complete or fail.

> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index e4c2fe70fa705..40ac7fb614567 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c

[ ... ]

> @@ -1350,6 +1352,13 @@ static int snd_pcm_action_group(const struct action_ops *ops,
>  					ops->undo_action(s1, state);
>  				}
>  			}
> +			if (ops->complete_action) {
> +				snd_pcm_group_for_each_entry(s1, substream) {
> +					ops->complete_action(s1, state, false);
> +					if (s1 == s)
> +						break;
> +				}
> +			}
>  			s = NULL; /* unlock all */
>  			goto _unlock;
>  		}
> @@ -1357,6 +1366,10 @@ static int snd_pcm_action_group(const struct action_ops *ops,
>  	snd_pcm_group_for_each_entry(s, substream) {
>  		ops->post_action(s, state);
>  	}
> +	if (ops->complete_action) {
> +		snd_pcm_group_for_each_entry(s, substream)
> +			ops->complete_action(s, state, true);
> +	}

[Severity: Medium]
Can another caller observe SNDRV_PCM_STATE_PREPARED and invoke ops->ack()
or ops->trigger() in the unlocked window between snd_pcm_post_prepare()
and snd_pcm_complete_prepare()?

snd_pcm_post_prepare() calls snd_pcm_set_state(), which acquires
pcm_stream_lock_irq(substream), publishes SNDRV_PCM_STATE_PREPARED, and
unlocks before snd_pcm_complete_prepare() re-acquires the stream lock to
call ops->prepare_complete().

CPU0                              CPU1
snd_pcm_prepare()
  ...
  snd_pcm_post_prepare(s)
    snd_pcm_set_state(s, PREPARED)
      lock(s)
      __snd_pcm_set_state(s, PREPARED)
      unlock(s)
                                  snd_pcm_common_ioctl()
                                    snd_pcm_sync_ptr(s)
                                      lock(s)
                                      pcm_lib_apply_appl_ptr(s)
                                        s->ops->ack(s)  <-- rejected
  snd_pcm_complete_prepare(s, true)
    lock(s)
    s->ops->prepare_complete(s, true)
    unlock(s)

Because the driver still has admission closed until prepare_complete()
runs, concurrent SNDRV_PCM_IOCTL_SYNC_PTR or SNDRV_PCM_IOCTL_START calls
seeing SNDRV_PCM_STATE_PREPARED can fail with -EBADFD.  In addition, if a
concurrent snd_pcm_drop(), snd_pcm_drain(), or snd_pcm_suspend_all()
changes runtime->state during the unlocked window,
snd_pcm_complete_prepare() still calls prepare_complete(substream, true)
on success, or overwrites SNDRV_PCM_STATE_SUSPENDED with
SNDRV_PCM_STATE_SETUP on failure.

[ ... ]

> @@ -2099,10 +2118,29 @@ static void snd_pcm_post_prepare(struct snd_pcm_substream *substream,
>  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_PREPARED);
>  }
>  
> +static void snd_pcm_complete_prepare(struct snd_pcm_substream *substream,
> +				     snd_pcm_state_t state, bool success)
> +{
> +	if (!substream->ops->prepare_complete)
> +		return;
> +
> +	guard(pcm_stream_lock_irq)(substream);
> +	if (!success && substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED) {
> +		if (substream->runtime->state != SNDRV_PCM_STATE_SETUP) {
> +			__snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_SETUP);
> +			snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP);
> +		}

[Severity: Medium]
Does snd_pcm_complete_prepare() emit a spurious SNDRV_TIMER_EVENT_MSTOP
notification when prepare fails?

In snd_pcm_prepare(), PAUSED and SUSPENDED streams are already stopped
via snd_pcm_stop() before snd_pcm_action_prepare runs, and
snd_pcm_pre_prepare() rejects RUNNING and playback DRAINING streams with
-EBUSY.  Any substream reaching this path with runtime->state !=
SNDRV_PCM_STATE_SETUP (such as a re-prepare from PREPARED, XRUN, or
capture DRAINING) does not have an active master timer, so calling
snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP) delivers a
master-stop event with a zero or stale runtime->trigger_tstamp to attached
slave timers.

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