On Thu, 11 Nov 2021 18:39:36 +0100, Kai Vehmanen wrote:
Hi,
On Thu, 11 Nov 2021, Takashi Iwai wrote:
A potential problem with the current code is that it doesn't disable the runtime PM at the release procedure. Could you try the patch below? You can put WARN_ON(!chip) at azx_runtime_idle(), too, for catching the invalid runtime call.
[...]
--- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1347,8 +1347,13 @@ static void azx_free(struct azx *chip) if (hda->freed) return;
- if (azx_has_pm_runtime(chip) && chip->running)
- if (azx_has_pm_runtime(chip) && chip->running) { pm_runtime_get_noresume(&pci->dev);
pm_runtime_forbid(&pci->dev);pm_runtime_dont_use_autosuspend(&pci->dev);pm_runtime_disable(&pci->dev);- }
- chip->running = 0;
Tested with next-20211019 (first next tag where I've seen test failures) and your patch, and this seems to do the trick. I didn't have my drvdata patch included when I ran the test. No rpm_idle() calls anymore after azx_remove(), so the bug is not hit.
So far, so good...
azx_del_card_list(chip); @@ -2320,6 +2325,7 @@ static int azx_probe_continue(struct azx *chip) set_default_power_save(chip);
if (azx_has_pm_runtime(chip)) {
pm_runtime_enable(&pci->dev);This does generate warnings [ 13.495059] snd_hda_intel 0000:00:1f.3: Unbalanced pm_runtime_enable!
And later [ 54.770701] Enabling runtime PM for inactive device (0000:00:1f.3) with active children [ 54.770718] WARNING: CPU: 0 PID: 10 at drivers/base/power/runtime.c:1439 pm_runtime_enable+0x98/0xb0
Adding a "pm_runtime_set_active(&pci->dev)" to both azx_free() and azx_probe_continue() seems to help and fix still works.
Ah yes, I was confused as if it were already called in hdac_device.c, but this was about the HD-audio bus controller, not the codec.
Below is the revised one.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda: intel: More comprehensive PM runtime setup for controller driver
Currently we haven't explicitly enable and allow/forbid the runtime PM at the probe and the remove phases of HD-audio controller driver, and this was the reason of a GPF mentioned in the commit e81478bbe7a1 ("ALSA: hda: fix general protection fault in azx_runtime_idle"); namely, even after the resources are released, the runtime PM might be still invoked by the bound graphics driver during the remove of the controller driver. Although we've fixed it by clearing the drvdata reference, it'd be also better to cover the runtime PM issue more properly.
This patch adds a few more pm_runtime_*() calls at the probe and the remove time for setting and cleaning up the runtime PM. Particularly, now more explicitly pm_runtime_enable() and _disable() get called as well as pm_runtime_forbid() call at the remove callback, so that a use-after-free should be avoided.
Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_intel.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index fe51163f2d82..45e85180048c 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1347,8 +1347,14 @@ static void azx_free(struct azx *chip) if (hda->freed) return;
- if (azx_has_pm_runtime(chip) && chip->running) + if (azx_has_pm_runtime(chip) && chip->running) { pm_runtime_get_noresume(&pci->dev); + pm_runtime_disable(&pci->dev); + pm_runtime_set_suspended(&pci->dev); + pm_runtime_forbid(&pci->dev); + pm_runtime_dont_use_autosuspend(&pci->dev); + } + chip->running = 0;
azx_del_card_list(chip); @@ -2322,6 +2328,8 @@ 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_set_active(&pci->dev); + pm_runtime_enable(&pci->dev); pm_runtime_put_autosuspend(&pci->dev); }