Re: [PATCH] ALSA: hda: Workaround for spurious wakeups on some Intel platforms - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>, alsa-devel@alsa-project.org
Subject: Re: [PATCH] ALSA: hda: Workaround for spurious wakeups on some Intel platforms
Date: Mon, 27 Jul 2020 13:05:06 -0500	[thread overview]
Message-ID: <ad7b6d05-4266-a04e-5d21-31fb835a7d95@linux.intel.com> (raw)
In-Reply-To: <20200727164443.4233-1-tiwai@suse.de>

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);
> 