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 <[email protected]>
Cc: Imre Deak <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Harry Wentland <[email protected]>
Cc: Daniel Vetter <[email protected]>
Signed-off-by: Lyude Paul <[email protected]>
---
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
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 <[email protected]>
> Cc: Imre Deak <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Cc: Harry Wentland <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Signed-off-by: Lyude Paul <[email protected]>
Nice catch! Same comment here re: port->mutex, but we can sort that out on the
other thread
Reviewed-by: Sean Paul <[email protected]>
> ---
> 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