On Wed, 2019-09-25 at 14:16 -0400, Sean Paul wrote:

On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:

...

When reprobing an MST topology during resume, we have to account for the fact that while we were suspended it's possible that mstbs may have been removed from any ports in the topology. Since iterating downwards in the topology requires that we hold &mgr->lock, destroying MSTBs from this context would result in attempting to lock &mgr->lock a second time and deadlocking.

So, fix this by first moving destruction of MSTBs into destroy_connector_work, then rename destroy_connector_work and friends to reflect that they now destroy both ports and mstbs.

Changes since v1:

• s/destroy_connector_list/destroy_port_list/ s/connector_destroy_lock/delayed_destroy_lock/ s/connector_destroy_work/delayed_destroy_work/ s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/ s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
  • danvet
• Use two loops in drm_dp_delayed_destroy_work() - danvet
• Better explain why we need to do this - danvet
• Use cancel_work_sync() instead of flush_work() - flush_work() doesn't account for work requeing

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 Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Took me a while to grok this, and I'm still not 100% confident my mental model is correct, so please bear with me while I ask silly questions :)

Now that the destroy is delayed, and the port remains in the topology, is it possible we will underflow the topology kref by calling put_mstb multiple times? It looks like that would result in a WARN from refcount.c, and wouldn't call the destroy function multiple times, so that's nice :)

Similarly, is there any defense against calling get_mstb() between destroy() and the delayed destroy worker running?

Good question! There's only one instance where we unconditionally grab an MSTB ref, drm_dp_mst_topology_mgr_set_mst(), and in that location we're guaranteed to be the only one with access to that mstb until we drop &mgr->lock. Everywhere else we use drm_dp_mst_topology_try_get_mstb(), which uses kref_get_unless_zero() to protect against that kind of situation (and forces the caller to check with __must_check)

Sean

...

---

drivers/gpu/drm/drm_dp_mst_topology.c | 162 +++++++++++++++++--------- include/drm/drm_dp_mst_helper.h | 26 +++-- 2 files changed, 127 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 3054ec622506..738f260d4b15 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1113,34 +1113,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);

• INIT_LIST_HEAD(&mstb->destroy_next);

• if (wake_tx)

• wake_up_all(&mstb->mgr->tx_waitq);
  

• 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->delayed_destroy_lock);
• list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list);
• mutex_unlock(&mgr->delayed_destroy_lock);
• schedule_work(&mgr->delayed_destroy_work);

}

/** @@ -1255,10 +1238,10 @@ static void drm_dp_destroy_port(struct kref *kref) * 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);
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	list_add(&port->next, &mgr->destroy_port_list);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	schedule_work(&mgr->delayed_destroy_work);
  return;
  

  } /* no need to clean up vcpi

@@ -2792,7 +2775,7 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(&mgr->lock); flush_work(&mgr->work);

• flush_work(&mgr->destroy_connector_work);

• flush_work(&mgr->delayed_destroy_work);

} EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);

@@ -3740,34 +3723,104 @@ 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) +static inline void +drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) {

• 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;

• 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_delayed_destroy_mstb(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_delayed_destroy_work(struct work_struct *work) +{

• struct drm_dp_mst_topology_mgr *mgr =

• container_of(work, struct drm_dp_mst_topology_mgr,
  

• 	     delayed_destroy_work);
  

• bool send_hotplug = false, go_again;
• /*
  • Not a regular list traverse as we have to drop the destroy

• * connector lock before destroying the connector, to avoid AB->BA
  

• * connector lock before destroying the mstb/port, to avoid AB->BA
  

  • ordering between this lock and the config mutex.
  */

• for (;;) {

• 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;
  

• do {

• go_again = false;
  

• for (;;) {
  

• 	struct drm_dp_mst_branch *mstb;
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	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);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	if (!mstb)
  

• 		break;
  

• 	drm_dp_delayed_destroy_mstb(mstb);
  

• 	go_again = true;
  

  }

• list_del(&port->next);
  

• mutex_unlock(&mgr->destroy_connector_lock);
  
  

• mgr->cbs->destroy_connector(mgr, port->connector);
  

• for (;;) {
  

• 	struct drm_dp_mst_port *port;
  
  

• drm_dp_port_teardown_pdt(port, port->pdt);
  

• port->pdt = DP_PEER_DEVICE_NONE;
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	port = list_first_entry_or_null(&mgr-
  

...

destroy_port_list,

• 					struct
  

drm_dp_mst_port,

• 					next);
  

• 	if (port)
  

• 		list_del(&port->next);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	if (!port)
  

• 		break;
  

• 	drm_dp_delayed_destroy_port(port);
  

• 	send_hotplug = true;
  

• 	go_again = true;
  

• }
  

• } while (go_again);

• drm_dp_mst_put_port_malloc(port);
  

• send_hotplug = true;
  

• } if (send_hotplug) drm_kms_helper_hotplug_event(mgr->dev);

} @@ -3957,12 +4010,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);

• mutex_init(&mgr->delayed_destroy_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq);

• INIT_LIST_HEAD(&mgr->destroy_connector_list);

• INIT_LIST_HEAD(&mgr->destroy_port_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);

• INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work); init_waitqueue_head(&mgr->tx_waitq); mgr->dev = dev; mgr->aux = aux;

@@ -4005,7 +4059,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) { drm_dp_mst_topology_mgr_set_mst(mgr, false); flush_work(&mgr->work);

• flush_work(&mgr->destroy_connector_work);

• cancel_work_sync(&mgr->delayed_destroy_work); mutex_lock(&mgr->payload_lock); kfree(mgr->payloads); mgr->payloads = NULL;

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index fc349204a71b..4a4507fe928d 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_delayed_destroy_work()
  

• */
  

• struct list_head destroy_next;
• u8 rad[8]; u8 lct; int num_ports;

@@ -575,18 +581,24 @@ struct drm_dp_mst_topology_mgr { struct work_struct tx_work;

/**

• * @destroy_connector_list: List of to be destroyed connectors.
  

• * @destroy_port_list: List of to be destroyed connectors.
  

• */
  

• struct list_head destroy_port_list;
• /**

• * @destroy_branch_device_list: List of to be destroyed branch
  

• * devices.
  

  */

• struct list_head destroy_connector_list;

• struct list_head destroy_branch_device_list; /**

• * @destroy_connector_lock: Protects @connector_list.
  

• * @delayed_destroy_lock: Protects @destroy_port_list and
  

• * @destroy_branch_device_list.
  

  */

• struct mutex destroy_connector_lock;

• struct mutex delayed_destroy_lock; /**

• * @destroy_connector_work: Work item to destroy connectors. Needed to
  

• * avoid locking inversion.
  

• * @delayed_destroy_work: Work item to destroy MST port and branch
  

• * devices, needed to avoid locking inversion.
  

  */

• struct work_struct destroy_connector_work;

• struct work_struct delayed_destroy_work;

};

int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,

2.21.0

On Fri, Sep 27, 2019 at 09:31:07AM -0400, Sean Paul wrote:

On Wed, Sep 25, 2019 at 04:08:22PM -0400, Lyude Paul wrote:

...

On Wed, 2019-09-25 at 14:16 -0400, Sean Paul wrote:

...

On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:

...

When reprobing an MST topology during resume, we have to account for the fact that while we were suspended it's possible that mstbs may have been removed from any ports in the topology. Since iterating downwards in the topology requires that we hold &mgr->lock, destroying MSTBs from this context would result in attempting to lock &mgr->lock a second time and deadlocking.

So, fix this by first moving destruction of MSTBs into destroy_connector_work, then rename destroy_connector_work and friends to reflect that they now destroy both ports and mstbs.

Changes since v1:

• s/destroy_connector_list/destroy_port_list/ s/connector_destroy_lock/delayed_destroy_lock/ s/connector_destroy_work/delayed_destroy_work/ s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/ s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
  • danvet
• Use two loops in drm_dp_delayed_destroy_work() - danvet
• Better explain why we need to do this - danvet
• Use cancel_work_sync() instead of flush_work() - flush_work() doesn't account for work requeing

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 Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Took me a while to grok this, and I'm still not 100% confident my mental model is correct, so please bear with me while I ask silly questions :)

Now that the destroy is delayed, and the port remains in the topology, is it possible we will underflow the topology kref by calling put_mstb multiple times? It looks like that would result in a WARN from refcount.c, and wouldn't call the destroy function multiple times, so that's nice :)

Similarly, is there any defense against calling get_mstb() between destroy() and the delayed destroy worker running?

Good question! There's only one instance where we unconditionally grab an MSTB ref, drm_dp_mst_topology_mgr_set_mst(), and in that location we're guaranteed to be the only one with access to that mstb until we drop &mgr->lock. Everywhere else we use drm_dp_mst_topology_try_get_mstb(), which uses kref_get_unless_zero() to protect against that kind of situation (and forces the caller to check with __must_check)

Imo would be good to add this to the commit message when merging as a Q&A, just for the record. At least I like to do that with the silly-but-no-so-silly questions that come up in review. -Daniel

Awesome, thanks for the breakdown!

Reviewed-by: Sean Paul sean@poorly.run

...

...

Sean

...

---

drivers/gpu/drm/drm_dp_mst_topology.c | 162 +++++++++++++++++--------- include/drm/drm_dp_mst_helper.h | 26 +++-- 2 files changed, 127 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 3054ec622506..738f260d4b15 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1113,34 +1113,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);

• INIT_LIST_HEAD(&mstb->destroy_next);

• if (wake_tx)

• wake_up_all(&mstb->mgr->tx_waitq);
  

• 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->delayed_destroy_lock);
• list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list);
• mutex_unlock(&mgr->delayed_destroy_lock);
• schedule_work(&mgr->delayed_destroy_work);

}

/** @@ -1255,10 +1238,10 @@ static void drm_dp_destroy_port(struct kref *kref) * 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);
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	list_add(&port->next, &mgr->destroy_port_list);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	schedule_work(&mgr->delayed_destroy_work);
  return;
  

  } /* no need to clean up vcpi

@@ -2792,7 +2775,7 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(&mgr->lock); flush_work(&mgr->work);

• flush_work(&mgr->destroy_connector_work);

• flush_work(&mgr->delayed_destroy_work);

} EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);

@@ -3740,34 +3723,104 @@ 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) +static inline void +drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) {

• 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;

• 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_delayed_destroy_mstb(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_delayed_destroy_work(struct work_struct *work) +{

• struct drm_dp_mst_topology_mgr *mgr =

• container_of(work, struct drm_dp_mst_topology_mgr,
  

• 	     delayed_destroy_work);
  

• bool send_hotplug = false, go_again;
• /*
  • Not a regular list traverse as we have to drop the destroy

• * connector lock before destroying the connector, to avoid AB->BA
  

• * connector lock before destroying the mstb/port, to avoid AB->BA
  

  • ordering between this lock and the config mutex.
  */

• for (;;) {

• 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;
  

• do {

• go_again = false;
  

• for (;;) {
  

• 	struct drm_dp_mst_branch *mstb;
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	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);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	if (!mstb)
  

• 		break;
  

• 	drm_dp_delayed_destroy_mstb(mstb);
  

• 	go_again = true;
  

  }

• list_del(&port->next);
  

• mutex_unlock(&mgr->destroy_connector_lock);
  
  

• mgr->cbs->destroy_connector(mgr, port->connector);
  

• for (;;) {
  

• 	struct drm_dp_mst_port *port;
  
  

• drm_dp_port_teardown_pdt(port, port->pdt);
  

• port->pdt = DP_PEER_DEVICE_NONE;
  

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	port = list_first_entry_or_null(&mgr-
  

...

destroy_port_list,

• 					struct
  

drm_dp_mst_port,

• 					next);
  

• 	if (port)
  

• 		list_del(&port->next);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	if (!port)
  

• 		break;
  

• 	drm_dp_delayed_destroy_port(port);
  

• 	send_hotplug = true;
  

• 	go_again = true;
  

• }
  

• } while (go_again);

• drm_dp_mst_put_port_malloc(port);
  

• send_hotplug = true;
  

• } if (send_hotplug) drm_kms_helper_hotplug_event(mgr->dev);

} @@ -3957,12 +4010,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);

• mutex_init(&mgr->delayed_destroy_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq);

• INIT_LIST_HEAD(&mgr->destroy_connector_list);

• INIT_LIST_HEAD(&mgr->destroy_port_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);

• INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work); init_waitqueue_head(&mgr->tx_waitq); mgr->dev = dev; mgr->aux = aux;

@@ -4005,7 +4059,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) { drm_dp_mst_topology_mgr_set_mst(mgr, false); flush_work(&mgr->work);

• flush_work(&mgr->destroy_connector_work);

• cancel_work_sync(&mgr->delayed_destroy_work); mutex_lock(&mgr->payload_lock); kfree(mgr->payloads); mgr->payloads = NULL;

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index fc349204a71b..4a4507fe928d 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_delayed_destroy_work()
  

• */
  

• struct list_head destroy_next;
• u8 rad[8]; u8 lct; int num_ports;

@@ -575,18 +581,24 @@ struct drm_dp_mst_topology_mgr { struct work_struct tx_work;

/**

• * @destroy_connector_list: List of to be destroyed connectors.
  

• * @destroy_port_list: List of to be destroyed connectors.
  

• */
  

• struct list_head destroy_port_list;
• /**

• * @destroy_branch_device_list: List of to be destroyed branch
  

• * devices.
  

  */

• struct list_head destroy_connector_list;

• struct list_head destroy_branch_device_list; /**

• * @destroy_connector_lock: Protects @connector_list.
  

• * @delayed_destroy_lock: Protects @destroy_port_list and
  

• * @destroy_branch_device_list.
  

  */

• struct mutex destroy_connector_lock;

• struct mutex delayed_destroy_lock; /**

• * @destroy_connector_work: Work item to destroy connectors. Needed to
  

• * avoid locking inversion.
  

• * @delayed_destroy_work: Work item to destroy MST port and branch
  

• * devices, needed to avoid locking inversion.
  

  */

• struct work_struct destroy_connector_work;

• struct work_struct delayed_destroy_work;

};

int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,

2.21.0

-- Cheers, Lyude Paul

-- Sean Paul, Software Engineer, Google / Chromium OS

On Tue, Sep 03, 2019 at 04:45:42PM -0400, Lyude Paul wrote:

Yes, apparently we've been testing this for every single driver load for quite a long time now. At least that means our PBN calculation is solid!

Anyway, introduce self tests for MST and move this into there.

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 Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Sean Paul sean@poorly.run

---

drivers/gpu/drm/drm_dp_mst_topology.c | 27 --------------- drivers/gpu/drm/selftests/Makefile | 2 +- .../gpu/drm/selftests/drm_modeset_selftests.h | 1 + .../drm/selftests/test-drm_dp_mst_helper.c | 34 +++++++++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + 5 files changed, 37 insertions(+), 28 deletions(-) create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 738f260d4b15..6f7f449ca12b 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -47,7 +47,6 @@ */ static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr, char *buf); -static int test_calc_pbn_mode(void);

static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);

@@ -3561,30 +3560,6 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) } EXPORT_SYMBOL(drm_dp_calc_pbn_mode);

-static int test_calc_pbn_mode(void) -{

• int ret;
• ret = drm_dp_calc_pbn_mode(154000, 30);
• if (ret != 689) {

• DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n",
  

• 		154000, 30, 689, ret);
  

• return -EINVAL;
  

• }
• ret = drm_dp_calc_pbn_mode(234000, 30);
• if (ret != 1047) {

• DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n",
  

• 		234000, 30, 1047, ret);
  

• return -EINVAL;
  

• }
• ret = drm_dp_calc_pbn_mode(297000, 24);
• if (ret != 1063) {

• DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n",
  

• 		297000, 24, 1063, ret);
  

• return -EINVAL;
  

• }
• return 0;

-}

/* we want to kick the TX after we've ack the up/down IRQs. */ static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr) { @@ -4033,8 +4008,6 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (!mgr->proposed_vcpis) return -ENOMEM; set_bit(0, &mgr->payload_mask);

• if (test_calc_pbn_mode() < 0)

• DRM_ERROR("MST PBN self-test failed\n");
  
  

  mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL); if (mst_state == NULL)

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile index aae88f8a016c..d2137342b371 100644 --- a/drivers/gpu/drm/selftests/Makefile +++ b/drivers/gpu/drm/selftests/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \ test-drm_format.o test-drm_framebuffer.o \

•       test-drm_damage_helper.o
  

•       test-drm_damage_helper.o test-drm_dp_mst_helper.o
  
  

obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h index 464753746013..dec3ee3ec96f 100644 --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h @@ -32,3 +32,4 @@ selftest(damage_iter_damage_one_intersect, igt_damage_iter_damage_one_intersect) selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside) selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved) selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible) +selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode) diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c new file mode 100644 index 000000000000..9baa5171988d --- /dev/null +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Test cases for for the DRM DP MST helpers
• */

+#include <drm/drm_dp_mst_helper.h> +#include <drm/drm_print.h>

+#include "test-drm_modeset_common.h"

+int igt_dp_mst_calc_pbn_mode(void *ignored) +{

• int pbn, i;
• const struct {

• int rate;
  

• int bpp;
  

• int expected;
  

• } test_params[] = {

• { 154000, 30, 689 },
  

• { 234000, 30, 1047 },
  

• { 297000, 24, 1063 },
  

• };
• for (i = 0; i < ARRAY_SIZE(test_params); i++) {

• pbn = drm_dp_calc_pbn_mode(test_params[i].rate,
  

• 			   test_params[i].bpp);
  

• FAIL(pbn != test_params[i].expected,
  

•      "Expected PBN %d for clock %d bpp %d, got %d\n",
  

•      test_params[i].expected, test_params[i].rate,
  

•      test_params[i].bpp, pbn);
  

• }
• return 0;

+} diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h index 8c76f09c12d1..590bda35a683 100644 --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h @@ -39,5 +39,6 @@ int igt_damage_iter_damage_one_intersect(void *ignored); int igt_damage_iter_damage_one_outside(void *ignored); int igt_damage_iter_damage_src_moved(void *ignored); int igt_damage_iter_damage_not_visible(void *ignored); +int igt_dp_mst_calc_pbn_mode(void *ignored);

#endif

2.21.0

Noticed this while working on adding a drm_dp_decode_sideband_req(). DP_POWER_DOWN_PHY/DP_POWER_UP_PHY both use the same struct as DP_ENUM_PATH_RESOURCES, so we can just combine their cases.

Changes since v2: * Fix commit message

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@ffwll.ch 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 --- drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 6f7f449ca12b..1c862749cb63 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -271,6 +271,8 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req,

switch (req->req_type) { case DP_ENUM_PATH_RESOURCES: + case DP_POWER_DOWN_PHY: + case DP_POWER_UP_PHY: buf[idx] = (req->u.port_num.port_number & 0xf) << 4; idx++; break; @@ -358,12 +360,6 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, memcpy(&buf[idx], req->u.i2c_write.bytes, req->u.i2c_write.num_bytes); idx += req->u.i2c_write.num_bytes; break; - - case DP_POWER_DOWN_PHY: - case DP_POWER_UP_PHY: - buf[idx] = (req->u.port_num.port_number & 0xf) << 4; - idx++; - break; } raw->cur_len = idx; }

On Tue, 03 Sep 2019, Lyude Paul lyude@redhat.com wrote:

Unfortunately the DP MST helpers do not have much in the way of debugging utilities. So, let's add some!

This adds basic debugging output for down sideband requests that we send from the driver, so that we can actually discern what's happening when sideband requests timeout.

Since there wasn't really a good way of testing that any of this worked, I ended up writing simple selftests that lightly test sideband message encoding and decoding as well. Enjoy!

Changes since v1:

• Clean up DO_TEST() and sideband_msg_req_encode_decode() - danvet
• Get rid of pr_fmt(), just define a prefix string instead and use drm_printf()
• Check highest bit of VCPI in drm_dp_decode_sideband_req() - danvet
• Make the switch case order between drm_dp_decode_sideband_req() and drm_dp_encode_sideband_req() the same - danvet
• Only check DRM_UT_DP - danvet
• Clean up sideband_msg_req_equal() from selftests a bit, and add comments explaining why we can't just use memcmp - danvet

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 Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 309 +++++++++++++++++- .../gpu/drm/drm_dp_mst_topology_internal.h | 24 ++ .../gpu/drm/selftests/drm_modeset_selftests.h | 1 + .../drm/selftests/test-drm_dp_mst_helper.c | 204 ++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + include/drm/drm_dp_mst_helper.h | 2 +- 6 files changed, 536 insertions(+), 5 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_mst_topology_internal.h

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 1c862749cb63..f5f1d8b50fb6 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -37,6 +37,7 @@ #include <drm/drm_probe_helper.h>

#include "drm_crtc_helper_internal.h" +#include "drm_dp_mst_topology_internal.h"

/**

• DOC: dp mst helper

@@ -73,6 +74,8 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);

+#define DBG_PREFIX "[dp_mst]"

#define DP_STR(x) [DP_ ## x] = #x

static const char *drm_dp_mst_req_type_str(u8 req_type) @@ -129,6 +132,43 @@ static const char *drm_dp_mst_nak_reason_str(u8 nak_reason) }

#undef DP_STR +#define DP_STR(x) [DRM_DP_SIDEBAND_TX_ ## x] = #x

+static const char *drm_dp_mst_sideband_tx_state_str(int state) +{

• static const char * const sideband_reason_str[] = {

• DP_STR(QUEUED),
  

• DP_STR(START_SEND),
  

• DP_STR(SENT),
  

• DP_STR(RX),
  

• DP_STR(TIMEOUT),
  

• };
• if (state >= ARRAY_SIZE(sideband_reason_str) ||

•    !sideband_reason_str[state])
  

• return "unknown";
  

• return sideband_reason_str[state];

+}

+static int +drm_dp_mst_rad_to_str(const u8 rad[8], u8 lct, char *out, size_t len) +{

• int i;
• u8 unpacked_rad[16];
• for (i = 0; i < lct; i++) {

• if (i % 2)
  

• 	unpacked_rad[i] = rad[i / 2] >> 4;
  

• else
  

• 	unpacked_rad[i] = rad[i / 2] & BIT_MASK(4);
  

• }
• /* TODO: Eventually add something to printk so we can format the rad

• * like this: 1.2.3
  

• */
  

• return snprintf(out, len, "%*phC", lct, unpacked_rad);

+}

/* sideband msg handling */ static u8 drm_dp_msg_header_crc4(const uint8_t *data, size_t num_nibbles) @@ -261,8 +301,9 @@ static bool drm_dp_decode_sideband_msg_hdr(struct drm_dp_sideband_msg_hdr *hdr, return true; }

-static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req,

• 		       struct drm_dp_sideband_msg_tx *raw)
  

+void +drm_dp_encode_sideband_req(const struct drm_dp_sideband_msg_req_body *req,

• 	   struct drm_dp_sideband_msg_tx *raw)
  

{ int idx = 0; int i; @@ -363,6 +404,251 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, } raw->cur_len = idx; } +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_encode_sideband_req);

+/* Decode a sideband request we've encoded, mainly used for debugging */ +int +drm_dp_decode_sideband_req(const struct drm_dp_sideband_msg_tx *raw,

• 	   struct drm_dp_sideband_msg_req_body *req)
  

+{

• const u8 *buf = raw->msg;
• int i, idx = 0;
• req->req_type = buf[idx++] & 0x7f;
• switch (req->req_type) {
• case DP_ENUM_PATH_RESOURCES:
• case DP_POWER_DOWN_PHY:
• case DP_POWER_UP_PHY:

• req->u.port_num.port_number = (buf[idx] >> 4) & 0xf;
  

• break;
  

• case DP_ALLOCATE_PAYLOAD:

• {
  

• 	struct drm_dp_allocate_payload *a =
  

• 		&req->u.allocate_payload;
  

• 	a->number_sdp_streams = buf[idx] & 0xf;
  

• 	a->port_number = (buf[idx] >> 4) & 0xf;
  

• 	WARN_ON(buf[++idx] & 0x80);
  

• 	a->vcpi = buf[idx] & 0x7f;
  

• 	a->pbn = buf[++idx] << 8;
  

• 	a->pbn |= buf[++idx];
  

• 	idx++;
  

• 	for (i = 0; i < a->number_sdp_streams; i++) {
  

• 		a->sdp_stream_sink[i] =
  

• 			(buf[idx + (i / 2)] >> ((i % 2) ? 0 : 4)) & 0xf;
  

• 	}
  

• }
  

• break;
  

• case DP_QUERY_PAYLOAD:

• req->u.query_payload.port_number = (buf[idx] >> 4) & 0xf;
  

• WARN_ON(buf[++idx] & 0x80);
  

• req->u.query_payload.vcpi = buf[idx] & 0x7f;
  

• break;
  

• case DP_REMOTE_DPCD_READ:

• {
  

• 	struct drm_dp_remote_dpcd_read *r = &req->u.dpcd_read;
  

• 	r->port_number = (buf[idx] >> 4) & 0xf;
  

• 	r->dpcd_address = (buf[idx] << 16) & 0xf0000;
  

• 	r->dpcd_address |= (buf[++idx] << 8) & 0xff00;
  

• 	r->dpcd_address |= buf[++idx] & 0xff;
  

• 	r->num_bytes = buf[++idx];
  

• }
  

• break;
  

• case DP_REMOTE_DPCD_WRITE:

• {
  

• 	struct drm_dp_remote_dpcd_write *w =
  

• 		&req->u.dpcd_write;
  

• 	w->port_number = (buf[idx] >> 4) & 0xf;
  

• 	w->dpcd_address = (buf[idx] << 16) & 0xf0000;
  

• 	w->dpcd_address |= (buf[++idx] << 8) & 0xff00;
  

• 	w->dpcd_address |= buf[++idx] & 0xff;
  

• 	w->num_bytes = buf[++idx];
  

• 	w->bytes = kmemdup(&buf[++idx], w->num_bytes,
  

• 			   GFP_KERNEL);
  

• 	if (!w->bytes)
  

• 		return -ENOMEM;
  

• }
  

• break;
  

• case DP_REMOTE_I2C_READ:

• {
  

• 	struct drm_dp_remote_i2c_read *r = &req->u.i2c_read;
  

• 	struct drm_dp_remote_i2c_read_tx *tx;
  

• 	bool failed = false;
  

• 	r->num_transactions = buf[idx] & 0x3;
  

• 	r->port_number = (buf[idx] >> 4) & 0xf;
  

• 	for (i = 0; i < r->num_transactions; i++) {
  

• 		tx = &r->transactions[i];
  

• 		tx->i2c_dev_id = buf[++idx] & 0x7f;
  

• 		tx->num_bytes = buf[++idx];
  

• 		tx->bytes = kmemdup(&buf[++idx],
  

• 				    tx->num_bytes,
  

• 				    GFP_KERNEL);
  

• 		if (!tx->bytes) {
  

• 			failed = true;
  

• 			break;
  

• 		}
  

• 		idx += tx->num_bytes;
  

• 		tx->no_stop_bit = (buf[idx] >> 5) & 0x1;
  

• 		tx->i2c_transaction_delay = buf[idx] & 0xf;
  

• 	}
  

• 	if (failed) {
  

• 		for (i = 0; i < r->num_transactions; i++)
  

• 			kfree(tx->bytes);
  

• 		return -ENOMEM;
  

• 	}
  

• 	r->read_i2c_device_id = buf[++idx] & 0x7f;
  

• 	r->num_bytes_read = buf[++idx];
  

• }
  

• break;
  

• case DP_REMOTE_I2C_WRITE:

• {
  

• 	struct drm_dp_remote_i2c_write *w = &req->u.i2c_write;
  

• 	w->port_number = (buf[idx] >> 4) & 0xf;
  

• 	w->write_i2c_device_id = buf[++idx] & 0x7f;
  

• 	w->num_bytes = buf[++idx];
  

• 	w->bytes = kmemdup(&buf[++idx], w->num_bytes,
  

• 			   GFP_KERNEL);
  

• 	if (!w->bytes)
  

• 		return -ENOMEM;
  

• }
  

• break;
  

• }
• return 0;

+} +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_decode_sideband_req);

+void +drm_dp_dump_sideband_msg_req_body(const struct drm_dp_sideband_msg_req_body *req,

• 		  int indent, struct drm_printer *printer)
  

+{

• int i;

+#define P(f, ...) drm_printf_indent(printer, indent, f, ##__VA_ARGS__)

• if (req->req_type == DP_LINK_ADDRESS) {

• /* No contents to print */
  

• P("type=%s\n", drm_dp_mst_req_type_str(req->req_type));
  

• return;
  

• }
• P("type=%s contents:\n", drm_dp_mst_req_type_str(req->req_type));
• indent++;
• switch (req->req_type) {
• case DP_ENUM_PATH_RESOURCES:
• case DP_POWER_DOWN_PHY:
• case DP_POWER_UP_PHY:

• P("port=%d\n", req->u.port_num.port_number);
  

• break;
  

• case DP_ALLOCATE_PAYLOAD:

• P("port=%d vcpi=%d pbn=%d sdp_streams=%d %*ph\n",
  

•   req->u.allocate_payload.port_number,
  

•   req->u.allocate_payload.vcpi, req->u.allocate_payload.pbn,
  

•   req->u.allocate_payload.number_sdp_streams,
  

•   req->u.allocate_payload.number_sdp_streams,
  

•   req->u.allocate_payload.sdp_stream_sink);
  

• break;
  

• case DP_QUERY_PAYLOAD:

• P("port=%d vcpi=%d\n",
  

•   req->u.query_payload.port_number,
  

•   req->u.query_payload.vcpi);
  

• break;
  

• case DP_REMOTE_DPCD_READ:

• P("port=%d dpcd_addr=%05x len=%d\n",
  

•   req->u.dpcd_read.port_number, req->u.dpcd_read.dpcd_address,
  

•   req->u.dpcd_read.num_bytes);
  

• break;
  

• case DP_REMOTE_DPCD_WRITE:

• P("port=%d addr=%05x len=%d: %*ph\n",
  

•   req->u.dpcd_write.port_number,
  

•   req->u.dpcd_write.dpcd_address,
  

•   req->u.dpcd_write.num_bytes, req->u.dpcd_write.num_bytes,
  

•   req->u.dpcd_write.bytes);
  

• break;
  

• case DP_REMOTE_I2C_READ:

• P("port=%d num_tx=%d id=%d size=%d:\n",
  

•   req->u.i2c_read.port_number,
  

•   req->u.i2c_read.num_transactions,
  

•   req->u.i2c_read.read_i2c_device_id,
  

•   req->u.i2c_read.num_bytes_read);
  

• indent++;
  

• for (i = 0; i < req->u.i2c_read.num_transactions; i++) {
  

• 	const struct drm_dp_remote_i2c_read_tx *rtx =
  

• 		&req->u.i2c_read.transactions[i];
  

• 	P("%d: id=%03d size=%03d no_stop_bit=%d tx_delay=%03d: %*ph\n",
  

• 	  i, rtx->i2c_dev_id, rtx->num_bytes,
  

• 	  rtx->no_stop_bit, rtx->i2c_transaction_delay,
  

• 	  rtx->num_bytes, rtx->bytes);
  

• }
  

• break;
  

• case DP_REMOTE_I2C_WRITE:

• P("port=%d id=%d size=%d: %*ph\n",
  

•   req->u.i2c_write.port_number,
  

•   req->u.i2c_write.write_i2c_device_id,
  

•   req->u.i2c_write.num_bytes, req->u.i2c_write.num_bytes,
  

•   req->u.i2c_write.bytes);
  

• break;
  

• default:

• P("???\n");
  

• break;
  

• }

+#undef P +} +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_dump_sideband_msg_req_body);

+static inline void +drm_dp_mst_dump_sideband_msg_tx(struct drm_printer *p,

• 		const struct drm_dp_sideband_msg_tx *txmsg)
  

+{

• struct drm_dp_sideband_msg_req_body req;
• char buf[64];
• int ret;
• int i;
• drm_dp_mst_rad_to_str(txmsg->dst->rad, txmsg->dst->lct, buf,

• 	      sizeof(buf));
  

• drm_printf(p, "txmsg cur_offset=%x cur_len=%x seqno=%x state=%s path_msg=%d dst=%s\n",

•    txmsg->cur_offset, txmsg->cur_len, txmsg->seqno,
  

•    drm_dp_mst_sideband_tx_state_str(txmsg->state),
  

•    txmsg->path_msg, buf);
  

• ret = drm_dp_decode_sideband_req(txmsg, &req);
• if (ret) {

• drm_printf(p, "<failed to decode sideband req: %d>\n", ret);
  

• return;
  

• }
• drm_dp_dump_sideband_msg_req_body(&req, 1, p);
• switch (req.req_type) {
• case DP_REMOTE_DPCD_WRITE:

• kfree(req.u.dpcd_write.bytes);
  

• break;
  

• case DP_REMOTE_I2C_READ:

• for (i = 0; i < req.u.i2c_read.num_transactions; i++)
  

• 	kfree(req.u.i2c_read.transactions[i].bytes);
  

• break;
  

• case DP_REMOTE_I2C_WRITE:

• kfree(req.u.i2c_write.bytes);
  

• break;
  

• }

+}

static void drm_dp_crc_sideband_chunk_req(u8 *msg, u8 len) { @@ -894,6 +1180,11 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb, } } out:

• if (unlikely(ret == -EIO && drm_debug & DRM_UT_DP)) {

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

• } mutex_unlock(&mgr->qlock);

  return ret;

@@ -2013,8 +2304,11 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr, idx += tosend + 1;

ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);

• if (ret) {

• DRM_DEBUG_KMS("sideband msg failed to send\n");
  

• if (unlikely(ret && drm_debug & DRM_UT_DP)) {

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_printf(&p, "sideband msg failed to send\n");
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

  return ret; }

@@ -2076,6 +2370,13 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr, { mutex_lock(&mgr->qlock); list_add_tail(&txmsg->next, &mgr->tx_msg_downq);

• if (unlikely(drm_debug & DRM_UT_DP)) {

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

• }
• if (list_is_singular(&mgr->tx_msg_downq)) process_single_down_tx_qlock(mgr); mutex_unlock(&mgr->qlock);

diff --git a/drivers/gpu/drm/drm_dp_mst_topology_internal.h b/drivers/gpu/drm/drm_dp_mst_topology_internal.h new file mode 100644 index 000000000000..eeda9a61c657 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_mst_topology_internal.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only

• • Declarations for DP MST related functions which are only used in selftests
• • Copyright © 2018 Red Hat
• • Authors:

• • Lyude Paul <lyude@redhat.com>
    

• */

+#ifndef _DRM_DP_MST_HELPER_INTERNAL_H_ +#define _DRM_DP_MST_HELPER_INTERNAL_H_

+#include <drm/drm_dp_mst_helper.h>

+void +drm_dp_encode_sideband_req(const struct drm_dp_sideband_msg_req_body *req,

• 	   struct drm_dp_sideband_msg_tx *raw);
  

+int drm_dp_decode_sideband_req(const struct drm_dp_sideband_msg_tx *raw,

• 	       struct drm_dp_sideband_msg_req_body *req);
  

+void +drm_dp_dump_sideband_msg_req_body(const struct drm_dp_sideband_msg_req_body *req,

• 		  int indent, struct drm_printer *printer);
  

+#endif /* !_DRM_DP_MST_HELPER_INTERNAL_H_ */ diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h index dec3ee3ec96f..1898de0b4a4d 100644 --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h @@ -33,3 +33,4 @@ selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside) selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved) selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible) selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode) +selftest(dp_mst_sideband_msg_req_decode, igt_dp_mst_sideband_msg_req_decode) diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c index 9baa5171988d..af2b2de65316 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -3,9 +3,12 @@

• Test cases for for the DRM DP MST helpers

*/

+#define PREFIX_STR "[drm_dp_mst_helper]"

#include <drm/drm_dp_mst_helper.h> #include <drm/drm_print.h>

+#include "../drm_dp_mst_topology_internal.h" #include "test-drm_modeset_common.h"

int igt_dp_mst_calc_pbn_mode(void *ignored) @@ -32,3 +35,204 @@ int igt_dp_mst_calc_pbn_mode(void *ignored)

return 0; }

+static bool +sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,

•        const struct drm_dp_sideband_msg_req_body *out)
  

+{

• const struct drm_dp_remote_i2c_read_tx *txin, *txout;
• int i;
• if (in->req_type != out->req_type)

• return false;
  

• switch (in->req_type) {
• /*

• * Compare struct members manually for request types which can't be
  

• * compared simply using memcmp(). This is because said request types
  

• * contain pointers to other allocated structs
  

• */
  

• case DP_REMOTE_I2C_READ:

+#define IN in->u.i2c_read +#define OUT out->u.i2c_read

• if (IN.num_bytes_read != OUT.num_bytes_read ||
  

•     IN.num_transactions != OUT.num_transactions ||
  

•     IN.port_number != OUT.port_number ||
  

•     IN.read_i2c_device_id != OUT.read_i2c_device_id)
  

• 	return false;
  

• for (i = 0; i < IN.num_transactions; i++) {
  

• 	txin = &IN.transactions[i];
  

• 	txout = &OUT.transactions[i];
  

• 	if (txin->i2c_dev_id != txout->i2c_dev_id ||
  

• 	    txin->no_stop_bit != txout->no_stop_bit ||
  

• 	    txin->num_bytes != txout->num_bytes ||
  

• 	    txin->i2c_transaction_delay !=
  

• 	    txout->i2c_transaction_delay)
  

• 		return false;
  

• 	if (memcmp(txin->bytes, txout->bytes,
  

• 		   txin->num_bytes) != 0)
  

• 		return false;
  

• }
  

• break;
  

+#undef IN +#undef OUT

• case DP_REMOTE_DPCD_WRITE:

+#define IN in->u.dpcd_write +#define OUT out->u.dpcd_write

• if (IN.dpcd_address != OUT.dpcd_address ||
  

•     IN.num_bytes != OUT.num_bytes ||
  

•     IN.port_number != OUT.port_number)
  

• 	return false;
  

• return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
  

+#undef IN +#undef OUT

• case DP_REMOTE_I2C_WRITE:

+#define IN in->u.i2c_write +#define OUT out->u.i2c_write

• if (IN.port_number != OUT.port_number ||
  

•     IN.write_i2c_device_id != OUT.write_i2c_device_id ||
  

•     IN.num_bytes != OUT.num_bytes)
  

• 	return false;
  

• return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
  

+#undef IN +#undef OUT

• default:

• return memcmp(in, out, sizeof(*in)) == 0;
  

• }
• return true;

+}

+static bool +sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in) +{

• struct drm_dp_sideband_msg_req_body out = {0};
• struct drm_printer p = drm_err_printer(PREFIX_STR);
• struct drm_dp_sideband_msg_tx txmsg;
• int i, ret;
• drm_dp_encode_sideband_req(in, &txmsg);
• ret = drm_dp_decode_sideband_req(&txmsg, &out);
• if (ret < 0) {

• drm_printf(&p, "Failed to decode sideband request: %d\n",
  

• 	   ret);
  

Is this the sole reason of adding drm_err_printer()? Where do you intend to take error printing next, in general? Why isn't DRM_ERROR() the right thing to do here?

It's just that I think debug and error printing from drm is a mess, and instead of cleaning it up, we seem to be piling on new stuff without direction.

BR, Jani.

• return false;
  

• }
• if (!sideband_msg_req_equal(in, &out)) {

• drm_printf(&p, "Encode/decode failed, expected:\n");
  

• drm_dp_dump_sideband_msg_req_body(in, 1, &p);
  

• drm_printf(&p, "Got:\n");
  

• drm_dp_dump_sideband_msg_req_body(&out, 1, &p);
  

• return false;
  

• }
• switch (in->req_type) {
• case DP_REMOTE_DPCD_WRITE:

• kfree(out.u.dpcd_write.bytes);
  

• break;
  

• case DP_REMOTE_I2C_READ:

• for (i = 0; i < out.u.i2c_read.num_transactions; i++)
  

• 	kfree(out.u.i2c_read.transactions[i].bytes);
  

• break;
  

• case DP_REMOTE_I2C_WRITE:

• kfree(out.u.i2c_write.bytes);
  

• break;
  

• }
• /* Clear everything but the req_type for the input */
• memset(&in->u, 0, sizeof(in->u));
• return true;

+}

+int igt_dp_mst_sideband_msg_req_decode(void *unused) +{

• struct drm_dp_sideband_msg_req_body in = { 0 };
• u8 data[] = { 0xff, 0x0, 0xdd };
• int i;

+#define DO_TEST() FAIL_ON(!sideband_msg_req_encode_decode(&in))

• in.req_type = DP_ENUM_PATH_RESOURCES;
• in.u.port_num.port_number = 5;
• DO_TEST();
• in.req_type = DP_POWER_UP_PHY;
• in.u.port_num.port_number = 5;
• DO_TEST();
• in.req_type = DP_POWER_DOWN_PHY;
• in.u.port_num.port_number = 5;
• DO_TEST();
• in.req_type = DP_ALLOCATE_PAYLOAD;
• in.u.allocate_payload.number_sdp_streams = 3;
• for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++)

• in.u.allocate_payload.sdp_stream_sink[i] = i + 1;
  

• DO_TEST();
• in.u.allocate_payload.port_number = 0xf;
• DO_TEST();
• in.u.allocate_payload.vcpi = 0x7f;
• DO_TEST();
• in.u.allocate_payload.pbn = U16_MAX;
• DO_TEST();
• in.req_type = DP_QUERY_PAYLOAD;
• in.u.query_payload.port_number = 0xf;
• DO_TEST();
• in.u.query_payload.vcpi = 0x7f;
• DO_TEST();
• in.req_type = DP_REMOTE_DPCD_READ;
• in.u.dpcd_read.port_number = 0xf;
• DO_TEST();
• in.u.dpcd_read.dpcd_address = 0xfedcb;
• DO_TEST();
• in.u.dpcd_read.num_bytes = U8_MAX;
• DO_TEST();
• in.req_type = DP_REMOTE_DPCD_WRITE;
• in.u.dpcd_write.port_number = 0xf;
• DO_TEST();
• in.u.dpcd_write.dpcd_address = 0xfedcb;
• DO_TEST();
• in.u.dpcd_write.num_bytes = ARRAY_SIZE(data);
• in.u.dpcd_write.bytes = data;
• DO_TEST();
• in.req_type = DP_REMOTE_I2C_READ;
• in.u.i2c_read.port_number = 0xf;
• DO_TEST();
• in.u.i2c_read.read_i2c_device_id = 0x7f;
• DO_TEST();
• in.u.i2c_read.num_transactions = 3;
• in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3;
• for (i = 0; i < in.u.i2c_read.num_transactions; i++) {

• in.u.i2c_read.transactions[i].bytes = data;
  

• in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data);
  

• in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i;
  

• in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i;
  

• }
• DO_TEST();
• in.req_type = DP_REMOTE_I2C_WRITE;
• in.u.i2c_write.port_number = 0xf;
• DO_TEST();
• in.u.i2c_write.write_i2c_device_id = 0x7f;
• DO_TEST();
• in.u.i2c_write.num_bytes = ARRAY_SIZE(data);
• in.u.i2c_write.bytes = data;
• DO_TEST();

+#undef DO_TEST

• return 0;

+} diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h index 590bda35a683..0fcb8bbc6a1b 100644 --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h @@ -40,5 +40,6 @@ int igt_damage_iter_damage_one_outside(void *ignored); int igt_damage_iter_damage_src_moved(void *ignored); int igt_damage_iter_damage_not_visible(void *ignored); int igt_dp_mst_calc_pbn_mode(void *ignored); +int igt_dp_mst_sideband_msg_req_decode(void *ignored);

#endif diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 4a4507fe928d..5423a8adda78 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -293,7 +293,7 @@ struct drm_dp_remote_dpcd_write { struct drm_dp_remote_i2c_read { u8 num_transactions; u8 port_number;

• struct {

• struct drm_dp_remote_i2c_read_tx { u8 i2c_dev_id; u8 num_bytes; u8 *bytes;

On Tue, Sep 03, 2019 at 04:45:46PM -0400, Lyude Paul wrote:

This will allow us to add some locking for port->* members, in particular the PDT and ->connector, which can't be done from drm_dp_destroy_port() since we don't know what locks the caller might be holding.

Might be nice to mention that this is already done in the delayed destroy worker so readers don't need to go looking for it. Perhaps update this when you apply the patch.

Changes since v2:

• Clarify commit message

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Sean Paul sean@poorly.run

---

drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++++++++++---------------- 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index f5f1d8b50fb6..af3189df28aa 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1511,31 +1511,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);
  

• /* There's nothing that needs locking to destroy an input port yet */
• if (port->input) {

• drm_dp_mst_put_port_malloc(port);
  

• return;
  

• }

• /*
  

•  * 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 */
  

• kfree(port->cached_edid);

• 	mutex_lock(&mgr->delayed_destroy_lock);
  

• 	list_add(&port->next, &mgr->destroy_port_list);
  

• 	mutex_unlock(&mgr->delayed_destroy_lock);
  

• 	schedule_work(&mgr->delayed_destroy_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;
  

• }
• drm_dp_mst_put_port_malloc(port);

• /*

• * we can't destroy the connector here, as we might be holding the
  

• * mode_config.mutex from an EDID retrieval
  

• */
  

• mutex_lock(&mgr->delayed_destroy_lock);
• list_add(&port->next, &mgr->destroy_port_list);
• mutex_unlock(&mgr->delayed_destroy_lock);
• schedule_work(&mgr->delayed_destroy_work);

}

/** @@ -3998,7 +3989,8 @@ static void drm_dp_tx_work(struct work_struct *work) static inline void drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) {

• port->mgr->cbs->destroy_connector(port->mgr, port->connector);

• 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;

-- 2.21.0

On Wed, 4 Sep 2019 at 06:48, Lyude Paul lyude@redhat.com wrote:

And it's helper, we'll be using this in just a moment.

Reviewed-by: Dave Airlie airlied@redhat.com

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 43452872efad..b44d3696c09a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2060,7 +2060,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_

static struct drm_dp_mst_branch *get_mst_branch_device_by_guid_helper( struct drm_dp_mst_branch *mstb,

•   uint8_t *guid)
  

•   const uint8_t *guid)
  

{ struct drm_dp_mst_branch *found_mstb; struct drm_dp_mst_port *port; @@ -2084,7 +2084,7 @@ static struct drm_dp_mst_branch *get_mst_branch_device_by_guid_helper(

static struct drm_dp_mst_branch * drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr,

•                                uint8_t *guid)
  

•                                const uint8_t *guid)
  

{ struct drm_dp_mst_branch *mstb; int ret; -- 2.21.0

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx

On Tue, Sep 03, 2019 at 04:45:52PM -0400, Lyude Paul wrote:

Turns out we've been forgetting for a while now to actually destroy any of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do that.

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Cleanup is overrated :)

Reviewed-by: Sean Paul sean@poorly.run

---

drivers/gpu/drm/drm_dp_mst_topology.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 74161f442584..2f88cc173500 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4339,6 +4339,11 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mgr->aux = NULL; drm_atomic_private_obj_fini(&mgr->base); mgr->funcs = NULL;

• mutex_destroy(&mgr->delayed_destroy_lock);
• mutex_destroy(&mgr->payload_lock);
• mutex_destroy(&mgr->qlock);
• mutex_destroy(&mgr->lock);

} EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);

-- 2.21.0

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@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@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

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@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@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@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

On Tue, Sep 03, 2019 at 04:45:55PM -0400, Lyude Paul wrote:

The names for these functions are rather confusing. drm_dp_add_port() sounds like a function that would simply create a port and add it to a topology, and do nothing more. Similarly, drm_dp_update_port() would be assumed to be the function that should be used to update port information after initial creation.

While those assumptions are currently correct in how these functions are used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port() can also update the information on a port, and seems explicitly designed to do so. This can be explained pretty simply by the fact that there's more situations that would involve updating the port information based on a link address response as opposed to a connection status notification than the driver's initial topology probe. Case in point: reprobing link addresses after suspend/resume.

Since we're about to start using drm_dp_add_port() differently for suspend/resume reprobing, let's rename both functions to clarify what they actually do.

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Sean Paul sean@poorly.run

---

drivers/gpu/drm/drm_dp_mst_topology.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 9944ef2ce885..cfaf9eb7ace9 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1900,9 +1900,10 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);

-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)
  

+static void +drm_dp_mst_handle_link_address_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; @@ -2011,8 +2012,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, 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)
  

+static void +drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,

• 	    struct drm_dp_connection_status_notify *conn_stat)
  

{ struct drm_dp_mst_port *port; int old_ddps; @@ -2464,7 +2466,8 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, drm_dp_check_mstb_guid(mstb, reply->guid);

for (i = 0; i < reply->nports; i++)

• drm_dp_add_port(mstb, mgr->dev, &reply->ports[i]);
  

• drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
  

• 				    &reply->ports[i]);
  
  

  drm_kms_helper_hotplug_event(mgr->dev);

@@ -3324,7 +3327,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) }

if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {

• drm_dp_update_port(mstb, &msg.u.conn_stat);
  

• drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat);
  
  

  DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number,

-- 2.21.0

On Tue, Sep 03, 2019 at 04:45:56PM -0400, Lyude Paul wrote:

These are most certainly accessed from far more than the mgr work. In fact, up_req_recv is -only- ever accessed from outside the mgr work.

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Sean Paul sean@poorly.run

---

include/drm/drm_dp_mst_helper.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index f253ee43e9d9..8ba2a01324bb 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -489,15 +489,11 @@ struct drm_dp_mst_topology_mgr { int conn_base_id;

/**

• * @down_rep_recv: Message receiver state for down replies. This and
  

• * @up_req_recv are only ever access from the work item, which is
  

• * serialised.
  

• * @down_rep_recv: Message receiver state for down replies.
  

  */ struct drm_dp_sideband_msg_rx down_rep_recv; /**

• * @up_req_recv: Message receiver state for up requests. This and
  

• * @down_rep_recv are only ever access from the work item, which is
  

• * serialised.
  

• * @up_req_recv: Message receiver state for up requests.
  

  */ struct drm_dp_sideband_msg_rx up_req_recv;

-- 2.21.0

On Tue, Sep 03, 2019 at 04:45:57PM -0400, Lyude Paul wrote:

Once upon a time, hotplugging devices on MST branches actually worked in DRM. Now, it only works in amdgpu (likely because of how it's hotplug handlers are implemented). On both i915 and nouveau, hotplug notifications from MST branches are noticed - but trying to respond to them causes messaging timeouts and causes the whole topology state to go out of sync with reality, usually resulting in the user needing to replug the entire topology in hopes that it actually fixes things.

The reason for this is because the way we currently handle UP requests in MST is completely bogus. drm_dp_mst_handle_up_req() is called from drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug handler. Because we handle sending the hotplug event from this function, we actually cause the driver's hotplug handler (and in turn, all sideband transactions) to block on drm_device->mode_config.connection_mutex. This makes it impossible to send any sideband messages from the driver's connector probing functions, resulting in the aforementioned sideband message timeout.

There's even more problems with this beyond breaking hotplugging on MST branch devices. It also makes it almost impossible to protect drm_dp_mst_port struct members under a lock because we then have to worry about dealing with all of the lock dependency issues that ensue.

So, let's finally actually fix this issue by handling the processing of up requests asyncronously. This way we can send sideband messages from most contexts without having to deal with getting blocked if we hold connection_mutex. This also fixes MST branch device hotplugging on i915, finally!

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

Looks really good!

Reviewed-by: Sean Paul sean@poorly.run

---

drivers/gpu/drm/drm_dp_mst_topology.c | 146 +++++++++++++++++++------- include/drm/drm_dp_mst_helper.h | 16 +++ 2 files changed, 122 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index cfaf9eb7ace9..5101eeab4485 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -46,6 +46,12 @@

• protocol. The helpers contain a topology manager and bandwidth manager.
• The helpers encapsulate the sending and received of sideband msgs.

*/ +struct drm_dp_pending_up_req {

• struct drm_dp_sideband_msg_hdr hdr;
• struct drm_dp_sideband_msg_req_body msg;
• struct list_head next;

+};

static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr, char *buf);

@@ -3109,6 +3115,7 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(&mgr->lock);

• flush_work(&mgr->up_req_work); flush_work(&mgr->work); flush_work(&mgr->delayed_destroy_work);

} @@ -3281,12 +3288,70 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr) return 0; }

+static inline void +drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,

• 	  struct drm_dp_pending_up_req *up_req)
  

+{

• struct drm_dp_mst_branch *mstb = NULL;
• struct drm_dp_sideband_msg_req_body *msg = &up_req->msg;
• struct drm_dp_sideband_msg_hdr *hdr = &up_req->hdr;
• if (hdr->broadcast) {

• const u8 *guid = NULL;
  

• if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY)
  

• 	guid = msg->u.conn_stat.guid;
  

• else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY)
  

• 	guid = msg->u.resource_stat.guid;
  

• mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
  

• } else {

• mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
  

• }
• if (!mstb) {

• DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
  

• 	      hdr->lct);
  

• return;
  

• }
• /* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */
• if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {

• drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
  

• drm_kms_helper_hotplug_event(mgr->dev);
  

• }
• drm_dp_mst_topology_put_mstb(mstb);

+}

+static void drm_dp_mst_up_req_work(struct work_struct *work) +{

• struct drm_dp_mst_topology_mgr *mgr =

• container_of(work, struct drm_dp_mst_topology_mgr,
  

• 	     up_req_work);
  

• struct drm_dp_pending_up_req *up_req;
• while (true) {

• mutex_lock(&mgr->up_req_lock);
  

• up_req = list_first_entry_or_null(&mgr->up_req_list,
  

• 				  struct drm_dp_pending_up_req,
  

• 				  next);
  

• if (up_req)
  

• 	list_del(&up_req->next);
  

• mutex_unlock(&mgr->up_req_lock);
  

• if (!up_req)
  

• 	break;
  

• drm_dp_mst_process_up_req(mgr, up_req);
  

• kfree(up_req);
  

• }

+}

static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) {

• struct drm_dp_sideband_msg_req_body msg; struct drm_dp_sideband_msg_hdr *hdr = &mgr->up_req_recv.initial_hdr;
• struct drm_dp_mst_branch *mstb = NULL;
• const u8 *guid;

• struct drm_dp_pending_up_req *up_req; bool seqno;

  if (!drm_dp_get_one_sb_msg(mgr, true))

@@ -3295,56 +3360,53 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) if (!mgr->up_req_recv.have_eomt) return 0;

• if (!hdr->broadcast) {

• mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
  

• if (!mstb) {
  

• 	DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
  

• 		      hdr->lct);
  

• 	goto out;
  

• }
  

• up_req = kzalloc(sizeof(*up_req), GFP_KERNEL);

• if (!up_req) {

• DRM_ERROR("Not enough memory to process MST up req\n");
  

• return -ENOMEM;
  

  }

• INIT_LIST_HEAD(&up_req->next);

  seqno = hdr->seqno;

• drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg);

• drm_dp_sideband_parse_req(&mgr->up_req_recv, &up_req->msg);

• if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY)

• guid = msg.u.conn_stat.guid;
  

• else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY)

• guid = msg.u.resource_stat.guid;
  

• else

• if (up_req->msg.req_type != DP_CONNECTION_STATUS_NOTIFY &&

•    up_req->msg.req_type != DP_RESOURCE_STATUS_NOTIFY) {
  

• DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n",
  

• 	      up_req->msg.req_type);
  

• kfree(up_req);
  

  goto out;

• drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno,

• 		 false);
  

• if (!mstb) {

• mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
  

• if (!mstb) {
  

• 	DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
  

• 		      hdr->lct);
  

• 	goto out;
  

• }
  

  }

• if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {

• drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat);
  

• drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, up_req->msg.req_type,

• 		 seqno, false);
  

• if (up_req->msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {

• const struct drm_dp_connection_status_notify *conn_stat =
  

• 	&up_req->msg.u.conn_stat;
  
  

  DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n",

• 	      msg.u.conn_stat.port_number,
  

• 	      msg.u.conn_stat.legacy_device_plug_status,
  

• 	      msg.u.conn_stat.displayport_device_plug_status,
  

• 	      msg.u.conn_stat.message_capability_status,
  

• 	      msg.u.conn_stat.input_port,
  

• 	      msg.u.conn_stat.peer_device_type);
  

• 	      conn_stat->port_number,
  

• 	      conn_stat->legacy_device_plug_status,
  

• 	      conn_stat->displayport_device_plug_status,
  

• 	      conn_stat->message_capability_status,
  

• 	      conn_stat->input_port,
  

• 	      conn_stat->peer_device_type);
  

• } else if (up_req->msg.req_type == DP_RESOURCE_STATUS_NOTIFY) {

• const struct drm_dp_resource_status_notify *res_stat =
  

• 	&up_req->msg.u.resource_stat;
  
  

• drm_kms_helper_hotplug_event(mgr->dev);
  

• } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n",

• 	      msg.u.resource_stat.port_number,
  

• 	      msg.u.resource_stat.available_pbn);
  

• 	      res_stat->port_number,
  

• 	      res_stat->available_pbn);
  

  }

• drm_dp_mst_topology_put_mstb(mstb);

• up_req->hdr = *hdr;
• mutex_lock(&mgr->up_req_lock);
• list_add_tail(&up_req->next, &mgr->up_req_list);
• mutex_unlock(&mgr->up_req_lock);
• queue_work(system_long_wq, &mgr->up_req_work);

out: memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); return 0; @@ -4320,12 +4382,15 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->qlock); mutex_init(&mgr->payload_lock); mutex_init(&mgr->delayed_destroy_lock);

• mutex_init(&mgr->up_req_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_port_list); INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
• INIT_LIST_HEAD(&mgr->up_req_list); INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work); INIT_WORK(&mgr->tx_work, drm_dp_tx_work); INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work);
• INIT_WORK(&mgr->up_req_work, drm_dp_mst_up_req_work); init_waitqueue_head(&mgr->tx_waitq); mgr->dev = dev; mgr->aux = aux;

@@ -4382,6 +4447,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mutex_destroy(&mgr->payload_lock); mutex_destroy(&mgr->qlock); mutex_destroy(&mgr->lock);

• mutex_destroy(&mgr->up_req_lock);

} EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8ba2a01324bb..7d80c38ee00e 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -597,6 +597,22 @@ struct drm_dp_mst_topology_mgr { * devices, needed to avoid locking inversion. */ struct work_struct delayed_destroy_work;

• /**

• * @up_req_list: List of pending up requests from the topology that
  

• * need to be processed, in chronological order.
  

• */
  

• struct list_head up_req_list;
• /**

• * @up_req_lock: Protects @up_req_list
  

• */
  

• struct mutex up_req_lock;
• /**

• * @up_req_work: Work item to process up requests received from the
  

• * topology. Needed to avoid blocking hotplug handling and sideband
  

• * transmissions.
  

• */
  

• struct work_struct up_req_work;

};

int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,

2.21.0

On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:

Yes-you read that right. Currently there is literally no locking in place for any of the drm_dp_mst_port struct members that can be modified in response to a link address response, or a connection status response. Which literally means if we're unlucky enough to have any sort of hotplugging event happen before we're finished with reprobing link addresses, we'll race and the contents of said struct members becomes undefined. Fun!

So, finally add some simple locking protections to our MST helpers by protecting any drm_dp_mst_port members which can be changed by link address responses or connection status notifications under drm_device->mode_config.connection_mutex.

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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++++++++++++++++++------- include/drm/drm_dp_mst_helper.h | 39 +++++-- 2 files changed, 133 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5101eeab4485..259634c5d6dc 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref) container_of(kref, struct drm_dp_mst_port, malloc_kref);

drm_dp_mst_put_mstb_malloc(port->parent);

• mutex_destroy(&port->lock); kfree(port);

}

@@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);

+static void +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,

• 	      struct drm_dp_mst_port *port)
  

+{

• struct drm_dp_mst_topology_mgr *mgr = port->mgr;
• char proppath[255];
• int ret;
• build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
• port->connector = mgr->cbs->add_connector(mgr, port, proppath);
• if (!port->connector) {

• ret = -ENOMEM;
  

• goto error;
  

• }
• if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||

•     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
  

•    port->port_num >= DP_MST_LOGICAL_PORT_0) {
  

• port->cached_edid = drm_get_edid(port->connector,
  

• 				 &port->aux.ddc);
  

• drm_connector_set_tile_property(port->connector);
  

• }
• mgr->cbs->register_connector(port->connector);
• return;

+error:

• DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);

+}

static void drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_device *dev, @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port;

• bool created = false;
• int old_ddps = 0;

• struct drm_dp_mst_branch *child_mstb = NULL;

• struct drm_connector *connector_to_destroy = NULL;

• int old_ddps = 0, ret;

• u8 new_pdt = DP_PEER_DEVICE_NONE;

• bool created = false, send_link_addr = false,

•     create_connector = false;
  
  

  port = drm_dp_get_port(mstb, port_msg->port_number); if (!port) {

@@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, return; kref_init(&port->topology_kref); kref_init(&port->malloc_kref);

• mutex_init(&port->lock);
  

  port->parent = mstb; port->port_num = port_msg->port_number; port->mgr = mgr;

@@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, drm_dp_mst_get_mstb_malloc(mstb);

created = true;

• } else {

• old_ddps = port->ddps;
  

  }

• mutex_lock(&port->lock);
• drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
• if (!created)

• old_ddps = port->ddps;
  

• port->input = port_msg->input_port;
• if (!port->input)

• new_pdt = port_msg->peer_device_type;
  

  port->mcs = port_msg->mcs; port->ddps = port_msg->ddps; port->ldps = port_msg->legacy_device_plug_status;

@@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *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;
  

• ret = drm_dp_port_set_pdt(port, new_pdt);
• if (ret == 1) {

• send_link_addr = true;
  

• } else if (ret < 0) {

• DRM_ERROR("Failed to change PDT on port %p: %d\n",
  

• 	  port, ret);
  

• goto fail_unlock;
  

• }
• if (send_link_addr) {

• mutex_lock(&mgr->lock);
  

• if (port->mstb) {
  

• 	child_mstb = port->mstb;
  

• 	drm_dp_mst_get_mstb_malloc(child_mstb);
  

  }

• mutex_unlock(&mgr->lock);
  

  }

• if (created && !port->input) {

• char proppath[255];
  

• /*

• * We unset port->connector before dropping connection_mutex so that
  

• * there's no chance any of the atomic MST helpers can accidentally
  

• * associate a to-be-destroyed connector with a port.
  

• */
  

• if (port->connector && port->input) {

• connector_to_destroy = port->connector;
  

• port->connector = NULL;
  

• } else if (!port->connector && !port->input) {

• create_connector = true;
  

• }

• build_mst_prop_path(mstb, port->port_num, proppath,
  

• 		    sizeof(proppath));
  

• port->connector = (*mgr->cbs->add_connector)(mgr, port,
  

• 					     proppath);
  

• if (!port->connector)
  

• 	goto fail;
  

• drm_modeset_unlock(&dev->mode_config.connection_mutex);

Do you drop this early b/c it deadlocks with something upstack? If so, I wonder if you could plumb an acquire context through the appropriate functions to avoid needing the port->lock at all?

Sean

• if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
  

•      port->pdt == DP_PEER_DEVICE_SST_SINK) &&
  

•     port->port_num >= DP_MST_LOGICAL_PORT_0) {
  

• 	port->cached_edid = drm_get_edid(port->connector,
  

• 					 &port->aux.ddc);
  

• 	drm_connector_set_tile_property(port->connector);
  

• }
  

• if (connector_to_destroy)

• mgr->cbs->destroy_connector(mgr, connector_to_destroy);
  

• else if (create_connector)

• drm_dp_mst_port_add_connector(mstb, port);
  

• mutex_unlock(&port->lock);

• (*mgr->cbs->register_connector)(port->connector);
  

• if (send_link_addr && child_mstb) {

• drm_dp_send_link_address(mgr, child_mstb);
  

• drm_dp_mst_put_mstb_malloc(child_mstb);
  

  }

  /* put reference to this port */ drm_dp_mst_topology_put_port(port); return;

-fail: +fail_unlock:

• drm_modeset_unlock(&dev->mode_config.connection_mutex);
• mutex_unlock(&port->lock);
• /* Remove it from the port list */ mutex_lock(&mgr->lock); list_del(&port->next);

@@ -2022,6 +2078,7 @@ static void drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, struct drm_dp_connection_status_notify *conn_stat) {

• struct drm_device *dev = mstb->mgr->dev; struct drm_dp_mst_port *port; int old_ddps; bool dowork = false;

@@ -2030,6 +2087,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, if (!port) return;

• drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
• old_ddps = port->ddps; port->mcs = conn_stat->message_capability_status; port->ldps = conn_stat->legacy_device_plug_status;

@@ -2055,6 +2114,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, } }

• drm_modeset_unlock(&dev->mode_config.connection_mutex); drm_dp_mst_topology_put_port(port); if (dowork) queue_work(system_long_wq, &mstb->mgr->work);

@@ -2147,28 +2207,34 @@ drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr, static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) {

• struct drm_device *dev = mgr->dev; 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);

  list_for_each_entry(port, &mstb->ports, next) {

• if (port->input)
  

• 	continue;
  

• struct drm_dp_mst_branch *mstb_child = NULL;
  

• drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
  
  

• if (!port->ddps)
  

• if (port->input || !port->ddps) {
  

• 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
  continue;
  

• }
  
  

  if (!port->available_pbn) drm_dp_send_enum_path_resources(mgr, mstb, port);

• if (port->mstb) {
  

• if (port->mstb)
  mstb_child = drm_dp_mst_topology_get_mstb_validated(
      mgr, port->mstb);
  

• 	if (mstb_child) {
  

• 		drm_dp_check_and_send_link_address(mgr, mstb_child);
  

• 		drm_dp_mst_topology_put_mstb(mstb_child);
  

• 	}
  

• drm_modeset_unlock(&dev->mode_config.connection_mutex);
  

• if (mstb_child) {
  

• 	drm_dp_check_and_send_link_address(mgr, mstb_child);
  

• 	drm_dp_mst_topology_put_mstb(mstb_child);
  

  } }

} diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 7d80c38ee00e..1efbb086f7ac 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -45,23 +45,34 @@ struct drm_dp_vcpi { /**

• struct drm_dp_mst_port - MST port
• @port_num: port number

• • @input: if this port is an input port.
• • @mcs: message capability status - DP 1.2 spec.
• • @ddps: DisplayPort Device Plug Status - DP 1.2
• • @pdt: Peer Device Type
• • @ldps: Legacy Device Plug Status
• • @dpcd_rev: DPCD revision of device on this port
• • @num_sdp_streams: Number of simultaneous streams
• • @num_sdp_stream_sinks: Number of stream sinks
• • @available_pbn: Available bandwidth for this port.

• • @input: if this port is an input port. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @mcs: message capability status - DP 1.2 spec. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @pdt: Peer Device Type. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @ldps: Legacy Device Plug Status. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @dpcd_rev: DPCD revision of device on this port. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @num_sdp_streams: Number of simultaneous streams. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @num_sdp_stream_sinks: Number of stream sinks. Protected by
• • &drm_device.mode_config.connection_mutex.
• • @available_pbn: Available bandwidth for this port. Protected by
• • &drm_device.mode_config.connection_mutex.
  • @next: link to next port on this branch device
  • @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

• • by &drm_device.mode_config.connection_mutex.
  • @parent: branch device parent of this port
  • @vcpi: Virtual Channel Payload info for this port.

• • @connector: DRM connector this port is connected to.

• • @connector: DRM connector this port is connected to. Protected by @lock.
• • When there is already a connector registered for this port, this is also
• • protected by &drm_device.mode_config.connection_mutex.
  • @mgr: topology manager this port lives under.
  • This structure represents an MST port endpoint on a device somewhere

@@ -100,6 +111,12 @@ struct drm_dp_mst_port { struct drm_connector *connector; struct drm_dp_mst_topology_mgr *mgr;

• /**

• * @lock: Protects @connector. If needed, this lock should be grabbed
  

• * before &drm_device.mode_config.connection_mutex.
  

• */
  

• struct mutex lock;
• /**
  • @cached_edid: for DP logical ports - make tiling work by ensuring
  • that the EDID for all connectors is read immediately.

-- 2.21.0