Re: [PATCH v3] thunderbolt: Assert downstream port reset on shutdown

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Wed, Jun 10, 2026 at 07:02:02PM +0530, Basavaraj Natikar wrote:
> Hi,
> 
> 
> On 6/10/2026 5:28 PM, Mika Westerberg wrote:
> > On Wed, Jun 10, 2026 at 05:02:56PM +0530, Basavaraj Natikar wrote:
> > > On 6/10/2026 4:27 PM, Mika Westerberg wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Jun 10, 2026 at 02:20:06PM +0530, Basavaraj Natikar wrote:
> > > > > On shutdown the connection manager tears down the router tree without
> > > > > signalling connected devices. A Thunderbolt 3 device directly connected
> > > > > to a USB4 host never receives a disconnect indication and during shutdown
> > > > > this can cause polling the dead link for up to 60 seconds. On some
> > > > > platforms this behavior leads to a warm reset instead of a shutdown due
> > > > > to this timeout.
> > > > > 
> > > > > Fix this by asserting PORT_CS_19.DPR on each connected downstream port
> > > > > before tearing down the router tree. This drives SBTX low (USB4 spec
> > > > > section 6.9), causing the device to detect SBRX low and transition to
> > > > > Uninitialized Unplugged state immediately.
> > > > > 
> > > > > Always do this on system shutdown/reboot by forcing host_reset in the
> > > > > PCI ->shutdown callback. On plain driver unload only do it when the host
> > > > > router was actually reset on load (host_reset=1), since in that case the
> > > > > tunnels are not preserved across reload anyway; with host_reset=0 the
> > > > > tunnels are kept alive across unload/reload so the links are left intact.
> > > > > Restrict the reset to Thunderbolt 3 devices.
> > > > > 
> > > > > Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > > > Co-developed-by: Sanath S <Sanath.S@amd.com>
> > > > > Signed-off-by: Sanath S <Sanath.S@amd.com>
> > > > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> > > > > ---
> > > > > v3:
> > > > >    - Rebase on top of the thunderbolt next branch; the PCI ->shutdown hook
> > > > >      now lives in drivers/thunderbolt/pci.c (nhi_pci_driver_shutdown()).
> > > > >    - Add kernel-doc for tb_port_reset() now that it is exported.
> > > > >    - Move the host_reset flag into struct tb_nhi instead of struct tb, and
> > > > >      set it from nhi_probe() instead of tb_domain_add().
> > > > >    - Spell out "Thunderbolt 3" instead of "TBT3" in the kernel-doc.
> > > > >    - Reword the debug message to "downstream port reset failed, continuing".
> > > > > 
> > > > > v2:
> > > > >    - Add Reviewed-by tag from Mario Limonciello.
> > > > >    - Restrict the DPR assertion to Thunderbolt 3 devices, since the issue
> > > > >      only affects Thunderbolt 3 devices.
> > > > >    - Only assert DPR on system shutdown/reboot (the PCI ->shutdown callback
> > > > >      forces host_reset) or when the host router was actually reset on load.
> > > > >    - Reword comments and commit message to refer to the "router tree"
> > > > >      instead of the "switch tree".
> > > > > ---
> > > > >    drivers/thunderbolt/nhi.c    |  2 ++
> > > > >    drivers/thunderbolt/pci.c    | 16 +++++++++++++++-
> > > > >    drivers/thunderbolt/switch.c | 11 ++++++++++-
> > > > >    drivers/thunderbolt/tb.c     | 21 +++++++++++++++++++++
> > > > >    drivers/thunderbolt/tb.h     |  1 +
> > > > >    include/linux/thunderbolt.h  |  6 ++++++
> > > > >    6 files changed, 55 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > > > > index 0f795ea58756..698fb124d529 100644
> > > > > --- a/drivers/thunderbolt/nhi.c
> > > > > +++ b/drivers/thunderbolt/nhi.c
> > > > > @@ -1235,6 +1235,8 @@ int nhi_probe(struct tb_nhi *nhi)
> > > > >    	init_completion(&nhi->domain_released);
> > > > > +	nhi->host_reset = host_reset;
> > > > > +
> > > > >    	res = tb_domain_add(tb, host_reset);
> > > > >    	if (res) {
> > > > >    		/*
> > > > > diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
> > > > > index bbd186c29ef7..be63a4647b59 100644
> > > > > --- a/drivers/thunderbolt/pci.c
> > > > > +++ b/drivers/thunderbolt/pci.c
> > > > > @@ -493,6 +493,20 @@ static void nhi_pci_remove(struct pci_dev *pdev)
> > > > >    	nhi_shutdown(nhi);
> > > > >    }
> > > > > +static void nhi_pci_driver_shutdown(struct pci_dev *pdev)
> > > > nhi_pci_shutdown()
> > > > 
> > > > > +{
> > > > > +	struct tb *tb = pci_get_drvdata(pdev);
> > > > > +	struct tb_nhi *nhi = tb->nhi;
> > > > > +
> > > > > +	/*
> > > > > +	 * Force host_reset so the connection manager asserts DPR and signals
> > > > > +	 * disconnect to connected devices before the router tree is removed.
> > > > > +	 * Only Thunderbolt 3 devices are reset.
> > > > > +	 */
> > > > > +	nhi->host_reset = true;
> > > > > +	nhi_pci_remove(pdev);
> > > > > +}
> > > > > +
> > > > >    static struct pci_device_id nhi_ids[] = {
> > > > >    	/*
> > > > >    	 * We have to specify class, the TB bridges use the same device and
> > > > > @@ -593,7 +607,7 @@ static struct pci_driver nhi_driver = {
> > > > >    	.id_table = nhi_ids,
> > > > >    	.probe = nhi_pci_probe,
> > > > >    	.remove = nhi_pci_remove,
> > > > > -	.shutdown = nhi_pci_remove,
> > > > > +	.shutdown = nhi_pci_driver_shutdown,
> > > > nhi_pci_shutdown
> > > > 
> > > > Otherwise looks good.
> > > Thank you Mika for the review.
> > > 
> > > There is already a static nhi_pci_shutdown() in pci.c (the
> > > tb_nhi_ops.shutdown handler at line 233, also called from nhi_shutdown()),
> > > so reusing that name for the pci_driver ->shutdown callback would collide
> > > and fail to build.
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git/tree/drivers/thunderbolt/pci.c?h=next#n233
> > > 
> > > Would you prefer:
> > > 
> > >    1. Keeping a distinct name for the pci_driver hook, e.g.
> > >       nhi_pci_driver_shutdown() as in this version, or
> > > 
> > >    2. Renaming the existing tb_nhi_ops handler (say, to
> > >       nhi_pci_ops_shutdown()) so the new pci_driver ->shutdown callback
> > >       can take the nhi_pci_shutdown() name?
> > > 
> > > I'm fine with either; please let me know which you prefer and I'll send v4
> > > accordingly.
> > Put this code to nhi.c::nhi_shutdown() as that's applicable also to non-PCI
> > hosts, right?
> 
> I tried that, but nhi_shutdown() runs too late in the teardown to be
> useful here. On the shutdown/remove path:
>   nhi_pci_remove()
>     -> tb_domain_remove()
>          -> cm_ops->stop() == tb_stop()   /* reads host_reset, asserts DPR */
>     -> nhi_shutdown()                      /* ring/HW teardown, runs last */
> tb_stop() is what reads host_reset and asserts the DPR, and it runs
> inside tb_domain_remove() -- before nhi_shutdown(). So host_reset has to
> be set *before* tb_domain_remove(), and nhi_shutdown() would set it after
> tb_stop() has already run.

Ah okay.

> To still keep it generic for non-PCI hosts, how about a small generic
> helper in nhi.c that just forces the flag, called from the bus ->shutdown
> callback before the normal removal:
>   /* nhi.c */
>   void nhi_host_shutdown(struct tb_nhi *nhi)
>   {
>     /* Force DPR on connected Thunderbolt 3 devices, see tb_stop(). */
>     nhi->host_reset = true;
>   }
>   /* pci.c */
>   static void nhi_pci_host_shutdown(struct pci_dev *pdev)
>   {
>     struct tb *tb = pci_get_drvdata(pdev);
>     nhi_host_shutdown(tb->nhi);
>     nhi_pci_remove(pdev);
>   }
>   ...
>     .shutdown = nhi_pci_host_shutdown,
> 
> This leaves nhi_pci_remove() (and nhi_shutdown()) unchanged, and the
> host_reset logic lives in nhi.c so a future non-PCI host can reuse
> nhi_host_shutdown() the same way. Does that work for you, or would you
> 
> prefer it shaped differently?

I really would like to name the PCI driver shutdown hook as
nhi_pci_shutdown. What about this:

1. Rename nhi_pci_shutdown -> nhi_pci_release_irq as that's what it does
   (keep the callback member name shutdown still)

2. Rename nhi_pci_remove -> nhi_pci_do_remove that takes new parameter
   'reset' or so and passes this down to the tb_domain_remove() and also
   ->tb_stop(). 

3. Add nhi_pci_shutdown() that calls nhi_pci_do_remove(pdev, true);
4. Add nhi_pci_remove() that calls nhi_pci_do_remove(pdev, false);

Or something along those lines. If it looks horrible then we can do the
same but with the flag instead of passing the parameter down.