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

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

[Cezary Rojewski]

Hello,

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:

	5d9fa03e6c3514266fa94851ab1b6dd6e0595a13
	ASoC: soc-pcm: tidyup soc_pcm_open() order

this is no longer true. ::shutdown() can and will be invoked for given 
cpu_dai despite ::startup()'s failure. This complicated further since 
the merging of cpu & codec dais.

The same applies to codec_dais: notice the usage of 
for_each_rtd_codec_dai_rollback macro instead of for_each_rtd_dais in 
error path in v5.6.

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).
Or, perhaps behavior change should be reverted with ::shutdown() routine 
again being called only after successful ::startup()?

Czarek

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Cezary Rojewski]

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

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

[Kuninori Morimoto]

Hi Cezary

> 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.

Oh, yes. We need _rollback() for it.
I will try to fix it

Thank you for your help !!

Best regards
---
Kuninori Morimoto

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

[Kuninori Morimoto]

Hi Cezary

> 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:
> 
> 	5d9fa03e6c3514266fa94851ab1b6dd6e0595a13
> 	ASoC: soc-pcm: tidyup soc_pcm_open() order
> 
> this is no longer true. ::shutdown() can and will be invoked for given
> cpu_dai despite ::startup()'s failure. This complicated further since
> the merging of cpu & codec dais.
> 
> The same applies to codec_dais: notice the usage of
> for_each_rtd_codec_dai_rollback macro instead of for_each_rtd_dais in
> error path in v5.6.
> 
> 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).
> Or, perhaps behavior change should be reverted with ::shutdown()
> routine again being called only after successful ::startup()?

I'm sorry but I couldn't 100% understand your opinion.
But I understand that it is related to rollback order.
Now I'm posting patch for it. It is not yet 100% but 1st step.
Does it help for you ?

https://lore.kernel.org/r/87wo1kvozz.wl-kuninori.morimoto.gx@renesas.com

Thank you for your help !!

Best regards
---
Kuninori Morimoto

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 863 bytes --]

On Fri, Sep 04, 2020 at 09:01:03AM +0900, Kuninori Morimoto wrote:

> > 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).
> > Or, perhaps behavior change should be reverted with ::shutdown()
> > routine again being called only after successful ::startup()?

> I'm sorry but I couldn't 100% understand your opinion.
> But I understand that it is related to rollback order.
> Now I'm posting patch for it. It is not yet 100% but 1st step.
> Does it help for you ?

> https://lore.kernel.org/r/87wo1kvozz.wl-kuninori.morimoto.gx@renesas.com

It's not just the ordering, it's more the fact that we will clean up
things which failed to initialize.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]