Re: [PATCH] drm/dp/mst: close deadlock in connector destruction.

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 15 Jun 2015, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> I've only seen this once, and I failed to capture the
> lockdep backtrace, but I did some investigations.
>
> If we are calling into the MST layer from EDID probing,
> we have the mode_config mutex held, if during that EDID
> probing, the MST hub goes away, then we can get a deadlock
> where the connector destruction function in the driver
> tries to retake the mode config mutex.
>
> This offloads connector destruction to a workqueue,
> and avoid the subsequenct lock ordering issue.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++++++++++++++++++++++++++++++++--
>  include/drm/drm_crtc.h                |  2 ++
>  include/drm/drm_dp_mst_helper.h       |  4 ++++
>  3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index a88ae51..c98c96d 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -871,8 +871,16 @@ static void drm_dp_destroy_port(struct kref *kref)
>  		port->vcpi.num_slots = 0;
>  
>  		kfree(port->cached_edid);
> -		if (port->connector)
> -			(*port->mgr->cbs->destroy_connector)(mgr, port->connector);
> +
> +		/* we can destroy the connector here, as

we *can't* ?

BR,
Jani.

> +		   we might be holding the mode_config.mutex
> +		   from an EDID retrieval */
> +		if (port->connector) {
> +			mutex_lock(&mgr->destroy_connector_lock);
> +			list_add(&port->connector->destroy_list, &mgr->destroy_connector_list);
> +			mutex_unlock(&mgr->destroy_connector_lock);
> +			schedule_work(&mgr->destroy_connector_work);
> +		}
>  		drm_dp_port_teardown_pdt(port, port->pdt);
>  
>  		if (!port->input && port->vcpi.vcpi > 0)
> @@ -2652,6 +2660,30 @@ static void drm_dp_tx_work(struct work_struct *work)
>  	mutex_unlock(&mgr->qlock);
>  }
>  
> +static void drm_dp_destroy_connector_work(struct work_struct *work)
> +{
> +	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work);
> +	struct drm_connector *connector;
> +
> +	/*
> +	 * Not a regular list traverse as we have to drop the destroy
> +	 * connector lock before destroying the connector, to avoid AB->BA
> +	 * ordering between this lock and the config mutex.
> +	 */
> +	for (;;) {
> +		mutex_lock(&mgr->destroy_connector_lock);
> +		connector = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_connector, destroy_list);
> +		if (!connector) {
> +			mutex_unlock(&mgr->destroy_connector_lock);
> +			break;
> +		}
> +		list_del(&connector->destroy_list);
> +		mutex_unlock(&mgr->destroy_connector_lock);
> +
> +		mgr->cbs->destroy_connector(mgr, connector);
> +	}
> +}
> +
>  /**
>   * drm_dp_mst_topology_mgr_init - initialise a topology manager
>   * @mgr: manager struct to initialise
> @@ -2671,10 +2703,13 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>  	mutex_init(&mgr->lock);
>  	mutex_init(&mgr->qlock);
>  	mutex_init(&mgr->payload_lock);
> +	mutex_init(&mgr->destroy_connector_lock);
>  	INIT_LIST_HEAD(&mgr->tx_msg_upq);
>  	INIT_LIST_HEAD(&mgr->tx_msg_downq);
> +	INIT_LIST_HEAD(&mgr->destroy_connector_list);
>  	INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
>  	INIT_WORK(&mgr->tx_work, drm_dp_tx_work);
> +	INIT_WORK(&mgr->destroy_connector_work, drm_dp_destroy_connector_work);
>  	init_waitqueue_head(&mgr->tx_waitq);
>  	mgr->dev = dev;
>  	mgr->aux = aux;
> @@ -2699,6 +2734,7 @@ EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
>   */
>  void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
>  {
> +	flush_work(&mgr->destroy_connector_work);
>  	mutex_lock(&mgr->payload_lock);
>  	kfree(mgr->payloads);
>  	mgr->payloads = NULL;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ca71c03..5423358 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -731,6 +731,8 @@ struct drm_connector {
>  	uint8_t num_h_tile, num_v_tile;
>  	uint8_t tile_h_loc, tile_v_loc;
>  	uint16_t tile_h_size, tile_v_size;
> +
> +	struct list_head destroy_list;
>  };
>  
>  /**
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 04e668a..4786919 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -464,6 +464,10 @@ struct drm_dp_mst_topology_mgr {
>  	struct work_struct work;
>  
>  	struct work_struct tx_work;
> +
> +	struct list_head destroy_connector_list;
> +	struct mutex destroy_connector_lock;
> +	struct work_struct destroy_connector_work;
>  };
>  
>  int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, struct device *dev, struct drm_dp_aux *aux, int max_dpcd_transaction_bytes, int max_payloads, int conn_base_id);
> -- 
> 2.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel