Re: [PATCH] drm/dp_mst: Skip validating ports during destruction, just ref

[Lyude Paul]

On Thu, 2018-11-22 at 09:34 +0100, Daniel Vetter wrote:
> On Tue, Nov 13, 2018 at 11:47 PM Lyude Paul <lyude@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Fixes: 263efde31f97 ("drm/dp/mst: Get validated port ref in
> > drm_dp_update_payload_part1()")
> > Reported-by: Jerry Zuo <Jerry.Zuo@xxxxxxx>
> > Cc: Jerry Zuo <Jerry.Zuo@xxxxxxx>
> > Cc: Harry Wentland <Harry.Wentland@xxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
-- 
Cheers,
	Lyude Paul