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

[Sven Peter]

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: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Sven Peter <sven@xxxxxxxxxx>
> > > ---
> > > 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