* [PATCH 0/1] PCI: Add mandatory delay for D3cold resume @ 2026-07-08 15:26 Mario Limonciello 2026-07-08 15:26 ` [PATCH] PCI: Apply mandatory recovery delay on return from D3cold Mario Limonciello 0 siblings, 1 reply; 4+ messages in thread From: Mario Limonciello @ 2026-07-08 15:26 UTC (permalink / raw) To: Bjorn Helgaas Cc: open list:PCI SUBSYSTEM, open list:PCI SUBSYSTEM, open list, Mario Limonciello, mrh, stern, hannes, jase_harley, superveridical, david.c.hubbard, bugzilla, michal.pecio An issue has been reported that xHCI host controller doesn't come back from resume on some systems. This issue has a failure rate, but that failure rate is confusingly tied to the size of the VRAM reservation that the BIOS makes. That is if the VRAM reservation is small (for example 512MB) then no failures. There are some comments that mention that the larger the VRAM size the higher the failure rate. I personally don't really understand why this would matter, but I would like to mention it in case it jogs anyone else's brain. Various workarounds work: * Using legacy interrupts instead of MSI * Resetting controller on resume * Creating device link dependency between XHCI and display PCI devices (for device resume ordering) None of those workarounds appear to get at the root of the problem to me though. I did some tracing around the code and cross referenced the PCI spec. The XHCI controller should be in D3cold during suspend and thus should go through a D3->D0 transition. The PCI spec says that D3 transitions need 10ms delay. However we don't have any mandatory delays for endpoints in D3cold, only bridges have this. For endpoints we only have mandatory delays in D3hot. So my suspicion is that adding a mandatory delay for the D3cold to D0 transition will help this issue, which is what this patch series does. Cc: mrh@frame.work Cc: stern@rowland.harvard.edu Cc: hannes@vonhaugwitz.com Cc: jase_harley@protonmail.com Cc: superveridical@gmail.com Cc: david.c.hubbard@gmail.com Cc: bugzilla@logical.ink Cc: michal.pecio@gmail.com Mario Limonciello (1): PCI: Apply mandatory recovery delay on return from D3cold drivers/pci/pci.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] PCI: Apply mandatory recovery delay on return from D3cold 2026-07-08 15:26 [PATCH 0/1] PCI: Add mandatory delay for D3cold resume Mario Limonciello @ 2026-07-08 15:26 ` Mario Limonciello 2026-07-10 20:16 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Mario Limonciello @ 2026-07-08 15:26 UTC (permalink / raw) To: Bjorn Helgaas Cc: open list:PCI SUBSYSTEM, open list:PCI SUBSYSTEM, open list, Mario Limonciello, mrh, stern, hannes, jase_harley, superveridical, david.c.hubbard, bugzilla, michal.pecio Per the "PCI Bus Power Management Interface Specification", rev. 1.2, sec. 5.4, there is a minimum recovery time between programming a function from D3 to D0 and accessing it. The spec does not limit this to D3hot; it applies to the D3cold to D0 transition as well. pci_power_up() only honors this delay on the D3hot branch. When a device returns from D3cold, platform_pci_set_power_state() has already restored main power before PCI_PM_CTRL is read, so the state read from the register is D0 and the transition delay block is skipped by the if (state == PCI_D0) goto end; early return. The register value is masked with PCI_PM_CTRL_STATE_MASK and cannot represent D3cold, so only dev->current_state still reflects the D3cold origin at this point. Apply the delay based on dev->current_state, ahead of the early return, so it takes effect on the D3cold to D0 path before the device is accessed. Use the device's d3cold_delay, which the platform may tune via _DSM and quirks may raise, rather than the D3hot delay. To keep the existing D3hot callers unchanged, pci_dev_d3_sleep() now takes the delay in milliseconds and a pci_dev_d3hot_sleep() wrapper supplies the D3hot delay as before. Reported-by: mrh@frame.work Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221073 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- Cc: mrh@frame.work Cc: stern@rowland.harvard.edu Cc: hannes@vonhaugwitz.com Cc: jase_harley@protonmail.com Cc: superveridical@gmail.com Cc: david.c.hubbard@gmail.com Cc: bugzilla@logical.ink Cc: michal.pecio@gmail.com --- drivers/pci/pci.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 77b17b13ee615..e09cfb28fe61c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -81,9 +81,8 @@ struct pci_pme_device { */ #define PCIE_RESET_READY_POLL_MS 60000 /* msec */ -static void pci_dev_d3_sleep(struct pci_dev *dev) +static void pci_dev_d3_sleep(unsigned int delay_ms) { - unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay); unsigned int upper; if (delay_ms) { @@ -94,6 +93,11 @@ static void pci_dev_d3_sleep(struct pci_dev *dev) } } +static void pci_dev_d3hot_sleep(struct pci_dev *dev) +{ + pci_dev_d3_sleep(max(dev->d3hot_delay, pci_pm_d3hot_delay)); +} + bool pci_reset_supported(struct pci_dev *dev) { return dev->reset_methods[0] != 0; @@ -1333,6 +1337,16 @@ int pci_power_up(struct pci_dev *dev) need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET); + /* + * A device returning from D3cold has already been powered back on by + * platform_pci_set_power_state() above, so PCI_PM_CTRL now reads back + * as D0 and the transition delays below are skipped. PCI PM 1.2 still + * requires a minimum recovery time on the D3 to D0 transition, so apply + * the device's D3cold recovery delay here before it is accessed. + */ + if (dev->current_state == PCI_D3cold) + pci_dev_d3_sleep(dev->d3cold_delay); + if (state == PCI_D0) goto end; @@ -1344,7 +1358,7 @@ int pci_power_up(struct pci_dev *dev) /* Mandatory transition delays; see PCI PM 1.2. */ if (state == PCI_D3hot) { - pci_dev_d3_sleep(dev); + pci_dev_d3hot_sleep(dev); if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) { ret = pci_dev_wait(dev, "power up D3hot->D0uninitialized", PCIE_RESET_READY_POLL_MS); @@ -1514,7 +1528,7 @@ static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state, bool /* Mandatory power management transition delays; see PCI PM 1.2. */ if (state == PCI_D3hot) - pci_dev_d3_sleep(dev); + pci_dev_d3hot_sleep(dev); else if (state == PCI_D2) udelay(PCI_PM_D2_DELAY); @@ -4511,12 +4525,12 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe) csr &= ~PCI_PM_CTRL_STATE_MASK; csr |= PCI_D3hot; pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); - pci_dev_d3_sleep(dev); + pci_dev_d3hot_sleep(dev); csr &= ~PCI_PM_CTRL_STATE_MASK; csr |= PCI_D0; pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); - pci_dev_d3_sleep(dev); + pci_dev_d3hot_sleep(dev); ret = pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS); pci_dev_reset_iommu_done(dev); -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: Apply mandatory recovery delay on return from D3cold 2026-07-08 15:26 ` [PATCH] PCI: Apply mandatory recovery delay on return from D3cold Mario Limonciello @ 2026-07-10 20:16 ` Bjorn Helgaas 2026-07-10 20:19 ` Mario Limonciello 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2026-07-10 20:16 UTC (permalink / raw) To: Mario Limonciello Cc: Bjorn Helgaas, linux-pci, linux-usb, linux-kernel, mrh, stern, hannes, jase_harley, superveridical, david.c.hubbard, bugzilla, michal.pecio, Rafael J. Wysocki, linux-pm [+cc Rafael, linux-pm] On Wed, Jul 08, 2026 at 10:26:50AM -0500, Mario Limonciello wrote: > Per the "PCI Bus Power Management Interface Specification", rev. 1.2, > sec. 5.4, there is a minimum recovery time between programming a function > from D3 to D0 and accessing it. The spec does not limit this to D3hot; it > applies to the D3cold to D0 transition as well. > > pci_power_up() only honors this delay on the D3hot branch. When a device > returns from D3cold, platform_pci_set_power_state() has already restored > main power before PCI_PM_CTRL is read, so the state read from the register > is D0 and the transition delay block is skipped by the > > if (state == PCI_D0) > goto end; > > early return. The register value is masked with PCI_PM_CTRL_STATE_MASK > and cannot represent D3cold, so only dev->current_state still reflects the > D3cold origin at this point. > > Apply the delay based on dev->current_state, ahead of the early return, so > it takes effect on the D3cold to D0 path before the device is accessed. > Use the device's d3cold_delay, which the platform may tune via _DSM and > quirks may raise, rather than the D3hot delay. > > To keep the existing D3hot callers unchanged, pci_dev_d3_sleep() now takes > the delay in milliseconds and a pci_dev_d3hot_sleep() wrapper supplies the > D3hot delay as before. > > Reported-by: mrh@frame.work > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221073 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > Cc: mrh@frame.work > Cc: stern@rowland.harvard.edu > Cc: hannes@vonhaugwitz.com > Cc: jase_harley@protonmail.com > Cc: superveridical@gmail.com > Cc: david.c.hubbard@gmail.com > Cc: bugzilla@logical.ink > Cc: michal.pecio@gmail.com > --- > drivers/pci/pci.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 77b17b13ee615..e09cfb28fe61c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -81,9 +81,8 @@ struct pci_pme_device { > */ > #define PCIE_RESET_READY_POLL_MS 60000 /* msec */ > > -static void pci_dev_d3_sleep(struct pci_dev *dev) > +static void pci_dev_d3_sleep(unsigned int delay_ms) > { > - unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay); > unsigned int upper; > > if (delay_ms) { > @@ -94,6 +93,11 @@ static void pci_dev_d3_sleep(struct pci_dev *dev) > } > } > > +static void pci_dev_d3hot_sleep(struct pci_dev *dev) > +{ > + pci_dev_d3_sleep(max(dev->d3hot_delay, pci_pm_d3hot_delay)); > +} > + > bool pci_reset_supported(struct pci_dev *dev) > { > return dev->reset_methods[0] != 0; > @@ -1333,6 +1337,16 @@ int pci_power_up(struct pci_dev *dev) > need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) && > !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET); > > + /* > + * A device returning from D3cold has already been powered back on by > + * platform_pci_set_power_state() above, so PCI_PM_CTRL now reads back > + * as D0 and the transition delays below are skipped. PCI PM 1.2 still > + * requires a minimum recovery time on the D3 to D0 transition, so apply > + * the device's D3cold recovery delay here before it is accessed. > + */ > + if (dev->current_state == PCI_D3cold) > + pci_dev_d3_sleep(dev->d3cold_delay); My understanding is that the "mandatory transition delays" below only cover a transition caused by the write to PCI_PM_CTRL, which would be from D1, D2, or D3hot to D0. If the device started in D3cold, platform_pci_set_power_state(PCI_D0) should transition it to D0uninitialized *and* take care of any required delays [1]. The device should be Configuration-Ready upon return (and we've already read PCI_PM_CTRL above, and the read returned something other than PCI_ERROR_RESPONSE). But evidently adding more delay does make a difference, even though the config read of PCI_PM_CTRL seemed successful. I guess it's conceivable that platform AML doesn't wait quite long enough, although it does seem to affect more than one platform (AMD Strix Halo, Framework Desktop/AMD Ryzen AI Max 300, Lenovo ThinkPad T14 Gen 6 AMD Ryzen AI 7 Pro 350), and I *assume* the issue doesn't happen under Windows? [1] https://lore.kernel.org/linux-pci/CAJZ5v0iZN5NtUztqe=MxCRcXdBaaqzZ749OqSUkadwwBy0ugUQ@mail.gmail.com/ > if (state == PCI_D0) > goto end; > > @@ -1344,7 +1358,7 @@ int pci_power_up(struct pci_dev *dev) > > /* Mandatory transition delays; see PCI PM 1.2. */ > if (state == PCI_D3hot) { > - pci_dev_d3_sleep(dev); > + pci_dev_d3hot_sleep(dev); > if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) { > ret = pci_dev_wait(dev, "power up D3hot->D0uninitialized", > PCIE_RESET_READY_POLL_MS); > @@ -1514,7 +1528,7 @@ static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state, bool > > /* Mandatory power management transition delays; see PCI PM 1.2. */ > if (state == PCI_D3hot) > - pci_dev_d3_sleep(dev); > + pci_dev_d3hot_sleep(dev); > else if (state == PCI_D2) > udelay(PCI_PM_D2_DELAY); > > @@ -4511,12 +4525,12 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe) > csr &= ~PCI_PM_CTRL_STATE_MASK; > csr |= PCI_D3hot; > pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); > - pci_dev_d3_sleep(dev); > + pci_dev_d3hot_sleep(dev); > > csr &= ~PCI_PM_CTRL_STATE_MASK; > csr |= PCI_D0; > pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); > - pci_dev_d3_sleep(dev); > + pci_dev_d3hot_sleep(dev); > > ret = pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS); > pci_dev_reset_iommu_done(dev); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: Apply mandatory recovery delay on return from D3cold 2026-07-10 20:16 ` Bjorn Helgaas @ 2026-07-10 20:19 ` Mario Limonciello 0 siblings, 0 replies; 4+ messages in thread From: Mario Limonciello @ 2026-07-10 20:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, linux-pci, linux-usb, linux-kernel, mrh, stern, hannes, jase_harley, superveridical, david.c.hubbard, bugzilla, michal.pecio, Rafael J. Wysocki, linux-pm On 7/10/26 15:16, Bjorn Helgaas wrote: > [+cc Rafael, linux-pm] > > On Wed, Jul 08, 2026 at 10:26:50AM -0500, Mario Limonciello wrote: >> Per the "PCI Bus Power Management Interface Specification", rev. 1.2, >> sec. 5.4, there is a minimum recovery time between programming a function >> from D3 to D0 and accessing it. The spec does not limit this to D3hot; it >> applies to the D3cold to D0 transition as well. >> >> pci_power_up() only honors this delay on the D3hot branch. When a device >> returns from D3cold, platform_pci_set_power_state() has already restored >> main power before PCI_PM_CTRL is read, so the state read from the register >> is D0 and the transition delay block is skipped by the >> >> if (state == PCI_D0) >> goto end; >> >> early return. The register value is masked with PCI_PM_CTRL_STATE_MASK >> and cannot represent D3cold, so only dev->current_state still reflects the >> D3cold origin at this point. >> >> Apply the delay based on dev->current_state, ahead of the early return, so >> it takes effect on the D3cold to D0 path before the device is accessed. >> Use the device's d3cold_delay, which the platform may tune via _DSM and >> quirks may raise, rather than the D3hot delay. >> >> To keep the existing D3hot callers unchanged, pci_dev_d3_sleep() now takes >> the delay in milliseconds and a pci_dev_d3hot_sleep() wrapper supplies the >> D3hot delay as before. >> >> Reported-by: mrh@frame.work >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221073 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> Cc: mrh@frame.work >> Cc: stern@rowland.harvard.edu >> Cc: hannes@vonhaugwitz.com >> Cc: jase_harley@protonmail.com >> Cc: superveridical@gmail.com >> Cc: david.c.hubbard@gmail.com >> Cc: bugzilla@logical.ink >> Cc: michal.pecio@gmail.com >> --- >> drivers/pci/pci.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 77b17b13ee615..e09cfb28fe61c 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -81,9 +81,8 @@ struct pci_pme_device { >> */ >> #define PCIE_RESET_READY_POLL_MS 60000 /* msec */ >> >> -static void pci_dev_d3_sleep(struct pci_dev *dev) >> +static void pci_dev_d3_sleep(unsigned int delay_ms) >> { >> - unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay); >> unsigned int upper; >> >> if (delay_ms) { >> @@ -94,6 +93,11 @@ static void pci_dev_d3_sleep(struct pci_dev *dev) >> } >> } >> >> +static void pci_dev_d3hot_sleep(struct pci_dev *dev) >> +{ >> + pci_dev_d3_sleep(max(dev->d3hot_delay, pci_pm_d3hot_delay)); >> +} >> + >> bool pci_reset_supported(struct pci_dev *dev) >> { >> return dev->reset_methods[0] != 0; >> @@ -1333,6 +1337,16 @@ int pci_power_up(struct pci_dev *dev) >> need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) && >> !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET); >> >> + /* >> + * A device returning from D3cold has already been powered back on by >> + * platform_pci_set_power_state() above, so PCI_PM_CTRL now reads back >> + * as D0 and the transition delays below are skipped. PCI PM 1.2 still >> + * requires a minimum recovery time on the D3 to D0 transition, so apply >> + * the device's D3cold recovery delay here before it is accessed. >> + */ >> + if (dev->current_state == PCI_D3cold) >> + pci_dev_d3_sleep(dev->d3cold_delay); > > My understanding is that the "mandatory transition delays" below only > cover a transition caused by the write to PCI_PM_CTRL, which would be > from D1, D2, or D3hot to D0. > > If the device started in D3cold, platform_pci_set_power_state(PCI_D0) > should transition it to D0uninitialized *and* take care of any > required delays [1]. The device should be Configuration-Ready upon > return (and we've already read PCI_PM_CTRL above, and the read > returned something other than PCI_ERROR_RESPONSE). FWIW - I was wondering if we shouldn't be reading PCI_PM_CTRL until the mandatory delay too. > > But evidently adding more delay does make a difference, even though > the config read of PCI_PM_CTRL seemed successful. I guess it's > conceivable that platform AML doesn't wait quite long enough, although > it does seem to affect more than one platform (AMD Strix Halo, > Framework Desktop/AMD Ryzen AI Max 300, Lenovo ThinkPad T14 Gen 6 AMD > Ryzen AI 7 Pro 350), and I *assume* the issue doesn't happen under > Windows? I don't want to make any rash decisions until we have more testing evidence than one person testing 4 cycles. I would like more of the people who reported this to confirm the patches really help and it wasn't just luck in those 4 cycles for that 1 person. The part that still doesn't sit well with me is this behavior being tied to the VRAM size. I don't feel these should be connected. > > [1] https://lore.kernel.org/linux-pci/CAJZ5v0iZN5NtUztqe=MxCRcXdBaaqzZ749OqSUkadwwBy0ugUQ@mail.gmail.com/ > >> if (state == PCI_D0) >> goto end; >> >> @@ -1344,7 +1358,7 @@ int pci_power_up(struct pci_dev *dev) >> >> /* Mandatory transition delays; see PCI PM 1.2. */ >> if (state == PCI_D3hot) { >> - pci_dev_d3_sleep(dev); >> + pci_dev_d3hot_sleep(dev); >> if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) { >> ret = pci_dev_wait(dev, "power up D3hot->D0uninitialized", >> PCIE_RESET_READY_POLL_MS); >> @@ -1514,7 +1528,7 @@ static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state, bool >> >> /* Mandatory power management transition delays; see PCI PM 1.2. */ >> if (state == PCI_D3hot) >> - pci_dev_d3_sleep(dev); >> + pci_dev_d3hot_sleep(dev); >> else if (state == PCI_D2) >> udelay(PCI_PM_D2_DELAY); >> >> @@ -4511,12 +4525,12 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe) >> csr &= ~PCI_PM_CTRL_STATE_MASK; >> csr |= PCI_D3hot; >> pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); >> - pci_dev_d3_sleep(dev); >> + pci_dev_d3hot_sleep(dev); >> >> csr &= ~PCI_PM_CTRL_STATE_MASK; >> csr |= PCI_D0; >> pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); >> - pci_dev_d3_sleep(dev); >> + pci_dev_d3hot_sleep(dev); >> >> ret = pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS); >> pci_dev_reset_iommu_done(dev); >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-10 20:19 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-08 15:26 [PATCH 0/1] PCI: Add mandatory delay for D3cold resume Mario Limonciello 2026-07-08 15:26 ` [PATCH] PCI: Apply mandatory recovery delay on return from D3cold Mario Limonciello 2026-07-10 20:16 ` Bjorn Helgaas 2026-07-10 20:19 ` Mario Limonciello
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox