| Message ID | 20190718014329.8107-3-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DP MST Refactors + debugging tools + suspend/resume reprobing | expand |
On Wed, Jul 17, 2019 at 09:42:25PM -0400, Lyude Paul wrote: > Currently we remove MST branch devices from the in-memory topology > immediately when their topology refcount reaches 0. This works just fine > at the moment, but since we're about to add suspend/resume reprobing for > MST topologies we're going to need to be able to travel through the > topology and drop topology refs on branch devices while holding > mgr->mutex. Since we currently can't do this due to the circular locking > dependencies that would incur, move all of the actual work for > drm_dp_destroy_mst_branch_device() into drm_dp_destroy_connector_work() > so we can drop topology refs on MSTBs in any locking context. Would be good to point at where exactly the problem is here, maybe also mentioned the exact future patch that causes the problem. I did go look around a bit, but didn't spot it. > > Cc: Juston Li <juston.li@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Harry Wentland <hwentlan@amd.com> > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 121 +++++++++++++++++--------- > include/drm/drm_dp_mst_helper.h | 10 +++ > 2 files changed, 90 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 998081b9b205..d7c3d9233834 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1108,34 +1108,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref) > struct drm_dp_mst_branch *mstb = > container_of(kref, struct drm_dp_mst_branch, topology_kref); > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > - struct drm_dp_mst_port *port, *tmp; > - bool wake_tx = false; > - > - mutex_lock(&mgr->lock); > - list_for_each_entry_safe(port, tmp, &mstb->ports, next) { > - list_del(&port->next); > - drm_dp_mst_topology_put_port(port); > - } > - mutex_unlock(&mgr->lock); > - > - /* drop any tx slots msg */ > - mutex_lock(&mstb->mgr->qlock); > - if (mstb->tx_slots[0]) { > - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > - mstb->tx_slots[0] = NULL; > - wake_tx = true; > - } > - if (mstb->tx_slots[1]) { > - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > - mstb->tx_slots[1] = NULL; > - wake_tx = true; > - } > - mutex_unlock(&mstb->mgr->qlock); > > - if (wake_tx) > - wake_up_all(&mstb->mgr->tx_waitq); > + INIT_LIST_HEAD(&mstb->destroy_next); > > - drm_dp_mst_put_mstb_malloc(mstb); > + /* > + * This can get called under mgr->mutex, so we need to perform the > + * actual destruction of the mstb in another worker > + */ > + mutex_lock(&mgr->destroy_connector_lock); > + list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list); > + mutex_unlock(&mgr->destroy_connector_lock); > + schedule_work(&mgr->destroy_connector_work); > } > > /** > @@ -3618,10 +3601,56 @@ static void drm_dp_tx_work(struct work_struct *work) > mutex_unlock(&mgr->qlock); > } > > +static inline void > +drm_dp_finish_destroy_port(struct drm_dp_mst_port *port) Bikeshed: I'd call this _delayed_destroy, I think that makes a bit clearer why there's 2 stages to destroying stuff. > +{ > + INIT_LIST_HEAD(&port->next); > + > + 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_mst_put_port_malloc(port); > +} > + > +static inline void > +drm_dp_finish_destroy_mst_branch_device(struct drm_dp_mst_branch *mstb) > +{ > + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > + struct drm_dp_mst_port *port, *tmp; > + bool wake_tx = false; > + > + mutex_lock(&mgr->lock); > + list_for_each_entry_safe(port, tmp, &mstb->ports, next) { > + list_del(&port->next); > + drm_dp_mst_topology_put_port(port); > + } > + mutex_unlock(&mgr->lock); > + > + /* drop any tx slots msg */ > + mutex_lock(&mstb->mgr->qlock); > + if (mstb->tx_slots[0]) { > + mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > + mstb->tx_slots[0] = NULL; > + wake_tx = true; > + } > + if (mstb->tx_slots[1]) { > + mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > + mstb->tx_slots[1] = NULL; > + wake_tx = true; > + } > + mutex_unlock(&mstb->mgr->qlock); > + > + if (wake_tx) > + wake_up_all(&mstb->mgr->tx_waitq); > + > + drm_dp_mst_put_mstb_malloc(mstb); > +} > + > 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_dp_mst_port *port; > bool send_hotplug = false; > /* > * Not a regular list traverse as we have to drop the destroy > @@ -3629,24 +3658,33 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) > * ordering between this lock and the config mutex. > */ > for (;;) { > + struct drm_dp_mst_branch *mstb = NULL; > + struct drm_dp_mst_port *port = NULL; > + > + /* Destroy any MSTBs first, and then their ports second */ > mutex_lock(&mgr->destroy_connector_lock); > - port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next); > - if (!port) { > - mutex_unlock(&mgr->destroy_connector_lock); > - break; > + mstb = list_first_entry_or_null(&mgr->destroy_branch_device_list, > + struct drm_dp_mst_branch, > + destroy_next); > + if (mstb) { > + list_del(&mstb->destroy_next); > + } else { > + port = list_first_entry_or_null(&mgr->destroy_connector_list, > + struct drm_dp_mst_port, > + next); > + if (port) > + list_del(&port->next); > } Control flow looks rather awkward here. I'd do either two loops, or if you want to have just one, rename it to ->delayed_destroy_list and have a ->delayed_destry_cb next to it? Cheers, Daniel > - list_del(&port->next); > mutex_unlock(&mgr->destroy_connector_lock); > > - INIT_LIST_HEAD(&port->next); > - > - mgr->cbs->destroy_connector(mgr, port->connector); > - > - drm_dp_port_teardown_pdt(port, port->pdt); > - port->pdt = DP_PEER_DEVICE_NONE; > - > - drm_dp_mst_put_port_malloc(port); > - send_hotplug = true; > + if (mstb) { > + drm_dp_finish_destroy_mst_branch_device(mstb); > + } else if (port) { > + drm_dp_finish_destroy_port(port); > + send_hotplug = true; > + } else { > + break; > + } > } > if (send_hotplug) > drm_kms_helper_hotplug_event(mgr->dev); > @@ -3840,6 +3878,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, > mutex_init(&mgr->destroy_connector_lock); > INIT_LIST_HEAD(&mgr->tx_msg_downq); > INIT_LIST_HEAD(&mgr->destroy_connector_list); > + INIT_LIST_HEAD(&mgr->destroy_branch_device_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); > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 8c97a5f92c47..c01f3ea72756 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -143,6 +143,12 @@ struct drm_dp_mst_branch { > */ > struct kref malloc_kref; > > + /** > + * @destroy_next: linked-list entry used by > + * drm_dp_destroy_connector_work() > + */ > + struct list_head destroy_next; > + > u8 rad[8]; > u8 lct; > int num_ports; > @@ -578,6 +584,10 @@ struct drm_dp_mst_topology_mgr { > * @destroy_connector_list: List of to be destroyed connectors. > */ > struct list_head destroy_connector_list; > + /** > + * @destroy_branch_device_list: List of to be destroyed branch devices > + */ > + struct list_head destroy_branch_device_list; > /** > * @destroy_connector_lock: Protects @connector_list. > */ > -- > 2.21.0 >
On Tue, 2019-08-13 at 15:00 +0200, Daniel Vetter wrote: > On Wed, Jul 17, 2019 at 09:42:25PM -0400, Lyude Paul wrote: > > Currently we remove MST branch devices from the in-memory topology > > immediately when their topology refcount reaches 0. This works just fine > > at the moment, but since we're about to add suspend/resume reprobing for > > MST topologies we're going to need to be able to travel through the > > topology and drop topology refs on branch devices while holding > > mgr->mutex. Since we currently can't do this due to the circular locking > > dependencies that would incur, move all of the actual work for > > drm_dp_destroy_mst_branch_device() into drm_dp_destroy_connector_work() > > so we can drop topology refs on MSTBs in any locking context. > > Would be good to point at where exactly the problem is here, maybe also > mentioned the exact future patch that causes the problem. I did go look > around a bit, but didn't spot it. Ah, I didn't do a great job of explaining whoops! Basically, the issue is that when reprobing the state during suspend/resume, there's of course going to be a chance that any MST ports in the topology could have had their PDT changed while we were suspended. This in turn means that while we're iterating through each mstb's ports in drm_dp_send_link_address() we want to be able to remove MSTBs from while under &mgr->lock, something we can't do without handling MSTB destruction in another thread. I'll mention this in the next version of this patch > > > Cc: Juston Li <juston.li@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Harry Wentland <hwentlan@amd.com> > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 121 +++++++++++++++++--------- > > include/drm/drm_dp_mst_helper.h | 10 +++ > > 2 files changed, 90 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > index 998081b9b205..d7c3d9233834 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -1108,34 +1108,17 @@ static void > > drm_dp_destroy_mst_branch_device(struct kref *kref) > > struct drm_dp_mst_branch *mstb = > > container_of(kref, struct drm_dp_mst_branch, topology_kref); > > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > > - struct drm_dp_mst_port *port, *tmp; > > - bool wake_tx = false; > > - > > - mutex_lock(&mgr->lock); > > - list_for_each_entry_safe(port, tmp, &mstb->ports, next) { > > - list_del(&port->next); > > - drm_dp_mst_topology_put_port(port); > > - } > > - mutex_unlock(&mgr->lock); > > - > > - /* drop any tx slots msg */ > > - mutex_lock(&mstb->mgr->qlock); > > - if (mstb->tx_slots[0]) { > > - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > > - mstb->tx_slots[0] = NULL; > > - wake_tx = true; > > - } > > - if (mstb->tx_slots[1]) { > > - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > > - mstb->tx_slots[1] = NULL; > > - wake_tx = true; > > - } > > - mutex_unlock(&mstb->mgr->qlock); > > > > - if (wake_tx) > > - wake_up_all(&mstb->mgr->tx_waitq); > > + INIT_LIST_HEAD(&mstb->destroy_next); > > > > - drm_dp_mst_put_mstb_malloc(mstb); > > + /* > > + * This can get called under mgr->mutex, so we need to perform the > > + * actual destruction of the mstb in another worker > > + */ > > + mutex_lock(&mgr->destroy_connector_lock); > > + list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list); > > + mutex_unlock(&mgr->destroy_connector_lock); > > + schedule_work(&mgr->destroy_connector_work); > > } > > > > /** > > @@ -3618,10 +3601,56 @@ static void drm_dp_tx_work(struct work_struct > > *work) > > mutex_unlock(&mgr->qlock); > > } > > > > +static inline void > > +drm_dp_finish_destroy_port(struct drm_dp_mst_port *port) > > Bikeshed: I'd call this _delayed_destroy, I think that makes a bit clearer > why there's 2 stages to destroying stuff. > > > +{ > > + INIT_LIST_HEAD(&port->next); > > + > > + 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_mst_put_port_malloc(port); > > +} > > + > > +static inline void > > +drm_dp_finish_destroy_mst_branch_device(struct drm_dp_mst_branch *mstb) > > +{ > > + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > > + struct drm_dp_mst_port *port, *tmp; > > + bool wake_tx = false; > > + > > + mutex_lock(&mgr->lock); > > + list_for_each_entry_safe(port, tmp, &mstb->ports, next) { > > + list_del(&port->next); > > + drm_dp_mst_topology_put_port(port); > > + } > > + mutex_unlock(&mgr->lock); > > + > > + /* drop any tx slots msg */ > > + mutex_lock(&mstb->mgr->qlock); > > + if (mstb->tx_slots[0]) { > > + mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > > + mstb->tx_slots[0] = NULL; > > + wake_tx = true; > > + } > > + if (mstb->tx_slots[1]) { > > + mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; > > + mstb->tx_slots[1] = NULL; > > + wake_tx = true; > > + } > > + mutex_unlock(&mstb->mgr->qlock); > > + > > + if (wake_tx) > > + wake_up_all(&mstb->mgr->tx_waitq); > > + > > + drm_dp_mst_put_mstb_malloc(mstb); > > +} > > + > > 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_dp_mst_port *port; > > bool send_hotplug = false; > > /* > > * Not a regular list traverse as we have to drop the destroy > > @@ -3629,24 +3658,33 @@ static void drm_dp_destroy_connector_work(struct > > work_struct *work) > > * ordering between this lock and the config mutex. > > */ > > for (;;) { > > + struct drm_dp_mst_branch *mstb = NULL; > > + struct drm_dp_mst_port *port = NULL; > > + > > + /* Destroy any MSTBs first, and then their ports second */ > > mutex_lock(&mgr->destroy_connector_lock); > > - port = list_first_entry_or_null(&mgr->destroy_connector_list, > > struct drm_dp_mst_port, next); > > - if (!port) { > > - mutex_unlock(&mgr->destroy_connector_lock); > > - break; > > + mstb = list_first_entry_or_null(&mgr- > > >destroy_branch_device_list, > > + struct drm_dp_mst_branch, > > + destroy_next); > > + if (mstb) { > > + list_del(&mstb->destroy_next); > > + } else { > > + port = list_first_entry_or_null(&mgr- > > >destroy_connector_list, > > + struct > > drm_dp_mst_port, > > + next); > > + if (port) > > + list_del(&port->next); > > } > > Control flow looks rather awkward here. I'd do either two loops, or if you > want to have just one, rename it to ->delayed_destroy_list and have a > ->delayed_destry_cb next to it? > > Cheers, Daniel > > > > - list_del(&port->next); > > mutex_unlock(&mgr->destroy_connector_lock); > > > > - INIT_LIST_HEAD(&port->next); > > - > > - mgr->cbs->destroy_connector(mgr, port->connector); > > - > > - drm_dp_port_teardown_pdt(port, port->pdt); > > - port->pdt = DP_PEER_DEVICE_NONE; > > - > > - drm_dp_mst_put_port_malloc(port); > > - send_hotplug = true; > > + if (mstb) { > > + drm_dp_finish_destroy_mst_branch_device(mstb); > > + } else if (port) { > > + drm_dp_finish_destroy_port(port); > > + send_hotplug = true; > > + } else { > > + break; > > + } > > } > > if (send_hotplug) > > drm_kms_helper_hotplug_event(mgr->dev); > > @@ -3840,6 +3878,7 @@ int drm_dp_mst_topology_mgr_init(struct > > drm_dp_mst_topology_mgr *mgr, > > mutex_init(&mgr->destroy_connector_lock); > > INIT_LIST_HEAD(&mgr->tx_msg_downq); > > INIT_LIST_HEAD(&mgr->destroy_connector_list); > > + INIT_LIST_HEAD(&mgr->destroy_branch_device_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); > > diff --git a/include/drm/drm_dp_mst_helper.h > > b/include/drm/drm_dp_mst_helper.h > > index 8c97a5f92c47..c01f3ea72756 100644 > > --- a/include/drm/drm_dp_mst_helper.h > > +++ b/include/drm/drm_dp_mst_helper.h > > @@ -143,6 +143,12 @@ struct drm_dp_mst_branch { > > */ > > struct kref malloc_kref; > > > > + /** > > + * @destroy_next: linked-list entry used by > > + * drm_dp_destroy_connector_work() > > + */ > > + struct list_head destroy_next; > > + > > u8 rad[8]; > > u8 lct; > > int num_ports; > > @@ -578,6 +584,10 @@ struct drm_dp_mst_topology_mgr { > > * @destroy_connector_list: List of to be destroyed connectors. > > */ > > struct list_head destroy_connector_list; > > + /** > > + * @destroy_branch_device_list: List of to be destroyed branch devices > > + */ > > + struct list_head destroy_branch_device_list; > > /** > > * @destroy_connector_lock: Protects @connector_list. > > */ > > -- > > 2.21.0 > >
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 998081b9b205..d7c3d9233834 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1108,34 +1108,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref) struct drm_dp_mst_branch *mstb = container_of(kref, struct drm_dp_mst_branch, topology_kref); struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; - struct drm_dp_mst_port *port, *tmp; - bool wake_tx = false; - - mutex_lock(&mgr->lock); - list_for_each_entry_safe(port, tmp, &mstb->ports, next) { - list_del(&port->next); - drm_dp_mst_topology_put_port(port); - } - mutex_unlock(&mgr->lock); - - /* drop any tx slots msg */ - mutex_lock(&mstb->mgr->qlock); - if (mstb->tx_slots[0]) { - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; - mstb->tx_slots[0] = NULL; - wake_tx = true; - } - if (mstb->tx_slots[1]) { - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; - mstb->tx_slots[1] = NULL; - wake_tx = true; - } - mutex_unlock(&mstb->mgr->qlock); - if (wake_tx) - wake_up_all(&mstb->mgr->tx_waitq); + INIT_LIST_HEAD(&mstb->destroy_next); - drm_dp_mst_put_mstb_malloc(mstb); + /* + * This can get called under mgr->mutex, so we need to perform the + * actual destruction of the mstb in another worker + */ + mutex_lock(&mgr->destroy_connector_lock); + list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list); + mutex_unlock(&mgr->destroy_connector_lock); + schedule_work(&mgr->destroy_connector_work); } /** @@ -3618,10 +3601,56 @@ static void drm_dp_tx_work(struct work_struct *work) mutex_unlock(&mgr->qlock); } +static inline void +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); + + drm_dp_port_teardown_pdt(port, port->pdt); + port->pdt = DP_PEER_DEVICE_NONE; + + drm_dp_mst_put_port_malloc(port); +} + +static inline void +drm_dp_finish_destroy_mst_branch_device(struct drm_dp_mst_branch *mstb) +{ + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; + struct drm_dp_mst_port *port, *tmp; + bool wake_tx = false; + + mutex_lock(&mgr->lock); + list_for_each_entry_safe(port, tmp, &mstb->ports, next) { + list_del(&port->next); + drm_dp_mst_topology_put_port(port); + } + mutex_unlock(&mgr->lock); + + /* drop any tx slots msg */ + mutex_lock(&mstb->mgr->qlock); + if (mstb->tx_slots[0]) { + mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; + mstb->tx_slots[0] = NULL; + wake_tx = true; + } + if (mstb->tx_slots[1]) { + mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; + mstb->tx_slots[1] = NULL; + wake_tx = true; + } + mutex_unlock(&mstb->mgr->qlock); + + if (wake_tx) + wake_up_all(&mstb->mgr->tx_waitq); + + drm_dp_mst_put_mstb_malloc(mstb); +} + 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_dp_mst_port *port; bool send_hotplug = false; /* * Not a regular list traverse as we have to drop the destroy @@ -3629,24 +3658,33 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) * ordering between this lock and the config mutex. */ for (;;) { + struct drm_dp_mst_branch *mstb = NULL; + struct drm_dp_mst_port *port = NULL; + + /* Destroy any MSTBs first, and then their ports second */ mutex_lock(&mgr->destroy_connector_lock); - port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next); - if (!port) { - mutex_unlock(&mgr->destroy_connector_lock); - break; + mstb = list_first_entry_or_null(&mgr->destroy_branch_device_list, + struct drm_dp_mst_branch, + destroy_next); + if (mstb) { + list_del(&mstb->destroy_next); + } else { + port = list_first_entry_or_null(&mgr->destroy_connector_list, + struct drm_dp_mst_port, + next); + if (port) + list_del(&port->next); } - list_del(&port->next); mutex_unlock(&mgr->destroy_connector_lock); - INIT_LIST_HEAD(&port->next); - - mgr->cbs->destroy_connector(mgr, port->connector); - - drm_dp_port_teardown_pdt(port, port->pdt); - port->pdt = DP_PEER_DEVICE_NONE; - - drm_dp_mst_put_port_malloc(port); - send_hotplug = true; + if (mstb) { + drm_dp_finish_destroy_mst_branch_device(mstb); + } else if (port) { + drm_dp_finish_destroy_port(port); + send_hotplug = true; + } else { + break; + } } if (send_hotplug) drm_kms_helper_hotplug_event(mgr->dev); @@ -3840,6 +3878,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->destroy_connector_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_connector_list); + INIT_LIST_HEAD(&mgr->destroy_branch_device_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); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8c97a5f92c47..c01f3ea72756 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -143,6 +143,12 @@ struct drm_dp_mst_branch { */ struct kref malloc_kref; + /** + * @destroy_next: linked-list entry used by + * drm_dp_destroy_connector_work() + */ + struct list_head destroy_next; + u8 rad[8]; u8 lct; int num_ports; @@ -578,6 +584,10 @@ struct drm_dp_mst_topology_mgr { * @destroy_connector_list: List of to be destroyed connectors. */ struct list_head destroy_connector_list; + /** + * @destroy_branch_device_list: List of to be destroyed branch devices + */ + struct list_head destroy_branch_device_list; /** * @destroy_connector_lock: Protects @connector_list. */
Currently we remove MST branch devices from the in-memory topology immediately when their topology refcount reaches 0. This works just fine at the moment, but since we're about to add suspend/resume reprobing for MST topologies we're going to need to be able to travel through the topology and drop topology refs on branch devices while holding mgr->mutex. Since we currently can't do this due to the circular locking dependencies that would incur, move all of the actual work for drm_dp_destroy_mst_branch_device() into drm_dp_destroy_connector_work() so we can drop topology refs on MSTBs in any locking context. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 121 +++++++++++++++++--------- include/drm/drm_dp_mst_helper.h | 10 +++ 2 files changed, 90 insertions(+), 41 deletions(-)