ALSA: hda - Fix runtime PM
diff mbox

Message ID 41bbf8fd990e3247026beeb4564260b6777b1062.1527181037.git.lukas@wunner.de
State New
Headers show

Commit Message

Lukas Wunner May 24, 2018, 5:01 p.m. UTC
Before commit 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
to sync power state"), hda_set_power_state() returned the response to
the Get Power State verb, a 32-bit unsigned integer whose expected value
is 0x233 after transitioning a codec to D3, and 0x0 after transitioning
it to D0.

The response value is significant because hda_codec_runtime_suspend()
does not clear the codec's bit in the codec_powered bitmask unless the
AC_PWRST_CLK_STOP_OK bit (0x200) is set in the response value.  That in
turn prevents the HDA controller from runtime suspending because
azx_runtime_idle() checks that the codec_powered bitmask is zero.

Since commit 3b5b899ca67d, hda_set_power_state() only returns 0x0 or
0x1, thereby breaking runtime PM for any HDA controller.  That's because
an inline function introduced by the commit returns a bool instead of a
32-bit unsigned int.  The change was likely erroneous and resulted from
copying and pasting snd_hda_check_power_state(), which is immediately
preceding the newly introduced inline function.  Fix it.

Link: https://bugs.freedesktop.org/show_bug.cgi?id=106597
Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to sync power state")
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Abhijeet Kumar <abhijeet.kumar@intel.com>
Reported-and-tested-by: Gunnar Krüger <taijian@posteo.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 sound/pci/hda/hda_local.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Deucher, Alexander May 24, 2018, 5:12 p.m. UTC | #1
> -----Original Message-----

> From: Lukas Wunner [mailto:lukas@wunner.de]

> Sent: Thursday, May 24, 2018 1:01 PM

> To: Takashi Iwai <tiwai@suse.com>

> Cc: Jaroslav Kysela <perex@perex.cz>; Abhijeet Kumar

> <abhijeet.kumar@intel.com>; Deucher, Alexander

> <Alexander.Deucher@amd.com>; Gunnar Krueger <taijian@posteo.de>;

> alsa-devel@alsa-project.org

> Subject: [PATCH] ALSA: hda - Fix runtime PM

> 

> Before commit 3b5b899ca67d ("ALSA: hda: Make use of core codec functions

> to sync power state"), hda_set_power_state() returned the response to the

> Get Power State verb, a 32-bit unsigned integer whose expected value is

> 0x233 after transitioning a codec to D3, and 0x0 after transitioning it to D0.

> 

> The response value is significant because hda_codec_runtime_suspend()

> does not clear the codec's bit in the codec_powered bitmask unless the

> AC_PWRST_CLK_STOP_OK bit (0x200) is set in the response value.  That in

> turn prevents the HDA controller from runtime suspending because

> azx_runtime_idle() checks that the codec_powered bitmask is zero.

> 

> Since commit 3b5b899ca67d, hda_set_power_state() only returns 0x0 or 0x1,

> thereby breaking runtime PM for any HDA controller.  That's because an

> inline function introduced by the commit returns a bool instead of a 32-bit

> unsigned int.  The change was likely erroneous and resulted from copying

> and pasting snd_hda_check_power_state(), which is immediately preceding

> the newly introduced inline function.  Fix it.

> 

> Link: https://bugs.freedesktop.org/show_bug.cgi?id=106597

> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to sync

> power state")

> Cc: Alex Deucher <alexander.deucher@amd.com>

> Cc: Abhijeet Kumar <abhijeet.kumar@intel.com>

> Reported-and-tested-by: Gunnar Krüger <taijian@posteo.de>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>

> ---


Acked-by: Alex Deucher <alexander.deucher@amd.com>



>  sound/pci/hda/hda_local.h | 6 ++++--

>  1 file changed, 4 insertions(+), 2 deletions(-)

> 

> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index

> 321e78baa63c..9bd935216c18 100644

> --- a/sound/pci/hda/hda_local.h

> +++ b/sound/pci/hda/hda_local.h

> @@ -622,8 +622,10 @@ snd_hda_check_power_state(struct hda_codec

> *codec, hda_nid_t nid,  {

>  	return snd_hdac_check_power_state(&codec->core, nid,

> target_state);  } -static inline bool snd_hda_sync_power_state(struct

> hda_codec *codec,

> -			   hda_nid_t nid, unsigned int target_state)

> +

> +static inline unsigned int snd_hda_sync_power_state(struct hda_codec

> *codec,

> +						    hda_nid_t nid,

> +						    unsigned int target_state)

>  {

>  	return snd_hdac_sync_power_state(&codec->core, nid,

> target_state);  }

> --

> 2.17.0
Takashi Iwai May 24, 2018, 6:17 p.m. UTC | #2
On Thu, 24 May 2018 19:01:07 +0200,
Lukas Wunner wrote:
> 
> Before commit 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
> to sync power state"), hda_set_power_state() returned the response to
> the Get Power State verb, a 32-bit unsigned integer whose expected value
> is 0x233 after transitioning a codec to D3, and 0x0 after transitioning
> it to D0.
> 
> The response value is significant because hda_codec_runtime_suspend()
> does not clear the codec's bit in the codec_powered bitmask unless the
> AC_PWRST_CLK_STOP_OK bit (0x200) is set in the response value.  That in
> turn prevents the HDA controller from runtime suspending because
> azx_runtime_idle() checks that the codec_powered bitmask is zero.
> 
> Since commit 3b5b899ca67d, hda_set_power_state() only returns 0x0 or
> 0x1, thereby breaking runtime PM for any HDA controller.  That's because
> an inline function introduced by the commit returns a bool instead of a
> 32-bit unsigned int.  The change was likely erroneous and resulted from
> copying and pasting snd_hda_check_power_state(), which is immediately
> preceding the newly introduced inline function.  Fix it.
> 
> Link: https://bugs.freedesktop.org/show_bug.cgi?id=106597
> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to sync power state")
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Abhijeet Kumar <abhijeet.kumar@intel.com>
> Reported-and-tested-by: Gunnar Krüger <taijian@posteo.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Gah, that one was a silly mistake.
Applied now.  Thanks for spotting out!


Takashi

Patch
diff mbox

diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 321e78baa63c..9bd935216c18 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -622,8 +622,10 @@  snd_hda_check_power_state(struct hda_codec *codec, hda_nid_t nid,
 {
 	return snd_hdac_check_power_state(&codec->core, nid, target_state);
 }
-static inline bool snd_hda_sync_power_state(struct hda_codec *codec,
-			   hda_nid_t nid, unsigned int target_state)
+
+static inline unsigned int snd_hda_sync_power_state(struct hda_codec *codec,
+						    hda_nid_t nid,
+						    unsigned int target_state)
 {
 	return snd_hdac_sync_power_state(&codec->core, nid, target_state);
 }