* [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() @ 2020-01-17 22:47 Lyude Paul 2020-01-17 22:47 ` [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable Lyude Paul 2020-01-22 18:45 ` [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Ville Syrjälä 0 siblings, 2 replies; 4+ messages in thread From: Lyude Paul @ 2020-01-17 22:47 UTC (permalink / raw) To: dri-devel; +Cc: David Airlie, linux-kernel This has always bugged me but somehow I've never remembered to actually fix it. So let's do that. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 8fd85a33ed1b..89c2a7505cbd 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3521,7 +3521,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms drm_dp_mst_topology_get_mstb(mgr->mst_primary); ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, - DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC); + DP_MST_EN | + DP_UP_REQ_EN | + DP_UPSTREAM_IS_SRC); if (ret < 0) goto out_unlock; -- 2.24.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable 2020-01-17 22:47 [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Lyude Paul @ 2020-01-17 22:47 ` Lyude Paul 2020-01-22 18:44 ` Ville Syrjälä 2020-01-22 18:45 ` [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Ville Syrjälä 1 sibling, 1 reply; 4+ messages in thread From: Lyude Paul @ 2020-01-17 22:47 UTC (permalink / raw) To: dri-devel; +Cc: David Airlie, linux-kernel, Wayne Lin, Sean Paul The issues caused by: 64e62bdf04ab ("drm/dp_mst: Remove VCPI while disabling topology mgr") Prompted me to take a closer look at how we clear the payload state in general when disabling the topology, and it turns out there's actually two subtle issues here. The first is that we're not grabbing &mgr.payload_lock when clearing the payloads in drm_dp_mst_topology_mgr_set_mst(). Seeing as the canonical lock order is &mgr.payload_lock -> &mgr.lock (because we always want &mgr.lock to be the inner-most lock so topology validation always works), this makes perfect sense. It also means that -technically- there could be racing between someone calling drm_dp_mst_topology_mgr_set_mst() to disable the topology, along with a modeset occurring that's modifying the payload state at the same time. The second is the more obvious issue that Wayne Lin discovered, that we're not clearing proposed_payloads when disabling the topology. I actually can't see any obvious places where the racing caused by the first issue would break something, and it could be that some of our higher-level locks already prevent this by happenstance, but better safe then sorry. So, let's make it so that drm_dp_mst_topology_mgr_set_mst() first grabs &mgr.payload_lock followed by &mgr.lock so that we never race when modifying the payload state. Then, we also clear proposed_payloads to fix the original issue of enabling a new topology with a dirty payload state. This doesn't clear any of the drm_dp_vcpi structures, but those are getting destroyed along with the ports anyway. Cc: Sean Paul <sean@poorly.run> Cc: Wayne Lin <Wayne.Lin@amd.com> Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 89c2a7505cbd..58287f4c1baf 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3483,6 +3483,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms int ret = 0; struct drm_dp_mst_branch *mstb = NULL; + mutex_lock(&mgr->payload_lock); mutex_lock(&mgr->lock); if (mst_state == mgr->mst_state) goto out_unlock; @@ -3541,7 +3542,10 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms /* this can fail if the device is gone */ drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); ret = 0; - memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct drm_dp_payload)); + memset(mgr->payloads, 0, + mgr->max_payloads * sizeof(struct drm_dp_payload)); + memset(mgr->proposed_vcpis, 0, + mgr->max_payloads * sizeof(void*)); mgr->payload_mask = 0; set_bit(0, &mgr->payload_mask); mgr->vcpi_mask = 0; @@ -3550,6 +3554,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms out_unlock: mutex_unlock(&mgr->lock); + mutex_unlock(&mgr->payload_lock); if (mstb) drm_dp_mst_topology_put_mstb(mstb); return ret; -- 2.24.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable 2020-01-17 22:47 ` [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable Lyude Paul @ 2020-01-22 18:44 ` Ville Syrjälä 0 siblings, 0 replies; 4+ messages in thread From: Ville Syrjälä @ 2020-01-22 18:44 UTC (permalink / raw) To: Lyude Paul; +Cc: David Airlie, Sean Paul, linux-kernel, dri-devel, Wayne Lin On Fri, Jan 17, 2020 at 05:47:49PM -0500, Lyude Paul wrote: > The issues caused by: > > 64e62bdf04ab ("drm/dp_mst: Remove VCPI while disabling topology mgr") > > Prompted me to take a closer look at how we clear the payload state in > general when disabling the topology, and it turns out there's actually > two subtle issues here. > > The first is that we're not grabbing &mgr.payload_lock when clearing the > payloads in drm_dp_mst_topology_mgr_set_mst(). Seeing as the canonical > lock order is &mgr.payload_lock -> &mgr.lock (because we always want > &mgr.lock to be the inner-most lock so topology validation always > works), this makes perfect sense. It also means that -technically- there > could be racing between someone calling > drm_dp_mst_topology_mgr_set_mst() to disable the topology, along with a > modeset occurring that's modifying the payload state at the same time. > > The second is the more obvious issue that Wayne Lin discovered, that > we're not clearing proposed_payloads when disabling the topology. > > I actually can't see any obvious places where the racing caused by the > first issue would break something, and it could be that some of our > higher-level locks already prevent this by happenstance, but better safe > then sorry. So, let's make it so that drm_dp_mst_topology_mgr_set_mst() > first grabs &mgr.payload_lock followed by &mgr.lock so that we never > race when modifying the payload state. Then, we also clear > proposed_payloads to fix the original issue of enabling a new topology > with a dirty payload state. This doesn't clear any of the drm_dp_vcpi > structures, but those are getting destroyed along with the ports anyway. > > Cc: Sean Paul <sean@poorly.run> > Cc: Wayne Lin <Wayne.Lin@amd.com> > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 89c2a7505cbd..58287f4c1baf 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -3483,6 +3483,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms > int ret = 0; > struct drm_dp_mst_branch *mstb = NULL; > > + mutex_lock(&mgr->payload_lock); > mutex_lock(&mgr->lock); > if (mst_state == mgr->mst_state) > goto out_unlock; > @@ -3541,7 +3542,10 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms > /* this can fail if the device is gone */ > drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); > ret = 0; > - memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct drm_dp_payload)); > + memset(mgr->payloads, 0, > + mgr->max_payloads * sizeof(struct drm_dp_payload)); > + memset(mgr->proposed_vcpis, 0, > + mgr->max_payloads * sizeof(void*)); void* is an odd choice. sizeof(foo[0]) would be more future proof (for both of these). Also might be a good idea to update the docs to mention max_payloads defines the size of these arrays. > mgr->payload_mask = 0; > set_bit(0, &mgr->payload_mask); > mgr->vcpi_mask = 0; > @@ -3550,6 +3554,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms > > out_unlock: > mutex_unlock(&mgr->lock); > + mutex_unlock(&mgr->payload_lock); > if (mstb) > drm_dp_mst_topology_put_mstb(mstb); > return ret; > -- > 2.24.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() 2020-01-17 22:47 [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Lyude Paul 2020-01-17 22:47 ` [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable Lyude Paul @ 2020-01-22 18:45 ` Ville Syrjälä 1 sibling, 0 replies; 4+ messages in thread From: Ville Syrjälä @ 2020-01-22 18:45 UTC (permalink / raw) To: Lyude Paul; +Cc: David Airlie, linux-kernel, dri-devel On Fri, Jan 17, 2020 at 05:47:48PM -0500, Lyude Paul wrote: > This has always bugged me but somehow I've never remembered to actually > fix it. So let's do that. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 8fd85a33ed1b..89c2a7505cbd 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -3521,7 +3521,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms > drm_dp_mst_topology_get_mstb(mgr->mst_primary); > > ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, > - DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC); > + DP_MST_EN | > + DP_UP_REQ_EN | > + DP_UPSTREAM_IS_SRC); Doesn't fit on one line? Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > if (ret < 0) > goto out_unlock; > > -- > 2.24.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-22 18:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-17 22:47 [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Lyude Paul 2020-01-17 22:47 ` [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable Lyude Paul 2020-01-22 18:44 ` Ville Syrjälä 2020-01-22 18:45 ` [PATCH 1/2] drm/dp_mst: Fix indenting in drm_dp_mst_topology_mgr_set_mst() Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).