On Tue, 2020-07-28 at 15:57 +0900, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
