Re: [PATCH net] net: thunderbolt: Tear down DMA paths before stopping the rings

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

Hi,

On Fri, Jul 31, 2026 at 05:04:42PM +0000, Fan XinRan wrote:
> tbnet_tear_down() stops both rings and frees their frame buffers before
> calling tb_xdomain_disable_paths().  tb_ring_stop() zeroes the ring's
> descriptor base and tbnet_free_buffers() unmaps and frees the pages the
> frames sit in, so by the time __tb_path_deactivate_hop() polls the hop's
> 'pending' bit, anything still in flight has nowhere to drain to.
> 
> This is the mirror image of the setup path.  tbnet_connected_work()
> already documents the invariant:
> 
> 	/* Both logins successful so enable the rings, high-speed DMA
> 	 * paths and start the network device queue.
> 	 *
> 	 * Note we enable the DMA paths last to make sure we have primed
> 	 * the Rx ring before any incoming packets are allowed to
> 	 * arrive.
> 	 */
> 
> Teardown should undo that in reverse, but does not.
> 
> On an ASMedia ASM4242 host router the 'pending' bit then never clears:
> every teardown burns the full 500 ms timeout and
> __tb_path_deactivate_hop() returns -ETIMEDOUT.  Raising the timeout to
> 5 s does not help, so the hop is not slow to drain, it never drains
> at all.
> 
> The failure is invisible above the thunderbolt core.
> __tb_path_deactivate_hops() is void and only calls tb_port_warn();
> tb_path_deactivate(), tb_tunnel_deactivate() and
> __tb_disconnect_xdomain_paths() are void as well, and
> tb_disconnect_xdomain_paths() ends in an unconditional "return 0".  So
> tb_xdomain_disable_paths() reports success and the netdev_warn() below
> it never fires.  Repeated teardowns eventually take the XDomain control
> channel down, after which the peer node is gone and only a power cycle
> brings the controller back.
> 
> Deactivating the paths first fixes it.  Measured with kretprobes on a
> stock v6.17 tree with no other patches applied, on a link that was up
> and had just carried traffic:
> 
>   before: __tb_path_deactivate_hop() returns 0 for the first hop, then
>           -ETIMEDOUT for the second 500335 us later
>   after:  0 for both, 525 us apart
> 
> Alternating the two orderings ABBA over three load levels, four
> teardowns per arm: every teardown failed before the change (21 of 21
> that ran), none failed after (0 of 24).  The before arms ran short
> because the link died partway through.  The same split shows up when
> the interface is enslaved to a bond instead of just brought down, which
> is how I ran into this in the first place.  Throughput and latency after
> the change are unchanged.
> 
> Hosts whose routers drain the hop despite the stale descriptor base see
> no functional difference, since the paths end up deactivated either way.
> 
> Fixes: 4944269305df ("thunderbolt: Properly disable path")

I don't think this is correct tag. IIRC at that point the networking driver
did not even exist yet.

> Signed-off-by: Fan XinRan <shinjiangjiang@gmail.com>

This makes the driver follow the CM guide mode closely too as  it suggests
that the paths are torn down in this order.

I tested on a couple of Intel systems and did not see any issues either.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
> Notes for reviewers, not for the commit log:
> 
> I can only test this on an ASMedia ASM4242 host router - I have no Intel
> host router to check for regressions on, and that is the gap I would most
> like a second opinion on.  The argument that other hosts are unaffected is
> that the paths end up deactivated in both orderings, and that the window
> this opens (rings still armed while tb_xdomain_disable_paths() runs) cannot
> take new traffic: __tb_path_deactivate_hop() clears hop.enable before it
> starts polling, and tbnet_tear_down() has already called netif_stop_queue().
> That is an argument, not a measurement.
> 
> Details left out of the commit log to keep it short:
> 
> - The three load levels were idle, 100 pings, and 3 s of iperf3 before each
>   teardown.  The 21 vs 24 asymmetry is because the "before" arms stopped
>   early when the link died: 8 teardowns completed idle, 7 under light load,
>   6 under heavy load.  Only one arm per load level died, so I would not read
>   a dose-response into that on its own.
> 
> - The failing hop is an ingress hop on a non-NHI port:
>     thunderbolt 0000:70:00.0: 0:5: hop deactivation failed for hop 0, index 1
> 
> - The enslave run is a smaller sample (the link dies faster there, so the
>   before arm only got two teardowns in): -ETIMEDOUT on both before the
>   change, 0 after.  Note the interface also gets destroyed on enslave
>   regardless of the ordering - that looks like a separate problem and this
>   patch does not claim to fix it.
> 
>  drivers/net/thunderbolt/main.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index 02a9165..a04c090 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c
> @@ -386,11 +386,16 @@ static void tbnet_tear_down(struct tbnet *net, bool send_logout)
>  				break;
>  		}
> 
> -		tb_ring_stop(net->rx_ring.ring);
> -		tb_ring_stop(net->tx_ring.ring);
> -		tbnet_free_buffers(&net->rx_ring);
> -		tbnet_free_buffers(&net->tx_ring);
> -
> +		/* Tear the paths down before stopping the rings.  This mirrors
> +		 * tbnet_connected_work(), which enables the paths last so the
> +		 * Rx ring is primed before packets can arrive.  Stopping a
> +		 * ring zeroes its descriptor base and tbnet_free_buffers()
> +		 * unmaps and frees the frame buffers, leaving anything still
> +		 * in flight with nowhere to drain to;
> +		 * __tb_path_deactivate_hop() then waits for the hop's
> +		 * 'pending' bit, which on some host routers never clears in
> +		 * that state.
> +		 */
>  		ret = tb_xdomain_disable_paths(net->xd,
>  					       net->local_transmit_path,
>  					       net->tx_ring.ring->hop,
> @@ -399,6 +404,11 @@ static void tbnet_tear_down(struct tbnet *net, bool send_logout)
>  		if (ret)
>  			netdev_warn(net->dev, "failed to disable DMA paths\n");
> 
> +		tb_ring_stop(net->rx_ring.ring);
> +		tb_ring_stop(net->tx_ring.ring);
> +		tbnet_free_buffers(&net->rx_ring);
> +		tbnet_free_buffers(&net->tx_ring);
> +
>  		tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
>  		net->remote_transmit_path = 0;
>  	}
> --
> 2.43.0