Re: [RFC] soc_pcm_open: error path behavior change since v5.6

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2020-09-03 3:16 PM, Mark Brown wrote:
> On Thu, Sep 03, 2020 at 10:31:35AM +0200, Cezary Rojewski wrote:
> 
>> Some time ago negative-tests found out that behavior of soc_pcm_open has
>> changed, quite sure this might be a regression hence my email. Till v5.6
>> soc_pcm_open was invoking ::shutdown() for cpu_dai in error path only if
>> ::startup() succeeded first (label: 'out'). After addition of commit:
> 
> Please don't invent new notation that nobody else uses, it just makes
> your messages harder to read.
> 
>> Should dai's ::shutdown() be introducing some kind of state-check from now
>> on? - similarly to how developers deal with some of the core pcm operations
>> e.g.: ::prepare() (as it may get invoked multiple times in a row so check is
>> there to prevent redundancy).
> 
> If there are stateful things it's probably better to do that from a
> robustness point of view whatever is going on.
> 
>> Or, perhaps behavior change should be reverted with ::shutdown() routine
>> again being called only after successful ::startup()?
> 
> IIRC part of the thinking there was that we were getting the keeping
> track part of things wrong and sometimes missing things that should be
> being shut down in error paths.  Anything that tries to stop extra calls
> would need to be very clearly robust and easily maintainable.
> 

I'm sorry if my explanation was somewhat lackluster. In fact, thread's 
name is misleading too -> regression sincec v5.7, not v5.6. Comparison 
of code pieces found below should make it clearer:

v5.6 soc_pcm_open:
https://elixir.bootlin.com/linux/v5.6.19/source/sound/soc/soc-pcm.c#L534

static int soc_pcm_open(struct snd_pcm_substream *substream)
{

(...)

	/* startup the audio subsystem */
	ret = snd_soc_dai_startup(cpu_dai, substream);
	if (ret < 0) {
		dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n",
			cpu_dai->name, ret);
		goto out;
	}

	ret = soc_pcm_components_open(substream, &component);
	if (ret < 0)
		goto component_err;

	for_each_rtd_codec_dai(rtd, i, codec_dai) {
		ret = snd_soc_dai_startup(codec_dai, substream);
		if (ret < 0) {
			dev_err(codec_dai->dev,
				"ASoC: can't open codec %s: %d\n",
				codec_dai->name, ret);
			goto codec_dai_err;
		}

(...)

codec_dai_err:
	for_each_rtd_codec_dai_rollback(rtd, i, codec_dai)
		snd_soc_dai_shutdown(codec_dai, substream);

component_err:
	soc_pcm_components_close(substream, component);

	snd_soc_dai_shutdown(cpu_dai, substream);
out:
	mutex_unlock(&rtd->card->pcm_mutex);


-

Now the equivalent from newer kernel e.g. v5.8:
https://elixir.bootlin.com/linux/v5.8.6/source/sound/soc/soc-pcm.c#L711

static int soc_pcm_open(struct snd_pcm_substream *substream)
{

(...)

	/* startup the audio subsystem */
	for_each_rtd_dais(rtd, i, dai) {
		ret = snd_soc_dai_startup(dai, substream);
		if (ret < 0) {
			dev_err(dai->dev,
				"ASoC: can't open DAI %s: %d\n",
				dai->name, ret);
			goto config_err;
		}

		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
			dai->tx_mask = 0;
		else
			dai->rx_mask = 0;
	}

(...)

config_err:
	for_each_rtd_dais(rtd, i, dai)
		snd_soc_dai_shutdown(dai, substream);

-

Let's assume we have 10 dais. In newer kernels, if snd_soc_dai_startup() 
fails at i=5, error path will attempt to perform snd_soc_dai_shutdown() 
for all dais (all 10) regardless if respective dai was opened or not. 
This is a clear behavior change when compared to v5.6 where cpu_dai was 
cleaned-up only if it was previously started successfully. Due to usage 
of for_each_rtd_codec_dai_rollback macro, the same applies to codec_dais.

Czarek