[PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[U. Artie Eoff]

Crash can occur if runtime PM is triggered before the async probe
finishes via the azx_firmware_cb when CONFIG_SND_HDA_PATCH_LOADER
is defined.

Don't execute PM ops if the async probe has not completed yet.

The probe is finished when chip->running is true.

Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
---
 sound/pci/hda/hda_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 735bdcb04ce8..b729b25a6ad6 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device *dev)
 
 	chip = card->private_data;
 	hda = container_of(chip, struct hda_intel, chip);
-	if (chip->disabled || hda->init_failed)
+	if (chip->disabled || hda->init_failed || !chip->running)
 		return 0;
 
 	if (!power_save_controller || !azx_has_pm_runtime(chip) ||
-- 
1.9.3

[PATCH 2/2] ALSA: hda - Fix race condition between HDA driver and runtime PM

[U. Artie Eoff]

Don't execute runtime suspend if HDA is not finished initializing.
Otherwise, the following errors can occur in hda:

snd_hda_intel 0000:00:1b.0: CORB reset timeout#2, CORBRP = 65535
snd_hda_intel 0000:00:1b.0: no codecs initialized

Debugged and root-cause found by zhuo-hao.lee@intel.com

Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
---
 sound/pci/hda/hda_intel.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b729b25a6ad6..062f2400dbc7 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1017,17 +1017,20 @@ static int azx_runtime_idle(struct device *dev)
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
 	struct hda_intel *hda;
+	struct hdac_bus *bus;
 
 	if (!card)
 		return 0;
 
 	chip = card->private_data;
 	hda = container_of(chip, struct hda_intel, chip);
-	if (chip->disabled || hda->init_failed || !chip->running)
+	bus = azx_bus(chip);
+	if (chip->disabled || hda->init_failed ||
+		(bus->chip_init && !chip->running))
 		return 0;
 
 	if (!power_save_controller || !azx_has_pm_runtime(chip) ||
-	    azx_bus(chip)->codec_powered)
+	    bus->codec_powered || !bus->chip_init)
 		return -EBUSY;
 
 	return 0;
-- 
1.9.3

Re: [PATCH 2/2] ALSA: hda - Fix race condition between HDA driver and runtime PM

[Yang, Libin]

> -----Original Message-----
> From: Eoff, Ullysses A
> Sent: Tuesday, July 28, 2015 5:35 AM
> To: alsa-devel@alsa-project.org; tiwai@suse.de
> Cc: Lee, Zhuo-hao; Yang, Libin; Eoff, Ullysses A
> Subject: [PATCH 2/2] ALSA: hda - Fix race condition between HDA
> driver and runtime PM
> 
> Don't execute runtime suspend if HDA is not finished initializing.
> Otherwise, the following errors can occur in hda:
> 
> snd_hda_intel 0000:00:1b.0: CORB reset timeout#2, CORBRP = 65535
> snd_hda_intel 0000:00:1b.0: no codecs initialized
> 
> Debugged and root-cause found by zhuo-hao.lee@intel.com
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> ---
>  sound/pci/hda/hda_intel.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index b729b25a6ad6..062f2400dbc7 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1017,17 +1017,20 @@ static int azx_runtime_idle(struct device
> *dev)
>  	struct snd_card *card = dev_get_drvdata(dev);
>  	struct azx *chip;
>  	struct hda_intel *hda;
> +	struct hdac_bus *bus;
> 
>  	if (!card)
>  		return 0;
> 
>  	chip = card->private_data;
>  	hda = container_of(chip, struct hda_intel, chip);
> -	if (chip->disabled || hda->init_failed || !chip->running)
> +	bus = azx_bus(chip);
> +	if (chip->disabled || hda->init_failed ||
> +		(bus->chip_init && !chip->running))
>  		return 0;

If you only want not to execute runtime suspend
when HDA is not finished initializing, 
(bus->chip_init && !chip->running) is not necessary here.

> 
>  	if (!power_save_controller || !azx_has_pm_runtime(chip) ||
> -	    azx_bus(chip)->codec_powered)
> +	    bus->codec_powered || !bus->chip_init)
>  		return -EBUSY;
> 
>  	return 0;
> --
> 1.9.3

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Takashi Iwai]

On Mon, 27 Jul 2015 23:34:53 +0200,
U. Artie Eoff wrote:
> 
> Crash can occur if runtime PM is triggered before the async probe
> finishes via the azx_firmware_cb when CONFIG_SND_HDA_PATCH_LOADER
> is defined.
> 
> Don't execute PM ops if the async probe has not completed yet.
> 
> The probe is finished when chip->running is true.
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> ---
>  sound/pci/hda/hda_intel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 735bdcb04ce8..b729b25a6ad6 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device *dev)
>  
>  	chip = card->private_data;
>  	hda = container_of(chip, struct hda_intel, chip);
> -	if (chip->disabled || hda->init_failed)
> +	if (chip->disabled || hda->init_failed || !chip->running)
>  		return 0;

For !chip->running, it's better to return -EBUSY.

The disabled and init_failed flags are checked in runtime_suspend()
and runtime_resume() to skip the whole procedures, too.

And I guess the check of chip->running should suffice even without the
second patch.  It assures that the bus is powered on.


thanks,

Takashi

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Eoff, Ullysses A]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Monday, July 27, 2015 10:39 PM
> To: Eoff, Ullysses A
> Cc: Yang, Libin; alsa-devel@alsa-project.org; Lee, Zhuo-hao
> Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished
> 
> On Mon, 27 Jul 2015 23:34:53 +0200,
> U. Artie Eoff wrote:
> >
> > Crash can occur if runtime PM is triggered before the async probe
> > finishes via the azx_firmware_cb when CONFIG_SND_HDA_PATCH_LOADER
> > is defined.
> >
> > Don't execute PM ops if the async probe has not completed yet.
> >
> > The probe is finished when chip->running is true.
> >
> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > ---
> >  sound/pci/hda/hda_intel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 735bdcb04ce8..b729b25a6ad6 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device *dev)
> >
> >  	chip = card->private_data;
> >  	hda = container_of(chip, struct hda_intel, chip);
> > -	if (chip->disabled || hda->init_failed)
> > +	if (chip->disabled || hda->init_failed || !chip->running)
> >  		return 0;
> 
> For !chip->running, it's better to return -EBUSY.
> 
> The disabled and init_failed flags are checked in runtime_suspend()
> and runtime_resume() to skip the whole procedures, too.
> 
> And I guess the check of chip->running should suffice even without the
> second patch.  It assures that the bus is powered on.
>

Ok, I will test your suggested change to see if it addresses
both elusive race conditions and report back.  It sounds like
it'll be the right fix.  Thanks for the feedback.

U. Artie
 
> 
> thanks,
> 
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, July 28, 2015 1:39 PM
> To: Eoff, Ullysses A
> Cc: alsa-devel@alsa-project.org; Lee, Zhuo-hao; Yang, Libin
> Subject: Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is
> not finished
> 
> On Mon, 27 Jul 2015 23:34:53 +0200,
> U. Artie Eoff wrote:
> >
> > Crash can occur if runtime PM is triggered before the async probe
> > finishes via the azx_firmware_cb when
> CONFIG_SND_HDA_PATCH_LOADER
> > is defined.
> >
> > Don't execute PM ops if the async probe has not completed yet.
> >
> > The probe is finished when chip->running is true.
> >
> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > ---
> >  sound/pci/hda/hda_intel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 735bdcb04ce8..b729b25a6ad6 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device
> *dev)
> >
> >  	chip = card->private_data;
> >  	hda = container_of(chip, struct hda_intel, chip);
> > -	if (chip->disabled || hda->init_failed)
> > +	if (chip->disabled || hda->init_failed || !chip->running)
> >  		return 0;
> 
> For !chip->running, it's better to return -EBUSY.
> 
> The disabled and init_failed flags are checked in runtime_suspend()
> and runtime_resume() to skip the whole procedures, too.
> 
> And I guess the check of chip->running should suffice even without
> the
> second patch.  It assures that the bus is powered on.

As, in runtime_resume will call azx_init_chip(), which may sleep,
is there any race when calling azx_runtime_resume(), 
azx_runtime_idle is called and enter azx_runtime_suspend()?
Or the ACPI subsystem will handle such situation?

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Takashi Iwai]

On Tue, 28 Jul 2015 08:05:08 +0200,
Yang, Libin wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, July 28, 2015 1:39 PM
> > To: Eoff, Ullysses A
> > Cc: alsa-devel@alsa-project.org; Lee, Zhuo-hao; Yang, Libin
> > Subject: Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is
> > not finished
> > 
> > On Mon, 27 Jul 2015 23:34:53 +0200,
> > U. Artie Eoff wrote:
> > >
> > > Crash can occur if runtime PM is triggered before the async probe
> > > finishes via the azx_firmware_cb when
> > CONFIG_SND_HDA_PATCH_LOADER
> > > is defined.
> > >
> > > Don't execute PM ops if the async probe has not completed yet.
> > >
> > > The probe is finished when chip->running is true.
> > >
> > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > > ---
> > >  sound/pci/hda/hda_intel.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index 735bdcb04ce8..b729b25a6ad6 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device
> > *dev)
> > >
> > >  	chip = card->private_data;
> > >  	hda = container_of(chip, struct hda_intel, chip);
> > > -	if (chip->disabled || hda->init_failed)
> > > +	if (chip->disabled || hda->init_failed || !chip->running)
> > >  		return 0;
> > 
> > For !chip->running, it's better to return -EBUSY.
> > 
> > The disabled and init_failed flags are checked in runtime_suspend()
> > and runtime_resume() to skip the whole procedures, too.
> > 
> > And I guess the check of chip->running should suffice even without
> > the
> > second patch.  It assures that the bus is powered on.
> 
> As, in runtime_resume will call azx_init_chip(), which may sleep,
> is there any race when calling azx_runtime_resume(), 
> azx_runtime_idle is called and enter azx_runtime_suspend()?
> Or the ACPI subsystem will handle such situation?

The reentrace is avoided in the runtime PM core side, IIRC.


Takashi

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, July 28, 2015 2:13 PM
> To: Yang, Libin
> Cc: Eoff, Ullysses A; alsa-devel@alsa-project.org; Lee, Zhuo-hao
> Subject: Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is
> not finished
> 
> On Tue, 28 Jul 2015 08:05:08 +0200,
> Yang, Libin wrote:
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Tuesday, July 28, 2015 1:39 PM
> > > To: Eoff, Ullysses A
> > > Cc: alsa-devel@alsa-project.org; Lee, Zhuo-hao; Yang, Libin
> > > Subject: Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async
> probe is
> > > not finished
> > >
> > > On Mon, 27 Jul 2015 23:34:53 +0200,
> > > U. Artie Eoff wrote:
> > > >
> > > > Crash can occur if runtime PM is triggered before the async probe
> > > > finishes via the azx_firmware_cb when
> > > CONFIG_SND_HDA_PATCH_LOADER
> > > > is defined.
> > > >
> > > > Don't execute PM ops if the async probe has not completed yet.
> > > >
> > > > The probe is finished when chip->running is true.
> > > >
> > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > > > ---
> > > >  sound/pci/hda/hda_intel.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/pci/hda/hda_intel.c
> b/sound/pci/hda/hda_intel.c
> > > > index 735bdcb04ce8..b729b25a6ad6 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device
> > > *dev)
> > > >
> > > >  	chip = card->private_data;
> > > >  	hda = container_of(chip, struct hda_intel, chip);
> > > > -	if (chip->disabled || hda->init_failed)
> > > > +	if (chip->disabled || hda->init_failed || !chip->running)
> > > >  		return 0;
> > >
> > > For !chip->running, it's better to return -EBUSY.
> > >
> > > The disabled and init_failed flags are checked in runtime_suspend()
> > > and runtime_resume() to skip the whole procedures, too.
> > >
> > > And I guess the check of chip->running should suffice even without
> > > the
> > > second patch.  It assures that the bus is powered on.
> >
> > As, in runtime_resume will call azx_init_chip(), which may sleep,
> > is there any race when calling azx_runtime_resume(),
> > azx_runtime_idle is called and enter azx_runtime_suspend()?
> > Or the ACPI subsystem will handle such situation?
> 
> The reentrace is avoided in the runtime PM core side, IIRC.

Got it. Thanks.

> 
> 
> Takashi

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Eoff, Ullysses A]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Monday, July 27, 2015 10:39 PM
> To: Eoff, Ullysses A
> Cc: Yang, Libin; alsa-devel@alsa-project.org; Lee, Zhuo-hao
> Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished
> 
> On Mon, 27 Jul 2015 23:34:53 +0200,
> U. Artie Eoff wrote:
> >
> > Crash can occur if runtime PM is triggered before the async probe
> > finishes via the azx_firmware_cb when CONFIG_SND_HDA_PATCH_LOADER
> > is defined.
> >
> > Don't execute PM ops if the async probe has not completed yet.
> >
> > The probe is finished when chip->running is true.
> >
> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > ---
> >  sound/pci/hda/hda_intel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 735bdcb04ce8..b729b25a6ad6 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device *dev)
> >
> >  	chip = card->private_data;
> >  	hda = container_of(chip, struct hda_intel, chip);
> > -	if (chip->disabled || hda->init_failed)
> > +	if (chip->disabled || hda->init_failed || !chip->running)
> >  		return 0;
> 
> For !chip->running, it's better to return -EBUSY.
> 
> The disabled and init_failed flags are checked in runtime_suspend()
> and runtime_resume() to skip the whole procedures, too.
> 
> And I guess the check of chip->running should suffice even without the
> second patch.  It assures that the bus is powered on.
> 

Hmm... wait a second.  I think we still need to check bus->chip_init
as in the second patch.  I just noticed that the async probe 
finishes (i.e. chip->running = 1) before chip init executes (i.e. before
bus->chip_init is set).  This allows a small window for the issue
I described in my second patch to occur.

I already sent another patch to replace these two... but I didn't
add the check for bus->chip_init in that patch.  I'll create a v2
for that but will wait for your feedback first.

> 
> thanks,
> 
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished

[Eoff, Ullysses A]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Eoff, Ullysses A
> Sent: Tuesday, July 28, 2015 2:38 PM
> To: Takashi Iwai
> Cc: Yang, Libin; alsa-devel@alsa-project.org; Lee, Zhuo-hao
> Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished
> 
> 
> > -----Original Message-----
> > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > Sent: Monday, July 27, 2015 10:39 PM
> > To: Eoff, Ullysses A
> > Cc: Yang, Libin; alsa-devel@alsa-project.org; Lee, Zhuo-hao
> > Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: hda - skip runtime PM if async probe is not finished
> >
> > On Mon, 27 Jul 2015 23:34:53 +0200,
> > U. Artie Eoff wrote:
> > >
> > > Crash can occur if runtime PM is triggered before the async probe
> > > finishes via the azx_firmware_cb when CONFIG_SND_HDA_PATCH_LOADER
> > > is defined.
> > >
> > > Don't execute PM ops if the async probe has not completed yet.
> > >
> > > The probe is finished when chip->running is true.
> > >
> > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > > ---
> > >  sound/pci/hda/hda_intel.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index 735bdcb04ce8..b729b25a6ad6 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -1023,7 +1023,7 @@ static int azx_runtime_idle(struct device *dev)
> > >
> > >  	chip = card->private_data;
> > >  	hda = container_of(chip, struct hda_intel, chip);
> > > -	if (chip->disabled || hda->init_failed)
> > > +	if (chip->disabled || hda->init_failed || !chip->running)
> > >  		return 0;
> >
> > For !chip->running, it's better to return -EBUSY.
> >
> > The disabled and init_failed flags are checked in runtime_suspend()
> > and runtime_resume() to skip the whole procedures, too.
> >
> > And I guess the check of chip->running should suffice even without the
> > second patch.  It assures that the bus is powered on.
> >
> 
> Hmm... wait a second.  I think we still need to check bus->chip_init
> as in the second patch.  I just noticed that the async probe
> finishes (i.e. chip->running = 1) before chip init executes (i.e. before
> bus->chip_init is set).  This allows a small window for the issue
> I described in my second patch to occur.
> 
> I already sent another patch to replace these two... but I didn't
> add the check for bus->chip_init in that patch.  I'll create a v2
> for that but will wait for your feedback first.
> 

Oh nevermind... silly me!  I put prints in the wrong place when
testing this theory (at top of the probe cb).  Once I corrected that, I now
see that chip_init=1 happens during the probe cb before running=1
gets set.  I'm still quite new to this driver so bear with me ;-) 
Appologies for the noise.

THEREFORE, my latest patch should be good to go (pending
your feedback of course :-).

Cheers,
U. Artie

> >
> > thanks,
> >
> > Takashi
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel