Re: [PATCH] ASoC: samsung: i2s: release the runtime PM reference if config_setup fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: samsung: i2s: double pm_runtime_put() on failed trigger start in i2s_trigger()
--

commit 1c9f9f4fb51c6368b6950d91e87f392b539a7a79
Author: Wentao Liang <vulab@iscas.ac.cn>

ASoC: samsung: i2s: release the runtime PM reference if config_setup fails

This commit releases the runtime PM reference on dai->dev in i2s_trigger()
when config_setup() fails during a start, resume, or pause-release trigger.

> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 442e70b107dac..d2060c3ff6e8b 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -924,6 +924,7 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  	int capture = (substream->stream == SNDRV_PCM_STREAM_CAPTURE);
>  	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>  	struct i2s_dai *i2s = to_info(snd_soc_rtd_to_cpu(rtd, 0));
> +	bool failed = false;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> @@ -935,8 +936,10 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  			priv->fixup_early(substream, dai);
>  
>  		scoped_guard(spinlock_irqsave, &priv->lock) {
> -			if (config_setup(i2s))
> -				return -EINVAL;
> +			if (config_setup(i2s)) {
> +				failed = true;
> +				break;
> +			}
>  
>  			if (priv->fixup_late)
>  				priv->fixup_late(substream, dai);
> @@ -946,6 +949,11 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  			else
>  				i2s_txctrl(i2s, 1);
>  		}
> +
> +		if (failed) {
> +			pm_runtime_put(dai->dev);
> +			return -EINVAL;
> +		}

[Severity: High]
Does dropping the runtime PM reference here cause a double
pm_runtime_put(dai->dev) when config_setup(i2s) fails during
SNDRV_PCM_TRIGGER_START or SNDRV_PCM_TRIGGER_PAUSE_RELEASE on a non-linked,
non-DPCM substream?

When snd_pcm_start() or snd_pcm_pause() runs on a single substream,
snd_pcm_action_single() sets substream->runtime->trigger_master = substream
in pre_action, then calls do_action which invokes:

  soc_pcm_trigger()
    -> snd_soc_pcm_dai_trigger(substream, cmd, 0)
      -> soc_dai_trigger()
        -> i2s_trigger()

If config_setup(i2s) fails, this patch calls pm_runtime_put(dai->dev) and
returns -EINVAL. After soc_pcm_trigger() returns that error to
snd_pcm_action_single(), the ALSA PCM core invokes ops->undo_action
(snd_pcm_undo_start() or snd_pcm_undo_pause()):

sound/core/pcm_native.c:snd_pcm_action_single() {
	res = ops->do_action(substream, state);
	if (res == 0)
		ops->post_action(substream, state);
	else if (ops->undo_action)
		ops->undo_action(substream, state);
}

sound/core/pcm_native.c:snd_pcm_undo_start() {
	if (substream->runtime->trigger_master == substream) {
		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
		substream->runtime->stop_operating = true;
	}
}

Because snd_pcm_undo_start() and snd_pcm_undo_pause() invoke
soc_pcm_trigger() as a fresh SNDRV_PCM_TRIGGER_STOP or
SNDRV_PCM_TRIGGER_PAUSE_PUSH call with rollback == 0,
snd_soc_pcm_dai_trigger() does not check soc_dai_mark_match():

sound/soc/soc-dai.c:snd_soc_pcm_dai_trigger() {
	...
	for_each_rtd_dais(rtd, i, dai) {
		if (rollback && !soc_dai_mark_match(dai, substream, trigger))
			continue;
		...
		r = soc_dai_trigger(dai, substream, cmd);
}

This enters the STOP / PAUSE_PUSH branch of i2s_trigger(), which does not
check whether the stream was actually started and unconditionally calls
pm_runtime_put(dai->dev) a second time:

sound/soc/samsung/i2s.c:i2s_trigger() {
	...
	case SNDRV_PCM_TRIGGER_STOP:
	case SNDRV_PCM_TRIGGER_SUSPEND:
	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
		scoped_guard(spinlock_irqsave, &priv->lock) {
			...
		}
		pm_runtime_put(dai->dev);
		break;
}

Wouldn't each failed start or pause-release trigger therefore perform one
pm_runtime_get_sync(dai->dev) and two pm_runtime_put(dai->dev) calls,
consuming the runtime PM reference held while the substream is open and
underflowing dev->power.usage_count on close?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104024050.1508669-1-vulab@iscas.ac.cn?part=1