[RFC] soc_pcm_open: error path behavior change since v5.6
Cezary Rojewski
cezary.rojewski at intel.com
Fri Sep 4 11:35:30 CEST 2020
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
More information about the Alsa-devel
mailing list