Nouveau - Sep 2019 - [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking

On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul
wrote:
> Since we're going to be implementing suspend/resume reprobing very
soon,
> we need to make sure we are extra careful to ensure that our locking
> actually protects the topology state where we expect it to. Turns out
> this isn't the case with drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt(), both of which change port->mstb without
> grabbing &mgr->lock.
> 
> Additionally, since most callers of these functions are just using it to
> teardown the port's previous PDT and setup a new one we can simplify
> things a bit and combine drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt() into a single function:
> drm_dp_port_set_pdt(). This function also handles actually ensuring that
> we grab the correct locks when we need to modify port->mstb.
> 
> Cc: Juston Li <juston.li at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Harry Wentland <hwentlan at amd.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++++++++++++++-----------
>  include/drm/drm_dp_mst_helper.h       |   6 +-
>  2 files changed, 110 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d1610434a0cb..9944ef2ce885 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
drm_dp_mst_branch *mstb)
>  	kref_put(&mstb->topology_kref, drm_dp_destroy_mst_branch_device);
>  }
>  
> -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
old_pdt)
> -{
> -	struct drm_dp_mst_branch *mstb;
> -
> -	switch (old_pdt) {
> -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> -	case DP_PEER_DEVICE_SST_SINK:
> -		/* remove i2c over sideband */
> -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> -		break;
> -	case DP_PEER_DEVICE_MST_BRANCHING:
> -		mstb = port->mstb;
> -		port->mstb = NULL;
> -		drm_dp_mst_topology_put_mstb(mstb);
> -		break;
> -	}
> -}
> -
>  static void drm_dp_destroy_port(struct kref *kref)
>  {
>  	struct drm_dp_mst_port *port > @@ -1714,38 +1696,79 @@ static u8
drm_dp_calculate_rad(struct drm_dp_mst_port *port,
>  	return parent_lct + 1;
>  }
>  
> -/*
> - * return sends link address for new mstb
> - */
> -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
>  {
> -	int ret;
> -	u8 rad[6], lct;
> -	bool send_link = false;
> +	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> +	struct drm_dp_mst_branch *mstb;
> +	u8 rad[8], lct;
> +	int ret = 0;
> +
> +	if (port->pdt == new_pdt)
Shouldn't we also ensure that access to port->pdt is also locked?

Sean
> +		return 0;
> +
> +	/* Teardown the old pdt, if there is one */
> +	switch (port->pdt) {
> +	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> +	case DP_PEER_DEVICE_SST_SINK:
> +		/*
> +		 * If the new PDT would also have an i2c bus, don't bother
> +		 * with reregistering it
> +		 */
> +		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> +		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> +			port->pdt = new_pdt;
> +			return 0;
> +		}
> +
> +		/* remove i2c over sideband */
> +		drm_dp_mst_unregister_i2c_bus(&port->aux);
> +		break;
> +	case DP_PEER_DEVICE_MST_BRANCHING:
> +		mutex_lock(&mgr->lock);
> +		drm_dp_mst_topology_put_mstb(port->mstb);
> +		port->mstb = NULL;
> +		mutex_unlock(&mgr->lock);
> +		break;
> +	}
> +
> +	port->pdt = new_pdt;
>  	switch (port->pdt) {
>  	case DP_PEER_DEVICE_DP_LEGACY_CONV:
>  	case DP_PEER_DEVICE_SST_SINK:
>  		/* add i2c over sideband */
>  		ret = drm_dp_mst_register_i2c_bus(&port->aux);
>  		break;
> +
>  	case DP_PEER_DEVICE_MST_BRANCHING:
>  		lct = drm_dp_calculate_rad(port, rad);
> +		mstb = drm_dp_add_mst_branch_device(lct, rad);
> +		if (!mstb) {
> +			ret = -ENOMEM;
> +			DRM_ERROR("Failed to create MSTB for port %p", port);
> +			goto out;
> +		}
>  
> -		port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> -		if (port->mstb) {
> -			port->mstb->mgr = port->mgr;
> -			port->mstb->port_parent = port;
> -			/*
> -			 * Make sure this port's memory allocation stays
> -			 * around until its child MSTB releases it
> -			 */
> -			drm_dp_mst_get_port_malloc(port);
> +		mutex_lock(&mgr->lock);
> +		port->mstb = mstb;
> +		mstb->mgr = port->mgr;
> +		mstb->port_parent = port;
>  
> -			send_link = true;
> -		}
> +		/*
> +		 * Make sure this port's memory allocation stays
> +		 * around until its child MSTB releases it
> +		 */
> +		drm_dp_mst_get_port_malloc(port);
> +		mutex_unlock(&mgr->lock);
> +
> +		/* And make sure we send a link address for this */
> +		ret = 1;
>  		break;
>  	}
> -	return send_link;
> +
> +out:
> +	if (ret < 0)
> +		port->pdt = DP_PEER_DEVICE_NONE;
> +	return ret;
>  }
>  
>  /**
> @@ -1881,10 +1904,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
*mstb,
>  			    struct drm_device *dev,
>  			    struct drm_dp_link_addr_reply_port *port_msg)
>  {
> +	struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>  	struct drm_dp_mst_port *port;
> -	bool ret;
>  	bool created = false;
> -	int old_pdt = 0;
>  	int old_ddps = 0;
>  
>  	port = drm_dp_get_port(mstb, port_msg->port_number);
> @@ -1896,7 +1918,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
*mstb,
>  		kref_init(&port->malloc_kref);
>  		port->parent = mstb;
>  		port->port_num = port_msg->port_number;
> -		port->mgr = mstb->mgr;
> +		port->mgr = mgr;
>  		port->aux.name = "DPMST";
>  		port->aux.dev = dev->dev;
>  		port->aux.is_remote = true;
> @@ -1909,11 +1931,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
*mstb,
>  
>  		created = true;
>  	} else {
> -		old_pdt = port->pdt;
>  		old_ddps = port->ddps;
>  	}
>  
> -	port->pdt = port_msg->peer_device_type;
>  	port->input = port_msg->input_port;
>  	port->mcs = port_msg->mcs;
>  	port->ddps = port_msg->ddps;
> @@ -1925,29 +1945,33 @@ static void drm_dp_add_port(struct
drm_dp_mst_branch *mstb,
>  	/* manage mstb port lists with mgr lock - take a reference
>  	   for this list */
>  	if (created) {
> -		mutex_lock(&mstb->mgr->lock);
> +		mutex_lock(&mgr->lock);
>  		drm_dp_mst_topology_get_port(port);
>  		list_add(&port->next, &mstb->ports);
> -		mutex_unlock(&mstb->mgr->lock);
> +		mutex_unlock(&mgr->lock);
>  	}
>  
>  	if (old_ddps != port->ddps) {
>  		if (port->ddps) {
>  			if (!port->input) {
> -				drm_dp_send_enum_path_resources(mstb->mgr,
> -								mstb, port);
> +				drm_dp_send_enum_path_resources(mgr, mstb,
> +								port);
>  			}
>  		} else {
>  			port->available_pbn = 0;
>  		}
>  	}
>  
> -	if (old_pdt != port->pdt && !port->input) {
> -		drm_dp_port_teardown_pdt(port, old_pdt);
> -
> -		ret = drm_dp_port_setup_pdt(port);
> -		if (ret == true)
> -			drm_dp_send_link_address(mstb->mgr, port->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;
> +		}
>  	}
>  
>  	if (created && !port->input) {
> @@ -1955,18 +1979,11 @@ static void drm_dp_add_port(struct
drm_dp_mst_branch *mstb,
>  
>  		build_mst_prop_path(mstb, port->port_num, proppath,
>  				    sizeof(proppath));
> -		port->connector =
(*mstb->mgr->cbs->add_connector)(mstb->mgr,
> -								   port,
> -								   proppath);
> -		if (!port->connector) {
> -			/* remove it from the port list */
> -			mutex_lock(&mstb->mgr->lock);
> -			list_del(&port->next);
> -			mutex_unlock(&mstb->mgr->lock);
> -			/* drop port list reference */
> -			drm_dp_mst_topology_put_port(port);
> -			goto out;
> -		}
> +		port->connector = (*mgr->cbs->add_connector)(mgr, port,
> +							     proppath);
> +		if (!port->connector)
> +			goto fail;
> +
>  		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
>  		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
>  		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> @@ -1974,28 +1991,38 @@ static void drm_dp_add_port(struct
drm_dp_mst_branch *mstb,
>  							 &port->aux.ddc);
>  			drm_connector_set_tile_property(port->connector);
>  		}
> -		(*mstb->mgr->cbs->register_connector)(port->connector);
> +
> +		(*mgr->cbs->register_connector)(port->connector);
>  	}
>  
> -out:
>  	/* put reference to this port */
>  	drm_dp_mst_topology_put_port(port);
> +	return;
> +
> +fail:
> +	/* Remove it from the port list */
> +	mutex_lock(&mgr->lock);
> +	list_del(&port->next);
> +	mutex_unlock(&mgr->lock);
> +
> +	/* Drop the port list reference */
> +	drm_dp_mst_topology_put_port(port);
> +	/* And now drop our reference */
> +	drm_dp_mst_topology_put_port(port);
>  }
>  
>  static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
>  			       struct drm_dp_connection_status_notify *conn_stat)
>  {
>  	struct drm_dp_mst_port *port;
> -	int old_pdt;
>  	int old_ddps;
>  	bool dowork = false;
> +
>  	port = drm_dp_get_port(mstb, conn_stat->port_number);
>  	if (!port)
>  		return;
>  
>  	old_ddps = port->ddps;
> -	old_pdt = port->pdt;
> -	port->pdt = conn_stat->peer_device_type;
>  	port->mcs = conn_stat->message_capability_status;
>  	port->ldps = conn_stat->legacy_device_plug_status;
>  	port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2007,11 +2034,17 @@ static void drm_dp_update_port(struct
drm_dp_mst_branch *mstb,
>  			port->available_pbn = 0;
>  		}
>  	}
> -	if (old_pdt != port->pdt && !port->input) {
> -		drm_dp_port_teardown_pdt(port, old_pdt);
>  
> -		if (drm_dp_port_setup_pdt(port))
> +	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;
> +		}
>  	}
>  
>  	drm_dp_mst_topology_put_port(port);
> @@ -4003,9 +4036,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port
*port)
>  	if (port->connector)
>  		port->mgr->cbs->destroy_connector(port->mgr,
port->connector);
>  
> -	drm_dp_port_teardown_pdt(port, port->pdt);
> -	port->pdt = DP_PEER_DEVICE_NONE;
> -
> +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
>  	drm_dp_mst_put_port_malloc(port);
>  }
>  
> diff --git a/include/drm/drm_dp_mst_helper.h
b/include/drm/drm_dp_mst_helper.h
> index 5423a8adda78..f253ee43e9d9 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
>   * @num_sdp_stream_sinks: Number of stream sinks
>   * @available_pbn: Available bandwidth for this port.
>   * @next: link to next port on this branch device
> - * @mstb: branch device attach below this port
> - * @aux: i2c aux transport to talk to device connected to this port.
> + * @mstb: branch device on this port, protected by
> + * &drm_dp_mst_topology_mgr.lock
> + * @aux: i2c aux transport to talk to device connected to this port,
protected
> + * by &drm_dp_mst_topology_mgr.lock
>   * @parent: branch device parent of this port
>   * @vcpi: Virtual Channel Payload info for this port.
>   * @connector: DRM connector this port is connected to.
> -- 
> 2.21.0
> 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > Since we're going to be implementing suspend/resume reprobing very
soon,
> > we need to make sure we are extra careful to ensure that our locking
> > actually protects the topology state where we expect it to. Turns out
> > this isn't the case with drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > grabbing &mgr->lock.
> > 
> > Additionally, since most callers of these functions are just using it
to
> > teardown the port's previous PDT and setup a new one we can
simplify
> > things a bit and combine drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt() into a single function:
> > drm_dp_port_set_pdt(). This function also handles actually ensuring
that
> > we grab the correct locks when we need to modify port->mstb.
> > 
> > Cc: Juston Li <juston.li at intel.com>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Harry Wentland <hwentlan at amd.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Lyude Paul <lyude at redhat.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 181
+++++++++++++++-----------
> >  include/drm/drm_dp_mst_helper.h       |   6 +-
> >  2 files changed, 110 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d1610434a0cb..9944ef2ce885 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > drm_dp_mst_branch *mstb)
> >  	kref_put(&mstb->topology_kref,
drm_dp_destroy_mst_branch_device);
> >  }
> >  
> > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port,
int
> > old_pdt)
> > -{
> > -	struct drm_dp_mst_branch *mstb;
> > -
> > -	switch (old_pdt) {
> > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > -	case DP_PEER_DEVICE_SST_SINK:
> > -		/* remove i2c over sideband */
> > -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > -		break;
> > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > -		mstb = port->mstb;
> > -		port->mstb = NULL;
> > -		drm_dp_mst_topology_put_mstb(mstb);
> > -		break;
> > -	}
> > -}
> > -
> >  static void drm_dp_destroy_port(struct kref *kref)
> >  {
> >  	struct drm_dp_mst_port *port > > @@ -1714,38 +1696,79 @@
static u8 drm_dp_calculate_rad(struct
> > drm_dp_mst_port *port,
> >  	return parent_lct + 1;
> >  }
> >  
> > -/*
> > - * return sends link address for new mstb
> > - */
> > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8
new_pdt)
> >  {
> > -	int ret;
> > -	u8 rad[6], lct;
> > -	bool send_link = false;
> > +	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > +	struct drm_dp_mst_branch *mstb;
> > +	u8 rad[8], lct;
> > +	int ret = 0;
> > +
> > +	if (port->pdt == new_pdt)
> 
> Shouldn't we also ensure that access to port->pdt is also locked?
> 
It's specifically port->mstb that needs to be protected under lock. We
don't
use port->pdt for traversing the topology at all, so keeping it under
connection_mutex is sufficient.
> Sean
> 
> > +		return 0;
> > +
> > +	/* Teardown the old pdt, if there is one */
> > +	switch (port->pdt) {
> > +	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > +	case DP_PEER_DEVICE_SST_SINK:
> > +		/*
> > +		 * If the new PDT would also have an i2c bus, don't bother
> > +		 * with reregistering it
> > +		 */
> > +		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > +		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > +			port->pdt = new_pdt;
> > +			return 0;
> > +		}
> > +
> > +		/* remove i2c over sideband */
> > +		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > +		break;
> > +	case DP_PEER_DEVICE_MST_BRANCHING:
> > +		mutex_lock(&mgr->lock);
> > +		drm_dp_mst_topology_put_mstb(port->mstb);
> > +		port->mstb = NULL;
> > +		mutex_unlock(&mgr->lock);
> > +		break;
> > +	}
> > +
> > +	port->pdt = new_pdt;
> >  	switch (port->pdt) {
> >  	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> >  	case DP_PEER_DEVICE_SST_SINK:
> >  		/* add i2c over sideband */
> >  		ret = drm_dp_mst_register_i2c_bus(&port->aux);
> >  		break;
> > +
> >  	case DP_PEER_DEVICE_MST_BRANCHING:
> >  		lct = drm_dp_calculate_rad(port, rad);
> > +		mstb = drm_dp_add_mst_branch_device(lct, rad);
> > +		if (!mstb) {
> > +			ret = -ENOMEM;
> > +			DRM_ERROR("Failed to create MSTB for port %p", port);
> > +			goto out;
> > +		}
> >  
> > -		port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> > -		if (port->mstb) {
> > -			port->mstb->mgr = port->mgr;
> > -			port->mstb->port_parent = port;
> > -			/*
> > -			 * Make sure this port's memory allocation stays
> > -			 * around until its child MSTB releases it
> > -			 */
> > -			drm_dp_mst_get_port_malloc(port);
> > +		mutex_lock(&mgr->lock);
> > +		port->mstb = mstb;
> > +		mstb->mgr = port->mgr;
> > +		mstb->port_parent = port;
> >  
> > -			send_link = true;
> > -		}
> > +		/*
> > +		 * Make sure this port's memory allocation stays
> > +		 * around until its child MSTB releases it
> > +		 */
> > +		drm_dp_mst_get_port_malloc(port);
> > +		mutex_unlock(&mgr->lock);
> > +
> > +		/* And make sure we send a link address for this */
> > +		ret = 1;
> >  		break;
> >  	}
> > -	return send_link;
> > +
> > +out:
> > +	if (ret < 0)
> > +		port->pdt = DP_PEER_DEVICE_NONE;
> > +	return ret;
> >  }
> >  
> >  /**
> > @@ -1881,10 +1904,9 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> >  			    struct drm_device *dev,
> >  			    struct drm_dp_link_addr_reply_port *port_msg)
> >  {
> > +	struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> >  	struct drm_dp_mst_port *port;
> > -	bool ret;
> >  	bool created = false;
> > -	int old_pdt = 0;
> >  	int old_ddps = 0;
> >  
> >  	port = drm_dp_get_port(mstb, port_msg->port_number);
> > @@ -1896,7 +1918,7 @@ static void drm_dp_add_port(struct
drm_dp_mst_branch
> > *mstb,
> >  		kref_init(&port->malloc_kref);
> >  		port->parent = mstb;
> >  		port->port_num = port_msg->port_number;
> > -		port->mgr = mstb->mgr;
> > +		port->mgr = mgr;
> >  		port->aux.name = "DPMST";
> >  		port->aux.dev = dev->dev;
> >  		port->aux.is_remote = true;
> > @@ -1909,11 +1931,9 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> >  
> >  		created = true;
> >  	} else {
> > -		old_pdt = port->pdt;
> >  		old_ddps = port->ddps;
> >  	}
> >  
> > -	port->pdt = port_msg->peer_device_type;
> >  	port->input = port_msg->input_port;
> >  	port->mcs = port_msg->mcs;
> >  	port->ddps = port_msg->ddps;
> > @@ -1925,29 +1945,33 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> >  	/* manage mstb port lists with mgr lock - take a reference
> >  	   for this list */
> >  	if (created) {
> > -		mutex_lock(&mstb->mgr->lock);
> > +		mutex_lock(&mgr->lock);
> >  		drm_dp_mst_topology_get_port(port);
> >  		list_add(&port->next, &mstb->ports);
> > -		mutex_unlock(&mstb->mgr->lock);
> > +		mutex_unlock(&mgr->lock);
> >  	}
> >  
> >  	if (old_ddps != port->ddps) {
> >  		if (port->ddps) {
> >  			if (!port->input) {
> > -				drm_dp_send_enum_path_resources(mstb->mgr,
> > -								mstb, port);
> > +				drm_dp_send_enum_path_resources(mgr, mstb,
> > +								port);
> >  			}
> >  		} else {
> >  			port->available_pbn = 0;
> >  		}
> >  	}
> >  
> > -	if (old_pdt != port->pdt && !port->input) {
> > -		drm_dp_port_teardown_pdt(port, old_pdt);
> > -
> > -		ret = drm_dp_port_setup_pdt(port);
> > -		if (ret == true)
> > -			drm_dp_send_link_address(mstb->mgr, port->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;
> > +		}
> >  	}
> >  
> >  	if (created && !port->input) {
> > @@ -1955,18 +1979,11 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> >  
> >  		build_mst_prop_path(mstb, port->port_num, proppath,
> >  				    sizeof(proppath));
> > -		port->connector =
(*mstb->mgr->cbs->add_connector)(mstb->mgr,
> > -								   port,
> > -								   proppath);
> > -		if (!port->connector) {
> > -			/* remove it from the port list */
> > -			mutex_lock(&mstb->mgr->lock);
> > -			list_del(&port->next);
> > -			mutex_unlock(&mstb->mgr->lock);
> > -			/* drop port list reference */
> > -			drm_dp_mst_topology_put_port(port);
> > -			goto out;
> > -		}
> > +		port->connector = (*mgr->cbs->add_connector)(mgr, port,
> > +							     proppath);
> > +		if (!port->connector)
> > +			goto fail;
> > +
> >  		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> >  		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> >  		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > @@ -1974,28 +1991,38 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> >  							 &port->aux.ddc);
> >  			drm_connector_set_tile_property(port->connector);
> >  		}
> > -		(*mstb->mgr->cbs->register_connector)(port->connector);
> > +
> > +		(*mgr->cbs->register_connector)(port->connector);
> >  	}
> >  
> > -out:
> >  	/* put reference to this port */
> >  	drm_dp_mst_topology_put_port(port);
> > +	return;
> > +
> > +fail:
> > +	/* Remove it from the port list */
> > +	mutex_lock(&mgr->lock);
> > +	list_del(&port->next);
> > +	mutex_unlock(&mgr->lock);
> > +
> > +	/* Drop the port list reference */
> > +	drm_dp_mst_topology_put_port(port);
> > +	/* And now drop our reference */
> > +	drm_dp_mst_topology_put_port(port);
> >  }
> >  
> >  static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> >  			       struct drm_dp_connection_status_notify
> > *conn_stat)
> >  {
> >  	struct drm_dp_mst_port *port;
> > -	int old_pdt;
> >  	int old_ddps;
> >  	bool dowork = false;
> > +
> >  	port = drm_dp_get_port(mstb, conn_stat->port_number);
> >  	if (!port)
> >  		return;
> >  
> >  	old_ddps = port->ddps;
> > -	old_pdt = port->pdt;
> > -	port->pdt = conn_stat->peer_device_type;
> >  	port->mcs = conn_stat->message_capability_status;
> >  	port->ldps = conn_stat->legacy_device_plug_status;
> >  	port->ddps = conn_stat->displayport_device_plug_status;
> > @@ -2007,11 +2034,17 @@ static void drm_dp_update_port(struct
> > drm_dp_mst_branch *mstb,
> >  			port->available_pbn = 0;
> >  		}
> >  	}
> > -	if (old_pdt != port->pdt && !port->input) {
> > -		drm_dp_port_teardown_pdt(port, old_pdt);
> >  
> > -		if (drm_dp_port_setup_pdt(port))
> > +	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;
> > +		}
> >  	}
> >  
> >  	drm_dp_mst_topology_put_port(port);
> > @@ -4003,9 +4036,7 @@ drm_dp_delayed_destroy_port(struct
drm_dp_mst_port
> > *port)
> >  	if (port->connector)
> >  		port->mgr->cbs->destroy_connector(port->mgr,
port->connector);
> >  
> > -	drm_dp_port_teardown_pdt(port, port->pdt);
> > -	port->pdt = DP_PEER_DEVICE_NONE;
> > -
> > +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> >  	drm_dp_mst_put_port_malloc(port);
> >  }
> >  
> > diff --git a/include/drm/drm_dp_mst_helper.h
> > b/include/drm/drm_dp_mst_helper.h
> > index 5423a8adda78..f253ee43e9d9 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
> >   * @num_sdp_stream_sinks: Number of stream sinks
> >   * @available_pbn: Available bandwidth for this port.
> >   * @next: link to next port on this branch device
> > - * @mstb: branch device attach below this port
> > - * @aux: i2c aux transport to talk to device connected to this port.
> > + * @mstb: branch device on this port, protected by
> > + * &drm_dp_mst_topology_mgr.lock
> > + * @aux: i2c aux transport to talk to device connected to this port,
> > protected
> > + * by &drm_dp_mst_topology_mgr.lock
> >   * @parent: branch device parent of this port
> >   * @vcpi: Virtual Channel Payload info for this port.
> >   * @connector: DRM connector this port is connected to.
> > -- 
> > 2.21.0
> > 
-- 
Cheers,
	Lyude Paul

On Wed, Sep 25, 2019 at 05:00:00PM -0400, Lyude Paul
wrote:
> On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > > Since we're going to be implementing suspend/resume reprobing
very soon,
> > > we need to make sure we are extra careful to ensure that our
locking
> > > actually protects the topology state where we expect it to. Turns
out
> > > this isn't the case with drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt(), both of which change port->mstb
without
> > > grabbing &mgr->lock.
> > > 
> > > Additionally, since most callers of these functions are just
using it to
> > > teardown the port's previous PDT and setup a new one we can
simplify
> > > things a bit and combine drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt() into a single function:
> > > drm_dp_port_set_pdt(). This function also handles actually
ensuring that
> > > we grab the correct locks when we need to modify port->mstb.
> > > 
> > > Cc: Juston Li <juston.li at intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Harry Wentland <hwentlan at amd.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Lyude Paul <lyude at redhat.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 181
+++++++++++++++-----------
> > >  include/drm/drm_dp_mst_helper.h       |   6 +-
> > >  2 files changed, 110 insertions(+), 77 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d1610434a0cb..9944ef2ce885 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > > drm_dp_mst_branch *mstb)
> > >  	kref_put(&mstb->topology_kref,
drm_dp_destroy_mst_branch_device);
> > >  }
> > >  
> > > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port
*port, int
> > > old_pdt)
> > > -{
> > > -	struct drm_dp_mst_branch *mstb;
> > > -
> > > -	switch (old_pdt) {
> > > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > -	case DP_PEER_DEVICE_SST_SINK:
> > > -		/* remove i2c over sideband */
> > > -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > > -		break;
> > > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > > -		mstb = port->mstb;
> > > -		port->mstb = NULL;
> > > -		drm_dp_mst_topology_put_mstb(mstb);
> > > -		break;
> > > -	}
> > > -}
> > > -
> > >  static void drm_dp_destroy_port(struct kref *kref)
> > >  {
> > >  	struct drm_dp_mst_port *port > > > @@ -1714,38
+1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > > drm_dp_mst_port *port,
> > >  	return parent_lct + 1;
> > >  }
> > >  
> > > -/*
> > > - * return sends link address for new mstb
> > > - */
> > > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8
new_pdt)
> > >  {
> > > -	int ret;
> > > -	u8 rad[6], lct;
> > > -	bool send_link = false;
> > > +	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > > +	struct drm_dp_mst_branch *mstb;
> > > +	u8 rad[8], lct;
> > > +	int ret = 0;
> > > +
> > > +	if (port->pdt == new_pdt)
> > 
> > Shouldn't we also ensure that access to port->pdt is also
locked?
> > 
> 
> It's specifically port->mstb that needs to be protected under lock.
We don't
> use port->pdt for traversing the topology at all, so keeping it under
> connection_mutex is sufficient.
> 
I hadn't gotten to the connection_mutex patch yet when I made that comment
:)

Reviewed-by: Sean Paul <sean at poorly.run>

> > Sean
> > 
> > > +		return 0;
> > > +
> > > +	/* Teardown the old pdt, if there is one */
> > > +	switch (port->pdt) {
> > > +	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > +	case DP_PEER_DEVICE_SST_SINK:
> > > +		/*
> > > +		 * If the new PDT would also have an i2c bus, don't bother
> > > +		 * with reregistering it
> > > +		 */
> > > +		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > > +		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > > +			port->pdt = new_pdt;
> > > +			return 0;
> > > +		}
> > > +
> > > +		/* remove i2c over sideband */
> > > +		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > > +		break;
> > > +	case DP_PEER_DEVICE_MST_BRANCHING:
> > > +		mutex_lock(&mgr->lock);
> > > +		drm_dp_mst_topology_put_mstb(port->mstb);
> > > +		port->mstb = NULL;
> > > +		mutex_unlock(&mgr->lock);
> > > +		break;
> > > +	}
> > > +
> > > +	port->pdt = new_pdt;
> > >  	switch (port->pdt) {
> > >  	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > >  	case DP_PEER_DEVICE_SST_SINK:
> > >  		/* add i2c over sideband */
> > >  		ret = drm_dp_mst_register_i2c_bus(&port->aux);
> > >  		break;
> > > +
> > >  	case DP_PEER_DEVICE_MST_BRANCHING:
> > >  		lct = drm_dp_calculate_rad(port, rad);
> > > +		mstb = drm_dp_add_mst_branch_device(lct, rad);
> > > +		if (!mstb) {
> > > +			ret = -ENOMEM;
> > > +			DRM_ERROR("Failed to create MSTB for port %p",
port);
> > > +			goto out;
> > > +		}
> > >  
> > > -		port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> > > -		if (port->mstb) {
> > > -			port->mstb->mgr = port->mgr;
> > > -			port->mstb->port_parent = port;
> > > -			/*
> > > -			 * Make sure this port's memory allocation stays
> > > -			 * around until its child MSTB releases it
> > > -			 */
> > > -			drm_dp_mst_get_port_malloc(port);
> > > +		mutex_lock(&mgr->lock);
> > > +		port->mstb = mstb;
> > > +		mstb->mgr = port->mgr;
> > > +		mstb->port_parent = port;
> > >  
> > > -			send_link = true;
> > > -		}
> > > +		/*
> > > +		 * Make sure this port's memory allocation stays
> > > +		 * around until its child MSTB releases it
> > > +		 */
> > > +		drm_dp_mst_get_port_malloc(port);
> > > +		mutex_unlock(&mgr->lock);
> > > +
> > > +		/* And make sure we send a link address for this */
> > > +		ret = 1;
> > >  		break;
> > >  	}
> > > -	return send_link;
> > > +
> > > +out:
> > > +	if (ret < 0)
> > > +		port->pdt = DP_PEER_DEVICE_NONE;
> > > +	return ret;
> > >  }
> > >  
> > >  /**
> > > @@ -1881,10 +1904,9 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  			    struct drm_device *dev,
> > >  			    struct drm_dp_link_addr_reply_port *port_msg)
> > >  {
> > > +	struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > >  	struct drm_dp_mst_port *port;
> > > -	bool ret;
> > >  	bool created = false;
> > > -	int old_pdt = 0;
> > >  	int old_ddps = 0;
> > >  
> > >  	port = drm_dp_get_port(mstb, port_msg->port_number);
> > > @@ -1896,7 +1918,7 @@ static void drm_dp_add_port(struct
drm_dp_mst_branch
> > > *mstb,
> > >  		kref_init(&port->malloc_kref);
> > >  		port->parent = mstb;
> > >  		port->port_num = port_msg->port_number;
> > > -		port->mgr = mstb->mgr;
> > > +		port->mgr = mgr;
> > >  		port->aux.name = "DPMST";
> > >  		port->aux.dev = dev->dev;
> > >  		port->aux.is_remote = true;
> > > @@ -1909,11 +1931,9 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  
> > >  		created = true;
> > >  	} else {
> > > -		old_pdt = port->pdt;
> > >  		old_ddps = port->ddps;
> > >  	}
> > >  
> > > -	port->pdt = port_msg->peer_device_type;
> > >  	port->input = port_msg->input_port;
> > >  	port->mcs = port_msg->mcs;
> > >  	port->ddps = port_msg->ddps;
> > > @@ -1925,29 +1945,33 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  	/* manage mstb port lists with mgr lock - take a reference
> > >  	   for this list */
> > >  	if (created) {
> > > -		mutex_lock(&mstb->mgr->lock);
> > > +		mutex_lock(&mgr->lock);
> > >  		drm_dp_mst_topology_get_port(port);
> > >  		list_add(&port->next, &mstb->ports);
> > > -		mutex_unlock(&mstb->mgr->lock);
> > > +		mutex_unlock(&mgr->lock);
> > >  	}
> > >  
> > >  	if (old_ddps != port->ddps) {
> > >  		if (port->ddps) {
> > >  			if (!port->input) {
> > > -				drm_dp_send_enum_path_resources(mstb->mgr,
> > > -								mstb, port);
> > > +				drm_dp_send_enum_path_resources(mgr, mstb,
> > > +								port);
> > >  			}
> > >  		} else {
> > >  			port->available_pbn = 0;
> > >  		}
> > >  	}
> > >  
> > > -	if (old_pdt != port->pdt && !port->input) {
> > > -		drm_dp_port_teardown_pdt(port, old_pdt);
> > > -
> > > -		ret = drm_dp_port_setup_pdt(port);
> > > -		if (ret == true)
> > > -			drm_dp_send_link_address(mstb->mgr, port->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;
> > > +		}
> > >  	}
> > >  
> > >  	if (created && !port->input) {
> > > @@ -1955,18 +1979,11 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  
> > >  		build_mst_prop_path(mstb, port->port_num, proppath,
> > >  				    sizeof(proppath));
> > > -		port->connector =
(*mstb->mgr->cbs->add_connector)(mstb->mgr,
> > > -								   port,
> > > -								   proppath);
> > > -		if (!port->connector) {
> > > -			/* remove it from the port list */
> > > -			mutex_lock(&mstb->mgr->lock);
> > > -			list_del(&port->next);
> > > -			mutex_unlock(&mstb->mgr->lock);
> > > -			/* drop port list reference */
> > > -			drm_dp_mst_topology_put_port(port);
> > > -			goto out;
> > > -		}
> > > +		port->connector = (*mgr->cbs->add_connector)(mgr,
port,
> > > +							     proppath);
> > > +		if (!port->connector)
> > > +			goto fail;
> > > +
> > >  		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > >  		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > >  		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > > @@ -1974,28 +1991,38 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  							 &port->aux.ddc);
> > >  			drm_connector_set_tile_property(port->connector);
> > >  		}
> > > -	
(*mstb->mgr->cbs->register_connector)(port->connector);
> > > +
> > > +		(*mgr->cbs->register_connector)(port->connector);
> > >  	}
> > >  
> > > -out:
> > >  	/* put reference to this port */
> > >  	drm_dp_mst_topology_put_port(port);
> > > +	return;
> > > +
> > > +fail:
> > > +	/* Remove it from the port list */
> > > +	mutex_lock(&mgr->lock);
> > > +	list_del(&port->next);
> > > +	mutex_unlock(&mgr->lock);
> > > +
> > > +	/* Drop the port list reference */
> > > +	drm_dp_mst_topology_put_port(port);
> > > +	/* And now drop our reference */
> > > +	drm_dp_mst_topology_put_port(port);
> > >  }
> > >  
> > >  static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> > >  			       struct drm_dp_connection_status_notify
> > > *conn_stat)
> > >  {
> > >  	struct drm_dp_mst_port *port;
> > > -	int old_pdt;
> > >  	int old_ddps;
> > >  	bool dowork = false;
> > > +
> > >  	port = drm_dp_get_port(mstb, conn_stat->port_number);
> > >  	if (!port)
> > >  		return;
> > >  
> > >  	old_ddps = port->ddps;
> > > -	old_pdt = port->pdt;
> > > -	port->pdt = conn_stat->peer_device_type;
> > >  	port->mcs = conn_stat->message_capability_status;
> > >  	port->ldps = conn_stat->legacy_device_plug_status;
> > >  	port->ddps = conn_stat->displayport_device_plug_status;
> > > @@ -2007,11 +2034,17 @@ static void drm_dp_update_port(struct
> > > drm_dp_mst_branch *mstb,
> > >  			port->available_pbn = 0;
> > >  		}
> > >  	}
> > > -	if (old_pdt != port->pdt && !port->input) {
> > > -		drm_dp_port_teardown_pdt(port, old_pdt);
> > >  
> > > -		if (drm_dp_port_setup_pdt(port))
> > > +	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;
> > > +		}
> > >  	}
> > >  
> > >  	drm_dp_mst_topology_put_port(port);
> > > @@ -4003,9 +4036,7 @@ drm_dp_delayed_destroy_port(struct
drm_dp_mst_port
> > > *port)
> > >  	if (port->connector)
> > >  		port->mgr->cbs->destroy_connector(port->mgr,
port->connector);
> > >  
> > > -	drm_dp_port_teardown_pdt(port, port->pdt);
> > > -	port->pdt = DP_PEER_DEVICE_NONE;
> > > -
> > > +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> > >  	drm_dp_mst_put_port_malloc(port);
> > >  }
> > >  
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index 5423a8adda78..f253ee43e9d9 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
> > >   * @num_sdp_stream_sinks: Number of stream sinks
> > >   * @available_pbn: Available bandwidth for this port.
> > >   * @next: link to next port on this branch device
> > > - * @mstb: branch device attach below this port
> > > - * @aux: i2c aux transport to talk to device connected to this
port.
> > > + * @mstb: branch device on this port, protected by
> > > + * &drm_dp_mst_topology_mgr.lock
> > > + * @aux: i2c aux transport to talk to device connected to this
port,
> > > protected
> > > + * by &drm_dp_mst_topology_mgr.lock
> > >   * @parent: branch device parent of this port
> > >   * @vcpi: Virtual Channel Payload info for this port.
> > >   * @connector: DRM connector this port is connected to.
> > > -- 
> > > 2.21.0
> > > 
> -- 
> Cheers,
> 	Lyude Paul
> 
-- 
Sean Paul, Software Engineer, Google / Chromium OS