Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

On Tue, 2020-07-28 at 15:57 +0900, Kuninori Morimoto wrote:

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

soc_pcm_open() does rollback when failed (A), but, it is almost same as soc_pcm_close().

static int soc_pcm_open(xxx) { ... if (ret < 0) goto xxx_err; ... return 0;

^ config_err: | ... | rtd_startup_err: (A) ... | component_err: | ... v return ret; }

The difference is soc_pcm_close() is for all dai/component/substream, rollback is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs, thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling => 1) snd_soc_dai_startup/shutdown() 2) snd_soc_link_startup/shutdown() 3) snd_soc_component_module_get/put() 4) snd_soc_component_open/close() 5) pm_runtime_put/get()

This patch is for 1) snd_soc_dai_startup/shutdown(), and adds new substream mark. It will mark substream when startup was suceeded. If rollback happen *after* that, it will check rollback flag and marked substream.

It cares *previous* startup() only now, but we might want to check *whole* marked substream in the future. This patch is using macro so that it can be easily adjust to it.

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

include/sound/soc-dai.h | 5 ++++- sound/soc/soc-dai.c | 21 ++++++++++++++++++++- sound/soc/soc-dapm.c | 4 ++-- sound/soc/soc-pcm.c | 4 ++-- 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 776a60529e70..86411f503c1c 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai, int snd_soc_dai_startup(struct snd_soc_dai *dai, struct snd_pcm_substream *substream); void snd_soc_dai_shutdown(struct snd_soc_dai *dai,

• 	  struct snd_pcm_substream *substream);
  

• 	  struct snd_pcm_substream *substream, int
  

rollback); snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai, struct snd_pcm_substream *substream); void snd_soc_dai_suspend(struct snd_soc_dai *dai); @@ -388,6 +388,9 @@ struct snd_soc_dai {

struct list_head list;

• /* function mark */
• struct snd_pcm_substream *mark_startup;
• /* bit field */ unsigned int probed:1;

}; diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 693893420bf0..4f9f73800ab0 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai *dai, return ret; }

+/*

• • We might want to check substream by using list.
• • In such case, we can update these macros.
• */

+#define soc_dai_mark_push(dai, substream, tgt) ((dai)-

...

mark_##tgt = substream)

+#define soc_dai_mark_pop(dai, substream, tgt) ((dai)-

...

mark_##tgt = NULL)

+#define soc_dai_mark_match(dai, substream, tgt) ((dai)-

...

mark_##tgt == substream)

/**

• snd_soc_dai_set_sysclk - configure DAI system or master clock.
• @dai: DAI

@@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai);

• /* mark substream if succeeded */
• if (ret == 0)

• soc_dai_mark_push(dai, substream, startup);
  

• return soc_dai_ret(dai, ret);

}

void snd_soc_dai_shutdown(struct snd_soc_dai *dai,

• 	 struct snd_pcm_substream *substream)
  

• 	  struct snd_pcm_substream *substream,
  

• 	  int rollback)
  

{

• if (rollback && !soc_dai_mark_match(dai, substream, startup))

• return;
  

Morimoto-san, Im having a hard time undersdtanding why we need the second check? When will it ever be the case that this match will fail?

I think if we want to roll-back in case of errors , we could simply have a bool in snd_soc_dai to indicate that the dai has been started up already and check that here to decide whether we should shut it down or not. Ideally, a helper function like snd_soc_dai_shutdown_all() which iterates through all the rtd dai's and shuts all of them that are marked as started up should suffice and will be more intuitive.

Also, push/pop are associated with stacks and we're only really dealing with one object here. So it is a bit misleading nomemclature-wise.

What do you think?

Thanks, Ranjani