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.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com
Nice catch! Same comment here re: port->mutex, but we can sort that out on the other thread
Reviewed-by: Sean Paul sean@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