Re: [PATCH v5 0/5] Verify devices transition from D3cold to D0

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Dec 04, 2024 at 09:44:05PM -0600, Mario Limonciello wrote:
> On 12/4/2024 17:45, Bjorn Helgaas wrote:
> > On Wed, Dec 04, 2024 at 11:30:51AM -0600, Mario Limonciello wrote:
> > > On 8/23/2024 10:40, Mario Limonciello wrote:
> > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > 
> > > > Gary has reported that when a dock is plugged into a system at the same
> > > > time the autosuspend delay has tripped that the USB4 stack malfunctions.
> > > > 
> > > > Messages show up like this:
> > > > 
> > > > ```
> > > > thunderbolt 0000:e5:00.6: ring_interrupt_active: interrupt for TX ring 0 is already enabled
> > > > ```
> > > > 
> > > > Furthermore the USB4 router is non-functional at this point.
> > > > 
> > > > Those messages happen because the device is still in D3cold at the time
> > > > that the PCI core handed control back to the USB4 connection manager
> > > > (thunderbolt).
> > > > 
> > > > The issue is that it takes time for a device to enter D3cold and do a
> > > > conventional reset, and then more time for it to exit D3cold.
> > > > 
> > > > This appears not to be a new problem; previously there were very similar
> > > > reports from Ryzen XHCI controllers.  Quirks were added for those.
> > > > Furthermore; adding extra logging it's apparent that other PCI devices
> > > > in the system can take more than 10ms to recover from D3cold as well.
> > > > 
> > > > This series add a wait into pci_power_up() specifically for D3cold exit and
> > > > then drops the quirks that were previously used for the Ryzen XHCI controllers.
> > > > 
> > > > Mario Limonciello (5):
> > > >     PCI: Use an enum for reset type in pci_dev_wait()
> > > >     PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev_wait()
> > > >     PCI: Verify functions currently in D3cold have entered D0
> > > >     PCI: Allow Ryzen XHCI controllers into D3cold and drop delays
> > > >     PCI: Drop Radeon quirk for Macbook Pro 8.2
> > > > 
> > > >    drivers/pci/pci-driver.c    |  2 +-
> > > >    drivers/pci/pci.c           | 70 +++++++++++++++++++++++++++----------
> > > >    drivers/pci/pci.h           | 13 ++++++-
> > > >    drivers/pci/pcie/dpc.c      |  2 +-
> > > >    drivers/pci/quirks.c        | 25 -------------
> > > >    drivers/usb/host/xhci-pci.c | 11 ------
> > > >    6 files changed, 66 insertions(+), 57 deletions(-)
> > > 
> > > Bjorn,
> > > 
> > > This series has stalled a while.
> > > 
> > > Mika and I went back and forth and I think are generally in agreement so I
> > > think it's waiting on your feedback.
> > > 
> > > Can you take another look?
> > > 
> > > The alternative is to add some more piles of quirks, but I'm hoping that we
> > > can go this direction and drop a bunch of the old ones instead.
> > > 
> > > LMK if you want me to rebase it on 6.13-rc1 and resend a v6.
> > 
> > I'm still stuck on patch 2/5 because I'm not aware of any spec
> > language about polling PCI_PM_CTRL to wait for a power state
> > transition, so it seems really ad hoc.
> 
> I'm not really sure how to overcome this.  If I rebase everything I'll give
> specs another read through in case I missed anything, but I suspect you know
> these specs better than anyoe on this list.
> 
> Is it worth raising this to PCI-SIG to discuss?
> Did you perhaps already do that?

I haven't, but it seems like that might be appropriate.  Can we
formulate a question in terms of PCIe concepts?

I guess some of the pieces are:

  - ACPI puts device in D3cold
  - ACPI restores device to D0
  - Device is not yet in D0 when ACPI method has completed
  - OS read from device causes UR or RRS, so OS reads ~0

It's much more complicated if ACPI is involved because then we have
question of whether the ACPI method should have waited.

Patch 3/5 mentions "Full context restore or boot latency" language in
PCI PM r1.2, but I couldn't find section.  I don't know how a generic
spec could constrain D0->D3cold or D3cold->D0 transition time because
that depends on platform-specific power management hardware.

Bjorn