diff mbox series

ALSA: hda: Workaround for spurious wakeups on some Intel platforms

Message ID 20200727164443.4233-1-tiwai@suse.de (mailing list archive)
State New, archived
Headers show
Series ALSA: hda: Workaround for spurious wakeups on some Intel platforms | expand

Commit Message

Takashi Iwai July 27, 2020, 4:44 p.m. UTC 
We've received a regression report on Intel HD-audio controller that
wakes up immediately after S3 suspend.  The bisection leads to the
commit c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not
needed").  This commit replaces the system-suspend to use
pm_runtime_force_suspend() instead of the direct call of
__azx_runtime_suspend().  However, by some really mysterious reason,
pm_runtime_force_suspend() causes a spurious wakeup (although it calls
the same __azx_runtime_suspend() internally).

As an ugly workaround for now, revert the behavior to call
__azx_runtime_suspend() and __azx_runtime_resume() for those old Intel
platforms that may exhibit such a problem, while keeping the new
standard pm_runtime_force_suspend() and pm_runtime_force_resume()
pair for the remaining chips.

Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed")
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208649
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_controller.h |  2 +-
 sound/pci/hda/hda_intel.c      | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

Comments

Pierre-Louis Bossart July 27, 2020, 6:05 p.m. UTC | #1 
On 7/27/20 11:44 AM, Takashi Iwai wrote:
> We've received a regression report on Intel HD-audio controller that
> wakes up immediately after S3 suspend.  The bisection leads to the
> commit c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not
> needed").  This commit replaces the system-suspend to use
> pm_runtime_force_suspend() instead of the direct call of
> __azx_runtime_suspend().  However, by some really mysterious reason,
> pm_runtime_force_suspend() causes a spurious wakeup (although it calls
> the same __azx_runtime_suspend() internally).

Right, but __azx_runtime_suspend() is called after enabling a wake-up 
event, which is fine for pm_runtime but probably not so good for S3?

static int azx_runtime_suspend(struct device *dev)
{
	struct snd_card *card = dev_get_drvdata(dev);
	struct azx *chip;

	if (!azx_is_pm_ready(card))
		return 0;
	chip = card->private_data;

	/* enable controller wake up event */
	if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) {
		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
			   STATESTS_INT_MASK);
	}

	__azx_runtime_suspend(chip);
	trace_azx_runtime_suspend(chip);
	return 0;
}



> 
> As an ugly workaround for now, revert the behavior to call
> __azx_runtime_suspend() and __azx_runtime_resume() for those old Intel
> platforms that may exhibit such a problem, while keeping the new
> standard pm_runtime_force_suspend() and pm_runtime_force_resume()
> pair for the remaining chips.
> 
> Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed")
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208649
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/pci/hda/hda_controller.h |  2 +-
>   sound/pci/hda/hda_intel.c      | 17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
> index fe171685492d..be63ead8161f 100644
> --- a/sound/pci/hda/hda_controller.h
> +++ b/sound/pci/hda/hda_controller.h
> @@ -41,7 +41,7 @@
>   /* 24 unused */
>   #define AZX_DCAPS_COUNT_LPIB_DELAY  (1 << 25)	/* Take LPIB as delay */
>   #define AZX_DCAPS_PM_RUNTIME	(1 << 26)	/* runtime PM support */
> -/* 27 unused */
> +#define AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP (1 << 27) /* Workaround for spurious wakeups after suspend */
>   #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28)	/* CORBRP clears itself after reset */
>   #define AZX_DCAPS_NO_MSI64      (1 << 29)	/* Stick to 32-bit MSIs */
>   #define AZX_DCAPS_SEPARATE_STREAM_TAG	(1 << 30) /* capture and playback use separate stream tag */
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 9ba1fb8f0b7f..fb65450d8de1 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -297,7 +297,8 @@ enum {
>   /* PCH for HSW/BDW; with runtime PM */
>   /* no i915 binding for this as HSW/BDW has another controller for HDMI */
>   #define AZX_DCAPS_INTEL_PCH \
> -	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
> +	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
> +	 AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
>   
>   /* HSW HDMI */
>   #define AZX_DCAPS_INTEL_HASWELL \
> @@ -1026,7 +1027,14 @@ static int azx_suspend(struct device *dev)
>   	chip = card->private_data;
>   	bus = azx_bus(chip);
>   	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> -	pm_runtime_force_suspend(dev);
> +	/* An ugly workaround: direct call of __azx_runtime_suspend() and
> +	 * __azx_runtime_resume() for old Intel platforms that suffer from
> +	 * spurious wakeups after S3 suspend
> +	 */
> +	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
> +		__azx_runtime_suspend(chip);
> +	else
> +		pm_runtime_force_suspend(dev);
>   	if (bus->irq >= 0) {
>   		free_irq(bus->irq, chip);
>   		bus->irq = -1;
> @@ -1055,7 +1063,10 @@ static int azx_resume(struct device *dev)
>   	if (azx_acquire_irq(chip, 1) < 0)
>   		return -EIO;
>   
> -	pm_runtime_force_resume(dev);
> +	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
> +		__azx_runtime_resume(chip, false);
> +	else
> +		pm_runtime_force_resume(dev);
>   	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
>   
>   	trace_azx_resume(chip);
>
Takashi Iwai July 27, 2020, 6:13 p.m. UTC | #2 
On Mon, 27 Jul 2020 20:05:06 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 7/27/20 11:44 AM, Takashi Iwai wrote:
> > We've received a regression report on Intel HD-audio controller that
> > wakes up immediately after S3 suspend.  The bisection leads to the
> > commit c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not
> > needed").  This commit replaces the system-suspend to use
> > pm_runtime_force_suspend() instead of the direct call of
> > __azx_runtime_suspend().  However, by some really mysterious reason,
> > pm_runtime_force_suspend() causes a spurious wakeup (although it calls
> > the same __azx_runtime_suspend() internally).
> 
> Right, but __azx_runtime_suspend() is called after enabling a wake-up
> event, which is fine for pm_runtime but probably not so good for S3?

That's what I doubted at the first place, too, but unfortunately no,
it isn't the cause.

> static int azx_runtime_suspend(struct device *dev)
> {
> 	struct snd_card *card = dev_get_drvdata(dev);
> 	struct azx *chip;
> 
> 	if (!azx_is_pm_ready(card))
> 		return 0;
> 	chip = card->private_data;
> 
> 	/* enable controller wake up event */
> 	if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) {
> 		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> 			   STATESTS_INT_MASK);
> 	}

The code above contains the check of the current power state to
SNDRV_CTL_POWER_D0.  And azx_suspend() sets the power state to D3hot
before calling pm_runtime_force_suspend(), hence this WAKEEN shouldn't
be called.

And we tried to comment out this WAKEEN call completely, but the
spurious wakeup still happened.  Also, even after clearing WAKKEN
explicitly, the wakeup happened.  Last but not least, it happened even
after disabling the runtime PM (i.e. the pm state change happens at
first at azx_suspend() call).

So it's really mysterious.


Takashi
diff mbox series

Patch

diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index fe171685492d..be63ead8161f 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -41,7 +41,7 @@ 
 /* 24 unused */
 #define AZX_DCAPS_COUNT_LPIB_DELAY  (1 << 25)	/* Take LPIB as delay */
 #define AZX_DCAPS_PM_RUNTIME	(1 << 26)	/* runtime PM support */
-/* 27 unused */
+#define AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP (1 << 27) /* Workaround for spurious wakeups after suspend */
 #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28)	/* CORBRP clears itself after reset */
 #define AZX_DCAPS_NO_MSI64      (1 << 29)	/* Stick to 32-bit MSIs */
 #define AZX_DCAPS_SEPARATE_STREAM_TAG	(1 << 30) /* capture and playback use separate stream tag */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 9ba1fb8f0b7f..fb65450d8de1 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,7 +297,8 @@  enum {
 /* PCH for HSW/BDW; with runtime PM */
 /* no i915 binding for this as HSW/BDW has another controller for HDMI */
 #define AZX_DCAPS_INTEL_PCH \
-	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
+	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
+	 AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
 
 /* HSW HDMI */
 #define AZX_DCAPS_INTEL_HASWELL \
@@ -1026,7 +1027,14 @@  static int azx_suspend(struct device *dev)
 	chip = card->private_data;
 	bus = azx_bus(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	pm_runtime_force_suspend(dev);
+	/* An ugly workaround: direct call of __azx_runtime_suspend() and
+	 * __azx_runtime_resume() for old Intel platforms that suffer from
+	 * spurious wakeups after S3 suspend
+	 */
+	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
+		__azx_runtime_suspend(chip);
+	else
+		pm_runtime_force_suspend(dev);
 	if (bus->irq >= 0) {
 		free_irq(bus->irq, chip);
 		bus->irq = -1;
@@ -1055,7 +1063,10 @@  static int azx_resume(struct device *dev)
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
 
-	pm_runtime_force_resume(dev);
+	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
+		__azx_runtime_resume(chip, false);
+	else
+		pm_runtime_force_resume(dev);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
 	trace_azx_resume(chip);