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

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	Mark Brown <broonie@kernel.org>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>
Subject: Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()
Date: Tue, 28 Jul 2020 12:27:14 -0700	[thread overview]
Message-ID: <3ec084ee58b6e3490c668295370b58a9eeb986e8.camel@linux.intel.com> (raw)
In-Reply-To: <87v9i8ku04.wl-kuninori.morimoto.gx@renesas.com>

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