[alsa-devel] [PATCH RFC/untested] hda-intel: suspend HDA codecs and the PCI device in error cases
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com ---
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0) - goto out_free; + goto out_power_down;
#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0) - goto out_free; + goto out_power_down; #ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0) - goto out_free; + goto out_power_down; }
err = snd_card_register(chip->card); if (err < 0) - goto out_free; + goto out_power_down;
setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3
Hi Takashi,
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
Ok, understand, indeed, that would be a problem now. But isn't that also a problem, if azx_probe_continue() doesn't run at all before the azx_free()? Wouldn't this fix it?
@@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip);
- if (azx_has_pm_runtime(chip) && chip->running) + if (azx_has_pm_runtime(chip)) pm_runtime_get_noresume(&pci->dev); chip->running = 0;
Thanks Guennadi
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3
On Fri, 08 Feb 2019 12:33:14 +0100, Guennadi Liakhovetski wrote:
Hi Takashi,
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
Ok, understand, indeed, that would be a problem now. But isn't that also a problem, if azx_probe_continue() doesn't run at all before the azx_free()?
In that case, it won't go suspend, so it won't need resume at that point.
Wouldn't this fix it?
Not really. The chip->running is also checked in azx_runtime_idle().
Also, what you're trying to change is the behavior of HD-audio controller, not about codecs.
thanks,
Takashi
@@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip);
- if (azx_has_pm_runtime(chip) && chip->running)
- if (azx_has_pm_runtime(chip)) pm_runtime_get_noresume(&pci->dev); chip->running = 0;
Thanks Guennadi
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 12:33:14 +0100, Guennadi Liakhovetski wrote:
Hi Takashi,
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
Ok, understand, indeed, that would be a problem now. But isn't that also a problem, if azx_probe_continue() doesn't run at all before the azx_free()?
In that case, it won't go suspend, so it won't need resume at that point.
Mmh, ok, then we have two different things there - see below.
Wouldn't this fix it?
Not really. The chip->running is also checked in azx_runtime_idle().
Also, what you're trying to change is the behavior of HD-audio controller, not about codecs.
When talking about codecs I meant the path
azx_probe_continue() -> set_default_power_save() -> snd_hda_set_power_save() -> codec_set_power_save() -> pm_runtime_suspended()
and those codecs are first runtime-enabled via
azx_probe_continue() -> azx_probe_codecs() -> snd_hda_codec_new() -> snd_hda_codec_device_init() -> snd_hdac_device_init() -> pm_runtime_get_noresume()
So for symmetry they should also be runtime-disabled if probing doesn't complete successfully, right? And currently this isn't the case, so, there is a problem?
As for the controller, alright, I guess, we shouldn't touch its runtime PM if probe_continue() didn't succeed. So we only have to change the codec runtime PM status in case of a failure or have I misunderstood something about them and also their status is balanced in such cases?
Thanks Guennadi
thanks,
Takashi
@@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip);
- if (azx_has_pm_runtime(chip) && chip->running)
- if (azx_has_pm_runtime(chip)) pm_runtime_get_noresume(&pci->dev); chip->running = 0;
Thanks Guennadi
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3
On Fri, 08 Feb 2019 13:14:06 +0100, Guennadi Liakhovetski wrote:
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 12:33:14 +0100, Guennadi Liakhovetski wrote:
Hi Takashi,
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
Ok, understand, indeed, that would be a problem now. But isn't that also a problem, if azx_probe_continue() doesn't run at all before the azx_free()?
In that case, it won't go suspend, so it won't need resume at that point.
Mmh, ok, then we have two different things there - see below.
Wouldn't this fix it?
Not really. The chip->running is also checked in azx_runtime_idle().
Also, what you're trying to change is the behavior of HD-audio controller, not about codecs.
When talking about codecs I meant the path
azx_probe_continue() -> set_default_power_save() -> snd_hda_set_power_save() -> codec_set_power_save() -> pm_runtime_suspended()
and those codecs are first runtime-enabled via
azx_probe_continue() -> azx_probe_codecs() -> snd_hda_codec_new() -> snd_hda_codec_device_init() -> snd_hdac_device_init() -> pm_runtime_get_noresume()
So for symmetry they should also be runtime-disabled if probing doesn't complete successfully, right? And currently this isn't the case, so, there is a problem?
It's disabled in the counterpart, snd_hdac_device_exit().
As for the controller, alright, I guess, we shouldn't touch its runtime PM if probe_continue() didn't succeed. So we only have to change the codec runtime PM status in case of a failure or have I misunderstood something about them and also their status is balanced in such cases?
Actually, I don't care much about the power consumption in the driver failure path as long as it's balanced. Ideally we should turn off everything and sets to the minimum, but this is already an abnormal state, so you can't expect everything perfect in that state.
If there is a scenario that brings the improper PM refcount, we must fix it, yes. Or, we can simplify the code a lot, I'd love to apply, too. But for other optimization, it's in a much lower priority, honestly speaking...
thanks,
Takashi
Thanks Guennadi
thanks,
Takashi
@@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip);
- if (azx_has_pm_runtime(chip) && chip->running)
- if (azx_has_pm_runtime(chip)) pm_runtime_get_noresume(&pci->dev); chip->running = 0;
Thanks Guennadi
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3
participants (2)
-
Guennadi Liakhovetski -
Takashi Iwai