Re: [PATCH 5/5] thunderbolt: Cancel the DPRX read when the domain is stopped

[Sven Peter <sven@kernel.org>]

Hi,

On 8/18/26 08:17, Mika Westerberg wrote:
> Hi,
>
> On Mon, Aug 17, 2026 at 09:54:02PM +0200, Sven Peter wrote:
>> tb_stop only tears down DMA tunnels so a DP tunnel that is still
>> waiting for dprx_work to complete keeps that work queued while the
>> routers are removed and the control channel is stopped. The work only
>> stops once the DPRX timeout has passed and because it requeues itself
>> until then the flush_workqueue in tb_domain_remove won't wait for its
>> final run. The callback then runs against a domain that is already torn
>> down. A reference to that domain is kept so the completion waiting for
>> that domain to disappear in unbind will block until the timeout is
>> eventually reached.
>>
>> Just cancel the work in tb_stop. This doesn't affect DP tunnels that are
>> already alive and keeps those displays working.
>>
>> Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Sven Peter <sven@kernel.org>
>> ---
>> I also didn't run into this but noticed it when fixing the hop alloc thing
>> and think it makes sense to fix it anyway.
>> ---
>>   drivers/thunderbolt/tb.c     | 5 ++++-
>>   drivers/thunderbolt/tunnel.c | 9 +++++++++
>>   drivers/thunderbolt/tunnel.h | 1 +
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>> index e368a6b53f64..f7e68372da09 100644
>> --- a/drivers/thunderbolt/tb.c
>> +++ b/drivers/thunderbolt/tb.c
>> @@ -2958,10 +2958,13 @@ static void tb_stop(struct tb *tb)
>>   		/*
>>   		 * DMA tunnels require the driver to be functional so we
>>   		 * tear them down. Other protocol tunnels can be left
>> -		 * intact.
>> +		 * intact but a DPRX capabilities read that is still in
>> +		 * flight has to be canceled before the routers go away.
>>   		 */
>>   		if (tb_tunnel_is_dma(tunnel))
>>   			tb_tunnel_deactivate(tunnel);
>> +		else if (tb_tunnel_is_dp(tunnel))
>> +			tb_tunnel_cancel_dprx(tunnel);
> I prefer not to expose "non-generic" functions for the CM if possible. I
> wonder if this would work:
>
> 		else if (tb_tunnel_is_dp(tunnel) && !tb_tunnel_is_active(tunnel))
> 			tb_tunnel_deactivate(tunnel);

I think this might also tear down discovered tunnels because they aren't 
originally set to active in tb_tunnel_discover_dp().
Only on the resume path tb_tunnel_activate() is then called on 
everything in tcm->tunnel_list which will, because they have no 
callback, then reach tb_dp_dprx_start(), take the synchronous path and 
then finally end up in tb_tunnel_activate().

I think setting them to active in _discover is the correct thing to do 
here and should be fine, but I'm still not familiar enough with the code 
to know if this has any other side effects.

This is probably another reason to just make the callback mandatory and 
run it on resume as well so that we're not stuck for ~12 seconds in the 
synchronous path there in the worst case.


Best,


Sven