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@xxxxxxxxxx>
> Cc: Wayne Lin <Wayne.Lin@xxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
