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

Commit Message

Daniel Vetter June 19, 2015, 3:42 p.m. UTC

On Fri, Jun 19, 2015 at 10:53:10AM +1000, Dave Airlie wrote:
> From: Dave Airlie <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Dave Airlie <[e-mail]>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index fb65f5d..ad0d1bf 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1198,9 +1198,14 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_
>  }
>  
>  static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
> -					       struct drm_dp_mst_branch *mstb)
> +					       struct drm_dp_mst_branch *mstb_in)
>  {
>  	struct drm_dp_mst_port *port;
> +	struct drm_dp_mst_branch *mstb;
> +
> +	mstb = drm_dp_get_validated_mstb_ref(mgr, mstb_in);
> +	if (!mstb)
> +		return;
>  
>  	if (!mstb->link_address_sent) {
>  		drm_dp_send_link_address(mgr, mstb);
> @@ -1219,6 +1224,7 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m
>  		if (port->mstb)
>  			drm_dp_check_and_send_link_address(mgr, port->mstb);
>  	}
> +	drm_dp_put_mst_branch_device(mstb);
>  }

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.

Assuming the added locking doesn't piss off lockdep this is
Reviewed-by: Daniel Vetter <[e-mail]>
-Daniel
---

Comments

Dave Airlie June 22, 2015, 4:28 a.m. UTC | #1

On 20 June 2015 at 01:42, Daniel Vetter <[e-mail]> wrote:
> On Fri, Jun 19, 2015 at 10:53:10AM +1000, Dave Airlie wrote:
>> From: Dave Airlie <[e-mail]>
>>
>> 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.

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

Dave.

Daniel Vetter June 22, 2015, 7:04 a.m. UTC | #2

On Mon, Jun 22, 2015 at 02:28:33PM +1000, Dave Airlie wrote:
> On 20 June 2015 at 01:42, Daniel Vetter <[e-mail]> wrote:
> > On Fri, Jun 19, 2015 at 10:53:10AM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <[e-mail]>
> >>
> >> 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.

Dave Airlie June 22, 2015, 7:15 a.m. UTC | #3

>> 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 ;-)

That's why the follow up patch adds a comment. Also that starts down the road
of locks protecting code not data, the lock is only needed for the lookup
not the whole function. The problem being the function iterates, so then
we end up holding the lock for a lot longer than is required IMO.

Dave.

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 132581ca4ad8..08c3c65e7d08 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1213,7 +1213,9 @@  static void drm_dp_mst_link_probe_work(struct work_struct *work)
 {
 	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
 
+	mutex_lock(&mgr->lock);
 	drm_dp_check_and_send_link_address(mgr, mgr->mst_primary);
+	mutex_unlock(&mgr->lock);
 
 }
 
@@ -1921,6 +1923,8 @@  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 	} else {
 		/* disable MST on the device */
 		mstb = mgr->mst_primary;
+		/* inherit reference from ->mst_primary, will be cleaned up
+		 * at the end. */
 		mgr->mst_primary = NULL;
 		/* this can fail if the device is gone */
 		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);