Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex

2019-09-25 Thread Lyude Paul 
On Wed, 2019-09-25 at 16:00 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:
> > Yes-you read that right. Currently there is literally no locking in
> > place for any of the drm_dp_mst_port struct members that can be modified
> > in response to a link address response, or a connection status response.
> > Which literally means if we're unlucky enough to have any sort of
> > hotplugging event happen before we're finished with reprobing link
> > addresses, we'll race and the contents of said struct members becomes
> > undefined. Fun!
> > 
> > So, finally add some simple locking protections to our MST helpers by
> > protecting any drm_dp_mst_port members which can be changed by link
> > address responses or connection status notifications under
> > drm_device->mode_config.connection_mutex.
> > 
> > Cc: Juston Li 
> > Cc: Imre Deak 
> > Cc: Ville Syrjälä 
> > Cc: Harry Wentland 
> > Cc: Daniel Vetter 
> > Signed-off-by: Lyude Paul 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++---
> >  include/drm/drm_dp_mst_helper.h   |  39 +--
> >  2 files changed, 133 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 5101eeab4485..259634c5d6dc 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
> > container_of(kref, struct drm_dp_mst_port, malloc_kref);
> >  
> > drm_dp_mst_put_mstb_malloc(port->parent);
> > +   mutex_destroy(>lock);
> > kfree(port);
> >  }
> >  
> > @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct
> > drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
> >  
> > +static void
> > +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
> > + struct drm_dp_mst_port *port)
> > +{
> > +   struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > +   char proppath[255];
> > +   int ret;
> > +
> > +   build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> > +   port->connector = mgr->cbs->add_connector(mgr, port, proppath);
> > +   if (!port->connector) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > +
> > +   if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > +port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > +   port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > +   port->cached_edid = drm_get_edid(port->connector,
> > +>aux.ddc);
> > +   drm_connector_set_tile_property(port->connector);
> > +   }
> > +
> > +   mgr->cbs->register_connector(port->connector);
> > +   return;
> > +
> > +error:
> > +   DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
> > +}
> > +
> >  static void
> >  drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> > struct drm_device *dev,
> > @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  {
> > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > struct drm_dp_mst_port *port;
> > -   bool created = false;
> > -   int old_ddps = 0;
> > +   struct drm_dp_mst_branch *child_mstb = NULL;
> > +   struct drm_connector *connector_to_destroy = NULL;
> > +   int old_ddps = 0, ret;
> > +   u8 new_pdt = DP_PEER_DEVICE_NONE;
> > +   bool created = false, send_link_addr = false,
> > +create_connector = false;
> >  
> > port = drm_dp_get_port(mstb, port_msg->port_number);
> > if (!port) {
> > @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > return;
> > kref_init(>topology_kref);
> > kref_init(>malloc_kref);
> > +   mutex_init(>lock);
> > port->parent = mstb;
> > port->port_num = port_msg->port_number;
> > port->mgr = mgr;
> > @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > drm_dp_mst_get_mstb_malloc(mstb);
> >  
> > created = true;
> > -   } else {
> > -   old_ddps = port->ddps;
> > }
> >  
> > +   mutex_lock(>lock);
> > +   drm_modeset_lock(>mode_config.connection_mutex, NULL);
> > +
> > +   if (!created)
> > +   old_ddps = port->ddps;
> > +
> > port->input = port_msg->input_port;
> > +   if (!port->input)
> > +   new_pdt = port_msg->peer_device_type;
> > port->mcs = port_msg->mcs;
> > port->ddps = port_msg->ddps;
> > port->ldps = port_msg->legacy_device_plug_status;
> > @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > }
> > }
> >  
> > -   if (!port->input) {
> > -   int ret = drm_dp_port_set_pdt(port,
> > -

Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex

2019-09-25 Thread Sean Paul 
On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:
> Yes-you read that right. Currently there is literally no locking in
> place for any of the drm_dp_mst_port struct members that can be modified
> in response to a link address response, or a connection status response.
> Which literally means if we're unlucky enough to have any sort of
> hotplugging event happen before we're finished with reprobing link
> addresses, we'll race and the contents of said struct members becomes
> undefined. Fun!
> 
> So, finally add some simple locking protections to our MST helpers by
> protecting any drm_dp_mst_port members which can be changed by link
> address responses or connection status notifications under
> drm_device->mode_config.connection_mutex.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++---
>  include/drm/drm_dp_mst_helper.h   |  39 +--
>  2 files changed, 133 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5101eeab4485..259634c5d6dc 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
>   container_of(kref, struct drm_dp_mst_port, malloc_kref);
>  
>   drm_dp_mst_put_mstb_malloc(port->parent);
> + mutex_destroy(>lock);
>   kfree(port);
>  }
>  
> @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct 
> drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>  
> +static void
> +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
> +   struct drm_dp_mst_port *port)
> +{
> + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> + char proppath[255];
> + int ret;
> +
> + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> + port->connector = mgr->cbs->add_connector(mgr, port, proppath);
> + if (!port->connector) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> +  port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> + port->port_num >= DP_MST_LOGICAL_PORT_0) {
> + port->cached_edid = drm_get_edid(port->connector,
> +  >aux.ddc);
> + drm_connector_set_tile_property(port->connector);
> + }
> +
> + mgr->cbs->register_connector(port->connector);
> + return;
> +
> +error:
> + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
> +}
> +
>  static void
>  drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>   struct drm_device *dev,
> @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct 
> drm_dp_mst_branch *mstb,
>  {
>   struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>   struct drm_dp_mst_port *port;
> - bool created = false;
> - int old_ddps = 0;
> + struct drm_dp_mst_branch *child_mstb = NULL;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps = 0, ret;
> + u8 new_pdt = DP_PEER_DEVICE_NONE;
> + bool created = false, send_link_addr = false,
> +  create_connector = false;
>  
>   port = drm_dp_get_port(mstb, port_msg->port_number);
>   if (!port) {
> @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct 
> drm_dp_mst_branch *mstb,
>   return;
>   kref_init(>topology_kref);
>   kref_init(>malloc_kref);
> + mutex_init(>lock);
>   port->parent = mstb;
>   port->port_num = port_msg->port_number;
>   port->mgr = mgr;
> @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct 
> drm_dp_mst_branch *mstb,
>   drm_dp_mst_get_mstb_malloc(mstb);
>  
>   created = true;
> - } else {
> - old_ddps = port->ddps;
>   }
>  
> + mutex_lock(>lock);
> + drm_modeset_lock(>mode_config.connection_mutex, NULL);
> +
> + if (!created)
> + old_ddps = port->ddps;
> +
>   port->input = port_msg->input_port;
> + if (!port->input)
> + new_pdt = port_msg->peer_device_type;
>   port->mcs = port_msg->mcs;
>   port->ddps = port_msg->ddps;
>   port->ldps = port_msg->legacy_device_plug_status;
> @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct 
> drm_dp_mst_branch *mstb,
>   }
>   }
>  
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> -   port_msg->peer_device_type);
> - if (ret == 1) {
> - drm_dp_send_link_address(mgr, port->mstb);
> - }

[Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex

2019-09-03 Thread Lyude Paul 
Yes-you read that right. Currently there is literally no locking in
place for any of the drm_dp_mst_port struct members that can be modified
in response to a link address response, or a connection status response.
Which literally means if we're unlucky enough to have any sort of
hotplugging event happen before we're finished with reprobing link
addresses, we'll race and the contents of said struct members becomes
undefined. Fun!

So, finally add some simple locking protections to our MST helpers by
protecting any drm_dp_mst_port members which can be changed by link
address responses or connection status notifications under
drm_device->mode_config.connection_mutex.

Cc: Juston Li 
Cc: Imre Deak 
Cc: Ville Syrjälä 
Cc: Harry Wentland 
Cc: Daniel Vetter 
Signed-off-by: Lyude Paul 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++---
 include/drm/drm_dp_mst_helper.h   |  39 +--
 2 files changed, 133 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 5101eeab4485..259634c5d6dc 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
container_of(kref, struct drm_dp_mst_port, malloc_kref);
 
drm_dp_mst_put_mstb_malloc(port->parent);
+   mutex_destroy(>lock);
kfree(port);
 }
 
@@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct 
drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
 
+static void
+drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
+ struct drm_dp_mst_port *port)
+{
+   struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+   char proppath[255];
+   int ret;
+
+   build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
+   port->connector = mgr->cbs->add_connector(mgr, port, proppath);
+   if (!port->connector) {
+   ret = -ENOMEM;
+   goto error;
+   }
+
+   if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
+port->pdt == DP_PEER_DEVICE_SST_SINK) &&
+   port->port_num >= DP_MST_LOGICAL_PORT_0) {
+   port->cached_edid = drm_get_edid(port->connector,
+>aux.ddc);
+   drm_connector_set_tile_property(port->connector);
+   }
+
+   mgr->cbs->register_connector(port->connector);
+   return;
+
+error:
+   DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
+}
+
 static void
 drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
struct drm_device *dev,
@@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct 
drm_dp_mst_branch *mstb,
 {
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
-   bool created = false;
-   int old_ddps = 0;
+   struct drm_dp_mst_branch *child_mstb = NULL;
+   struct drm_connector *connector_to_destroy = NULL;
+   int old_ddps = 0, ret;
+   u8 new_pdt = DP_PEER_DEVICE_NONE;
+   bool created = false, send_link_addr = false,
+create_connector = false;
 
port = drm_dp_get_port(mstb, port_msg->port_number);
if (!port) {
@@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct 
drm_dp_mst_branch *mstb,
return;
kref_init(>topology_kref);
kref_init(>malloc_kref);
+   mutex_init(>lock);
port->parent = mstb;
port->port_num = port_msg->port_number;
port->mgr = mgr;
@@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct 
drm_dp_mst_branch *mstb,
drm_dp_mst_get_mstb_malloc(mstb);
 
created = true;
-   } else {
-   old_ddps = port->ddps;
}
 
+   mutex_lock(>lock);
+   drm_modeset_lock(>mode_config.connection_mutex, NULL);
+
+   if (!created)
+   old_ddps = port->ddps;
+
port->input = port_msg->input_port;
+   if (!port->input)
+   new_pdt = port_msg->peer_device_type;
port->mcs = port_msg->mcs;
port->ddps = port_msg->ddps;
port->ldps = port_msg->legacy_device_plug_status;
@@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct 
drm_dp_mst_branch *mstb,
}
}
 
-   if (!port->input) {
-   int ret = drm_dp_port_set_pdt(port,
- port_msg->peer_device_type);
-   if (ret == 1) {
-   drm_dp_send_link_address(mgr, port->mstb);
-   } else if (ret < 0) {
-   DRM_ERROR("Failed to change PDT on port %p: %d\n",
- port, ret);
-   goto fail;
+   ret =