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

Hi,

On Sun, Aug 23, 2026 at 01:34:17PM +0200, Sven Peter wrote:
> 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

Right we keep the tunnels up until the poweroff happens so that whatever
displays the user has stay enabled.

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

Indeed, I think this is a good reason to make the callback mandatory :)

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

Agree.