[PATCH AUTOSEL 5.6 10/38] ALSA: hda: Release resources at error in delayed probe

[PATCH AUTOSEL 5.6 10/38] ALSA: hda: Release resources at error in delayed probe

[Sasha Levin]

From: Takashi Iwai <tiwai@suse.de>

[ Upstream commit 2393e7555b531a534152ffe7bfd1862cacedaacb ]

snd-hda-intel driver handles the most of its probe task in the delayed
work (either via workqueue or via firmware loader).  When an error
happens in the later delayed probe, we can't deregister the device
itself because the probe callback already returned success and the
device was bound.  So, for now, we set hda->init_failed flag and make
the rest untouched until the device gets really unbound.
However, this leaves the device up running, keeping the resources
without any use that prevents other operations.

In this patch, we release the resources at first when a probe error
happens in the delayed probe stage, but keeps the top-level object, so
that the PM and other ops can still refer to the object itself.

Also for simplicity, snd_hda_intel object is allocated via devm, so
that we can get rid of the explicit kfree calls.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
Link: https://lore.kernel.org/r/20200413082034.25166-4-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 sound/pci/hda/hda_intel.c | 29 ++++++++++++++++-------------
 sound/pci/hda/hda_intel.h |  1 +
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f41d8b7864c1e..692857904d49e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1203,10 +1203,8 @@ static void azx_vs_set_state(struct pci_dev *pci,
 		if (!disabled) {
 			dev_info(chip->card->dev,
 				 "Start delayed initialization\n");
-			if (azx_probe_continue(chip) < 0) {
+			if (azx_probe_continue(chip) < 0)
 				dev_err(chip->card->dev, "initialization error\n");
-				hda->init_failed = true;
-			}
 		}
 	} else {
 		dev_info(chip->card->dev, "%s via vga_switcheroo\n",
@@ -1339,12 +1337,15 @@ static int register_vga_switcheroo(struct azx *chip)
 /*
  * destructor
  */
-static int azx_free(struct azx *chip)
+static void azx_free(struct azx *chip)
 {
 	struct pci_dev *pci = chip->pci;
 	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
 	struct hdac_bus *bus = azx_bus(chip);
 
+	if (hda->freed)
+		return;
+
 	if (azx_has_pm_runtime(chip) && chip->running)
 		pm_runtime_get_noresume(&pci->dev);
 	chip->running = 0;
@@ -1388,9 +1389,8 @@ static int azx_free(struct azx *chip)
 
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT)
 		snd_hdac_i915_exit(bus);
-	kfree(hda);
 
-	return 0;
+	hda->freed = 1;
 }
 
 static int azx_dev_disconnect(struct snd_device *device)
@@ -1406,7 +1406,8 @@ static int azx_dev_disconnect(struct snd_device *device)
 
 static int azx_dev_free(struct snd_device *device)
 {
-	return azx_free(device->device_data);
+	azx_free(device->device_data);
+	return 0;
 }
 
 #ifdef SUPPORT_VGA_SWITCHEROO
@@ -1773,7 +1774,7 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	if (err < 0)
 		return err;
 
-	hda = kzalloc(sizeof(*hda), GFP_KERNEL);
+	hda = devm_kzalloc(&pci->dev, sizeof(*hda), GFP_KERNEL);
 	if (!hda) {
 		pci_disable_device(pci);
 		return -ENOMEM;
@@ -1814,7 +1815,6 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 
 	err = azx_bus_init(chip, model[dev]);
 	if (err < 0) {
-		kfree(hda);
 		pci_disable_device(pci);
 		return err;
 	}
@@ -2340,13 +2340,16 @@ static int azx_probe_continue(struct azx *chip)
 		pm_runtime_put_autosuspend(&pci->dev);
 
 out_free:
-	if (err < 0 || !hda->need_i915_power)
+	if (err < 0) {
+		azx_free(chip);
+		return err;
+	}
+
+	if (!hda->need_i915_power)
 		display_power(chip, false);
-	if (err < 0)
-		hda->init_failed = 1;
 	complete_all(&hda->probe_wait);
 	to_hda_bus(bus)->bus_probing = 0;
-	return err;
+	return 0;
 }
 
 static void azx_remove(struct pci_dev *pci)
diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h
index 2acfff3da1a04..3fb119f090408 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -27,6 +27,7 @@ struct hda_intel {
 	unsigned int use_vga_switcheroo:1;
 	unsigned int vga_switcheroo_registered:1;
 	unsigned int init_failed:1; /* delayed init failed */
+	unsigned int freed:1; /* resources already released */
 
 	bool need_i915_power:1; /* the hda controller needs i915 power */
 };
-- 
2.20.1

[PATCH AUTOSEL 5.6 11/38] ALSA: hda: Keep the controller initialization even if no codecs found

[Sasha Levin]

From: Takashi Iwai <tiwai@suse.de>

[ Upstream commit 9479e75fca370a5220784f7596bf598c4dad0b9b ]

Currently, when the HD-audio controller driver doesn't detect any
codecs, it tries to abort the probe.  But this abort happens at the
delayed probe, i.e. the primary probe call already returned success,
hence the driver is never unbound until user does so explicitly.
As a result, it may leave the HD-audio device in the running state
without the runtime PM.  More badly, if the device is a HD-audio bus
that is tied with a GPU, GPU cannot reach to the full power down and
consumes unnecessarily much power.

This patch changes the logic after no-codec situation; it continues
probing without the further codec initialization but keep the
controller driver running normally.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
Tested-by: Roy Spliet <nouveau@spliet.org>
Link: https://lore.kernel.org/r/20200413082034.25166-5-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 sound/pci/hda/hda_intel.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 692857904d49e..aa0be85614b6c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2009,7 +2009,7 @@ static int azx_first_init(struct azx *chip)
 	/* codec detection */
 	if (!azx_bus(chip)->codec_mask) {
 		dev_err(card->dev, "no codecs found!\n");
-		return -ENODEV;
+		/* keep running the rest for the runtime PM */
 	}
 
 	if (azx_acquire_irq(chip, 0) < 0)
@@ -2303,9 +2303,11 @@ static int azx_probe_continue(struct azx *chip)
 #endif
 
 	/* create codec instances */
-	err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]);
-	if (err < 0)
-		goto out_free;
+	if (bus->codec_mask) {
+		err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]);
+		if (err < 0)
+			goto out_free;
+	}
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (chip->fw) {
@@ -2319,7 +2321,7 @@ static int azx_probe_continue(struct azx *chip)
 #endif
 	}
 #endif
-	if ((probe_only[dev] & 1) == 0) {
+	if (bus->codec_mask && !(probe_only[dev] & 1)) {
 		err = azx_codec_configure(chip);
 		if (err < 0)
 			goto out_free;
-- 
2.20.1

[PATCH AUTOSEL 5.6 12/38] ALSA: hda: Skip controller resume if not needed

[Sasha Levin]

From: Takashi Iwai <tiwai@suse.de>

[ Upstream commit c4c8dd6ef807663e42a5f04ea77cd62029eb99fa ]

The HD-audio controller does system-suspend and resume operations by
directly calling its helpers __azx_runtime_suspend() and
__azx_runtime_resume().  However, in general, we don't have to resume
always the device fully at the system resume; typically, if a device
has been runtime-suspended, we can leave it to runtime resume.

Usually for achieving this, the driver would call
pm_runtime_force_suspend() and pm_runtime_force_resume() pairs in the
system suspend and resume ops.  Unfortunately, this doesn't work for
the resume path in our case.  For handling the jack detection at the
system resume, a child codec device may need the (literally) forcibly
resume even if it's been runtime-suspended, and for that, the
controller device must be also resumed even if it's been suspended.

This patch is an attempt to improve the situation.  It replaces the
direct __azx_runtime_suspend()/_resume() calls with with
pm_runtime_force_suspend() and pm_runtime_force_resume() with a slight
trick as we've done for the codec side.  More exactly:

- azx_has_pm_runtime() check is dropped from azx_runtime_suspend() and
  azx_runtime_resume(), so that it can be properly executed from the
  system-suspend/resume path

- The WAKEEN handling depends on the card's power state now; it's set
  and cleared only for the runtime-suspend

- azx_resume() checks whether any codec may need the forcible resume
  beforehand.  If the forcible resume is required, it does temporary
  PM refcount up/down for actually triggering the runtime resume.

- A new helper function, hda_codec_need_resume(), is introduced for
  checking whether the codec needs a forcible runtime-resume, and the
  existing code is rewritten with that.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
Link: https://lore.kernel.org/r/20200413082034.25166-6-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 include/sound/hda_codec.h |  5 +++++
 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 38 +++++++++++++++++++++++++++-----------
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 3ee8036f5436d..225154a4f2ed0 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -494,6 +494,11 @@ void snd_hda_update_power_acct(struct hda_codec *codec);
 static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
 #endif
 
+static inline bool hda_codec_need_resume(struct hda_codec *codec)
+{
+	return !codec->relaxed_resume && codec->jacktbl.used;
+}
+
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 /*
  * patch firmware
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 53e7732ef7520..aed1f8188e662 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2951,7 +2951,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 static int hda_codec_force_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
-	bool forced_resume = !codec->relaxed_resume && codec->jacktbl.used;
+	bool forced_resume = hda_codec_need_resume(codec);
 	int ret;
 
 	/* The get/put pair below enforces the runtime resume even if the
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index aa0be85614b6c..02c6308502b1e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1027,7 +1027,7 @@ static int azx_suspend(struct device *dev)
 	chip = card->private_data;
 	bus = azx_bus(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	__azx_runtime_suspend(chip);
+	pm_runtime_force_suspend(dev);
 	if (bus->irq >= 0) {
 		free_irq(bus->irq, chip);
 		bus->irq = -1;
@@ -1044,7 +1044,9 @@ static int azx_suspend(struct device *dev)
 static int azx_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
+	struct hda_codec *codec;
 	struct azx *chip;
+	bool forced_resume = false;
 
 	if (!azx_is_pm_ready(card))
 		return 0;
@@ -1055,7 +1057,20 @@ static int azx_resume(struct device *dev)
 			chip->msi = 0;
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
-	__azx_runtime_resume(chip, false);
+
+	/* check for the forced resume */
+	list_for_each_codec(codec, &chip->bus) {
+		if (hda_codec_need_resume(codec)) {
+			forced_resume = true;
+			break;
+		}
+	}
+
+	if (forced_resume)
+		pm_runtime_get_noresume(dev);
+	pm_runtime_force_resume(dev);
+	if (forced_resume)
+		pm_runtime_put(dev);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
 	trace_azx_resume(chip);
@@ -1102,12 +1117,12 @@ static int azx_runtime_suspend(struct device *dev)
 	if (!azx_is_pm_ready(card))
 		return 0;
 	chip = card->private_data;
-	if (!azx_has_pm_runtime(chip))
-		return 0;
 
 	/* enable controller wake up event */
-	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
-		  STATESTS_INT_MASK);
+	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);
@@ -1118,17 +1133,18 @@ 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;
-	if (!azx_has_pm_runtime(chip))
-		return 0;
-	__azx_runtime_resume(chip, true);
+	__azx_runtime_resume(chip, from_rt);
 
 	/* disable controller Wake Up event*/
-	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
-			~STATESTS_INT_MASK);
+	if (from_rt) {
+		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
+			   ~STATESTS_INT_MASK);
+	}
 
 	trace_azx_runtime_resume(chip);
 	return 0;
-- 
2.20.1

Re: [PATCH AUTOSEL 5.6 12/38] ALSA: hda: Skip controller resume if not needed

[Takashi Iwai]

On Fri, 24 Apr 2020 14:22:10 +0200,
Sasha Levin wrote:
> 
> From: Takashi Iwai <tiwai@suse.de>
> 
> [ Upstream commit c4c8dd6ef807663e42a5f04ea77cd62029eb99fa ]
> 
> The HD-audio controller does system-suspend and resume operations by
> directly calling its helpers __azx_runtime_suspend() and
> __azx_runtime_resume().  However, in general, we don't have to resume
> always the device fully at the system resume; typically, if a device
> has been runtime-suspended, we can leave it to runtime resume.
> 
> Usually for achieving this, the driver would call
> pm_runtime_force_suspend() and pm_runtime_force_resume() pairs in the
> system suspend and resume ops.  Unfortunately, this doesn't work for
> the resume path in our case.  For handling the jack detection at the
> system resume, a child codec device may need the (literally) forcibly
> resume even if it's been runtime-suspended, and for that, the
> controller device must be also resumed even if it's been suspended.
> 
> This patch is an attempt to improve the situation.  It replaces the
> direct __azx_runtime_suspend()/_resume() calls with with
> pm_runtime_force_suspend() and pm_runtime_force_resume() with a slight
> trick as we've done for the codec side.  More exactly:
> 
> - azx_has_pm_runtime() check is dropped from azx_runtime_suspend() and
>   azx_runtime_resume(), so that it can be properly executed from the
>   system-suspend/resume path
> 
> - The WAKEEN handling depends on the card's power state now; it's set
>   and cleared only for the runtime-suspend
> 
> - azx_resume() checks whether any codec may need the forcible resume
>   beforehand.  If the forcible resume is required, it does temporary
>   PM refcount up/down for actually triggering the runtime resume.
> 
> - A new helper function, hda_codec_need_resume(), is introduced for
>   checking whether the codec needs a forcible runtime-resume, and the
>   existing code is rewritten with that.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
> Link: https://lore.kernel.org/r/20200413082034.25166-6-tiwai@suse.de
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

This commit is known to cause a regression, and the fix patch is
included in today's pull request.  If we apply this, better to wait
for the next batch including its fix.


thanks,

Takashi


> ---
>  include/sound/hda_codec.h |  5 +++++
>  sound/pci/hda/hda_codec.c |  2 +-
>  sound/pci/hda/hda_intel.c | 38 +++++++++++++++++++++++++++-----------
>  3 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> index 3ee8036f5436d..225154a4f2ed0 100644
> --- a/include/sound/hda_codec.h
> +++ b/include/sound/hda_codec.h
> @@ -494,6 +494,11 @@ void snd_hda_update_power_acct(struct hda_codec *codec);
>  static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
>  #endif
>  
> +static inline bool hda_codec_need_resume(struct hda_codec *codec)
> +{
> +	return !codec->relaxed_resume && codec->jacktbl.used;
> +}
> +
>  #ifdef CONFIG_SND_HDA_PATCH_LOADER
>  /*
>   * patch firmware
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 53e7732ef7520..aed1f8188e662 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2951,7 +2951,7 @@ static int hda_codec_runtime_resume(struct device *dev)
>  static int hda_codec_force_resume(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
> -	bool forced_resume = !codec->relaxed_resume && codec->jacktbl.used;
> +	bool forced_resume = hda_codec_need_resume(codec);
>  	int ret;
>  
>  	/* The get/put pair below enforces the runtime resume even if the
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index aa0be85614b6c..02c6308502b1e 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1027,7 +1027,7 @@ static int azx_suspend(struct device *dev)
>  	chip = card->private_data;
>  	bus = azx_bus(chip);
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> -	__azx_runtime_suspend(chip);
> +	pm_runtime_force_suspend(dev);
>  	if (bus->irq >= 0) {
>  		free_irq(bus->irq, chip);
>  		bus->irq = -1;
> @@ -1044,7 +1044,9 @@ static int azx_suspend(struct device *dev)
>  static int azx_resume(struct device *dev)
>  {
>  	struct snd_card *card = dev_get_drvdata(dev);
> +	struct hda_codec *codec;
>  	struct azx *chip;
> +	bool forced_resume = false;
>  
>  	if (!azx_is_pm_ready(card))
>  		return 0;
> @@ -1055,7 +1057,20 @@ static int azx_resume(struct device *dev)
>  			chip->msi = 0;
>  	if (azx_acquire_irq(chip, 1) < 0)
>  		return -EIO;
> -	__azx_runtime_resume(chip, false);
> +
> +	/* check for the forced resume */
> +	list_for_each_codec(codec, &chip->bus) {
> +		if (hda_codec_need_resume(codec)) {
> +			forced_resume = true;
> +			break;
> +		}
> +	}
> +
> +	if (forced_resume)
> +		pm_runtime_get_noresume(dev);
> +	pm_runtime_force_resume(dev);
> +	if (forced_resume)
> +		pm_runtime_put(dev);
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
>  
>  	trace_azx_resume(chip);
> @@ -1102,12 +1117,12 @@ static int azx_runtime_suspend(struct device *dev)
>  	if (!azx_is_pm_ready(card))
>  		return 0;
>  	chip = card->private_data;
> -	if (!azx_has_pm_runtime(chip))
> -		return 0;
>  
>  	/* enable controller wake up event */
> -	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> -		  STATESTS_INT_MASK);
> +	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);
> @@ -1118,17 +1133,18 @@ 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;
> -	if (!azx_has_pm_runtime(chip))
> -		return 0;
> -	__azx_runtime_resume(chip, true);
> +	__azx_runtime_resume(chip, from_rt);
>  
>  	/* disable controller Wake Up event*/
> -	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> -			~STATESTS_INT_MASK);
> +	if (from_rt) {
> +		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> +			   ~STATESTS_INT_MASK);
> +	}
>  
>  	trace_azx_runtime_resume(chip);
>  	return 0;
> -- 
> 2.20.1
> 

Re: [PATCH AUTOSEL 5.6 12/38] ALSA: hda: Skip controller resume if not needed

[Roy Spliet]

(Minus "Linux kernel", that list has enough volume)

Op 24-04-2020 om 13:44 schreef Takashi Iwai:
> On Fri, 24 Apr 2020 14:22:10 +0200,
> Sasha Levin wrote:
>>
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> [ Upstream commit c4c8dd6ef807663e42a5f04ea77cd62029eb99fa ]
>>
>> The HD-audio controller does system-suspend and resume operations by
>> directly calling its helpers __azx_runtime_suspend() and
>> __azx_runtime_resume().  However, in general, we don't have to resume
>> always the device fully at the system resume; typically, if a device
>> has been runtime-suspended, we can leave it to runtime resume.
>>
>> Usually for achieving this, the driver would call
>> pm_runtime_force_suspend() and pm_runtime_force_resume() pairs in the
>> system suspend and resume ops.  Unfortunately, this doesn't work for
>> the resume path in our case.  For handling the jack detection at the
>> system resume, a child codec device may need the (literally) forcibly
>> resume even if it's been runtime-suspended, and for that, the
>> controller device must be also resumed even if it's been suspended.
>>
>> This patch is an attempt to improve the situation.  It replaces the
>> direct __azx_runtime_suspend()/_resume() calls with with
>> pm_runtime_force_suspend() and pm_runtime_force_resume() with a slight
>> trick as we've done for the codec side.  More exactly:
>>
>> - azx_has_pm_runtime() check is dropped from azx_runtime_suspend() and
>>    azx_runtime_resume(), so that it can be properly executed from the
>>    system-suspend/resume path
>>
>> - The WAKEEN handling depends on the card's power state now; it's set
>>    and cleared only for the runtime-suspend
>>
>> - azx_resume() checks whether any codec may need the forcible resume
>>    beforehand.  If the forcible resume is required, it does temporary
>>    PM refcount up/down for actually triggering the runtime resume.
>>
>> - A new helper function, hda_codec_need_resume(), is introduced for
>>    checking whether the codec needs a forcible runtime-resume, and the
>>    existing code is rewritten with that.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
>> Link: https://lore.kernel.org/r/20200413082034.25166-6-tiwai@suse.de
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> This commit is known to cause a regression, and the fix patch is
> included in today's pull request.  If we apply this, better to wait
> for the next batch including its fix.

These six patches, plus Takashi's fix on top of them, do not seem to 
have made it to 5.6.7 or 5.6.8 in the end. Is there a plan to include 
them in 5.6.9?
Thanks,

Roy

> 
> 
> thanks,
> 
> Takashi
> 
> 
>> ---
>>   include/sound/hda_codec.h |  5 +++++
>>   sound/pci/hda/hda_codec.c |  2 +-
>>   sound/pci/hda/hda_intel.c | 38 +++++++++++++++++++++++++++-----------
>>   3 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
>> index 3ee8036f5436d..225154a4f2ed0 100644
>> --- a/include/sound/hda_codec.h
>> +++ b/include/sound/hda_codec.h
>> @@ -494,6 +494,11 @@ void snd_hda_update_power_acct(struct hda_codec *codec);
>>   static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
>>   #endif
>>   
>> +static inline bool hda_codec_need_resume(struct hda_codec *codec)
>> +{
>> +	return !codec->relaxed_resume && codec->jacktbl.used;
>> +}
>> +
>>   #ifdef CONFIG_SND_HDA_PATCH_LOADER
>>   /*
>>    * patch firmware
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index 53e7732ef7520..aed1f8188e662 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -2951,7 +2951,7 @@ static int hda_codec_runtime_resume(struct device *dev)
>>   static int hda_codec_force_resume(struct device *dev)
>>   {
>>   	struct hda_codec *codec = dev_to_hda_codec(dev);
>> -	bool forced_resume = !codec->relaxed_resume && codec->jacktbl.used;
>> +	bool forced_resume = hda_codec_need_resume(codec);
>>   	int ret;
>>   
>>   	/* The get/put pair below enforces the runtime resume even if the
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index aa0be85614b6c..02c6308502b1e 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -1027,7 +1027,7 @@ static int azx_suspend(struct device *dev)
>>   	chip = card->private_data;
>>   	bus = azx_bus(chip);
>>   	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
>> -	__azx_runtime_suspend(chip);
>> +	pm_runtime_force_suspend(dev);
>>   	if (bus->irq >= 0) {
>>   		free_irq(bus->irq, chip);
>>   		bus->irq = -1;
>> @@ -1044,7 +1044,9 @@ static int azx_suspend(struct device *dev)
>>   static int azx_resume(struct device *dev)
>>   {
>>   	struct snd_card *card = dev_get_drvdata(dev);
>> +	struct hda_codec *codec;
>>   	struct azx *chip;
>> +	bool forced_resume = false;
>>   
>>   	if (!azx_is_pm_ready(card))
>>   		return 0;
>> @@ -1055,7 +1057,20 @@ static int azx_resume(struct device *dev)
>>   			chip->msi = 0;
>>   	if (azx_acquire_irq(chip, 1) < 0)
>>   		return -EIO;
>> -	__azx_runtime_resume(chip, false);
>> +
>> +	/* check for the forced resume */
>> +	list_for_each_codec(codec, &chip->bus) {
>> +		if (hda_codec_need_resume(codec)) {
>> +			forced_resume = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (forced_resume)
>> +		pm_runtime_get_noresume(dev);
>> +	pm_runtime_force_resume(dev);
>> +	if (forced_resume)
>> +		pm_runtime_put(dev);
>>   	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
>>   
>>   	trace_azx_resume(chip);
>> @@ -1102,12 +1117,12 @@ static int azx_runtime_suspend(struct device *dev)
>>   	if (!azx_is_pm_ready(card))
>>   		return 0;
>>   	chip = card->private_data;
>> -	if (!azx_has_pm_runtime(chip))
>> -		return 0;
>>   
>>   	/* enable controller wake up event */
>> -	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
>> -		  STATESTS_INT_MASK);
>> +	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);
>> @@ -1118,17 +1133,18 @@ 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;
>> -	if (!azx_has_pm_runtime(chip))
>> -		return 0;
>> -	__azx_runtime_resume(chip, true);
>> +	__azx_runtime_resume(chip, from_rt);
>>   
>>   	/* disable controller Wake Up event*/
>> -	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>> -			~STATESTS_INT_MASK);
>> +	if (from_rt) {
>> +		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>> +			   ~STATESTS_INT_MASK);
>> +	}
>>   
>>   	trace_azx_runtime_resume(chip);
>>   	return 0;
>> -- 
>> 2.20.1
>>

Re: [PATCH AUTOSEL 5.6 12/38] ALSA: hda: Skip controller resume if not needed

[Sasha Levin]

On Thu, Apr 30, 2020 at 11:36:05PM +0100, Roy Spliet wrote:
>(Minus "Linux kernel", that list has enough volume)
>
>Op 24-04-2020 om 13:44 schreef Takashi Iwai:
>>On Fri, 24 Apr 2020 14:22:10 +0200,
>>Sasha Levin wrote:
>>>
>>>From: Takashi Iwai <tiwai@suse.de>
>>>
>>>[ Upstream commit c4c8dd6ef807663e42a5f04ea77cd62029eb99fa ]
>>>
>>>The HD-audio controller does system-suspend and resume operations by
>>>directly calling its helpers __azx_runtime_suspend() and
>>>__azx_runtime_resume().  However, in general, we don't have to resume
>>>always the device fully at the system resume; typically, if a device
>>>has been runtime-suspended, we can leave it to runtime resume.
>>>
>>>Usually for achieving this, the driver would call
>>>pm_runtime_force_suspend() and pm_runtime_force_resume() pairs in the
>>>system suspend and resume ops.  Unfortunately, this doesn't work for
>>>the resume path in our case.  For handling the jack detection at the
>>>system resume, a child codec device may need the (literally) forcibly
>>>resume even if it's been runtime-suspended, and for that, the
>>>controller device must be also resumed even if it's been suspended.
>>>
>>>This patch is an attempt to improve the situation.  It replaces the
>>>direct __azx_runtime_suspend()/_resume() calls with with
>>>pm_runtime_force_suspend() and pm_runtime_force_resume() with a slight
>>>trick as we've done for the codec side.  More exactly:
>>>
>>>- azx_has_pm_runtime() check is dropped from azx_runtime_suspend() and
>>>   azx_runtime_resume(), so that it can be properly executed from the
>>>   system-suspend/resume path
>>>
>>>- The WAKEEN handling depends on the card's power state now; it's set
>>>   and cleared only for the runtime-suspend
>>>
>>>- azx_resume() checks whether any codec may need the forcible resume
>>>   beforehand.  If the forcible resume is required, it does temporary
>>>   PM refcount up/down for actually triggering the runtime resume.
>>>
>>>- A new helper function, hda_codec_need_resume(), is introduced for
>>>   checking whether the codec needs a forcible runtime-resume, and the
>>>   existing code is rewritten with that.
>>>
>>>BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
>>>Link: https://lore.kernel.org/r/20200413082034.25166-6-tiwai@suse.de
>>>Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>Signed-off-by: Sasha Levin <sashal@kernel.org>
>>
>>This commit is known to cause a regression, and the fix patch is
>>included in today's pull request.  If we apply this, better to wait
>>for the next batch including its fix.
>
>These six patches, plus Takashi's fix on top of them, do not seem to 
>have made it to 5.6.7 or 5.6.8 in the end. Is there a plan to include 

AUTOSEL stuff take a while to hit the stable trees, if you want patches
in quicker they should be tagged for stable...

>them in 5.6.9?

What are the commit ids?

-- 
Thanks,
Sasha

[PATCH AUTOSEL 5.6 13/38] ALSA: hda: Explicitly permit using autosuspend if runtime PM is supported

[Sasha Levin]

From: Roy Spliet <nouveau@spliet.org>

[ Upstream commit 3ba21113bd33d49f3c300a23fc08cf114c434995 ]

This fixes runtime PM not working after a suspend-to-RAM cycle at least for
the codec-less HDA device found on NVIDIA GPUs.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
Signed-off-by: Roy Spliet <nouveau@spliet.org>
Link: https://lore.kernel.org/r/20200413082034.25166-7-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 sound/pci/hda/hda_intel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 02c6308502b1e..8519051a426e7 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2354,8 +2354,10 @@ static int azx_probe_continue(struct azx *chip)
 
 	set_default_power_save(chip);
 
-	if (azx_has_pm_runtime(chip))
+	if (azx_has_pm_runtime(chip)) {
+		pm_runtime_use_autosuspend(&pci->dev);
 		pm_runtime_put_autosuspend(&pci->dev);
+	}
 
 out_free:
 	if (err < 0) {
-- 
2.20.1

[PATCH AUTOSEL 5.6 22/38] ALSA: hda: call runtime_allow() for all hda controllers

[Sasha Levin]

From: Hui Wang <hui.wang@canonical.com>

[ Upstream commit 9a6418487b566503c772cb6e7d3d44e652b019b0 ]

Before the pci_driver->probe() is called, the pci subsystem calls
runtime_forbid() and runtime_get_sync() on this pci dev, so only call
runtime_put_autosuspend() is not enough to enable the runtime_pm on
this device.

For controllers with vgaswitcheroo feature, the pci/quirks.c will call
runtime_allow() for this dev, then the controllers could enter
rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like
Intel hda controllers, the runtime_pm is not enabled because the
runtime_allow() is not called.

Since it is no harm calling runtime_allow() twice, here let hda
driver call runtime_allow() for all controllers. Then the runtime_pm
is enabled on all controllers after the put_autosuspend() is called.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
Link: https://lore.kernel.org/r/20200414142725.6020-1-hui.wang@canonical.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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 8519051a426e7..a5fab12defde2 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2356,6 +2356,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.20.1