[alsa-devel] [PROBLEM] ALSA: hda: PCM streams are suspended while opening

[Takashi Iwai]

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 <tiwai at suse.de>
> > 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 <jonathanh at nvidia.com>
> > Cc: <stable at vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  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 <jonathanh at nvidia.com>

OK, I'm going to queue the fix now.  Thanks for quick testing!


Takashi