From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PROBLEM] ALSA: hda: PCM streams are suspended while opening Date: Mon, 25 Mar 2019 16:36:55 +0100 Message-ID: References: <1553097854-10287-1-git-send-email-jonathanh@nvidia.com> <446faef9-a114-1b28-5636-a30bae04fa53@nvidia.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Jon Hunter Cc: linux-tegra@vger.kernel.org, alsa-devel@alsa-project.org, Sameer Pujar List-Id: linux-tegra@vger.kernel.org On Mon, 25 Mar 2019 16:31:26 +0100, Jon Hunter wrote: > > > On 25/03/2019 09:59, Takashi Iwai wrote: > > ... > > > I reconsidered the problem again, and noticed that the very same > > problem may appear with the system PM, not only with runtime PM. > > The suspend can happen at any time, so even a stream in OPEN state may > > go to suspend, and you'll hit the same problem. It's just a corner > > case so no one really cared much. > > > > So, I think the patch like below should fix the problem. > > This can be easily backported to all stable trees, and it alone should > > work without backporting the else intrusive changes. > > > > Could you check whether my theory is correct? > > > > > > thanks, > > > > Takashi > > > > --- > > From: Takashi Iwai > > Subject: [PATCH] ALSA: pcm: Don't suspend stream in unrecoverable PCM state > > > > Currently PCM core sets each opened stream forcibly to SUSPENDED state > > via snd_pcm_suspend_all() call, and the user-space is responsible for > > re-triggering the resume manually either via snd_pcm_resume() or > > prepare call. The scheme works fine usually, but there are corner > > cases where the stream can't be resumed by that call: the streams > > still in OPEN state before finishing hw_params. When they are > > suspended, user-space cannot perform resume or prepare because they > > haven't been set up yet. The only possible recovery is to re-open the > > device, which isn't nice at all. Similarly, when a stream is in > > DISCONNECTED state, it makes no sense to change it to SUSPENDED > > state. Ditto for in SETUP state; which you can re-prepare directly. > > > > So, this patch addresses these issues by filtering the PCM streams to > > be suspended by checking the PCM state. When a stream is in either > > OPEN, SETUP or DISCONNECTED as well as already SUSPENDED, the suspend > > action is skipped. > > > > To be noted, this problem was originally reported for the PCM runtime > > PM on HD-audio. And, the runtime PM problem itself was already > > addressed (although not intended) by the code refactoring commits > > 3d21ef0b49f8 ("ALSA: pcm: Suspend streams globally via device type PM > > ops") and 17bc4815de58 ("ALSA: pci: Remove superfluous > > snd_pcm_suspend*() calls"). These commits eliminated the > > snd_pcm_suspend*() calls from the runtime PM suspend callback code > > path, hence the racy OPEN state won't appear while runtime PM. > > (FWIW, the race window is between snd_pcm_open_substream() and the > > first power up in azx_pcm_open().) > > > > Although the runtime PM issue was already "fixed", the same problem is > > still present for the system PM, hence this patch is still needed. > > And for stable trees, this patch alone should suffice for fixing the > > runtime PM problem, too. > > > > Reported-by: Jonathan Hunter > > Cc: > > Signed-off-by: Takashi Iwai > > --- > > sound/core/pcm_native.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > > index f731f904e8cc..1d8452912b14 100644 > > --- a/sound/core/pcm_native.c > > +++ b/sound/core/pcm_native.c > > @@ -1445,8 +1445,15 @@ static int snd_pcm_pause(struct snd_pcm_substream *substream, int push) > > static int snd_pcm_pre_suspend(struct snd_pcm_substream *substream, int state) > > { > > struct snd_pcm_runtime *runtime = substream->runtime; > > - if (runtime->status->state == SNDRV_PCM_STATE_SUSPENDED) > > + switch (runtime->status->state) { > > + case SNDRV_PCM_STATE_SUSPENDED: > > return -EBUSY; > > + /* unresumable PCM state; return -EBUSY for skipping suspend */ > > + case SNDRV_PCM_STATE_OPEN: > > + case SNDRV_PCM_STATE_SETUP: > > + case SNDRV_PCM_STATE_DISCONNECTED: > > + return -EBUSY; > > + } > > runtime->trigger_master = substream; > > return 0; > > } > > Thanks, this works for me! I have tested this on stable branch > linux-5.0.y with Tegra194. So feel free to add my ... > > Tested-by: Jon Hunter OK, I'm going to queue the fix now. Thanks for quick testing! Takashi