On Tue, Sep 03, 2019 at 04:45:59PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
>
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
>
> Cc: Juston Li <juston...@intel.com>
> Cc: Imre Deak <imre.d...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Harry Wentland <hwent...@amd.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> Signed-off-by: Lyude Paul <ly...@redhat.com>
Nice catch! Same comment here re: port->mutex, but we can sort that out on the
other thread
Reviewed-by: Sean Paul <s...@poorly.run>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 259634c5d6dc..e407aba1fbd2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2078,18 +2078,23 @@ static void
> drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> struct drm_dp_connection_status_notify *conn_stat)
> {
> - struct drm_device *dev = mstb->mgr->dev;
> + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> + struct drm_device *dev = mgr->dev;
> struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>
> port = drm_dp_get_port(mstb, conn_stat->port_number);
> if (!port)
> return;
>
> + mutex_lock(&port->lock);
> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2102,23 +2107,41 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> - port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE :
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> + port, ret);
> + dowork = false;
> + }
> +
> + /*
> + * We unset port->connector before dropping connection_mutex so that
> + * there's no chance any of the atomic MST helpers can accidentally
> + * associate a to-be-destroyed connector with a port.
> + */
> + if (port->connector && port->input) {
> + connector_to_destroy = port->connector;
> + port->connector = NULL;
> + } else if (!port->connector && !port->input) {
> + create_connector = true;
> }
>
> drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> + if (connector_to_destroy)
> + mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
> +
> + mutex_unlock(&port->lock);
> +
> drm_dp_mst_topology_put_port(port);
> if (dowork)
> queue_work(system_long_wq, &mstb->mgr->work);
> -
> }
>
> static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct
> drm_dp_mst_topology_mgr *mgr,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
