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

hi,


On 8/21/26 19:58, Sven Peter wrote:
> 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: [email protected]
>>> Signed-off-by: Sven Peter <[email protected]>
>>> ---
>>> 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().

this is correct I think, but

> 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().

^-- is only true for the hibernation path:

nhi_freeze_noirq() -> tb_domain_freeze_noirq() -> tb_freeze_noirq() -> 
hotplug_active = false but nothing else, so tunnels stay in the list

and then on restore:

nhi_resume_noirq() -> tb_domain_resume_noirq() -> tb_resume_noirq() -> 
tb_tunnel_activate() -> tb_dp_activate(true) -> tb_dp_dprx_start() which 
then has no callback and takes the sync path.

while the non-hibernation suspend path ends up in tb_tunnel_deactivate() 
which drops it from tunnel_list and the resume path is fine then I think.


Sven