[PATCH] drm/dp: look up the mstb passed into work function

[Daniel Vetter]

On Mon, Jun 22, 2015 at 02:28:33PM +1000, Dave Airlie wrote:
> On 20 June 2015 at 01:42, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Fri, Jun 19, 2015 at 10:53:10AM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <airlied at redhat.com>
> >>
> >> We should validate the passed in mstb under the lock
> >> this should stop us getting an invalid mstb here.
> >>
> >> (first attempt with cancelling work has lockdep issues).
> >
> > Yeah cancel_work_sync is nasty that way ;-)
> >
> > Btw random bikeshed, but mgr->work would look nice as mgr->probe_work.
> >
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89366
> > Bugzilla: https://bugs.archlinux.org/task/45369
> >
> > seems the more accurate one, the fdo one is a mess.
> 
> yes probably.
> 
> > I am a bit concerned about the lifetime rules of the mgr->mst_primary
> > pointer. port->mstb is controlled by the lifetime of the port and that by
> > the parent mst branch, so I think we're covered dereferencing that one.
> >
> > But mgr->mst_primary seems to be protected only by mgr->lock, and you're
> > not holding that in the probe work. I did review
> > drm_dp_mst_topology_mgr_set_mst and that does seem to clean up
> > ->mst_primary correctly but might be helped with a comment. But we do need
> > the locking I think.
> 
> That won't work as we take the lock to do the lookups later,
> 
> It doesn't actually matter if mgr->mst_primary gets messed up here I
> don't think,
> as long as we validate it. So the value is going to be either a)
> correct, b) NULL,
> c) garbage between a and b assuming its not atomic, the validate function
> locks the mgr, and checks primary, at this point primary will either be a or b
> as we hold the lock, and if primary from outside the function is a, b or c won't
> matter as the validation will either pass or fail depending on the
> state under the
> lock.

Hm, I was afraid of some derefencing of the passed-in mstb pointer, but
indeed we seem to be covered. Still I think pulling the mgr->lock out and
instead using drm_dp_mst_get_validated_mstb_ref_locked in
drm_dp_check_and_send_link_address would result in less head-scratching
next time we read this code ;-)
-Daniel

> 
> Though in reviewing that I did find another bug with primary which
> I'll send another fix for.
> 
> Dave.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch