* [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) @ 2015-06-22 4:40 Dave Airlie 2015-06-22 4:40 ` [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq Dave Airlie 2015-06-22 7:18 ` [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Daniel Vetter 0 siblings, 2 replies; 5+ messages in thread From: Dave Airlie @ 2015-06-22 4:40 UTC (permalink / raw) To: dri-devel From: Dave Airlie <airlied@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). Bugzilla: https://bugs.archlinux.org/task/45369 v2: update bugzilla, add some notes on why passing mst_primary is okay. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index fb65f5d..3c7e5cd 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,12 +1224,20 @@ 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); } 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); - + /* + * Note this passes mgr->mst_primary without validation + * it could be a) valid, b) NULL, c) some value between + * since we haven't taken the lock, + * however the validation sequence in check and send + * will take the lock, and if the value isn't valid + * it will fail appropriately. + */ drm_dp_check_and_send_link_address(mgr, mgr->mst_primary); } -- 2.4.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq 2015-06-22 4:40 [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Dave Airlie @ 2015-06-22 4:40 ` Dave Airlie 2015-06-22 7:07 ` Daniel Vetter 2015-06-22 7:18 ` [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Daniel Vetter 1 sibling, 1 reply; 5+ messages in thread From: Dave Airlie @ 2015-06-22 4:40 UTC (permalink / raw) To: dri-devel From: Dave Airlie <airlied@redhat.com> If we are doing an MST transaction and we've gotten HPD and we lookup the device from the incoming msg, we should take the mgr lock around it, so that mst_primary and mstb->ports are valid. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 3c7e5cd..15a6d8e 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1175,6 +1175,8 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ struct drm_dp_mst_port *port; int i; /* find the port by iterating down */ + + mutex_lock(&mgr->lock); mstb = mgr->mst_primary; for (i = 0; i < lct - 1; i++) { @@ -1194,6 +1196,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ } } kref_get(&mstb->kref); + mutex_unlock(&mgr->lock); return mstb; } -- 2.4.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq 2015-06-22 4:40 ` [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq Dave Airlie @ 2015-06-22 7:07 ` Daniel Vetter 2015-10-16 9:49 ` Jani Nikula 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2015-06-22 7:07 UTC (permalink / raw) To: Dave Airlie; +Cc: dri-devel On Mon, Jun 22, 2015 at 02:40:44PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > If we are doing an MST transaction and we've gotten HPD and we > lookup the device from the incoming msg, we should take the mgr > lock around it, so that mst_primary and mstb->ports are valid. > > Signed-off-by: Dave Airlie <airlied@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 3c7e5cd..15a6d8e 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1175,6 +1175,8 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ > struct drm_dp_mst_port *port; > int i; > /* find the port by iterating down */ > + > + mutex_lock(&mgr->lock); > mstb = mgr->mst_primary; > > for (i = 0; i < lct - 1; i++) { > @@ -1194,6 +1196,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ > } > } > kref_get(&mstb->kref); > + mutex_unlock(&mgr->lock); > return mstb; > } > > -- > 2.4.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq 2015-06-22 7:07 ` Daniel Vetter @ 2015-10-16 9:49 ` Jani Nikula 0 siblings, 0 replies; 5+ messages in thread From: Jani Nikula @ 2015-10-16 9:49 UTC (permalink / raw) To: Daniel Vetter, Dave Airlie; +Cc: adam_richter2004, dri-devel On Mon, 22 Jun 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jun 22, 2015 at 02:40:44PM +1000, Dave Airlie wrote: >> From: Dave Airlie <airlied@redhat.com> >> >> If we are doing an MST transaction and we've gotten HPD and we >> lookup the device from the incoming msg, we should take the mgr >> lock around it, so that mst_primary and mstb->ports are valid. >> >> Signed-off-by: Dave Airlie <airlied@redhat.com> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> The error path fails to unlock the mutex, reported in [1]. There's a fix in the bug, I've asked Adam to post it to the list. BR, Jani. [1] https://bugs.freedesktop.org/show_bug.cgi?id=92480 > >> --- >> drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >> index 3c7e5cd..15a6d8e 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -1175,6 +1175,8 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ >> struct drm_dp_mst_port *port; >> int i; >> /* find the port by iterating down */ >> + >> + mutex_lock(&mgr->lock); >> mstb = mgr->mst_primary; >> >> for (i = 0; i < lct - 1; i++) { >> @@ -1194,6 +1196,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ >> } >> } >> kref_get(&mstb->kref); >> + mutex_unlock(&mgr->lock); >> return mstb; >> } >> >> -- >> 2.4.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) 2015-06-22 4:40 [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Dave Airlie 2015-06-22 4:40 ` [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq Dave Airlie @ 2015-06-22 7:18 ` Daniel Vetter 1 sibling, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2015-06-22 7:18 UTC (permalink / raw) To: Dave Airlie; +Cc: dri-devel On Mon, Jun 22, 2015 at 02:40:43PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@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). > Bugzilla: https://bugs.archlinux.org/task/45369 > > v2: update bugzilla, add some notes on why passing mst_primary > is okay. > > Signed-off-by: Dave Airlie <airlied@redhat.com> On 2nd look I realized that pulling out mgr->lock and using the _locked variant won't work cleanly since the _put might also take the mgr->lock. I guess we could fix that too by also pulling the ref grab/drop for its argument out of check_and_send like this: diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 132581ca4ad8..08c131536b2b 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1189,6 +1189,7 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m struct drm_dp_mst_branch *mstb) { struct drm_dp_mst_port *port; + struct drm_dp_mst_branch *mstb_child; if (!mstb->link_address_sent) { drm_dp_send_link_address(mgr, mstb); @@ -1204,17 +1205,31 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m if (!port->available_pbn) drm_dp_send_enum_path_resources(mgr, mstb, port); - if (port->mstb) - drm_dp_check_and_send_link_address(mgr, port->mstb); + if (port->mstb) { + mstb_child = drm_dp_get_validated_mstb_ref(mgr, + port->mstb); + if (mstb_child) { + drm_dp_check_and_send_link_address(mgr, + mstb_child); + drm_dp_put_mst_branch_device(mstb_child); + } + } } } 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); + struct drm_dp_mst_branch *mstb; - drm_dp_check_and_send_link_address(mgr, mgr->mst_primary); + mutex_lock(&mgr->lock); + kref_get(&mgr->mst_primary->kref); + mstb = mgr->mst_primary; + mutex_unlock(&mgr->lock); + drm_dp_check_and_send_link_address(mgr, mstb); + + drm_dp_put_mst_branch_device(mstb); } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, Upside is that we don't need a comment, and imo non-tricky code is better code. Or am I still missing something? -Daniel > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index fb65f5d..3c7e5cd 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,12 +1224,20 @@ 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); > } > > 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); > - > + /* > + * Note this passes mgr->mst_primary without validation > + * it could be a) valid, b) NULL, c) some value between > + * since we haven't taken the lock, > + * however the validation sequence in check and send > + * will take the lock, and if the value isn't valid > + * it will fail appropriately. > + */ > drm_dp_check_and_send_link_address(mgr, mgr->mst_primary); > > } > -- > 2.4.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-16 9:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-22 4:40 [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Dave Airlie 2015-06-22 4:40 ` [PATCH 2/2] drm/dp/mst: take lock around looking up the branch device on hpd irq Dave Airlie 2015-06-22 7:07 ` Daniel Vetter 2015-10-16 9:49 ` Jani Nikula 2015-06-22 7:18 ` [PATCH 1/2] drm/dp: look up the mstb passed into work function (v2) Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox