Re: [PATCH 06/26] drm/dp_mst: Move PDT teardown for ports into destroy_connector_work

[Daniel Vetter]

On Wed, Jul 17, 2019 at 09:42:29PM -0400, Lyude Paul wrote:
> This will allow us to add some locking for port PDTs, which can't be
> done from drm_dp_destroy_port() since we don't know what locks the
> caller might be holding. Also, this gets rid of a good bit of unneeded
> code.
> 
> Cc: Juston Li <juston.li@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Harry Wentland <hwentlan@xxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++----------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index defc5e09fb9a..0295e007c836 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1509,31 +1509,22 @@ static void drm_dp_destroy_port(struct kref *kref)
>  		container_of(kref, struct drm_dp_mst_port, topology_kref);
>  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>  
> -	if (!port->input) {
> -		kfree(port->cached_edid);
> -
> -		/*
> -		 * The only time we don't have a connector
> -		 * on an output port is if the connector init
> -		 * fails.
> -		 */
> -		if (port->connector) {
> -			/* we can't destroy the connector here, as
> -			 * we might be holding the mode_config.mutex
> -			 * from an EDID retrieval */
> -
> -			mutex_lock(&mgr->destroy_connector_lock);
> -			list_add(&port->next, &mgr->destroy_connector_list);
> -			mutex_unlock(&mgr->destroy_connector_lock);
> -			schedule_work(&mgr->destroy_connector_work);
> -			return;
> -		}
> -		/* no need to clean up vcpi
> -		 * as if we have no connector we never setup a vcpi */
> -		drm_dp_port_teardown_pdt(port, port->pdt);
> -		port->pdt = DP_PEER_DEVICE_NONE;
> +	/* There's nothing that needs locking to destroy an input port yet */
> +	if (port->input) {
> +		drm_dp_mst_put_port_malloc(port);
> +		return;
>  	}
> -	drm_dp_mst_put_port_malloc(port);
> +
> +	kfree(port->cached_edid);
> +
> +	/*
> +	 * we can't destroy the connector here, as we might be holding the
> +	 * mode_config.mutex from an EDID retrieval
> +	 */
> +	mutex_lock(&mgr->destroy_connector_lock);
> +	list_add(&port->next, &mgr->destroy_connector_list);
> +	mutex_unlock(&mgr->destroy_connector_lock);
> +	schedule_work(&mgr->destroy_connector_work);

So if I'm not completely blind this just flattens the above code flow (by
inverting the if (port->input)).

>  }
>  
>  /**
> @@ -3881,7 +3872,8 @@ drm_dp_finish_destroy_port(struct drm_dp_mst_port *port)
>  {
>  	INIT_LIST_HEAD(&port->next);
>  
> -	port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> +	if (port->connector)

And this here I can't connect with the commit message. I'm confused, did
something go wrong with some rebase here, and this patch should have a
different title/summary?
-Daniel

> +		port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>  
>  	drm_dp_port_teardown_pdt(port, port->pdt);
>  	port->pdt = DP_PEER_DEVICE_NONE;
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch