From 9254ec496a1dbdddeab50021a8138dc627a8166a Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Mon, 22 Jun 2015 17:31:59 +1000 Subject: drm/dp/mst: make sure mst_primary mstb is valid in work function This validates the mst_primary under the lock, and then calls into the check and send function. This makes the code a lot easier to understand the locking rules in. Signed-off-by: Daniel Vetter Reviewed-by: Daniel Vetter Cc: stable@vger.kernel.org Signed-off-by: Dave Airlie --- drivers/gpu/drm/drm_dp_mst_topology.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 132581ca4ad8..1c9670665281 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1189,7 +1189,7 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m struct drm_dp_mst_branch *mstb) { struct drm_dp_mst_port *port; - + struct drm_dp_mst_branch *mstb_child; if (!mstb->link_address_sent) { drm_dp_send_link_address(mgr, mstb); mstb->link_address_sent = true; @@ -1204,17 +1204,31 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m if (!port->available_pbn) drm_dp_send_enum_path_resources(mgr, mstb, port); - if (port->mstb) - drm_dp_check_and_send_link_address(mgr, port->mstb); + if (port->mstb) { + mstb_child = drm_dp_get_validated_mstb_ref(mgr, port->mstb); + if (mstb_child) { + drm_dp_check_and_send_link_address(mgr, mstb_child); + drm_dp_put_mst_branch_device(mstb_child); + } + } } } static void drm_dp_mst_link_probe_work(struct work_struct *work) { struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work); + struct drm_dp_mst_branch *mstb; - drm_dp_check_and_send_link_address(mgr, mgr->mst_primary); - + mutex_lock(&mgr->lock); + mstb = mgr->mst_primary; + if (mstb) { + kref_get(&mstb->kref); + } + mutex_unlock(&mgr->lock); + if (mstb) { + drm_dp_check_and_send_link_address(mgr, mstb); + drm_dp_put_mst_branch_device(mstb); + } } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, -- cgit v1.2.3 From 9eb1e57f564d4e6e10991402726cc83fe0b9172f Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Mon, 22 Jun 2015 14:40:44 +1000 Subject: drm/dp/mst: take lock around looking up the branch device on hpd irq If we are doing an MST transaction and we've gotten HPD and we lookup the device from the incoming msg, we should take the mgr lock around it, so that mst_primary and mstb->ports are valid. Reviewed-by: Daniel Vetter Cc: stable@vger.kernel.org Signed-off-by: Dave Airlie --- drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 1c9670665281..f7213497ad6c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1163,6 +1163,8 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ struct drm_dp_mst_port *port; int i; /* find the port by iterating down */ + + mutex_lock(&mgr->lock); mstb = mgr->mst_primary; for (i = 0; i < lct - 1; i++) { @@ -1182,6 +1184,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ } } kref_get(&mstb->kref); + mutex_unlock(&mgr->lock); return mstb; } -- cgit v1.2.3 From 6b8eeca65b18ae77e175cc2b6571731f0ee413bf Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Mon, 15 Jun 2015 10:34:28 +1000 Subject: drm/dp/mst: close deadlock in connector destruction. 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. Acked-by: Daniel Vetter Cc: stable@vger.kernel.org Signed-off-by: Dave Airlie --- 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(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index f7213497ad6c..778bbb6425b8 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -867,8 +867,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't destroy the connector here, as + 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) @@ -2649,6 +2657,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 @@ -2668,10 +2700,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; @@ -2696,6 +2731,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 3b4d8a4a23fb..57ca8cc383a6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -743,6 +743,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 a2507817be41..86d0b25ed054 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -463,6 +463,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); -- cgit v1.2.3