[PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[Takashi Iwai]

We got bug reports of the stalled HD-audio, typically after S3 or S4,
and it turned out that they seemed triggered by runtime PM on Lynx
Point and Lynx Point-LP controllers.  As there is no way to recover
properly from the stalled controller, it's safer to disable the
runtime PM support on these chips for now.

Further notes:
I actually could reproduce this on a few HP laptops here.
Go to S3 after runtime suspend, then the next playback fails,
resulting in either a codec stall or repeated sounds.

The problem seems lying in a deeper level.  The complete stall could
be avoided by disabling the call of azx_stop_chip() in
azx_runtime_suspend().  More specifically, it's the disablement of
CORB/RIRB in azx_free_cmd_io().  After removing this call, the sound
is resumed.

However, even with that workaround, the first playback after resume
stalls due to the missing RIRB interrupts (so you get "switch to
polling mode" kernel warning).  Interestingly, the codec communication
in the resume procedure does work.  The system goes to runtime suspend
immediately after resume, then something gets broken at that point.

This missing interrupt problem happens even if you do nothing in
runtime suspend/resume callback with empty callbacks.  This implies
that it's an issue in the underlying layer.  So, the only feasible
"fix" in the sound driver side to suppress the runtime PM, so far.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

Yet another note: the patch is based on v3.12, not on linux-next, so
that it can be backported cleanly for 3.12 and earlier kernels.

 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 6e61a019aa5e..27fc33e54a50 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3973,7 +3973,7 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = {
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
 	/* Lynx Point */
 	{ PCI_DEVICE(0x8086, 0x8c20),
-	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
 	/* Wellsburg */
 	{ PCI_DEVICE(0x8086, 0x8d20),
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
@@ -3981,10 +3981,10 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = {
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
 	/* Lynx Point-LP */
 	{ PCI_DEVICE(0x8086, 0x9c20),
-	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
 	/* Lynx Point-LP */
 	{ PCI_DEVICE(0x8086, 0x9c21),
-	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
 	/* Haswell */
 	{ PCI_DEVICE(0x8086, 0x0a0c),
 	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH |
-- 
1.8.4.3

Re: [PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

(Adding Mengdong to cc)

On 11/19/2013 05:51 PM, Takashi Iwai wrote:
> We got bug reports of the stalled HD-audio, typically after S3 or S4,
> and it turned out that they seemed triggered by runtime PM on Lynx
> Point and Lynx Point-LP controllers.  As there is no way to recover
> properly from the stalled controller, it's safer to disable the
> runtime PM support on these chips for now.

Oh, this is a bit sad news. Have you talked to Intel about it?

Anyway, I saw something similar a while ago, but never with access to
the hardware, and then it was difficult to reproduce for the person on
the other side. Nevertheless, when I read through the PM code I found
that the GCTL register was sometimes accessed with readb (although it is
a 32 bit register), so I wrote a patch for that, but the testing results
of this patch were a bit inconclusive, so I never upstreamed it.

Anyway, I'm attaching the draft patch. Do you think it could be related?

> 
> Further notes:
> I actually could reproduce this on a few HP laptops here.
> Go to S3 after runtime suspend, then the next playback fails,
> resulting in either a codec stall or repeated sounds.
> 
> The problem seems lying in a deeper level.  The complete stall could
> be avoided by disabling the call of azx_stop_chip() in
> azx_runtime_suspend().  More specifically, it's the disablement of
> CORB/RIRB in azx_free_cmd_io().  After removing this call, the sound
> is resumed.
> 
> However, even with that workaround, the first playback after resume
> stalls due to the missing RIRB interrupts (so you get "switch to
> polling mode" kernel warning).  Interestingly, the codec communication
> in the resume procedure does work.  The system goes to runtime suspend
> immediately after resume, then something gets broken at that point.
> 
> This missing interrupt problem happens even if you do nothing in
> runtime suspend/resume callback with empty callbacks.  This implies
> that it's an issue in the underlying layer.  So, the only feasible
> "fix" in the sound driver side to suppress the runtime PM, so far.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> 
> Yet another note: the patch is based on v3.12, not on linux-next, so
> that it can be backported cleanly for 3.12 and earlier kernels.
> 
>  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 6e61a019aa5e..27fc33e54a50 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -3973,7 +3973,7 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = {
>  	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
>  	/* Lynx Point */
>  	{ PCI_DEVICE(0x8086, 0x8c20),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
>  	/* Wellsburg */
>  	{ PCI_DEVICE(0x8086, 0x8d20),
>  	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
> @@ -3981,10 +3981,10 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = {
>  	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
>  	/* Lynx Point-LP */
>  	{ PCI_DEVICE(0x8086, 0x9c20),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
>  	/* Lynx Point-LP */
>  	{ PCI_DEVICE(0x8086, 0x9c21),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
>  	/* Haswell */
>  	{ PCI_DEVICE(0x8086, 0x0a0c),
>  	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH |
> 



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

[-- Attachment #2: gctl.patch --]
[-- Type: text/x-patch, Size: 1270 bytes --]

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index a0a06f7..3725020 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1138,7 +1138,7 @@ static void azx_enter_link_reset(struct azx *chip)
 	azx_writel(chip, GCTL, azx_readl(chip, GCTL) & ~ICH6_GCTL_RESET);
 
 	timeout = jiffies + msecs_to_jiffies(100);
-	while ((azx_readb(chip, GCTL) & ICH6_GCTL_RESET) &&
+	while ((azx_readl(chip, GCTL) & ICH6_GCTL_RESET) &&
 			time_before(jiffies, timeout))
 		usleep_range(500, 1000);
 }
@@ -1148,10 +1148,10 @@ static void azx_exit_link_reset(struct azx *chip)
 {
 	unsigned long timeout;
 
-	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
+	azx_writel(chip, GCTL, azx_readl(chip, GCTL) | ICH6_GCTL_RESET);
 
 	timeout = jiffies + msecs_to_jiffies(100);
-	while (!azx_readb(chip, GCTL) &&
+	while (!azx_readl(chip, GCTL) &&
 			time_before(jiffies, timeout))
 		usleep_range(500, 1000);
 }
@@ -1181,7 +1181,7 @@ static int azx_reset(struct azx *chip, int full_reset)
 
       __skip:
 	/* check to see if controller is ready */
-	if (!azx_readb(chip, GCTL)) {
+	if (!(azx_readl(chip, GCTL) & ICH6_GCTL_RESET)) {
 		snd_printd(SFX "%s: azx_reset: controller not ready!\n", pci_name(chip->pci));
 		return -EBUSY;
 	}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[Takashi Iwai]

At Wed, 20 Nov 2013 09:54:42 +0100,
David Henningsson wrote:
> 
> (Adding Mengdong to cc)
> 
> On 11/19/2013 05:51 PM, Takashi Iwai wrote:
> > We got bug reports of the stalled HD-audio, typically after S3 or S4,
> > and it turned out that they seemed triggered by runtime PM on Lynx
> > Point and Lynx Point-LP controllers.  As there is no way to recover
> > properly from the stalled controller, it's safer to disable the
> > runtime PM support on these chips for now.
> 
> Oh, this is a bit sad news. Have you talked to Intel about it?
> 
> Anyway, I saw something similar a while ago, but never with access to
> the hardware, and then it was difficult to reproduce for the person on
> the other side. Nevertheless, when I read through the PM code I found
> that the GCTL register was sometimes accessed with readb (although it is
> a 32 bit register), so I wrote a patch for that, but the testing results
> of this patch were a bit inconclusive, so I never upstreamed it.
> 
> Anyway, I'm attaching the draft patch. Do you think it could be related?

It didn't change the behavior although the change looks good.

After a long debugging session in this morning, I finally nailed
down.  This was the fault in the sound driver after all, shamefully :)

The fix patch is below.

The code needs a bit clean up, and I have it, but will apply this for
3.14.


Takashi

===

From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH 1/2] ALSA: hda - Fix unbalanced runtime PM notification at
 resume

When a codec is resumed, it keeps the power on while the resuming
phase via hda_keep_power_on(), then turns down via
snd_hda_power_down().  At that point, snd_hda_power_down() notifies
the power down to the controller, and this may confuse the refcount if
the codec was already powered up before the resume.

In the end result, the controller goes to runtime suspend even before
the codec is kicked off to the power save, and the communication
stalls happens.

The fix is to add the power-up notification together with
hda_keep_power_on(), and clears the flag appropriately.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index be60f5227b34..bada677df8a7 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4058,6 +4058,10 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 	 * in the resume / power-save sequence
 	 */
 	hda_keep_power_on(codec);
+	if (codec->pm_down_notified) {
+		codec->pm_down_notified = 0;
+		hda_call_pm_notify(codec->bus, true);
+	}
 	hda_set_power_state(codec, AC_PWRST_D0);
 	restore_shutup_pins(codec);
 	hda_exec_init_verbs(codec);
-- 
1.8.4.3

Re: [PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[Takashi Iwai]

At Wed, 20 Nov 2013 13:05:23 +0100,
Takashi Iwai wrote:
> 
> At Wed, 20 Nov 2013 09:54:42 +0100,
> David Henningsson wrote:
> > 
> > (Adding Mengdong to cc)
> > 
> > On 11/19/2013 05:51 PM, Takashi Iwai wrote:
> > > We got bug reports of the stalled HD-audio, typically after S3 or S4,
> > > and it turned out that they seemed triggered by runtime PM on Lynx
> > > Point and Lynx Point-LP controllers.  As there is no way to recover
> > > properly from the stalled controller, it's safer to disable the
> > > runtime PM support on these chips for now.
> > 
> > Oh, this is a bit sad news. Have you talked to Intel about it?
> > 
> > Anyway, I saw something similar a while ago, but never with access to
> > the hardware, and then it was difficult to reproduce for the person on
> > the other side. Nevertheless, when I read through the PM code I found
> > that the GCTL register was sometimes accessed with readb (although it is
> > a 32 bit register), so I wrote a patch for that, but the testing results
> > of this patch were a bit inconclusive, so I never upstreamed it.
> > 
> > Anyway, I'm attaching the draft patch. Do you think it could be related?
> 
> It didn't change the behavior although the change looks good.
> 
> After a long debugging session in this morning, I finally nailed
> down.  This was the fault in the sound driver after all, shamefully :)
> 
> The fix patch is below.
> 
> The code needs a bit clean up, and I have it, but will apply this for
> 3.14.
> 
> 
> Takashi
> 
> ===
> 
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH 1/2] ALSA: hda - Fix unbalanced runtime PM notification at
>  resume

BTW, it's marked as 1/2 just because of the clean up patch I mentioned
above.  Only this one is needed for now.


Takashi

Re: [PATCH] ALSA: hda - Disable runtime PM on LynxPoint(-LP) controllers

[David Henningsson]

On 11/20/2013 02:32 PM, Takashi Iwai wrote:
> At Wed, 20 Nov 2013 13:05:23 +0100,
> Takashi Iwai wrote:
>>
>> At Wed, 20 Nov 2013 09:54:42 +0100,
>> David Henningsson wrote:
>>>
>>> (Adding Mengdong to cc)
>>>
>>> On 11/19/2013 05:51 PM, Takashi Iwai wrote:
>>>> We got bug reports of the stalled HD-audio, typically after S3 or S4,
>>>> and it turned out that they seemed triggered by runtime PM on Lynx
>>>> Point and Lynx Point-LP controllers.  As there is no way to recover
>>>> properly from the stalled controller, it's safer to disable the
>>>> runtime PM support on these chips for now.
>>>
>>> Oh, this is a bit sad news. Have you talked to Intel about it?
>>>
>>> Anyway, I saw something similar a while ago, but never with access to
>>> the hardware, and then it was difficult to reproduce for the person on
>>> the other side. Nevertheless, when I read through the PM code I found
>>> that the GCTL register was sometimes accessed with readb (although it is
>>> a 32 bit register), so I wrote a patch for that, but the testing results
>>> of this patch were a bit inconclusive, so I never upstreamed it.
>>>
>>> Anyway, I'm attaching the draft patch. Do you think it could be related?
>>
>> It didn't change the behavior although the change looks good.

It looks good indeed, but it's always scary to do subtle changes without
testing on all the 50+ controllers we support...do you think I should
submit a proper patch for it?

>>
>> After a long debugging session in this morning, I finally nailed
>> down.  This was the fault in the sound driver after all, shamefully :)
>>
>> The fix patch is below.
>>
>> The code needs a bit clean up, and I have it, but will apply this for
>> 3.14.
>>
>>
>> Takashi
>>
>> ===
>>
>> From: Takashi Iwai <tiwai@suse.de>
>> Subject: [PATCH 1/2] ALSA: hda - Fix unbalanced runtime PM notification at
>>  resume
> 
> BTW, it's marked as 1/2 just because of the clean up patch I mentioned
> above.  Only this one is needed for now.

Ok, thanks for the clarification, and glad you finally found it! And so
are you I guess :-)


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic