[PATCH] ALSA: hda - Autosuspend controller after probe even if codecs are already suspended

[PATCH] ALSA: hda - Autosuspend controller after probe even if codecs are already suspended

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

azx_probe_continue() uses pm_runtime_put_noidle() to drop the rpm
usage_count, which means that if it's the last reference the
autosuspend of the controller won't actually happen. So if the codecs
autosuspend before the azx_probe_continue() drops the last
reference we'll fail to autosuspend the controller. This does happen
in practice, but not every time. As can be seen in [1] the controller
autosuspend attempt fails due to the usage_count when suspending the
codecs. A bit later we see the the contoller usage_count dropping to
zero without further attempts at autosuspend.

Fix the problem by using pm_runtime_put_autosuspend() instead, which
will kick off the autosuspend of the controller even if the codecs
are already asleep. As can be seen in [2] the controller autosuspend
still fails while suspending the codecs, but later on we see another
autosuspend attempt after dropping the usage_count to 0.

I was also a bit worried that there might still be a race between the
controller autosuspend and the rest of the code in azx_probe_continue().
So I also tried replacing the the put_noidle() with put_sync_suspend().
No explosions occurred, so I'm somewhat satisfied that there are no
serious problems in this area.

[1]
 kworker/1:2-122   [001] ....    63.661310: __pm_runtime_suspend: hdaudioC0D0 usage_count 0
 kworker/1:2-122   [001] d..2    63.661316: rpm_suspend: hdaudioC0D0 flags-d cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/1:2-122   [001] d..1    63.661317: rpm_check_suspend_allowed: hdaudioC0D0 retval 0
 kworker/1:2-122   [001] d..2    63.661332: rpm_return_int: rpm_suspend+0x406/0x5e8:hdaudioC0D0 ret=0
 kworker/1:1-72    [001] d..2    63.661543: rpm_suspend: hdaudioC0D0 flags-a cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/1:1-72    [001] d..1    63.661544: rpm_check_suspend_allowed: hdaudioC0D0 retval 0
 kworker/1:1-72    [001] ....    63.661545: hda_codec_runtime_suspend: hdaudioC0D0 suspend
 kworker/1:1-72    [001] d..2    63.661614: rpm_idle: 0000:00:03.0 flags-1 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
 kworker/1:1-72    [001] d..1    63.661615: rpm_check_suspend_allowed: 0000:00:03.0 usage_count 1
 kworker/1:1-72    [001] d..1    63.661615: rpm_check_suspend_allowed: 0000:00:03.0 retval -11
 kworker/1:1-72    [001] d..2    63.661616: rpm_return_int: rpm_idle+0x249/0x487:0000:00:03.0 ret=-11
 kworker/1:1-72    [001] d..2    63.661616: rpm_return_int: rpm_suspend+0x406/0x5e8:hdaudioC0D0 ret=0
 kworker/1:2-122   [001] d..2    63.664834: rpm_idle: hdaudioC0D0 flags-8 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/1:2-122   [001] d..1    63.664835: rpm_check_suspend_allowed: hdaudioC0D0 retval 1
 kworker/1:2-122   [001] d..2    63.664836: rpm_return_int: rpm_idle+0x249/0x487:hdaudioC0D0 ret=-11
 kworker/1:2-122   [001] d..2    63.664841: rpm_idle: hdaudioC0D0 flags-8 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/1:2-122   [001] d..1    63.664841: rpm_check_suspend_allowed: hdaudioC0D0 retval 1
 kworker/1:2-122   [001] d..2    63.664841: rpm_return_int: rpm_idle+0x249/0x487:hdaudioC0D0 ret=-11
 kworker/1:2-122   [001] ....    63.664842: azx_probe_continue: 0000:00:03.0 usage_count=0

[2]
 kworker/0:0-4     [000] ....    50.354567: __pm_runtime_suspend: hdaudioC0D0 usage_count 0
 kworker/0:0-4     [000] d..2    50.354574: rpm_suspend: hdaudioC0D0 flags-d cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:0-4     [000] d..1    50.354575: rpm_check_suspend_allowed: hdaudioC0D0 retval 0
 kworker/0:0-4     [000] d..2    50.354589: rpm_return_int: rpm_suspend+0x406/0x5e8:hdaudioC0D0 ret=0
 kworker/0:2-135   [000] d..2    50.354809: rpm_suspend: hdaudioC0D0 flags-a cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:2-135   [000] d..1    50.354810: rpm_check_suspend_allowed: hdaudioC0D0 retval 0
 kworker/0:2-135   [000] ....    50.354816: hda_codec_runtime_suspend: hdaudioC0D0 suspend
 kworker/0:2-135   [000] d..2    50.354908: rpm_idle: 0000:00:03.0 flags-1 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:2-135   [000] d..1    50.354909: rpm_check_suspend_allowed: 0000:00:03.0 usage_count 1
 kworker/0:2-135   [000] d..1    50.354909: rpm_check_suspend_allowed: 0000:00:03.0 retval -11
 kworker/0:2-135   [000] d..2    50.354909: rpm_return_int: rpm_idle+0x249/0x487:0000:00:03.0 ret=-11
 kworker/0:2-135   [000] d..2    50.354910: rpm_return_int: rpm_suspend+0x406/0x5e8:hdaudioC0D0 ret=0
 kworker/0:0-4     [000] d..2    50.373791: rpm_idle: hdaudioC0D0 flags-8 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:0-4     [000] d..1    50.373792: rpm_check_suspend_allowed: hdaudioC0D0 retval 1
 kworker/0:0-4     [000] d..2    50.373793: rpm_return_int: rpm_idle+0x249/0x487:hdaudioC0D0 ret=-11
 kworker/0:0-4     [000] d..2    50.373797: rpm_idle: hdaudioC0D0 flags-8 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:0-4     [000] d..1    50.373798: rpm_check_suspend_allowed: hdaudioC0D0 retval 1
 kworker/0:0-4     [000] d..2    50.373798: rpm_return_int: rpm_idle+0x249/0x487:hdaudioC0D0 ret=-11
 kworker/0:0-4     [000] ....    50.373799: __pm_runtime_suspend: 0000:00:03.0 usage_count 0
 kworker/0:0-4     [000] d..2    50.373800: rpm_suspend: 0000:00:03.0 flags-d cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:0-4     [000] d..1    50.373800: rpm_check_suspend_allowed: 0000:00:03.0 retval 0
 kworker/0:0-4     [000] d..2    50.373803: rpm_return_int: rpm_suspend+0x406/0x5e8:0000:00:03.0 ret=0
 kworker/0:0-4     [000] d..2    50.385164: rpm_suspend: 0000:00:03.0 flags-a cnt-0  dep-0  auto-1 p-0 irq-0 child-0
 kworker/0:0-4     [000] d..1    50.385165: rpm_check_suspend_allowed: 0000:00:03.0 retval 0
 kworker/0:0-4     [000] ....    50.385174: azx_runtime_suspend: 0000:00:03.0 azx suspend releaseing power well
 kworker/0:0-4     [000] ....    50.385179: azx_runtime_suspend: 0000:00:03.0 azx suspend
 kworker/0:0-4     [000] d..2    50.386872: rpm_return_int: rpm_suspend+0x406/0x5e8:0000:00:03.0 ret=0

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 e5240cb3749f..2624cfe98884 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2145,7 +2145,7 @@ static int azx_probe_continue(struct azx *chip)
 	azx_add_card_list(chip);
 	snd_hda_set_power_save(&chip->bus, power_save * 1000);
 	if (azx_has_pm_runtime(chip) || hda->use_vga_switcheroo)
-		pm_runtime_put_noidle(&pci->dev);
+		pm_runtime_put_autosuspend(&pci->dev);
 
 out_free:
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
-- 
2.4.10

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

Re: [PATCH] ALSA: hda - Autosuspend controller after probe even if codecs are already suspended

[Takashi Iwai]

On Fri, 26 Feb 2016 18:39:57 +0100,
ville.syrjala@linux.intel.com wrote:
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> azx_probe_continue() uses pm_runtime_put_noidle() to drop the rpm
> usage_count, which means that if it's the last reference the
> autosuspend of the controller won't actually happen. So if the codecs
> autosuspend before the azx_probe_continue() drops the last
> reference we'll fail to autosuspend the controller. This does happen
> in practice, but not every time. As can be seen in [1] the controller
> autosuspend attempt fails due to the usage_count when suspending the
> codecs. A bit later we see the the contoller usage_count dropping to
> zero without further attempts at autosuspend.
> 
> Fix the problem by using pm_runtime_put_autosuspend() instead, which
> will kick off the autosuspend of the controller even if the codecs
> are already asleep. As can be seen in [2] the controller autosuspend
> still fails while suspending the codecs, but later on we see another
> autosuspend attempt after dropping the usage_count to 0.
> 
> I was also a bit worried that there might still be a race between the
> controller autosuspend and the rest of the code in azx_probe_continue().
> So I also tried replacing the the put_noidle() with put_sync_suspend().
> No explosions occurred, so I'm somewhat satisfied that there are no
> serious problems in this area.

A good catch!

The change should be safe, as this is the point where already all
initialization has been done.

I queued it to for-next branch, since this is no super-urgent issue to
be fixed in 4.5.


thanks,

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