[PATCH v2 0/4] HDA controller and PM cleanups

[PATCH v2 0/4] HDA controller and PM cleanups

[Kai-Heng Feng]

While working on the issue "ALSA: hda: fix jack detection with Realtek
codecs when in D3", I've found using pm_runtime_force_{suspend,resume}()
have surprising behavior, specifically, pm_runtime_need_not_resume()
uses pm_runtime_need_not_resume() to avoid calling resume callback, so
jackpoll was used to really power up the codec.

We can use direct-complete to do the keep the codec suspended throughout
the system PM flow, namely, keep the codec suspended all the way, unless
the codec needs to be woken up after resume.

For HDA controller, PCI core may enable direct-complete for it if
conditions are met. So make runtime and system PM distinctive to always
apply correct wake up setting.

At least point, hopefully all runtime PM issues are solved, let's enable
runtime PM by default again.

Kai-Heng Feng (4):
  ALSA: hda: Refactor codec PM to use direct-complete optimization
  ALSA: hda: Stop mangling PCI IRQ
  ALSA: hda: Separate runtime and system suspend
  ALSA: hda: Reinstate runtime_allow() for all hda controllers

 sound/pci/hda/hda_codec.c      | 45 +++++++++++++-------
 sound/pci/hda/hda_controller.h |  3 +-
 sound/pci/hda/hda_intel.c      | 77 ++++++++++++++++------------------
 3 files changed, 67 insertions(+), 58 deletions(-)

-- 
2.17.1

[PATCH v2 0/4] HDA controller and PM cleanups

[Kai-Heng Feng]

While working on the issue "ALSA: hda: fix jack detection with Realtek
codecs when in D3", I've found using pm_runtime_force_{suspend,resume}()
have surprising behavior, specifically, pm_runtime_need_not_resume()
uses pm_runtime_need_not_resume() to avoid calling resume callback, so
jackpoll was used to really power up the codec.

We can use direct-complete to do the keep the codec suspended throughout
the system PM flow, namely, keep the codec suspended all the way, unless
the codec needs to be woken up after resume.

For HDA controller, PCI core may enable direct-complete for it if
conditions are met. So make runtime and system PM distinctive to always
apply correct wake up setting.

At least point, hopefully all runtime PM issues are solved, let's enable
runtime PM by default again.

Kai-Heng Feng (4):
  ALSA: hda: Refactor codec PM to use direct-complete optimization
  ALSA: hda: Stop mangling PCI IRQ
  ALSA: hda: Separate runtime and system suspend
  ALSA: hda: Reinstate runtime_allow() for all hda controllers

 sound/pci/hda/hda_codec.c      | 45 +++++++++++++-------
 sound/pci/hda/hda_controller.h |  3 +-
 sound/pci/hda/hda_intel.c      | 77 ++++++++++++++++------------------
 3 files changed, 67 insertions(+), 58 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] ALSA: hda: Refactor codec PM to use direct-complete optimization

[Kai-Heng Feng]

Upon system resume, hda_codec_pm_resume() uses hda_codec_force_resume()
to resume the codec. However, pm_runtime_force_resume() won't really
resume the codec because of pm_runtime_need_not_resume() check.

Hence, hda_codec_force_resume() schedules a jackpoll work, which is to
really power up the codec.

Instead of doing that, we can use direct-complete to make the PM flow
more straightforward, and keep codec always suspended through system PM
flow if conditions are met.

On system suspend, PM core will decide what to do based on
hda_codec_pm_prepare():
- If codec is not runtime-suspended, PM core will suspend and resume the
device as normal.
- If codec is runtime-suspended, PM core will try to keep it suspended.
If it's still suspended after system resume, we use
hda_codec_pm_complete() to resume codec if it's needed.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Also resume when codec->jackpoll_interval is set

 sound/pci/hda/hda_codec.c | 45 +++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index a356c21edb90..4bb58e8b08a8 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2934,7 +2934,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 	snd_hdac_leave_pm(&codec->core);
 }
 
-static int hda_codec_runtime_suspend(struct device *dev)
+static int hda_codec_suspend(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 	unsigned int state;
@@ -2953,7 +2953,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int hda_codec_runtime_resume(struct device *dev)
+static int hda_codec_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
@@ -2967,57 +2967,70 @@ static int hda_codec_runtime_resume(struct device *dev)
 	pm_runtime_mark_last_busy(dev);
 	return 0;
 }
+
+static int hda_codec_runtime_suspend(struct device *dev)
+{
+	return hda_codec_suspend(dev);
+}
+
+static int hda_codec_runtime_resume(struct device *dev)
+{
+	return hda_codec_resume(dev);
+}
+
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_PM_SLEEP
-static int hda_codec_force_resume(struct device *dev)
+static int hda_codec_pm_prepare(struct device *dev)
+{
+	return pm_runtime_suspended(dev);
+}
+
+static void hda_codec_pm_complete(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
-	int ret;
 
-	ret = pm_runtime_force_resume(dev);
-	/* schedule jackpoll work for jack detection update */
-	if (codec->jackpoll_interval ||
-	    (pm_runtime_suspended(dev) && hda_codec_need_resume(codec)))
-		schedule_delayed_work(&codec->jackpoll_work,
-				      codec->jackpoll_interval);
-	return ret;
+	if (pm_runtime_suspended(dev) && (codec->jackpoll_interval ||
+	    hda_codec_need_resume(codec) || codec->forced_resume))
+		pm_request_resume(dev);
 }
 
 static int hda_codec_pm_suspend(struct device *dev)
 {
 	dev->power.power_state = PMSG_SUSPEND;
-	return pm_runtime_force_suspend(dev);
+	return hda_codec_suspend(dev);
 }
 
 static int hda_codec_pm_resume(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESUME;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 
 static int hda_codec_pm_freeze(struct device *dev)
 {
 	dev->power.power_state = PMSG_FREEZE;
-	return pm_runtime_force_suspend(dev);
+	return hda_codec_suspend(dev);
 }
 
 static int hda_codec_pm_thaw(struct device *dev)
 {
 	dev->power.power_state = PMSG_THAW;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 
 static int hda_codec_pm_restore(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESTORE;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
 
 /* referred in hda_bind.c */
 const struct dev_pm_ops hda_codec_driver_pm = {
 #ifdef CONFIG_PM_SLEEP
+	.prepare = hda_codec_pm_prepare,
+	.complete = hda_codec_pm_complete,
 	.suspend = hda_codec_pm_suspend,
 	.resume = hda_codec_pm_resume,
 	.freeze = hda_codec_pm_freeze,
-- 
2.17.1

[PATCH v2 1/4] ALSA: hda: Refactor codec PM to use direct-complete optimization

[Kai-Heng Feng]

Upon system resume, hda_codec_pm_resume() uses hda_codec_force_resume()
to resume the codec. However, pm_runtime_force_resume() won't really
resume the codec because of pm_runtime_need_not_resume() check.

Hence, hda_codec_force_resume() schedules a jackpoll work, which is to
really power up the codec.

Instead of doing that, we can use direct-complete to make the PM flow
more straightforward, and keep codec always suspended through system PM
flow if conditions are met.

On system suspend, PM core will decide what to do based on
hda_codec_pm_prepare():
- If codec is not runtime-suspended, PM core will suspend and resume the
device as normal.
- If codec is runtime-suspended, PM core will try to keep it suspended.
If it's still suspended after system resume, we use
hda_codec_pm_complete() to resume codec if it's needed.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Also resume when codec->jackpoll_interval is set

 sound/pci/hda/hda_codec.c | 45 +++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index a356c21edb90..4bb58e8b08a8 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2934,7 +2934,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 	snd_hdac_leave_pm(&codec->core);
 }
 
-static int hda_codec_runtime_suspend(struct device *dev)
+static int hda_codec_suspend(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 	unsigned int state;
@@ -2953,7 +2953,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int hda_codec_runtime_resume(struct device *dev)
+static int hda_codec_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
@@ -2967,57 +2967,70 @@ static int hda_codec_runtime_resume(struct device *dev)
 	pm_runtime_mark_last_busy(dev);
 	return 0;
 }
+
+static int hda_codec_runtime_suspend(struct device *dev)
+{
+	return hda_codec_suspend(dev);
+}
+
+static int hda_codec_runtime_resume(struct device *dev)
+{
+	return hda_codec_resume(dev);
+}
+
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_PM_SLEEP
-static int hda_codec_force_resume(struct device *dev)
+static int hda_codec_pm_prepare(struct device *dev)
+{
+	return pm_runtime_suspended(dev);
+}
+
+static void hda_codec_pm_complete(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
-	int ret;
 
-	ret = pm_runtime_force_resume(dev);
-	/* schedule jackpoll work for jack detection update */
-	if (codec->jackpoll_interval ||
-	    (pm_runtime_suspended(dev) && hda_codec_need_resume(codec)))
-		schedule_delayed_work(&codec->jackpoll_work,
-				      codec->jackpoll_interval);
-	return ret;
+	if (pm_runtime_suspended(dev) && (codec->jackpoll_interval ||
+	    hda_codec_need_resume(codec) || codec->forced_resume))
+		pm_request_resume(dev);
 }
 
 static int hda_codec_pm_suspend(struct device *dev)
 {
 	dev->power.power_state = PMSG_SUSPEND;
-	return pm_runtime_force_suspend(dev);
+	return hda_codec_suspend(dev);
 }
 
 static int hda_codec_pm_resume(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESUME;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 
 static int hda_codec_pm_freeze(struct device *dev)
 {
 	dev->power.power_state = PMSG_FREEZE;
-	return pm_runtime_force_suspend(dev);
+	return hda_codec_suspend(dev);
 }
 
 static int hda_codec_pm_thaw(struct device *dev)
 {
 	dev->power.power_state = PMSG_THAW;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 
 static int hda_codec_pm_restore(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESTORE;
-	return hda_codec_force_resume(dev);
+	return hda_codec_resume(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
 
 /* referred in hda_bind.c */
 const struct dev_pm_ops hda_codec_driver_pm = {
 #ifdef CONFIG_PM_SLEEP
+	.prepare = hda_codec_pm_prepare,
+	.complete = hda_codec_pm_complete,
 	.suspend = hda_codec_pm_suspend,
 	.resume = hda_codec_pm_resume,
 	.freeze = hda_codec_pm_freeze,
-- 
2.17.1

[PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Kai-Heng Feng]

The code predates 2005, it should be unnecessary now as PCI core handles
IRQ much better nowadays.

So stop PCI IRQ mangling in suspend/resume callbacks.

Takashi Iwai mentioned that IRQ number can change after S3 on some
really old hardwares. We should use quirks to handle those platforms, as
most modern systems won't have that issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Wording.
 - Add info on IRQ # can change on old hardwares.

 sound/pci/hda/hda_intel.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 749b88090970..b4aa1dcf1aae 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	struct hdac_bus *bus;
 
 	if (!azx_is_pm_ready(card))
 		return 0;
 
 	chip = card->private_data;
-	bus = azx_bus(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	/* An ugly workaround: direct call of __azx_runtime_suspend() and
 	 * __azx_runtime_resume() for old Intel platforms that suffer from
@@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
 		__azx_runtime_suspend(chip);
 	else
 		pm_runtime_force_suspend(dev);
-	if (bus->irq >= 0) {
-		free_irq(bus->irq, chip);
-		bus->irq = -1;
-		chip->card->sync_irq = -1;
-	}
-
-	if (chip->msi)
-		pci_disable_msi(chip->pci);
 
 	trace_azx_suspend(chip);
 	return 0;
@@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
 		return 0;
 
 	chip = card->private_data;
-	if (chip->msi)
-		if (pci_enable_msi(chip->pci) < 0)
-			chip->msi = 0;
-	if (azx_acquire_irq(chip, 1) < 0)
-		return -EIO;
 
 	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
 		__azx_runtime_resume(chip, false);
-- 
2.17.1

[PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Kai-Heng Feng]

The code predates 2005, it should be unnecessary now as PCI core handles
IRQ much better nowadays.

So stop PCI IRQ mangling in suspend/resume callbacks.

Takashi Iwai mentioned that IRQ number can change after S3 on some
really old hardwares. We should use quirks to handle those platforms, as
most modern systems won't have that issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Wording.
 - Add info on IRQ # can change on old hardwares.

 sound/pci/hda/hda_intel.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 749b88090970..b4aa1dcf1aae 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	struct hdac_bus *bus;
 
 	if (!azx_is_pm_ready(card))
 		return 0;
 
 	chip = card->private_data;
-	bus = azx_bus(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	/* An ugly workaround: direct call of __azx_runtime_suspend() and
 	 * __azx_runtime_resume() for old Intel platforms that suffer from
@@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
 		__azx_runtime_suspend(chip);
 	else
 		pm_runtime_force_suspend(dev);
-	if (bus->irq >= 0) {
-		free_irq(bus->irq, chip);
-		bus->irq = -1;
-		chip->card->sync_irq = -1;
-	}
-
-	if (chip->msi)
-		pci_disable_msi(chip->pci);
 
 	trace_azx_suspend(chip);
 	return 0;
@@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
 		return 0;
 
 	chip = card->private_data;
-	if (chip->msi)
-		if (pci_enable_msi(chip->pci) < 0)
-			chip->msi = 0;
-	if (azx_acquire_irq(chip, 1) < 0)
-		return -EIO;
 
 	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
 		__azx_runtime_resume(chip, false);
-- 
2.17.1

Re: [PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Takashi Iwai]

On Tue, 27 Oct 2020 06:39:59 +0100,
Kai-Heng Feng wrote:
> 
> The code predates 2005, it should be unnecessary now as PCI core handles
> IRQ much better nowadays.
> 
> So stop PCI IRQ mangling in suspend/resume callbacks.
> 
> Takashi Iwai mentioned that IRQ number can change after S3 on some
> really old hardwares. We should use quirks to handle those platforms, as
> most modern systems won't have that issue.

I believe it was S4.  And this pretty much depends on BIOS, hence it's
hard to apply the quirk, honestly speaking.

And, if we know that we need a quirk, dropping the code completely now
is a bad move.  If any, this should be applied conditionally to the
"known to be modern" platforms, but this will make the code rather
messier, OTOH.

Do we need this change inevitably?  Otherwise I'd skip this one.


thanks,

Takashi


> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>  - Wording.
>  - Add info on IRQ # can change on old hardwares.
> 
>  sound/pci/hda/hda_intel.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 749b88090970..b4aa1dcf1aae 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
>  {
>  	struct snd_card *card = dev_get_drvdata(dev);
>  	struct azx *chip;
> -	struct hdac_bus *bus;
>  
>  	if (!azx_is_pm_ready(card))
>  		return 0;
>  
>  	chip = card->private_data;
> -	bus = azx_bus(chip);
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
>  	/* An ugly workaround: direct call of __azx_runtime_suspend() and
>  	 * __azx_runtime_resume() for old Intel platforms that suffer from
> @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
>  		__azx_runtime_suspend(chip);
>  	else
>  		pm_runtime_force_suspend(dev);
> -	if (bus->irq >= 0) {
> -		free_irq(bus->irq, chip);
> -		bus->irq = -1;
> -		chip->card->sync_irq = -1;
> -	}
> -
> -	if (chip->msi)
> -		pci_disable_msi(chip->pci);
>  
>  	trace_azx_suspend(chip);
>  	return 0;
> @@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
>  		return 0;
>  
>  	chip = card->private_data;
> -	if (chip->msi)
> -		if (pci_enable_msi(chip->pci) < 0)
> -			chip->msi = 0;
> -	if (azx_acquire_irq(chip, 1) < 0)
> -		return -EIO;
>  
>  	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
>  		__azx_runtime_resume(chip, false);
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Takashi Iwai]

On Tue, 27 Oct 2020 06:39:59 +0100,
Kai-Heng Feng wrote:
> 
> The code predates 2005, it should be unnecessary now as PCI core handles
> IRQ much better nowadays.
> 
> So stop PCI IRQ mangling in suspend/resume callbacks.
> 
> Takashi Iwai mentioned that IRQ number can change after S3 on some
> really old hardwares. We should use quirks to handle those platforms, as
> most modern systems won't have that issue.

I believe it was S4.  And this pretty much depends on BIOS, hence it's
hard to apply the quirk, honestly speaking.

And, if we know that we need a quirk, dropping the code completely now
is a bad move.  If any, this should be applied conditionally to the
"known to be modern" platforms, but this will make the code rather
messier, OTOH.

Do we need this change inevitably?  Otherwise I'd skip this one.


thanks,

Takashi


> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>  - Wording.
>  - Add info on IRQ # can change on old hardwares.
> 
>  sound/pci/hda/hda_intel.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 749b88090970..b4aa1dcf1aae 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
>  {
>  	struct snd_card *card = dev_get_drvdata(dev);
>  	struct azx *chip;
> -	struct hdac_bus *bus;
>  
>  	if (!azx_is_pm_ready(card))
>  		return 0;
>  
>  	chip = card->private_data;
> -	bus = azx_bus(chip);
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
>  	/* An ugly workaround: direct call of __azx_runtime_suspend() and
>  	 * __azx_runtime_resume() for old Intel platforms that suffer from
> @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
>  		__azx_runtime_suspend(chip);
>  	else
>  		pm_runtime_force_suspend(dev);
> -	if (bus->irq >= 0) {
> -		free_irq(bus->irq, chip);
> -		bus->irq = -1;
> -		chip->card->sync_irq = -1;
> -	}
> -
> -	if (chip->msi)
> -		pci_disable_msi(chip->pci);
>  
>  	trace_azx_suspend(chip);
>  	return 0;
> @@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
>  		return 0;
>  
>  	chip = card->private_data;
> -	if (chip->msi)
> -		if (pci_enable_msi(chip->pci) < 0)
> -			chip->msi = 0;
> -	if (azx_acquire_irq(chip, 1) < 0)
> -		return -EIO;
>  
>  	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
>  		__azx_runtime_resume(chip, false);
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Kai-Heng Feng]

> On Oct 27, 2020, at 15:36, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 27 Oct 2020 06:39:59 +0100,
> Kai-Heng Feng wrote:
>> 
>> The code predates 2005, it should be unnecessary now as PCI core handles
>> IRQ much better nowadays.
>> 
>> So stop PCI IRQ mangling in suspend/resume callbacks.
>> 
>> Takashi Iwai mentioned that IRQ number can change after S3 on some
>> really old hardwares. We should use quirks to handle those platforms, as
>> most modern systems won't have that issue.
> 
> I believe it was S4.  And this pretty much depends on BIOS, hence it's
> hard to apply the quirk, honestly speaking.

Ok, S4 is indeed hard to handle.

> 
> And, if we know that we need a quirk, dropping the code completely now
> is a bad move.  If any, this should be applied conditionally to the
> "known to be modern" platforms, but this will make the code rather
> messier, OTOH.
> 
> Do we need this change inevitably?  Otherwise I'd skip this one.

Ok, please drop this one.

Kai-Heng

> 
> 
> thanks,
> 
> Takashi
> 
> 
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> v2:
>> - Wording.
>> - Add info on IRQ # can change on old hardwares.
>> 
>> sound/pci/hda/hda_intel.c | 15 ---------------
>> 1 file changed, 15 deletions(-)
>> 
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index 749b88090970..b4aa1dcf1aae 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
>> {
>> 	struct snd_card *card = dev_get_drvdata(dev);
>> 	struct azx *chip;
>> -	struct hdac_bus *bus;
>> 
>> 	if (!azx_is_pm_ready(card))
>> 		return 0;
>> 
>> 	chip = card->private_data;
>> -	bus = azx_bus(chip);
>> 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
>> 	/* An ugly workaround: direct call of __azx_runtime_suspend() and
>> 	 * __azx_runtime_resume() for old Intel platforms that suffer from
>> @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
>> 		__azx_runtime_suspend(chip);
>> 	else
>> 		pm_runtime_force_suspend(dev);
>> -	if (bus->irq >= 0) {
>> -		free_irq(bus->irq, chip);
>> -		bus->irq = -1;
>> -		chip->card->sync_irq = -1;
>> -	}
>> -
>> -	if (chip->msi)
>> -		pci_disable_msi(chip->pci);
>> 
>> 	trace_azx_suspend(chip);
>> 	return 0;
>> @@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
>> 		return 0;
>> 
>> 	chip = card->private_data;
>> -	if (chip->msi)
>> -		if (pci_enable_msi(chip->pci) < 0)
>> -			chip->msi = 0;
>> -	if (azx_acquire_irq(chip, 1) < 0)
>> -		return -EIO;
>> 
>> 	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
>> 		__azx_runtime_resume(chip, false);
>> -- 
>> 2.17.1
>> 

Re: [PATCH v2 2/4] ALSA: hda: Stop mangling PCI IRQ

[Kai-Heng Feng]

> On Oct 27, 2020, at 15:36, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 27 Oct 2020 06:39:59 +0100,
> Kai-Heng Feng wrote:
>> 
>> The code predates 2005, it should be unnecessary now as PCI core handles
>> IRQ much better nowadays.
>> 
>> So stop PCI IRQ mangling in suspend/resume callbacks.
>> 
>> Takashi Iwai mentioned that IRQ number can change after S3 on some
>> really old hardwares. We should use quirks to handle those platforms, as
>> most modern systems won't have that issue.
> 
> I believe it was S4.  And this pretty much depends on BIOS, hence it's
> hard to apply the quirk, honestly speaking.

Ok, S4 is indeed hard to handle.

> 
> And, if we know that we need a quirk, dropping the code completely now
> is a bad move.  If any, this should be applied conditionally to the
> "known to be modern" platforms, but this will make the code rather
> messier, OTOH.
> 
> Do we need this change inevitably?  Otherwise I'd skip this one.

Ok, please drop this one.

Kai-Heng

> 
> 
> thanks,
> 
> Takashi
> 
> 
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> v2:
>> - Wording.
>> - Add info on IRQ # can change on old hardwares.
>> 
>> sound/pci/hda/hda_intel.c | 15 ---------------
>> 1 file changed, 15 deletions(-)
>> 
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index 749b88090970..b4aa1dcf1aae 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev)
>> {
>> 	struct snd_card *card = dev_get_drvdata(dev);
>> 	struct azx *chip;
>> -	struct hdac_bus *bus;
>> 
>> 	if (!azx_is_pm_ready(card))
>> 		return 0;
>> 
>> 	chip = card->private_data;
>> -	bus = azx_bus(chip);
>> 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
>> 	/* An ugly workaround: direct call of __azx_runtime_suspend() and
>> 	 * __azx_runtime_resume() for old Intel platforms that suffer from
>> @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev)
>> 		__azx_runtime_suspend(chip);
>> 	else
>> 		pm_runtime_force_suspend(dev);
>> -	if (bus->irq >= 0) {
>> -		free_irq(bus->irq, chip);
>> -		bus->irq = -1;
>> -		chip->card->sync_irq = -1;
>> -	}
>> -
>> -	if (chip->msi)
>> -		pci_disable_msi(chip->pci);
>> 
>> 	trace_azx_suspend(chip);
>> 	return 0;
>> @@ -1060,11 +1050,6 @@ static int azx_resume(struct device *dev)
>> 		return 0;
>> 
>> 	chip = card->private_data;
>> -	if (chip->msi)
>> -		if (pci_enable_msi(chip->pci) < 0)
>> -			chip->msi = 0;
>> -	if (azx_acquire_irq(chip, 1) < 0)
>> -		return -EIO;
>> 
>> 	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
>> 		__azx_runtime_resume(chip, false);
>> -- 
>> 2.17.1
>> 

[PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
some implicit checks, so it can make code flow more straightfoward if we
separate runtime and systemd suspend callbacks.

High Definition Audio Specification, 4.5.9.3 Codec Wake From System S3
states that codec can wake the system up from S3 if WAKEEN is toggled.
Since HDA controller has different wakeup settings for runtime and
system susend, we also need to explicitly disable direct-complete which
can be enabled automatically by PCI core. In addition to that, avoid
waking up codec if runtime resume is for system suspend, to not break
direct-complete for codecs.

While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
wakeups on some Intel platforms") solves doesn't happen with this
patch.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Make sure WAKEEN is disabled for system suspend.
 - Avoid waking up codec if runtime resume is for system suspend

 sound/pci/hda/hda_controller.h |  3 +-
 sound/pci/hda/hda_intel.c      | 61 +++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index be63ead8161f..7176ef8170ba 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 */
-#define AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP (1 << 27) /* Workaround for spurious wakeups after suspend */
+/* 27 unused */
 #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 */
@@ -143,6 +143,7 @@ struct azx {
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by vga_switcheroo */
+	unsigned int prepared:1;
 
 	/* GTS present */
 	unsigned int gts_present:1;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b4aa1dcf1aae..4cf90e4602c7 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,8 +297,7 @@ 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_SUSPEND_SPURIOUS_WAKEUP)
+	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
 
 /* HSW HDMI */
 #define AZX_DCAPS_INTEL_HASWELL \
@@ -1002,7 +1001,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 	azx_init_pci(chip);
 	hda_intel_init_chip(chip, true);
 
-	if (from_rt) {
+	/* Avoid codec resume if runtime resume is for system suspend */
+	if (from_rt && !chip->prepared) {
 		list_for_each_codec(codec, &chip->bus) {
 			if (codec->relaxed_resume)
 				continue;
@@ -1018,6 +1018,29 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int azx_prepare(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip;
+
+	chip = card->private_data;
+	chip->prepared = 1;
+
+	/* HDA controller always requires different WAKEEN for runtime suspend
+	 * and system suspend, so don't use direct-complete here.
+	 */
+	return 0;
+}
+
+static void azx_complete(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip;
+
+	chip = card->private_data;
+	chip->prepared = 0;
+}
+
 static int azx_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
@@ -1028,14 +1051,7 @@ static int azx_suspend(struct device *dev)
 
 	chip = card->private_data;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	/* 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);
+	__azx_runtime_suspend(chip);
 
 	trace_azx_suspend(chip);
 	return 0;
@@ -1050,11 +1066,7 @@ static int azx_resume(struct device *dev)
 		return 0;
 
 	chip = card->private_data;
-
-	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
-		__azx_runtime_resume(chip, false);
-	else
-		pm_runtime_force_resume(dev);
+	__azx_runtime_resume(chip, false);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
 	trace_azx_resume(chip);
@@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
 	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
+		   STATESTS_INT_MASK);
 
 	__azx_runtime_suspend(chip);
 	trace_azx_runtime_suspend(chip);
@@ -1117,18 +1127,15 @@ static int azx_runtime_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	bool from_rt = snd_power_get_state(card) == SNDRV_CTL_POWER_D0;
 
 	if (!azx_is_pm_ready(card))
 		return 0;
 	chip = card->private_data;
-	__azx_runtime_resume(chip, from_rt);
+	__azx_runtime_resume(chip, true);
 
 	/* disable controller Wake Up event*/
-	if (from_rt) {
-		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
-			   ~STATESTS_INT_MASK);
-	}
+	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
+		   ~STATESTS_INT_MASK);
 
 	trace_azx_runtime_resume(chip);
 	return 0;
@@ -1162,6 +1169,8 @@ static int azx_runtime_idle(struct device *dev)
 static const struct dev_pm_ops azx_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
 #ifdef CONFIG_PM_SLEEP
+	.prepare = azx_prepare,
+	.complete = azx_complete,
 	.freeze_noirq = azx_freeze_noirq,
 	.thaw_noirq = azx_thaw_noirq,
 #endif
-- 
2.17.1

[PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
some implicit checks, so it can make code flow more straightfoward if we
separate runtime and systemd suspend callbacks.

High Definition Audio Specification, 4.5.9.3 Codec Wake From System S3
states that codec can wake the system up from S3 if WAKEEN is toggled.
Since HDA controller has different wakeup settings for runtime and
system susend, we also need to explicitly disable direct-complete which
can be enabled automatically by PCI core. In addition to that, avoid
waking up codec if runtime resume is for system suspend, to not break
direct-complete for codecs.

While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
wakeups on some Intel platforms") solves doesn't happen with this
patch.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Make sure WAKEEN is disabled for system suspend.
 - Avoid waking up codec if runtime resume is for system suspend

 sound/pci/hda/hda_controller.h |  3 +-
 sound/pci/hda/hda_intel.c      | 61 +++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index be63ead8161f..7176ef8170ba 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 */
-#define AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP (1 << 27) /* Workaround for spurious wakeups after suspend */
+/* 27 unused */
 #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 */
@@ -143,6 +143,7 @@ struct azx {
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by vga_switcheroo */
+	unsigned int prepared:1;
 
 	/* GTS present */
 	unsigned int gts_present:1;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b4aa1dcf1aae..4cf90e4602c7 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,8 +297,7 @@ 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_SUSPEND_SPURIOUS_WAKEUP)
+	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
 
 /* HSW HDMI */
 #define AZX_DCAPS_INTEL_HASWELL \
@@ -1002,7 +1001,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 	azx_init_pci(chip);
 	hda_intel_init_chip(chip, true);
 
-	if (from_rt) {
+	/* Avoid codec resume if runtime resume is for system suspend */
+	if (from_rt && !chip->prepared) {
 		list_for_each_codec(codec, &chip->bus) {
 			if (codec->relaxed_resume)
 				continue;
@@ -1018,6 +1018,29 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int azx_prepare(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip;
+
+	chip = card->private_data;
+	chip->prepared = 1;
+
+	/* HDA controller always requires different WAKEEN for runtime suspend
+	 * and system suspend, so don't use direct-complete here.
+	 */
+	return 0;
+}
+
+static void azx_complete(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip;
+
+	chip = card->private_data;
+	chip->prepared = 0;
+}
+
 static int azx_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
@@ -1028,14 +1051,7 @@ static int azx_suspend(struct device *dev)
 
 	chip = card->private_data;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	/* 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);
+	__azx_runtime_suspend(chip);
 
 	trace_azx_suspend(chip);
 	return 0;
@@ -1050,11 +1066,7 @@ static int azx_resume(struct device *dev)
 		return 0;
 
 	chip = card->private_data;
-
-	if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP)
-		__azx_runtime_resume(chip, false);
-	else
-		pm_runtime_force_resume(dev);
+	__azx_runtime_resume(chip, false);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
 	trace_azx_resume(chip);
@@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
 	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
+		   STATESTS_INT_MASK);
 
 	__azx_runtime_suspend(chip);
 	trace_azx_runtime_suspend(chip);
@@ -1117,18 +1127,15 @@ static int azx_runtime_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	bool from_rt = snd_power_get_state(card) == SNDRV_CTL_POWER_D0;
 
 	if (!azx_is_pm_ready(card))
 		return 0;
 	chip = card->private_data;
-	__azx_runtime_resume(chip, from_rt);
+	__azx_runtime_resume(chip, true);
 
 	/* disable controller Wake Up event*/
-	if (from_rt) {
-		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
-			   ~STATESTS_INT_MASK);
-	}
+	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
+		   ~STATESTS_INT_MASK);
 
 	trace_azx_runtime_resume(chip);
 	return 0;
@@ -1162,6 +1169,8 @@ static int azx_runtime_idle(struct device *dev)
 static const struct dev_pm_ops azx_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
 #ifdef CONFIG_PM_SLEEP
+	.prepare = azx_prepare,
+	.complete = azx_complete,
 	.freeze_noirq = azx_freeze_noirq,
 	.thaw_noirq = azx_thaw_noirq,
 #endif
-- 
2.17.1

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 06:40:00 +0100,
Kai-Heng Feng wrote:
> @@ -1002,7 +1001,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
>  	azx_init_pci(chip);
>  	hda_intel_init_chip(chip, true);
>  
> -	if (from_rt) {
> +	/* Avoid codec resume if runtime resume is for system suspend */
> +	if (from_rt && !chip->prepared) {

The check of chip->prepared is superfluous here.  Its check should be
applied rather at setting WAKEEN, namely...

> @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
>  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> +		   STATESTS_INT_MASK);

... here we should have the check of chip->prepared, and set WAKEEN
only when it's false.  Otherwise WAKEEN is set up for the system
suspend, and it might lead to spurious wakeups.  (IOW, checking the
flag at resume doesn't help for preventing the spurious wakeup :)


thanks,

Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 06:40:00 +0100,
Kai-Heng Feng wrote:
> @@ -1002,7 +1001,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
>  	azx_init_pci(chip);
>  	hda_intel_init_chip(chip, true);
>  
> -	if (from_rt) {
> +	/* Avoid codec resume if runtime resume is for system suspend */
> +	if (from_rt && !chip->prepared) {

The check of chip->prepared is superfluous here.  Its check should be
applied rather at setting WAKEEN, namely...

> @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
>  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> +		   STATESTS_INT_MASK);

... here we should have the check of chip->prepared, and set WAKEEN
only when it's false.  Otherwise WAKEEN is set up for the system
suspend, and it might lead to spurious wakeups.  (IOW, checking the
flag at resume doesn't help for preventing the spurious wakeup :)


thanks,

Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 08:46:05 +0100,
Takashi Iwai wrote:
> > @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
> >  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > +		   STATESTS_INT_MASK);
> 
> ... here we should have the check of chip->prepared, and set WAKEEN
> only when it's false.  Otherwise WAKEEN is set up for the system
> suspend, and it might lead to spurious wakeups.  (IOW, checking the
> flag at resume doesn't help for preventing the spurious wakeup :)

Scratch my comment above; it's the code path only for the runtime
suspend in your new code, then this cleanup makes sense.


thanks,

Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 08:46:05 +0100,
Takashi Iwai wrote:
> > @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
> >  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > +		   STATESTS_INT_MASK);
> 
> ... here we should have the check of chip->prepared, and set WAKEEN
> only when it's false.  Otherwise WAKEEN is set up for the system
> suspend, and it might lead to spurious wakeups.  (IOW, checking the
> flag at resume doesn't help for preventing the spurious wakeup :)

Scratch my comment above; it's the code path only for the runtime
suspend in your new code, then this cleanup makes sense.


thanks,

Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 09:12:07 +0100,
Takashi Iwai wrote:
> 
> On Tue, 27 Oct 2020 08:46:05 +0100,
> Takashi Iwai wrote:
> > > @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
> > >  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > +		   STATESTS_INT_MASK);
> > 
> > ... here we should have the check of chip->prepared, and set WAKEEN
> > only when it's false.  Otherwise WAKEEN is set up for the system
> > suspend, and it might lead to spurious wakeups.  (IOW, checking the
> > flag at resume doesn't help for preventing the spurious wakeup :)
> 
> Scratch my comment above; it's the code path only for the runtime
> suspend in your new code, then this cleanup makes sense.

Also, as one more cleanup: from_rt in __azx_runtime_resume() can be
replaced with !chip->prepared flag, so the extra argument can be
dropped.


Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Takashi Iwai]

On Tue, 27 Oct 2020 09:12:07 +0100,
Takashi Iwai wrote:
> 
> On Tue, 27 Oct 2020 08:46:05 +0100,
> Takashi Iwai wrote:
> > > @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
> > >  	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > +		   STATESTS_INT_MASK);
> > 
> > ... here we should have the check of chip->prepared, and set WAKEEN
> > only when it's false.  Otherwise WAKEEN is set up for the system
> > suspend, and it might lead to spurious wakeups.  (IOW, checking the
> > flag at resume doesn't help for preventing the spurious wakeup :)
> 
> Scratch my comment above; it's the code path only for the runtime
> suspend in your new code, then this cleanup makes sense.

Also, as one more cleanup: from_rt in __azx_runtime_resume() can be
replaced with !chip->prepared flag, so the extra argument can be
dropped.


Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

> On Oct 27, 2020, at 16:15, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 27 Oct 2020 09:12:07 +0100,
> Takashi Iwai wrote:
>> 
>> On Tue, 27 Oct 2020 08:46:05 +0100,
>> Takashi Iwai wrote:
>>>> @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
>>>> 	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
>>>> +		   STATESTS_INT_MASK);
>>> 
>>> ... here we should have the check of chip->prepared, and set WAKEEN
>>> only when it's false.  Otherwise WAKEEN is set up for the system
>>> suspend, and it might lead to spurious wakeups.  (IOW, checking the
>>> flag at resume doesn't help for preventing the spurious wakeup :)
>> 
>> Scratch my comment above; it's the code path only for the runtime
>> suspend in your new code, then this cleanup makes sense.
> 
> Also, as one more cleanup: from_rt in __azx_runtime_resume() can be
> replaced with !chip->prepared flag, so the extra argument can be
> dropped.

Ok, will send v3 to address it.

Kai-Heng

> 
> 
> Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

> On Oct 27, 2020, at 16:15, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 27 Oct 2020 09:12:07 +0100,
> Takashi Iwai wrote:
>> 
>> On Tue, 27 Oct 2020 08:46:05 +0100,
>> Takashi Iwai wrote:
>>>> @@ -1103,10 +1115,8 @@ static int azx_runtime_suspend(struct device *dev)
>>>> 	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_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
>>>> +		   STATESTS_INT_MASK);
>>> 
>>> ... here we should have the check of chip->prepared, and set WAKEEN
>>> only when it's false.  Otherwise WAKEEN is set up for the system
>>> suspend, and it might lead to spurious wakeups.  (IOW, checking the
>>> flag at resume doesn't help for preventing the spurious wakeup :)
>> 
>> Scratch my comment above; it's the code path only for the runtime
>> suspend in your new code, then this cleanup makes sense.
> 
> Also, as one more cleanup: from_rt in __azx_runtime_resume() can be
> replaced with !chip->prepared flag, so the extra argument can be
> dropped.

Ok, will send v3 to address it.

Kai-Heng

> 
> 
> Takashi

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai Vehmanen]

Hi,

thanks, this looks like a good improvement! Some minor notes:

On Tue, 27 Oct 2020, Kai-Heng Feng wrote:

> Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
> some implicit checks, so it can make code flow more straightfoward if we
> separate runtime and systemd suspend callbacks.

straightforward -> straightforward

and systemd? Maybe just "system suspend"? :)

> While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
> original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
> wakeups on some Intel platforms") solves doesn't happen with this
> patch.

Hmm, so this was gone already with the v1 version (so not related to 
programming the WAKEEN when going to system suspend)?

> @@ -143,6 +143,7 @@ struct azx {
>  	unsigned int align_buffer_size:1;
>  	unsigned int region_requested:1;
>  	unsigned int disabled:1; /* disabled by vga_switcheroo */
> +	unsigned int prepared:1;

I wonder if "pm_prepared" would be better as ALSA API has a prepare method 
as well and this is not related. OTOH, if ok to Takashi, ok for me as 
well. 

> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> +		   ~STATESTS_INT_MASK);

This would fit to one line now. 

Br, Kai

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai Vehmanen]

Hi,

thanks, this looks like a good improvement! Some minor notes:

On Tue, 27 Oct 2020, Kai-Heng Feng wrote:

> Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
> some implicit checks, so it can make code flow more straightfoward if we
> separate runtime and systemd suspend callbacks.

straightforward -> straightforward

and systemd? Maybe just "system suspend"? :)

> While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
> original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
> wakeups on some Intel platforms") solves doesn't happen with this
> patch.

Hmm, so this was gone already with the v1 version (so not related to 
programming the WAKEEN when going to system suspend)?

> @@ -143,6 +143,7 @@ struct azx {
>  	unsigned int align_buffer_size:1;
>  	unsigned int region_requested:1;
>  	unsigned int disabled:1; /* disabled by vga_switcheroo */
> +	unsigned int prepared:1;

I wonder if "pm_prepared" would be better as ALSA API has a prepare method 
as well and this is not related. OTOH, if ok to Takashi, ok for me as 
well. 

> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> +		   ~STATESTS_INT_MASK);

This would fit to one line now. 

Br, Kai

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

Hi Kai,

> On Oct 27, 2020, at 19:38, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote:
> 
> Hi,
> 
> thanks, this looks like a good improvement! Some minor notes:
> 
> On Tue, 27 Oct 2020, Kai-Heng Feng wrote:
> 
>> Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
>> some implicit checks, so it can make code flow more straightfoward if we
>> separate runtime and systemd suspend callbacks.
> 
> straightforward -> straightforward
> 
> and systemd? Maybe just "system suspend"? :)

Typos :)
Will update them in v3.

> 
>> While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
>> original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
>> wakeups on some Intel platforms") solves doesn't happen with this
>> patch.
> 
> Hmm, so this was gone already with the v1 version (so not related to 
> programming the WAKEEN when going to system suspend)?

Yes, I was worried that this cleanup may regress the user again.

> 
>> @@ -143,6 +143,7 @@ struct azx {
>> 	unsigned int align_buffer_size:1;
>> 	unsigned int region_requested:1;
>> 	unsigned int disabled:1; /* disabled by vga_switcheroo */
>> +	unsigned int prepared:1;
> 
> I wonder if "pm_prepared" would be better as ALSA API has a prepare method 
> as well and this is not related. OTOH, if ok to Takashi, ok for me as 
> well. 

Sure, I think we should use different terms.

> 
>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>> +		   ~STATESTS_INT_MASK);
> 
> This would fit to one line now. 

Ok, will concat the lines.

Kai-Heng

> 
> Br, Kai

Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend

[Kai-Heng Feng]

Hi Kai,

> On Oct 27, 2020, at 19:38, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote:
> 
> Hi,
> 
> thanks, this looks like a good improvement! Some minor notes:
> 
> On Tue, 27 Oct 2020, Kai-Heng Feng wrote:
> 
>> Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
>> some implicit checks, so it can make code flow more straightfoward if we
>> separate runtime and systemd suspend callbacks.
> 
> straightforward -> straightforward
> 
> and systemd? Maybe just "system suspend"? :)

Typos :)
Will update them in v3.

> 
>> While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
>> original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
>> wakeups on some Intel platforms") solves doesn't happen with this
>> patch.
> 
> Hmm, so this was gone already with the v1 version (so not related to 
> programming the WAKEEN when going to system suspend)?

Yes, I was worried that this cleanup may regress the user again.

> 
>> @@ -143,6 +143,7 @@ struct azx {
>> 	unsigned int align_buffer_size:1;
>> 	unsigned int region_requested:1;
>> 	unsigned int disabled:1; /* disabled by vga_switcheroo */
>> +	unsigned int prepared:1;
> 
> I wonder if "pm_prepared" would be better as ALSA API has a prepare method 
> as well and this is not related. OTOH, if ok to Takashi, ok for me as 
> well. 

Sure, I think we should use different terms.

> 
>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>> +		   ~STATESTS_INT_MASK);
> 
> This would fit to one line now. 

Ok, will concat the lines.

Kai-Heng

> 
> Br, Kai

[PATCH v2 4/4] ALSA: hda: Reinstate runtime_allow() for all hda controllers

[Kai-Heng Feng]

The broken jack detection should be fixed by commit a6e7d0a4bdb0 ("ALSA:
hda: fix jack detection with Realtek codecs when in D3"), let's try
enabling runtime PM by default again.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - No change.

 sound/pci/hda/hda_intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 4cf90e4602c7..573250d2c6c9 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2350,6 +2350,7 @@ static int azx_probe_continue(struct azx *chip)
 
 	if (azx_has_pm_runtime(chip)) {
 		pm_runtime_use_autosuspend(&pci->dev);
+		pm_runtime_allow(&pci->dev);
 		pm_runtime_put_autosuspend(&pci->dev);
 	}
 
-- 
2.17.1

[PATCH v2 4/4] ALSA: hda: Reinstate runtime_allow() for all hda controllers

[Kai-Heng Feng]

The broken jack detection should be fixed by commit a6e7d0a4bdb0 ("ALSA:
hda: fix jack detection with Realtek codecs when in D3"), let's try
enabling runtime PM by default again.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - No change.

 sound/pci/hda/hda_intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 4cf90e4602c7..573250d2c6c9 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2350,6 +2350,7 @@ static int azx_probe_continue(struct azx *chip)
 
 	if (azx_has_pm_runtime(chip)) {
 		pm_runtime_use_autosuspend(&pci->dev);
+		pm_runtime_allow(&pci->dev);
 		pm_runtime_put_autosuspend(&pci->dev);
 	}
 
-- 
2.17.1