Message-ID: <6f8b276f708404762557046065bfd8f4f07880dc.camel@redhat.com>
Date:   Mon, 26 Nov 2018 15:37:31 -0500
From:   Lyude Paul <lyude@...hat.com>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Sean Paul <sean@...rly.run>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>, Dave Airlie <airlied@...ux.ie>,
        Jerry Zuo <Jerry.Zuo@....com>
Subject: Re: [PATCH] drm/dp_mst: Skip validating ports during destruction,
 just ref

On Thu, 2018-11-22 at 09:34 +0100, Daniel Vetter wrote:
> On Tue, Nov 13, 2018 at 11:47 PM Lyude Paul <lyude@...hat.com> wrote:
> > Jerry Zuo pointed out a rather obscure hotplugging issue that it seems I
> > accidentally introduced into DRM two years ago.
> > 
> > Pretend we have a topology like this:
> > 
> > > - DP-1: mst_primary
> >    |- DP-4: active display
> >    |- DP-5: disconnected
> >    |- DP-6: active hub
> >       |- DP-7: active display
> >       |- DP-8: disconnected
> >       |- DP-9: disconnected
> > 
> > If we unplug DP-6, the topology starting at DP-7 will be destroyed but
> > it's payloads will live on in DP-1's VCPI allocations and thus require
> > removal. However, this removal currently fails because
> > drm_dp_update_payload_part1() will (rightly so) try to validate the port
> > before accessing it, fail then abort. If we keep going, eventually we
> > run the MST hub out of bandwidth and all new allocations will start to
> > fail (or in my case; all new displays just start flickering a ton).
> > 
> > We could just teach drm_dp_update_payload_part1() not to drop the port
> > ref in this case, but then we also need to teach
> > drm_dp_destroy_payload_step1() to do the same thing, then hope no one
> > ever adds anything to the that requires a validated port reference in
> > drm_dp_destroy_connector_work(). Kind of sketchy.
> > 
> > So let's go with a more clever solution: any port that
> > drm_dp_destroy_connector_work() interacts with is guaranteed to still
> > exist in memory until we say so. While said port might not be valid we
> > don't really care: that's the whole reason we're destroying it in the
> > first place! So, teach drm_dp_get_validated_port_ref() to use the all
> > mighty current_work() function to avoid attempting to validate ports
> > from the context of mgr->destroy_connector_work. I can't see any
> > situation where this wouldn't be safe, and this avoids having to play
> > whack-a-mole in the future of trying to work around port validation.
> > 
> > Signed-off-by: Lyude Paul <lyude@...hat.com>
> > Fixes: 263efde31f97 ("drm/dp/mst: Get validated port ref in
> > drm_dp_update_payload_part1()")
> > Reported-by: Jerry Zuo <Jerry.Zuo@....com>
> > Cc: Jerry Zuo <Jerry.Zuo@....com>
> > Cc: Harry Wentland <Harry.Wentland@....com>
> > Cc: <stable@...r.kernel.org> # v4.6+
> 
> Hm, sounds very similar to the bug I pointed out in your "make vcpi
> alloc more atomic" series. Will this all interact nicely with the
> solution we've worked out there (where we need to delay at least the
> kfree(port) until the last vcpi allocation is released?
Yes, it seems to work fine in my tests

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 529414556962..08978ad72f33 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1023,9 +1023,20 @@ static struct drm_dp_mst_port
> > *drm_dp_mst_get_port_ref_locked(struct drm_dp_mst_
> >  static struct drm_dp_mst_port *drm_dp_get_validated_port_ref(struct
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port)
> >  {
> >         struct drm_dp_mst_port *rport = NULL;
> > +
> >         mutex_lock(&mgr->lock);
> > -       if (mgr->mst_primary)
> > -               rport = drm_dp_mst_get_port_ref_locked(mgr->mst_primary,
> > port);
> > +       /*
> > +        * Port may or may not be 'valid' but we don't care about that
> > when
> > +        * destroying the port and we are guaranteed that the port pointer
> > +        * will be valid until we've finished
> > +        */
> > +       if (current_work() == &mgr->destroy_connector_work) {
> > +               kref_get(&port->kref);
> > +               rport = port;
> > +       } else if (mgr->mst_primary) {
> > +               rport = drm_dp_mst_get_port_ref_locked(mgr->mst_primary,
> > +                                                      port);
> > +       }
> >         mutex_unlock(&mgr->lock);
> >         return rport;
> >  }
> > --
> > 2.19.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@...ts.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
-- 
Cheers,
	Lyude Paul

Powered by blists - more mailing lists