Bug 214045

Summary:	HDA/KBL: Resetting link during shutdown hangs the machine
Product:	Drivers	Reporter:	Imre Deak (imre.deak)
Component:	Sound(ALSA)	Assignee:	Jaroslav Kysela (perex)
Status:	NEW ---
Severity:	normal	CC:	bugs, imre.deak, tiwai, youling257
Priority:	P1
Hardware:	x86-64
OS:	Linux
Kernel Version:	5.13-rc1	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	Fix to prevent link reset Patch1/3 for codec suspend-at-shutdown Patch2/3 for codec suspend-at-shutdown Patch3/3 for codec suspend-at-shutdown Fix to prevent link reset2

Description Imre Deak 2021-08-11 17:26:22 UTC

Created attachment 298291 [details]
Fix to prevent link reset

On a 7820HK KBL / ALC662 rev3 Analog machine, shutdown hangs starting with my

commit 472e18f63c425dda97b888f40f858ea54e3efc17
Author: Imre Deak <imre.deak@intel.com>
Date:   Wed Jun 23 16:46:00 2021 +0300

    ALSA: hda: Release controller display power during shutdown/reboot

commit, bisected by youling257@gmail.com. For the original report see
https://gitlab.freedesktop.org/drm/intel/-/issues/3618#note_1024665

The above commit resets the HDA controller-codec link besides powering down the controller. The power down step is a NOP on this machine, since the pre-shutdown runtime resume will power on and back down the controller. So the likely culprit is resetting the link (Youling's test confirms this), and all I can think of is that for some reason link reset itself would hang (not sure why since during system suspend this works fine) or some BIOS shutdown code depends on the link not to be reset.

Takshi, any idea what the actual root cause could be? Does the attached fix looks ok to you (also considering differences between HSW/BDW and other platforms like this)?

Comment 1 Takashi Iwai 2021-08-12 06:31:18 UTC

I'm fine with your suggested patch if that's confirmed to work.

Comment 2 Takashi Iwai 2021-08-12 11:15:16 UTC

My slight remaining suspect is about the leftover codec driver activity.
We have the shutdown hook for each codec driver, but it's not always set up, and it might be wise to forcibly runtime-suspend at shutdown.

The patch set below (consisting of 3 patches) does it.  It's totally untested.

Comment 3 Takashi Iwai 2021-08-12 11:16:39 UTC

Created attachment 298295 [details]
Patch1/3 for codec suspend-at-shutdown

Comment 4 Takashi Iwai 2021-08-12 11:17:01 UTC

Created attachment 298297 [details]
Patch2/3 for codec suspend-at-shutdown

Comment 5 Takashi Iwai 2021-08-12 11:17:22 UTC

Created attachment 298299 [details]
Patch3/3 for codec suspend-at-shutdown

Comment 6 Imre Deak 2021-08-12 11:41:39 UTC

Takashi, thanks.

Youling could you please give a go to Takashi's 3 patches instead of the one from me?

Comment 7 youling257 2021-08-12 16:15:48 UTC

test 3 patches can shutdown and suspend.

Comment 8 Takashi Iwai 2021-08-13 06:12:46 UTC

OK, thanks, that's a good news!

Though, those patches are a bit too intrusive for 5.14 release, as it has too wide influence.  So I'd like to push this for 5.15 at first, while we can take a less intrusive workaround by Imre.

Do I understand correctly that Imre's patch worked for you?

Comment 9 youling257 2021-08-13 19:52:45 UTC

Yes, Imre's patch worked for me.

Comment 10 Takashi Iwai 2021-08-14 06:36:06 UTC

Then Imre, could you submit a proper patch for merge?  I'd merge and push for 5.14-rc7.

Comment 11 Imre Deak 2021-08-16 10:09:02 UTC

Created attachment 298331 [details]
Fix to prevent link reset2

Takashi, ok will send it. I suppose the final correct state is still that the link needs to be reset before power-off. In that case are you going to revert later this fixup from me and would the attached patch be better with less changes?

Comment 12 Takashi Iwai 2021-08-16 11:50:17 UTC

Yes, that looks better!

BTW, I'm thinking whether we might want to address it in PCI core level.  Basically applying the runtime resume forcibly at PCI shutdown is utterly redundant for the devices like HD-audio that will do suspend again at its shutdown callback.  Instead, those devices prefer runtime-suspended at shutdown from the beginning.  It can be achieved by introducing a flag in struct pci_dev.
But this needs discussion with PCI people in anyway.

Comment 13 youling257 2021-09-13 10:23:33 UTC

test kernel 5.15-rc1. failed shutdown again.

Comment 14 youling257 2021-09-13 10:34:49 UTC

test Revert "ALSA: hda: Drop workaround for a hang at shutdown again" can shutdown.
the three patches not fix problem.

Comment 15 Takashi Iwai 2021-09-13 12:38:37 UTC

Hrm, that's bad.  Then I revert the change again.  Thanks for checking.

Comment 16 Maarten Lankhorst 2021-09-17 08:30:59 UTC

Looking at https://patchwork.freedesktop.org/series/94719/
I don't think the revert helps.

Under 'known issues' in BAT:

igt@core_hotunplug@unbind-rebind:

    fi-cfl-8700k: PASS -> INCOMPLETE (i915#4130)

    fi-rkl-11600: PASS -> INCOMPLETE (i915#4130)

    fi-cfl-8109u: PASS -> INCOMPLETE (i915#4130)

Comment 17 Takashi Iwai 2021-09-18 14:26:26 UTC

(In reply to Maarten Lankhorst from comment #16)
> Looking at https://patchwork.freedesktop.org/series/94719/
> I don't think the revert helps.
> 
> Under 'known issues' in BAT:
> 
> igt@core_hotunplug@unbind-rebind:
> 
>     fi-cfl-8700k: PASS -> INCOMPLETE (i915#4130)
> 
>     fi-rkl-11600: PASS -> INCOMPLETE (i915#4130)
> 
>     fi-cfl-8109u: PASS -> INCOMPLETE (i915#4130)

OK, could you try bisection please?