[PATCH 0/3] drm/dp_mst: Misc drive-by fixes

[PATCH 0/3] drm/dp_mst: Misc drive-by fixes

[Lyude Paul]

Two not terribly important fixes, and one actually important fix. Should
be pretty easy to review :)

Lyude Paul (3):
  drm/dp_mst: Remove lock check in drm_atomic_get_mst_topology_state()
  drm/dp_mst: Fix potential topology ref leak in
    drm_dp_atomic_find_vcpi_slots()
  drm/dp_mst: Fix topology ref leak in drm_dp_mst_allocate_vcpi()

 drivers/gpu/drm/drm_dp_mst_topology.c | 53 ++++++++-------------------
 include/drm/drm_dp_mst_helper.h       | 20 +++++++++-
 2 files changed, 33 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH 1/3] drm/dp_mst: Remove lock check in drm_atomic_get_mst_topology_state()

[Lyude Paul]

Since modesetting locks by default were added to private objects in:

commit b962a12050a3 ("drm/atomic: integrate modeset lock with private
objects")

This means that the atomic state of a drm_dp_mst_topology_mgr is now
protected by drm_dp_mst_topology_mgr->base.lock as opposed to the main
connection_status mutex. This also means that locking isn't left up to
the caller anymore, and is handled automatically in
drm_atomic_get_private_obj_state().

So, remove the WARN_ON() in drm_atomic_get_mst_topology_state() since
that's now incorrect, and update the kernel docs for the function.
Additionally since all function is now is a simple wrapper around
drm_atomic_get_private_obj_state(), it seems more reasonable to move
this out of drm_dp_mst_topology.c and turn it into an inline function in
drm_dp_mst_helper.h

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 25 -------------------------
 include/drm/drm_dp_mst_helper.h       | 20 ++++++++++++++++++--
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 196ebba8af5f..49575b80caeb 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3718,31 +3718,6 @@ const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs = {
 };
 EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
 
-/**
- * drm_atomic_get_mst_topology_state: get MST topology state
- *
- * @state: global atomic state
- * @mgr: MST topology manager, also the private object in this case
- *
- * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
- * state vtable so that the private object state returned is that of a MST
- * topology object. Also, drm_atomic_get_private_obj_state() expects the caller
- * to care of the locking, so warn if don't hold the connection_mutex.
- *
- * RETURNS:
- *
- * The MST topology state or error pointer.
- */
-struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
-								    struct drm_dp_mst_topology_mgr *mgr)
-{
-	struct drm_device *dev = mgr->dev;
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
-}
-EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
-
 /**
  * drm_dp_mst_topology_mgr_init - initialise a topology manager
  * @mgr: manager struct to initialise
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 8c97a5f92c47..263d82178ecd 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -643,8 +643,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int __must_check
 drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
-struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
-								    struct drm_dp_mst_topology_mgr *mgr);
 int __must_check
 drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 			      struct drm_dp_mst_topology_mgr *mgr,
@@ -756,4 +754,22 @@ __drm_dp_mst_state_iter_get(struct drm_atomic_state *state,
 	for ((__i) = 0; (__i) < (__state)->num_private_objs; (__i)++) \
 		for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), NULL, &(new_state), (__i)))
 
+/**
+ * drm_atomic_get_mst_topology_state() - get MST topology state
+ * @state: global atomic state
+ * @mgr: MST topology manager, also the private object in this case
+ *
+ * This function retrieves the atomic topology state for @mgr using
+ * drm_atomic_get_priv_obj_state().
+ *
+ * Returns:
+ * The atomic MST topology state or an error pointer.
+ */
+static inline struct drm_dp_mst_topology_state *
+drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr)
+{
+	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(
+		state, &mgr->base));
+}
 #endif
-- 
2.20.1

[PATCH 2/3] drm/dp_mst: Fix potential topology ref leak in drm_dp_atomic_find_vcpi_slots()

[Lyude Paul]

This is a very minute issue I introduced that I just noticed when
scrolling through drm_dp_mst_topology.c: if a driver uses
drm_dp_atomic_find_vcpi_slots() incorrectly, we'll forget to release the
topology reference we grabbed on port.

So, fix this by jumping to 'out' instead of returning -EINVAL
immediately.

Signed-off-by: Lyude Paul <lyude@redhat.com>

Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations")
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 49575b80caeb..d99560c5c693 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3070,7 +3070,8 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 			if (WARN_ON(!prev_slots)) {
 				DRM_ERROR("cannot allocate and release VCPI on [MST PORT:%p] in the same state\n",
 					  port);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto out;
 			}
 
 			break;
-- 
2.20.1

[PATCH 3/3] drm/dp_mst: Fix topology ref leak in drm_dp_mst_allocate_vcpi()

[Lyude Paul]

Another drive-by fix. If slots < 0, or drm_dp_init_vcpi() fails, we'll
end up returning from the function without dropping the topology ref
that we grabbed at the very start. This would result in an MST hub
and/or it's ports staying around even after the MST topology has been
removed from the system.

So, fix this by making sure that we always drop the topology ref to port
when returning from this function.

Additionally: it looks like this bug exists pre-topology & malloc krefs,
so let's also make sure this gets backported to stable.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper (v0.6)")
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: <stable@vger.kernel.org> # v3.17+
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d99560c5c693..403f035dc8b8 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3177,28 +3177,29 @@ EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
 bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_mst_port *port, int pbn, int slots)
 {
-	int ret;
+	int rc;
+	bool ret = false;
 
-	port = drm_dp_mst_topology_get_port_validated(mgr, port);
-	if (!port)
+	if (slots < 0)
 		return false;
 
-	if (slots < 0)
+	port = drm_dp_mst_topology_get_port_validated(mgr, port);
+	if (!port)
 		return false;
 
 	if (port->vcpi.vcpi > 0) {
 		DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn %d - requested pbn %d\n",
 			      port->vcpi.vcpi, port->vcpi.pbn, pbn);
 		if (pbn == port->vcpi.pbn) {
-			drm_dp_mst_topology_put_port(port);
-			return true;
+			ret = true;
+			goto out;
 		}
 	}
 
-	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
-	if (ret) {
+	rc = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
+	if (rc) {
 		DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n",
-			      DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
+			      DIV_ROUND_UP(pbn, mgr->pbn_div), rc);
 		goto out;
 	}
 	DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
@@ -3206,10 +3207,10 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 
 	/* Keep port allocated until it's payload has been removed */
 	drm_dp_mst_get_port_malloc(port);
-	drm_dp_mst_topology_put_port(port);
-	return true;
+	ret = true;
 out:
-	return false;
+	drm_dp_mst_topology_put_port(port);
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_mst_allocate_vcpi);
 
-- 
2.20.1