| Message ID | 1438115396-1476-1-git-send-email-ullysses.a.eoff@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 28 Jul 2015 22:29:56 +0200, U. Artie Eoff wrote: > > PM ops could be triggered before HDA is done initializing > and cause PM to set HDA controller to D3Hot. This can result > in "CORB reset timeout#2, CORBRP = 65535" and "no codecs > initialized". Additionally, PM ops can be triggered before > azx_probe_continue finishes (async probe). This can result > in a NULL deref kernel crash. > > To fix this, avoid PM ops if !chip->running. > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > --- > sound/pci/hda/hda_intel.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 735bdcb04ce8..c38c68f57938 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -867,7 +867,7 @@ static int azx_suspend(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) This is superfluous, as azx_runtime_idle() returns -EBUSY. > return 0; > > bus = azx_bus(chip); > @@ -902,7 +902,7 @@ static int azx_resume(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) Ditto. Takashi > return 0; > > if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL > @@ -1027,7 +1027,7 @@ static int azx_runtime_idle(struct device *dev) > return 0; > > if (!power_save_controller || !azx_has_pm_runtime(chip) || > - azx_bus(chip)->codec_powered) > + azx_bus(chip)->codec_powered || !chip->running) > return -EBUSY; > > return 0; > -- > 2.1.0 >
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Wednesday, July 29, 2015 12:03 AM > To: Eoff, Ullysses A > Cc: alsa-devel@alsa-project.org; Lee, Zhuo-hao; Yang, Libin; dgreid@chromium.org > Subject: Re: [PATCH] ALSA: hda - Fix race between PM ops and HDA init/probe > > On Tue, 28 Jul 2015 22:29:56 +0200, > U. Artie Eoff wrote: > > > > PM ops could be triggered before HDA is done initializing > > and cause PM to set HDA controller to D3Hot. This can result > > in "CORB reset timeout#2, CORBRP = 65535" and "no codecs > > initialized". Additionally, PM ops can be triggered before > > azx_probe_continue finishes (async probe). This can result > > in a NULL deref kernel crash. > > > > To fix this, avoid PM ops if !chip->running. > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > --- > > sound/pci/hda/hda_intel.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > index 735bdcb04ce8..c38c68f57938 100644 > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -867,7 +867,7 @@ static int azx_suspend(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) > > This is superfluous, as azx_runtime_idle() returns -EBUSY. > This is the sleep suspend, not runtime. Runtime idle result does not influence sleep pm ops. > > return 0; > > > > bus = azx_bus(chip); > > @@ -902,7 +902,7 @@ static int azx_resume(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) > > Ditto. > Similar here... not runtime. > > Takashi > > > return 0; > > > > if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL > > @@ -1027,7 +1027,7 @@ static int azx_runtime_idle(struct device *dev) > > return 0; > > > > if (!power_save_controller || !azx_has_pm_runtime(chip) || > > - azx_bus(chip)->codec_powered) > > + azx_bus(chip)->codec_powered || !chip->running) > > return -EBUSY; > > > > return 0; > > -- > > 2.1.0 > >
On Wed, 29 Jul 2015 16:45:52 +0200, Eoff, Ullysses A wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Wednesday, July 29, 2015 12:03 AM > > To: Eoff, Ullysses A > > Cc: alsa-devel@alsa-project.org; Lee, Zhuo-hao; Yang, Libin; dgreid@chromium.org > > Subject: Re: [PATCH] ALSA: hda - Fix race between PM ops and HDA init/probe > > > > On Tue, 28 Jul 2015 22:29:56 +0200, > > U. Artie Eoff wrote: > > > > > > PM ops could be triggered before HDA is done initializing > > > and cause PM to set HDA controller to D3Hot. This can result > > > in "CORB reset timeout#2, CORBRP = 65535" and "no codecs > > > initialized". Additionally, PM ops can be triggered before > > > azx_probe_continue finishes (async probe). This can result > > > in a NULL deref kernel crash. > > > > > > To fix this, avoid PM ops if !chip->running. > > > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > > --- > > > sound/pci/hda/hda_intel.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > > index 735bdcb04ce8..c38c68f57938 100644 > > > --- a/sound/pci/hda/hda_intel.c > > > +++ b/sound/pci/hda/hda_intel.c > > > @@ -867,7 +867,7 @@ static int azx_suspend(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) > > > > This is superfluous, as azx_runtime_idle() returns -EBUSY. > > > > This is the sleep suspend, not runtime. Runtime idle result does not > influence sleep pm ops. Fair enough, I applied the patch now. thanks, Takashi > > > > return 0; > > > > > > bus = azx_bus(chip); > > > @@ -902,7 +902,7 @@ static int azx_resume(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) > > > > Ditto. > > > > Similar here... not runtime. > > > > > Takashi > > > > > return 0; > > > > > > if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL > > > @@ -1027,7 +1027,7 @@ static int azx_runtime_idle(struct device *dev) > > > return 0; > > > > > > if (!power_save_controller || !azx_has_pm_runtime(chip) || > > > - azx_bus(chip)->codec_powered) > > > + azx_bus(chip)->codec_powered || !chip->running) > > > return -EBUSY; > > > > > > return 0; > > > -- > > > 2.1.0 > > > >
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 735bdcb04ce8..c38c68f57938 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -867,7 +867,7 @@ static int azx_suspend(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; bus = azx_bus(chip); @@ -902,7 +902,7 @@ static int azx_resume(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 (chip->driver_caps & AZX_DCAPS_I915_POWERWELL @@ -1027,7 +1027,7 @@ static int azx_runtime_idle(struct device *dev) return 0; if (!power_save_controller || !azx_has_pm_runtime(chip) || - azx_bus(chip)->codec_powered) + azx_bus(chip)->codec_powered || !chip->running) return -EBUSY; return 0;
PM ops could be triggered before HDA is done initializing and cause PM to set HDA controller to D3Hot. This can result in "CORB reset timeout#2, CORBRP = 65535" and "no codecs initialized". Additionally, PM ops can be triggered before azx_probe_continue finishes (async probe). This can result in a NULL deref kernel crash. To fix this, avoid PM ops if !chip->running. Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> --- sound/pci/hda/hda_intel.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)