Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@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 11:16:53 +0900
Message-ID: <875za5bvf4.wl-kuninori.morimoto.gx@renesas.com> (raw)
In-Reply-To: <5d7e4b05-4900-f276-b7d2-ac7c01ad730b@linux.intel.com>


Hi Pierre-Louis

Thank you for your review.

> > +#define soc_dai_mark_push(dai, substream, tgt)	((dai)->mark_##tgt = substream)
> 
> we may want a check that detects if the pointer is NULL before
> assigning it, otherwise we won't be able to detect bad configuration
> where a pointer is overwritten by 2 mark_push() calls on the same
> object?

One assumption here is that open() / close() pair are called same number of times.
open() / close() unbalance is not mentioned here, it is other problem.

My expectation for this mark is that it will be used only for rollback.
The necessary things in such case is that marked pointer is match to
current pointer, or not when rollback case.
Thus, overwritten is not problem in my understanding.

> I am a bit concerned here about the case of a bi-directional DAI, it's
> my understanding that the .startup() callback could be called for each
> direction?
> 
> soc-dapm.c:		ret = snd_soc_dai_startup(source, substream);
> soc-dapm.c:		ret = snd_soc_dai_startup(sink, substream);
> 
> To convince myself of this, I added a dummy startup routine and I do
> see it called when I do playback and capture at the same time:
> 
> [  179.057494] plb: ssp2 startup stream 0
> [  183.976963] plb: ssp2 startup stream 1
> 
> That makes me nervous about having a single pointer and unbalanced
> calls between startup and shutdown.
> 
> We had such issues in the past so I may be on the paranoid side here...

Thank you for sharing your experience.
As I mentioned above, this mark is used only for rollback of open(),
not related to close().

But hmm.. I now double checked the code, 1 concern is mutex_lock().
In final step, the soc_pcm_open() will be

	static int soc_pcm_open()
	{
		...
(1)		mutex_lock_nested(...);
		...
		for_each_rtd_dais(rtd, i, dai) {
(A)			ret = snd_soc_dai_startup(dai, substream);
			...
		}
		...
	err:
(2)		mutex_unlock(&rtd->card->pcm_mutex);

		if (ret < 0)
(B)			soc_pcm_clean(substream, 1);
		...
	}

(A) is called under (1) lock, but it will be unlocked at (2),
and (B) is called if rollback.

If
	- 2 x soc_pcm_open() were called in the same time
	- 1st soc_pcm_open() was failed
	- 2nd soc_pcm_open() was called between 1st soc_pcm_open()'s (2) and (B)

Indeed single pointer is not good...
!?
But, soc_pcm_open() itself is called under othere mutex_lock() of pcm_native.c
Above issue never happen ?

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 [this message]
2020-07-28 19:27   ` Ranjani Sridharan
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=875za5bvf4.wl-kuninori.morimoto.gx@renesas.com \
    --to=kuninori.morimoto.gx@renesas.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=pierre-louis.bossart@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