Re: [RFC][PATCH] thunderbolt: Quiesce AMD NHI during suspend

["Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>]

On Wed, Jul 08, 2026 at 01:37:55PM -0500, Mario Limonciello wrote:
> Some comments about your changes too.
>
> On 7/8/26 10:34, Mario Limonciello wrote:
> > ++ linux-pci
> > ++ Sanath && Basavaraj
> >
> > On 7/5/26 21:34, Chia-Lin Kao (AceLan) wrote:
> > > On some AMD USB4 systems, resuming from system suspend with a
> > > Thunderbolt dock in the picture hard-hangs the machine very early in the
> > > resume path, before the Thunderbolt driver gets a chance to rescan and
> > > re-establish the topology. It is a PCIe bus wedge: the system never
> > > reaches the point where the thunderbolt module could recover the dock,
> > > and the kernel journal does not persist past "PM: suspend entry
> > > (s2idle)".
> > >
> > > Reproduce:
> > >
> > >    1. Affected platform: an AMD USB4 host with the thunderbolt driver
> > >       bound to its NHI PCI functions. Observed on a Dell Pro 14 Plus
> > >       (PB14255, AMD Phoenix) which exposes two USB4 NHI functions
> > >       (0000:c6:00.5 and 0000:c6:00.6).
> > >
> > >    2. Put the system into s2idle (Low Power S0 Idle / Modern Standby)
> > >       with no dock attached.
> > >
> > >    3. While the system is suspended, plug in a Dell SD25TB4 Thunderbolt 4
> > >       dock (Goshen Ridge). The plug event wakes the system, and the
> > > dock's
> > >       downstream PCIe bridges are enumerated behind the AMD USB4 PCIe
> > >       tunnel during the resume.
> > >
> > >    4. The machine hard-hangs during the noirq resume phase, before the
> > >       thunderbolt driver re-establishes the topology. The Dell power
> > >       indicator flashes the "3A5W" code (3 amber + 5 white lights).
> > >
> > >    Removing the trigger makes resume reliable: unbinding the two NHI PCI
> > >    functions, or unloading the thunderbolt module, before suspend both
> > >    avoid the hang completely. The difference in those cases is the PCI
> > >    state the core restores for the driver-bound NHI on resume.
> > >
> > > EC team findings (from the platform EC log):
> > >
> > > The EC/PD/dock side is healthy across the failing cycle, which points
> > > the finger at the host resume path rather than the dock:
> > >
> > >    - Modern Standby entry and resume are seen on the EC side; the dock
> > >      plug interrupt during MS is handled and the full power sequence
> > >      recovers (SLP_S3# okay, PLTRST# received, U8-U20 sequence
> > > completed).
> > >
> > >    - The EC-to-dock DDM command exchange fully succeeds: all CMsd/CMgd
> > >      transactions return success (device type, TBT cable type, dock
> > >      module config for USB/TBT/Audio/LAN).
> > >
> > >    - PD/Type-C renegotiation is normal (power contracts up to 95W/100W);
> > >      no persistent protocol error appears in the log.
> > >
> > >    - The only suspicious symptom is host-side: the EC repeatedly logs
> > >      PmSt_Timeout_Count while waiting for the BIOS/host to consume the
> > >      wake (PWRB pressed) event. That indicates the host resume path is
> > >      late or stuck, not an EC-to-dock transport failure. The Dell "3A5W"
> > >      diagnostic LED code (3 amber + 5 white) maps in the EC code to an
> > >      SLP_S3#/PLTRST#/RUNPWROK failure indication.
> > >
> > > Root cause:
> > >
> > > With the driver bound, the PCI core restores the saved NHI config space
> > > during the noirq resume phase, including PCI_COMMAND with memory space
> > > and bus mastering enabled. This is the same phase in which the core
> > > restores the config space of the tunneled PCI bridges and whatever sits
> > > behind them (the dock). If the AMD host router is made active (memory +
> > > bus mastering) while the downstream USB4 PCIe tunnel to the dock is still
> > > being restored and is not yet ready, the host interface starts bus
> > > activity toward a not-yet-ready endpoint and the PCIe hierarchy wedges.
> > >
> > > This matches the workarounds: unbinding the NHI or unloading the module
> > > means the core does not restore an active NHI over the tunnel during
> > > noirq, and the hang disappears. Several Thunderbolt-side attempts did not
> > > help (returning early from nhi_resume_noirq(), disabling host-router wake
> > > flags, adding an unconditional delay before tunnel activation), which is
> > > why the actionable boundary is the PCI PM state of the driver-bound AMD
> > > NHI rather than a later Thunderbolt protocol step. We do not have
> > > visibility into what the host router does internally in that window; that
> > > uncertainty is the main reason for sending this as an RFC (see open
> > > questions below).
> > >
> > > What this patch does:
> > >
> > > Add a QUIRK_QUIESCE_ON_SUSPEND quirk, currently set for all AMD USB4 host
> > > routers (matched by vendor AMD + USB4 PCI class), and:
> > >
> > >    - clear PCI_COMMAND_MEMORY and PCI_COMMAND_MASTER before the PCI core
> > >      saves the NHI config space in runtime and system suspend, so the NHI
> > >      is restored inactive on resume;
> > >
> > >    - keep the NHI quiescent across nhi_resume_noirq() (return early
> > > without
> > >      touching the controller), so it stays inactive while the core
> > >      restores the tunneled bridges and the dock;
> > >
> > >    - once the whole system has resumed, re-enable memory space and bus
> > >      mastering and bring the domain back up from the PM complete callback
> > >      via tb_domain_resume_noirq(), and from nhi_runtime_resume() for the
> > >      runtime PM path.
> > >
> > > The result is that Thunderbolt/USB4 stays functional after resume instead
> > > of the machine hanging or the tunnels staying down until the module is
> > > reloaded.
> > >
> > > Test results:
> > >
> > > Tested on the Dell Pro 14 Plus (PB14255, AMD Phoenix, two USB4 host
> > > routers) with the Dell SD25TB4 dock. Without the patch, the sequence
> > > above hard-hangs during resume. With the patch, repeated suspend/resume
> > > cycles with the dock connected complete cleanly with no hang. After
> > > resume the NHI PCI_COMMAND reads back with MEMORY|MASTER set again, and
> > > the dock's downstream USB devices re-enumerate, confirming the USB tunnel
> > > is restored and the domain is functional.
> > >
> > > Caveats observed while the NHI is quiescent across noirq: config space
> > > reads to the host router time out and TMU configuration restore fails
> > > during that window; the controller and domain are fully brought back in
> > > nhi_complete(). The DP tunnel restore path was not exercised in this
> > > test, as no external display was driven through the dock during the
> > > tested cycle.
> > >
> > > Open questions for RFC discussion:
> > >
> > >    1. Root cause: what does the AMD host router actually do when it is
> > >       restored with memory + bus mastering enabled during noirq while the
> > >       downstream tunnel is not ready? Is this an expected ordering
> > >       constraint, a firmware behavior, or a hardware erratum?
> > >
> > >    2. Scope: the quirk currently matches all AMD USB4 host routers by
> > >       class. Which parts are actually affected? Should this be narrowed
> > >       (e.g. by DMI or specific device IDs) instead of applied to all AMD
> > >       USB4?
> > >
> > >    3. Mechanism: is clearing both PCI_COMMAND_MEMORY and
> > > PCI_COMMAND_MASTER
> > >       the right lever, or would clearing bus mastering alone suffice
> > > and be
> > >       less invasive? Is poking PCI_COMMAND directly acceptable in the NHI
> > >       driver, or should this move into a PCI backend op / PCI quirk?
> > >
> > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> >
> > I have no idea why I didn't receive this patch in my mailbox - I'm
> > obviously CC'ed.  But I checked my spam and it's not there either.
> >
> > But anyway, I am wondering if the real issue here is that we're
> > accessing the hardware before it's ready.  If you can readily reproduce
> > the issue, can you try this series to see if it helps?
> >
> > https://lore.kernel.org/linux-pci/20260708152650.536604-1-
> > mario.limonciello@amd.com/
> >
> > > ---
> > >   drivers/thunderbolt/nhi.c | 78 +++++++++++++++++++++++++++++++++++++--
> > >   drivers/thunderbolt/nhi.h |  5 ++-
> > >   drivers/thunderbolt/pci.c |  8 ++++
> > >   3 files changed, 85 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > > index 698fb124d529..787185c745ae 100644
> > > --- a/drivers/thunderbolt/nhi.c
> > > +++ b/drivers/thunderbolt/nhi.c
> > > @@ -16,6 +16,7 @@
> > >   #include <linux/interrupt.h>
> > >   #include <linux/iommu.h>
> > >   #include <linux/module.h>
> > > +#include <linux/pci.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/property.h>
> > >   #include <linux/string_choices.h>
> > > @@ -40,6 +41,31 @@ static bool host_reset = true;
> > >   module_param(host_reset, bool, 0444);
> > >   MODULE_PARM_DESC(host_reset, "reset USB4 host router (default: true)");
> > > +static bool nhi_quirk_quiesce_on_suspend(const struct tb_nhi *nhi)
> > > +{
> > > +    return nhi->quirks & QUIRK_QUIESCE_ON_SUSPEND;
> > > +}
> > > +
> > > +static void nhi_disable_pci_mem_master(struct tb_nhi *nhi)
> > > +{
> > > +    struct pci_dev *pdev = to_pci_dev(nhi->dev);
> > > +    u16 cmd;
> > > +
> > > +    pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > > +    cmd &= ~(PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > > +    pci_write_config_word(pdev, PCI_COMMAND, cmd);
> > > +}
> > > +
> > > +static void nhi_enable_pci_mem_master(struct tb_nhi *nhi)
> > > +{
> > > +    struct pci_dev *pdev = to_pci_dev(nhi->dev);
> > > +    u16 cmd;
> > > +
> > > +    pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > > +    cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> > > +    pci_write_config_word(pdev, PCI_COMMAND, cmd);
> > > +}
> > > +
>
> Is this basically an open coded version of pci_set_master() and
> pci_clear_master()?
Yes

>
> Or is the PCI_COMMAND_MEMORY actually important for this issue?
Did another try without PCI_COMMAND_MEMORY and it also works. So, this
flag is not needed.

>
> > >   static int ring_interrupt_index(const struct tb_ring *ring)
> > >   {
> > >       int bit = ring->hop;
> > > @@ -988,6 +1014,9 @@ static int __nhi_suspend_noirq(struct device
> > > *dev, bool wakeup)
> > >               return ret;
> > >       }
> > > +    if (nhi_quirk_quiesce_on_suspend(nhi))
> > > +        nhi_disable_pci_mem_master(nhi);
> > > +
>
> Isn't this going to break wakeup interrupts?
No, plugging in a dock and the system wakes up automatically.

>
> > >       return 0;
> > >   }
> > > @@ -1038,6 +1067,18 @@ static int nhi_resume_noirq(struct device *dev)
> > >       struct tb_nhi *nhi = tb->nhi;
> > >       int ret;
> > > +    /*
> > > +     * If the NHI was quiesced on suspend keep it quiescent (bus
> > > +     * mastering and memory access disabled) across the noirq phase.
> > > +     * The PCI core restores the config space of the tunneled PCI
> > > +     * bridges and anything behind them (e.g. a dock) here; letting the
> > > +     * AMD host router become active now can wedge the PCIe bus before
> > > +     * the dock side is ready. The controller and domain are brought
> > > +     * back up in nhi_complete() once the whole system has resumed.
> > > +     */
> > > +    if (nhi_quirk_quiesce_on_suspend(nhi))
> > > +        return 0;
> > > +
> > >       /*
> > >        * Check that the device is still there. It may be that the user
> > >        * unplugged last device which causes the host controller to go
> > > @@ -1064,16 +1105,33 @@ static int nhi_suspend(struct device *dev)
> > >   static void nhi_complete(struct device *dev)
> > >   {
> > >       struct tb *tb = dev_get_drvdata(dev);
> > > +    struct tb_nhi *nhi = tb->nhi;
> > >       /*
> > >        * If we were runtime suspended when system suspend started,
> > >        * schedule runtime resume now. It should bring the domain back
> > > -     * to functional state.
> > > +     * to functional state (and undo the quiesce, if any, via
> > > +     * nhi_runtime_resume()).
> > >        */
> > > -    if (pm_runtime_suspended(dev))
> > > +    if (pm_runtime_suspended(dev)) {
> > >           pm_runtime_resume(dev);
> > > -    else
> > > -        tb_domain_complete(tb);
> > > +        return;
> > > +    }
> > > +
> > > +    /*
> > > +     * A quiesced NHI (see __nhi_suspend_noirq()) was intentionally
> > > +     * left with bus mastering and memory access disabled and its
> > > +     * domain was not resumed in nhi_resume_noirq(). Now that the whole
> > > +     * system has resumed and the PCI core has finished restoring the
> > > +     * tunneled bridges, re-enable the controller and bring the domain
> > > +     * back up so Thunderbolt/USB4 keeps working after resume.
> > > +     */
> > > +    if (nhi_quirk_quiesce_on_suspend(nhi)) {
> > > +        nhi_enable_pci_mem_master(nhi);
> > > +        tb_domain_resume_noirq(tb);
> > > +    }
> > > +
> > > +    tb_domain_complete(tb);
> > >   }
> > >   static int nhi_runtime_suspend(struct device *dev)
> > > @@ -1091,6 +1149,10 @@ static int nhi_runtime_suspend(struct device *dev)
> > >           if (ret)
> > >               return ret;
> > >       }
> > > +
> > > +    if (nhi_quirk_quiesce_on_suspend(nhi))
> > > +        nhi_disable_pci_mem_master(nhi);
> > > +
> > >       return 0;
> > >   }
> > > @@ -1100,6 +1162,14 @@ static int nhi_runtime_resume(struct device *dev)
> > >       struct tb_nhi *nhi = tb->nhi;
> > >       int ret;
> > > +    /*
> > > +     * The NHI is quiesced (bus mastering and memory access disabled)
> > > +     * across runtime suspend as well; re-enable it before resuming the
> > > +     * domain.
> > > +     */
> > > +    if (nhi_quirk_quiesce_on_suspend(nhi))
> > > +        nhi_enable_pci_mem_master(nhi);
> > > +
> > >       if (nhi->ops->runtime_resume) {
> > >           ret = nhi->ops->runtime_resume(nhi);
> > >           if (ret)
> > > diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> > > index d488eadadfce..c8b973150434 100644
> > > --- a/drivers/thunderbolt/nhi.h
> > > +++ b/drivers/thunderbolt/nhi.h
> > > @@ -119,8 +119,9 @@ struct tb_nhi_ops {
> > >   #define PCI_CLASS_SERIAL_USB_USB4            0x0c0340
> > >   /* Host interface quirks */
> > > -#define QUIRK_AUTO_CLEAR_INT    BIT(0)
> > > -#define QUIRK_E2E        BIT(1)
> > > +#define QUIRK_AUTO_CLEAR_INT        BIT(0)
> > > +#define QUIRK_E2E            BIT(1)
> > > +#define QUIRK_QUIESCE_ON_SUSPEND    BIT(2)
> > >   /*
> > >    * Minimal number of vectors when we use MSI-X. Two for control channel
> > > diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
> > > index dbb6badda867..4cf8107f1ce8 100644
> > > --- a/drivers/thunderbolt/pci.c
> > > +++ b/drivers/thunderbolt/pci.c
> > > @@ -43,6 +43,14 @@ static void nhi_pci_check_quirks(struct
> > > tb_nhi_pci *nhi_pci)
> > >       struct tb_nhi *nhi = &nhi_pci->nhi;
> > >       struct pci_dev *pdev = to_pci_dev(nhi->dev);
> > > +    /*
> > > +     * AMD USB4 host routers may otherwise become active too early
> > > during
> > > +     * resume after the PCI core restores the saved COMMAND register.
> > > +     */
> > > +    if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> > > +        pdev->class == PCI_CLASS_SERIAL_USB_USB4)
> > > +        nhi->quirks |= QUIRK_QUIESCE_ON_SUSPEND;
> > > +
>
> If quirks do end up being correct direction thunderbolt quirks are applied
> via drivers/thunderbolt/quirks.c
Right, I should have built this RFC patch there.
But before we build a quirk, I'm more likely to figure out what the
problem is, and would like to dig deeper and do more testing if you
got any ideas.

>
> > >       if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> > >           /*
> > >            * Intel hardware supports auto clear of the interrupt
> >
>