* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.