Hi,
On Mon, May 17, 2021 at 03:33:56PM -0700, Rajat Jain wrote:
> [+Kai]
>
> Hi,
>
> I don't understand the power management very well, so pardon my
> ignorance but I have a question.
>
> On Mon, May 10, 2021 at 3:30 AM Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> >
> > ASMedia xHCI controller only supports PME from D3cold:
> >
> > 11:00.0 USB controller: ASMedia Technology Inc. ASM1042A USB 3.0 Host Controller (prog-if 30 [XHCI])
> > ...
> > Capabilities: [78] Power Management version 3
> > Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> > Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> >
> > Now, if the controller is part of a Thunderbolt device for instance, it
> > is connected to a PCIe switch downstream port. When the hierarchy then
> > enters D3cold as a result of s2idle cycle pci_target_state() returns D0
> > because the device does not support PME from the default target_state
> > (D3hot). So what happens is that the whole hierarchy is left into D0
> > breaking power management.at suspend time or resume time
>
> Can you please provide a small call stack, when this issue is seen?
> (I'm primarily trying to understand whether the issue is breaking
> suspend, or the suspend is fine, but resume is broken?)
It is on suspend path. I added WARN_ON() to log the whole chain:
[ 37.820164] RIP: 0010:pci_target_state+0x7f/0x100
[ 37.820172] Code: 9d 00 00 00 01 19 c0 f7 d0 83 e0 03 83 bb 98 00 00 00 04 74 1b 40 84 ed 74 e1 0f b6 93 9e 00 00 00 f6 c2 1f 74 3d 85 c0 75 1c <0f> 0b 31 c0 eb cb b8 04 00 00 00 40 84 ed 74 c1 0f b6 93 9e 00 00
[ 37.820180] RSP: 0018:ffff9e6bc037bd40 EFLAGS: 00010246
[ 37.820188] RAX: 0000000000000000 RBX: ffff8a5984da4000 RCX: 0000000000000000
[ 37.820194] RDX: 0000000000000010 RSI: 0000000000000001 RDI: 0000000000000000
[ 37.820199] RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000000
[ 37.820203] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 37.820208] R13: 0000000000000001 R14: 0000000000000002 R15: 0000000000000000
[ 37.820213] FS: 0000000000000000(0000) GS:ffff8a5d1f600000(0000) knlGS:0000000000000000
[ 37.820221] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 37.820243] CR2: 00007fe0fc96e1c8 CR3: 00000002a3012001 CR4: 0000000000770ee0
[ 37.820249] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 37.820253] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 37.820256] PKRU: 55555554
[ 37.820260] Call Trace:
[ 37.820265] pci_prepare_to_sleep+0x2e/0xc0
[ 37.820275] hcd_pci_suspend_noirq+0x58/0x130
[ 37.820281] ? find_held_lock+0x32/0x90
[ 37.820289] pci_pm_suspend_noirq+0x6d/0x290
[ 37.820297] ? lock_release+0x14f/0x430
[ 37.820306] ? pci_pm_suspend_late+0x30/0x30
[ 37.820315] dpm_run_callback+0x61/0x1d0
[ 37.820330] __device_suspend_noirq+0x84/0x270
[ 37.820340] async_suspend_noirq+0x16/0x90
[ 37.820348] async_run_entry_fn+0x2e/0x120
[ 37.820360] process_one_work+0x27c/0x540
[ 37.820378] worker_thread+0x4d/0x3f0
[ 37.820387] ? rescuer_thread+0x390/0x390
[ 37.820396] kthread+0x14c/0x170
[ 37.820403] ? __kthread_bind_mask+0x60/0x60
[ 37.820413] ret_from_fork+0x1f/0x30
> > For this reason choose target_state to be D3cold if there is a upstream
> > bridge that is power manageable.
>
> It seems to me that the goal of pci_target_state() is to find the
> lowest power state that a device can be put into, from which device
> can still generate PME (if needed). So I'm curious why it starts with
> target_state = PCI_D3hot in the first place? Wouldn't starting with
> PCI_D3cold will always be better (regardless of parent bridge
> capabilities)?
That one is not my code but I suspect that for two reasons: one is historic
(older devices did not have proper D3cold support), the other is that typically
with S3/S4 it is the BIOS that in the end configures wakes (which of course
does not work with s2idle and especially devices that are not-onboard to begin
with).
I may be wrong too.
> And then I came across the commit 8feaec33b986 ("PCI / PM: Always
> check PME wakeup capability for runtime wakeup support"), which
> addresses the same device that this patch addresses, and 1 excerpt
> from the commit log that stood out:
> ============================================================
> In addition, change wakeup flag passed to pci_target_state() from false
> to true, because we want to find the deepest state *different from D3cold*
> that the device can still generate PME#. In this case, it's D0 for the
> device in question.
> ============================================================
>
> So, does returning D3Cold from this function break any other
> assumption somewhere?
I can't be 100% sure but effectively the device is in D3cold once the parent
bridge is in D3hot as the device config space is not accessible anymore.
