Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
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
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


  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28  6:57 [PATCH 0/7] ASoC: merge soc_pcm_open() rollback and soc_pcm_close() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown() Kuninori Morimoto
2020-07-28 13:14   ` Pierre-Louis Bossart
2020-07-30  2:16     ` Kuninori Morimoto
2020-07-28 19:27   ` Ranjani Sridharan [this message]
2020-07-30  1:31     ` Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 2/7] ASoC: soc-link: add mark for snd_soc_link_startup/shutdown() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 3/7] ASoC: soc-component: add mark for soc_pcm_components_open/close() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 4/7] ASoC: soc-component: add mark for snd_soc_pcm_component_pm_runtime_get/put() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 5/7] ASoC: soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 6/7] ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_dai_startup() Kuninori Morimoto
2020-07-28  6:57 ` [PATCH 7/7] ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_component_module/open() Kuninori Morimoto

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3ec084ee58b6e3490c668295370b58a9eeb986e8.camel@linux.intel.com \
    --to=ranjani.sridharan@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=kuninori.morimoto.gx@renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git