Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()
Date: 30 Jul 2020 10:31:35 +0900
Message-ID: <877dulbxim.wl-kuninori.morimoto.gx@renesas.com> (raw)
In-Reply-To: <3ec084ee58b6e3490c668295370b58a9eeb986e8.camel@linux.intel.com>


Hi Ranjani

Thank you for your review

> >  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?

	for_each_rtd_dais(...) {
		ret = snd_soc_dai_startup(dai, substream);
		if (ret < 0)
			goto config_err;
		...
	}
	...
config_err:
        for_each_rtd_dais(rtd, i, dai)
		snd_soc_dai_shutdown(dai, substream, 1);

For example, if rtd had 10 DAIs, and .startup() failed at 3rd DAI,
the mark will be
     
	/* xxx here means unknown */
	dai[0].mark_startup = substream;
	dai[1].mark_startup = substream;
	dai[2].mark_startup = substream;
	dai[3].mark_startup = xxx;
	...
	dai[7].mark_startup = xxx;
	dai[8].mark_startup = xxx;
	dai[9].mark_startup = xxx;

Then, we need to call .shutdown() is for dai[0] - dai[2],
In such case, .shutdown() will be called with rollback flag.
if's second check will be failed on dai[3] - dai[9].
But please double check.

> 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.

Yes, my v1 patch was that.
But someone (I don't remember who was he) indicated that
it is not enough if same DAI was called many times.

In my understanding, for example, if same DAI is used for 2xPlaybacks,
and 1st Playback was successed, 2nd Playback was failed,
and if it was using bool instead of pointer,

2nd Playback rollback doesn't need to call shutdown,
but it has successed bit of 1st Playback.
Then, 2nd Playback rollback will call unneeded shutdown,
and 1st Playback's necessary shutdown will not be called.

We can avoid such things if we use pointer instead of bool.
But please double check.

> 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. 

Yes maybe.
The reason why I used push/pop was that it might be updated to handle
pointer list instead of single pointer (and maybe Pierre-Louis want it :).
I think I indicated it on git log and comment.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

  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
2020-07-30  1:31     ` Kuninori Morimoto [this message]
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=877dulbxim.wl-kuninori.morimoto.gx@renesas.com \
    --to=kuninori.morimoto.gx@renesas.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=ranjani.sridharan@linux.intel.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