lkml.org 
[lkml]   [2020]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] drm/dp_mst: Fix clearing payload state on topology disable
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

\
 
 \ /
  Last update: 2020-01-22 19:45    [W:0.046 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site