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

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	[thread overview]
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