Nouveau - Nov 2018 - [PATCH 0/6] Remove all bad dp_mst_port uses and hide struct def

So we don't ever have to worry about drivers touching drm_dp_mst_port
structs without verifying them and crashing again.

Lyude Paul (6):
  drm/dp_mst: Add drm_dp_get_payload_info()
  drm/nouveau: Use drm_dp_get_payload_info() for getting payload/vcpi
  drm/nouveau: Stop reading port->mgr in nv50_mstc_get_modes()
  drm/nouveau: Stop reading port->mgr in nv50_mstc_detect()
  drm/dp_mst: Hide drm_dp_mst_port contents from drivers
  drm/i915: Start using struct drm_dp_mst_port again

 drivers/gpu/drm/drm_dp_mst_topology.c   | 115 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c     |   2 +-
 drivers/gpu/drm/i915/intel_drv.h        |   2 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c |  60 +++++--------
 include/drm/drm_dp_mst_helper.h         |  65 ++------------
 5 files changed, 146 insertions(+), 98 deletions(-)

-- 
2.19.1

Some hardware (nvidia hardware in particular) needs to be notified of
the exact VCPI and payload settings that the topology manager decided on
for each mstb port. Since there isn't currently any way to get this
information without going through port (which drivers are very much not
supposed to do by themselves, ever), let's add one.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 56 +++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  5 ++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 529414556962..4336d17ce904 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1982,6 +1982,62 @@ int drm_dp_update_payload_part2(struct
drm_dp_mst_topology_mgr *mgr)
 }
 EXPORT_SYMBOL(drm_dp_update_payload_part2);
 
+/**
+ * drm_dp_get_payload_info() - Retrieve payload/vcpi information for the given
+ *                             @port
+ * @mgr: manager to use
+ * @port: the port to get the relevant payload information for
+ * @vcpi_out: where to copy the port's VCPI information to
+ * @payload_out: where to copy the port's payload information to
+ *
+ * Searches the current payloads for @mgr and finds the relevant payload and
+ * VCPI information that was programmed by the topology mgr, then copies it
+ * into @vcpi_out and @payload_out. Drivers which need to know this
+ * information must use this helper as opposed to checking @port themselves,
+ * as this helper will ensure the port reference is still valid and grab the
+ * appropriate locks in @mgr.
+ *
+ * Returns:
+ * 0 on success, negative error code if the port is no longer valid or a
+ * programmed payload could not be found for @port.
+ */
+int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
+			    struct drm_dp_mst_port *port,
+			    struct drm_dp_vcpi *vcpi_out,
+			    struct drm_dp_payload *payload_out)
+{
+	struct drm_dp_payload *payload = NULL;
+	int i;
+	int ret = 0;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (!port)
+		return -EINVAL;
+
+	mutex_lock(&mgr->payload_lock);
+	/* Figure out which of the payloads belongs to this port */
+	for (i = 0; i < mgr->max_payloads; i++) {
+		if (mgr->payloads[i].vcpi == port->vcpi.vcpi) {
+			payload = &mgr->payloads[i];
+			break;
+		}
+	}
+
+	if (!payload) {
+		DRM_DEBUG_KMS("Failed to find payload for port %p\n", port);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	*payload_out = *payload;
+	*vcpi_out = port->vcpi;
+out:
+	mutex_unlock(&mgr->payload_lock);
+	drm_dp_put_port(port);
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_get_payload_info);
+
 #if 0 /* unused as of yet */
 static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port,
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 59f005b419cf..9cc93ea60e7e 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -592,7 +592,10 @@ bool drm_dp_mst_allocate_vcpi(struct
drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_mst_port *port, int pbn, int slots);
 
 int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct
drm_dp_mst_port *port);
-
+int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
+			    struct drm_dp_mst_port *port,
+			    struct drm_dp_vcpi *vcpi_out,
+			    struct drm_dp_payload *payload_out);
 
 void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct
drm_dp_mst_port *port);
 
-- 
2.19.1

Currently, nouveau tries to go through the drm_dp_mst_port structures
itself in order to retrieve the relevant payload and VCPI information
that it needs to report to the GPU. This is wrong: mstc->port could be
destroyed at any point, and additionally the payload could be changed at
any point because it doesn't bother trying to grab the payload lock. So;
remove nv50_msto_payload entirely and use the new
drm_dp_get_payload_info() helper.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 55 ++++++++++---------------
 1 file changed, 21 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 6cbbae3f438b..e6f72ca0b1fa 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -665,41 +665,24 @@ struct nv50_msto {
 	bool disabled;
 };
 
-static struct drm_dp_payload *
-nv50_msto_payload(struct nv50_msto *msto)
-{
-	struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
-	struct nv50_mstc *mstc = msto->mstc;
-	struct nv50_mstm *mstm = mstc->mstm;
-	int vcpi = mstc->port->vcpi.vcpi, i;
-
-	NV_ATOMIC(drm, "%s: vcpi %d\n", msto->encoder.name, vcpi);
-	for (i = 0; i < mstm->mgr.max_payloads; i++) {
-		struct drm_dp_payload *payload = &mstm->mgr.payloads[i];
-		NV_ATOMIC(drm, "%s: %d: vcpi %d start 0x%02x slots 0x%02x\n",
-			  mstm->outp->base.base.name, i, payload->vcpi,
-			  payload->start_slot, payload->num_slots);
-	}
-
-	for (i = 0; i < mstm->mgr.max_payloads; i++) {
-		struct drm_dp_payload *payload = &mstm->mgr.payloads[i];
-		if (payload->vcpi == vcpi)
-			return payload;
-	}
-
-	return NULL;
-}
-
 static void
 nv50_msto_cleanup(struct nv50_msto *msto)
 {
 	struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
 	struct nv50_mstc *mstc = msto->mstc;
 	struct nv50_mstm *mstm = mstc->mstm;
+	struct drm_dp_payload payload;
+	struct drm_dp_vcpi vcpi;
+	int ret;
 
 	NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
-	if (mstc->port && mstc->port->vcpi.vcpi > 0 &&
!nv50_msto_payload(msto))
-		drm_dp_mst_deallocate_vcpi(&mstm->mgr, mstc->port);
+	if (mstc->port) {
+		ret = drm_dp_get_payload_info(&mstm->mgr, mstc->port,
+					      &vcpi, &payload);
+		if (!ret)
+			drm_dp_mst_deallocate_vcpi(&mstm->mgr, mstc->port);
+	}
+
 	if (msto->disabled) {
 		msto->mstc = NULL;
 		msto->head = NULL;
@@ -713,6 +696,9 @@ nv50_msto_prepare(struct nv50_msto *msto)
 	struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
 	struct nv50_mstc *mstc = msto->mstc;
 	struct nv50_mstm *mstm = mstc->mstm;
+	struct drm_dp_payload payload;
+	struct drm_dp_vcpi vcpi;
+	int ret;
 	struct {
 		struct nv50_disp_mthd_v1 base;
 		struct nv50_disp_sor_dp_mst_vcpi_v0 vcpi;
@@ -725,13 +711,14 @@ nv50_msto_prepare(struct nv50_msto *msto)
 	};
 
 	NV_ATOMIC(drm, "%s: msto prepare\n", msto->encoder.name);
-	if (mstc->port && mstc->port->vcpi.vcpi > 0) {
-		struct drm_dp_payload *payload = nv50_msto_payload(msto);
-		if (payload) {
-			args.vcpi.start_slot = payload->start_slot;
-			args.vcpi.num_slots = payload->num_slots;
-			args.vcpi.pbn = mstc->port->vcpi.pbn;
-			args.vcpi.aligned_pbn = mstc->port->vcpi.aligned_pbn;
+	if (mstc->port) {
+		ret = drm_dp_get_payload_info(&mstm->mgr, mstc->port,
+					      &vcpi, &payload);
+		if (!ret) {
+			args.vcpi.start_slot = payload.start_slot;
+			args.vcpi.num_slots = payload.num_slots;
+			args.vcpi.pbn = vcpi.pbn;
+			args.vcpi.aligned_pbn = vcpi.aligned_pbn;
 		}
 	}
 
-- 
2.19.1

mstc->port isn't validated here so it could be null or worse when we
access it. And drivers aren't ever supposed to be looking at it's
contents anyway. Plus, we can already get the MST manager from
&mstc->mstm->mgr.

Signed-off-by: Lyude Paul <lyude at redhat.com>
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index e6f72ca0b1fa..66c40b56a0cb 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -893,7 +893,8 @@ nv50_mstc_get_modes(struct drm_connector *connector)
 	struct nv50_mstc *mstc = nv50_mstc(connector);
 	int ret = 0;
 
-	mstc->edid = drm_dp_mst_get_edid(&mstc->connector,
mstc->port->mgr, mstc->port);
+	mstc->edid = drm_dp_mst_get_edid(&mstc->connector,
+					 &mstc->mstm->mgr, mstc->port);
 	drm_connector_update_edid_property(&mstc->connector, mstc->edid);
 	if (mstc->edid)
 		ret = drm_add_edid_modes(&mstc->connector, mstc->edid);
-- 
2.19.1

Same as the previous commit, but for nv50_mstc_detect() this time.

Signed-off-by: Lyude Paul <lyude at redhat.com>
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 66c40b56a0cb..a08dd827e892 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -930,7 +930,7 @@ nv50_mstc_detect(struct drm_connector *connector, bool
force)
 	if (ret < 0 && ret != -EACCES)
 		return connector_status_disconnected;
 
-	conn_status = drm_dp_mst_detect_port(connector, mstc->port->mgr,
+	conn_status = drm_dp_mst_detect_port(connector, &mstc->mstm->mgr,
 					     mstc->port);
 
 	pm_runtime_mark_last_busy(connector->dev->dev);
-- 
2.19.1

It hasn't been OK to access any of the contents of struct
drm_dp_mst_port without validating the port first for quite a long time
now, since a drm_dp_mst_port structure can be freed at any given moment
in time outside of the driver's contorl. Any kind of information a
driver needs from drm_dp_mst_port should be exposed through a helper
function instead that handles validating the port pointer, along with
anything else that's needed.

Since we've removed the last dangerous remanents of ->port accesses in
the DRM tree, let's finish this off and move the struct drm_dp_mst_port
definition out of drm_dp_mst_helper.h, into drm_dp_mst_topology.c, and
then replace it's header definition with an incomplete struct type. This
way drivers can still use the struct type, and no one else will make the
mistake of trying to access the contents of port.

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

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 4336d17ce904..5fa898a8a64d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -41,6 +41,65 @@
  * protocol. The helpers contain a topology manager and bandwidth manager.
  * The helpers encapsulate the sending and received of sideband msgs.
  */
+
+/**
+ * struct drm_dp_mst_port - MST port
+ * @kref: reference count for this 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.
+ * @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.
+ * @parent: branch device parent of this port
+ * @vcpi: Virtual Channel Payload info for this port.
+ * @connector: DRM connector this port is connected to.
+ * @mgr: topology manager this port lives under.
+ *
+ * This structure represents an MST port endpoint on a device somewhere
+ * in the MST topology.
+ */
+struct drm_dp_mst_port {
+	struct kref kref;
+
+	u8 port_num;
+	bool input;
+	bool mcs;
+	bool ddps;
+	u8 pdt;
+	bool ldps;
+	u8 dpcd_rev;
+	u8 num_sdp_streams;
+	u8 num_sdp_stream_sinks;
+	uint16_t available_pbn;
+	struct list_head next;
+	struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */
+	struct drm_dp_aux aux; /* i2c bus for this port? */
+	struct drm_dp_mst_branch *parent;
+
+	struct drm_dp_vcpi vcpi;
+	struct drm_connector *connector;
+	struct drm_dp_mst_topology_mgr *mgr;
+
+	/**
+	 * @cached_edid: for DP logical ports - make tiling work by ensuring
+	 * that the EDID for all connectors is read immediately.
+	 */
+	struct edid *cached_edid;
+	/**
+	 * @has_audio: Tracks whether the sink connector to this port is
+	 * audio-capable.
+	 */
+	bool has_audio;
+};
+
 static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
 				  char *buf);
 static int test_calc_pbn_mode(void);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 9cc93ea60e7e..3076a45aef4d 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -26,7 +26,7 @@
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_atomic.h>
 
-struct drm_dp_mst_branch;
+struct drm_dp_mst_port;
 
 /**
  * struct drm_dp_vcpi - Virtual Channel Payload Identifier
@@ -42,64 +42,6 @@ struct drm_dp_vcpi {
 	int num_slots;
 };
 
-/**
- * struct drm_dp_mst_port - MST port
- * @kref: reference count for this 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.
- * @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.
- * @parent: branch device parent of this port
- * @vcpi: Virtual Channel Payload info for this port.
- * @connector: DRM connector this port is connected to.
- * @mgr: topology manager this port lives under.
- *
- * This structure represents an MST port endpoint on a device somewhere
- * in the MST topology.
- */
-struct drm_dp_mst_port {
-	struct kref kref;
-
-	u8 port_num;
-	bool input;
-	bool mcs;
-	bool ddps;
-	u8 pdt;
-	bool ldps;
-	u8 dpcd_rev;
-	u8 num_sdp_streams;
-	u8 num_sdp_stream_sinks;
-	uint16_t available_pbn;
-	struct list_head next;
-	struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */
-	struct drm_dp_aux aux; /* i2c bus for this port? */
-	struct drm_dp_mst_branch *parent;
-
-	struct drm_dp_vcpi vcpi;
-	struct drm_connector *connector;
-	struct drm_dp_mst_topology_mgr *mgr;
-
-	/**
-	 * @cached_edid: for DP logical ports - make tiling work by ensuring
-	 * that the EDID for all connectors is read immediately.
-	 */
-	struct edid *cached_edid;
-	/**
-	 * @has_audio: Tracks whether the sink connector to this port is
-	 * audio-capable.
-	 */
-	bool has_audio;
-};
-
 /**
  * struct drm_dp_mst_branch - MST branch device.
  * @kref: reference count for this port.
-- 
2.19.1

Originally we started storing pointers to the drm_dp_mst_port struct for
each intel_connector as void* to stop people from trying to dereference
them. Now that we've removed the public struct definition for
drm_dp_mst_port however, it's no longer possible to dereference the port
structure even when using the proper type. So, move back to struct
drm_dp_mst_port from void* for clarity.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 4de247ddf05f..3408efe67694 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -39,7 +39,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder
*encoder,
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_connector *connector = conn_state->connector;
-	void *port = to_intel_connector(connector)->port;
+	struct drm_dp_mst_port *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
 	int bpp;
 	int lane_count, slots = 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f575ba2a59da..1bb69097d6cb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -415,7 +415,7 @@ struct intel_connector {
 	   state of connector->polled in case hotplug storm detection changes it */
 	u8 polled;
 
-	void *port; /* store this opaque as its illegal to dereference it */
+	struct drm_dp_mst_port *port;
 
 	struct intel_dp *mst_port;
 
-- 
2.19.1

Patches 1 and 5 are Acked-by: Karol Herbst <kherbst at redhat.com>
Patches 2-4 are Reviewed-by: Karol Herbst <kherbst at redhat.com>
On Sat, Nov 17, 2018 at 1:21 AM Lyude Paul <lyude at redhat.com>
wrote:
>
> So we don't ever have to worry about drivers touching drm_dp_mst_port
> structs without verifying them and crashing again.
>
> Lyude Paul (6):
>   drm/dp_mst: Add drm_dp_get_payload_info()
>   drm/nouveau: Use drm_dp_get_payload_info() for getting payload/vcpi
>   drm/nouveau: Stop reading port->mgr in nv50_mstc_get_modes()
>   drm/nouveau: Stop reading port->mgr in nv50_mstc_detect()
>   drm/dp_mst: Hide drm_dp_mst_port contents from drivers
>   drm/i915: Start using struct drm_dp_mst_port again
>
>  drivers/gpu/drm/drm_dp_mst_topology.c   | 115 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c     |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h        |   2 +-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c |  60 +++++--------
>  include/drm/drm_dp_mst_helper.h         |  65 ++------------
>  5 files changed, 146 insertions(+), 98 deletions(-)
>
> --
> 2.19.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On Fri, Nov 16, 2018 at 07:21:15PM -0500, Lyude Paul
wrote:
> Some hardware (nvidia hardware in particular) needs to be notified of
> the exact VCPI and payload settings that the topology manager decided on
> for each mstb port. Since there isn't currently any way to get this
> information without going through port (which drivers are very much not
> supposed to do by themselves, ever), let's add one.
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 56 +++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  5 ++-
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 529414556962..4336d17ce904 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1982,6 +1982,62 @@ int drm_dp_update_payload_part2(struct
drm_dp_mst_topology_mgr *mgr)
>  }
>  EXPORT_SYMBOL(drm_dp_update_payload_part2);
>  
> +/**
> + * drm_dp_get_payload_info() - Retrieve payload/vcpi information for the
given
> + *                             @port
> + * @mgr: manager to use
> + * @port: the port to get the relevant payload information for
> + * @vcpi_out: where to copy the port's VCPI information to
> + * @payload_out: where to copy the port's payload information to
> + *
> + * Searches the current payloads for @mgr and finds the relevant payload
and
> + * VCPI information that was programmed by the topology mgr, then copies
it
> + * into @vcpi_out and @payload_out. Drivers which need to know this
> + * information must use this helper as opposed to checking @port
themselves,
> + * as this helper will ensure the port reference is still valid and grab
the
> + * appropriate locks in @mgr.
> + *
> + * Returns:
> + * 0 on success, negative error code if the port is no longer valid or a
> + * programmed payload could not be found for @port.
> + */
> +int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
> +			    struct drm_dp_mst_port *port,
> +			    struct drm_dp_vcpi *vcpi_out,
> +			    struct drm_dp_payload *payload_out)
> +{
> +	struct drm_dp_payload *payload = NULL;
> +	int i;
> +	int ret = 0;
> +
> +	port = drm_dp_get_validated_port_ref(mgr, port);
> +	if (!port)
> +		return -EINVAL;
This is the part that I mean in our other/irc discussions. The
dp_get_validated_port here could fail when it's going to surprise the
driver. With the dp_port_malloc_get stuff we could instead just grab a
port_malloc_kref when storing the port in mgr->payload, which would
guarantee that the port based lookup below still works.
> +
> +	mutex_lock(&mgr->payload_lock);
> +	/* Figure out which of the payloads belongs to this port */
> +	for (i = 0; i < mgr->max_payloads; i++) {
> +		if (mgr->payloads[i].vcpi == port->vcpi.vcpi) {
Or maybe even rework the lookup here to use the port pointer (as an
abstract key) instead of port->vcpi.vcpi. With port_malloc_kref we could
guarantee that it keeps working, even after the port has been destroyed.

And (without checking) I think that's needed anyway to clean up the
payload update hacks in the connector destroy work ...
-Daniel
> +			payload = &mgr->payloads[i];
> +			break;
> +		}
> +	}
> +
> +	if (!payload) {
> +		DRM_DEBUG_KMS("Failed to find payload for port %p\n", port);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	*payload_out = *payload;
> +	*vcpi_out = port->vcpi;
> +out:
> +	mutex_unlock(&mgr->payload_lock);
> +	drm_dp_put_port(port);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_get_payload_info);
> +
>  #if 0 /* unused as of yet */
>  static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
>  				 struct drm_dp_mst_port *port,
> diff --git a/include/drm/drm_dp_mst_helper.h
b/include/drm/drm_dp_mst_helper.h
> index 59f005b419cf..9cc93ea60e7e 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -592,7 +592,10 @@ bool drm_dp_mst_allocate_vcpi(struct
drm_dp_mst_topology_mgr *mgr,
>  			      struct drm_dp_mst_port *port, int pbn, int slots);
>  
>  int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct
drm_dp_mst_port *port);
> -
> +int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
> +			    struct drm_dp_mst_port *port,
> +			    struct drm_dp_vcpi *vcpi_out,
> +			    struct drm_dp_payload *payload_out);
>  
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port);
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

For the series:

Acked-by: Ben Skeggs <bskeggs at redhat.com>
On Sat, 17 Nov 2018 at 10:21, Lyude Paul <lyude at redhat.com>
wrote:
>
> So we don't ever have to worry about drivers touching drm_dp_mst_port
> structs without verifying them and crashing again.
>
> Lyude Paul (6):
>   drm/dp_mst: Add drm_dp_get_payload_info()
>   drm/nouveau: Use drm_dp_get_payload_info() for getting payload/vcpi
>   drm/nouveau: Stop reading port->mgr in nv50_mstc_get_modes()
>   drm/nouveau: Stop reading port->mgr in nv50_mstc_detect()
>   drm/dp_mst: Hide drm_dp_mst_port contents from drivers
>   drm/i915: Start using struct drm_dp_mst_port again
>
>  drivers/gpu/drm/drm_dp_mst_topology.c   | 115 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c     |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h        |   2 +-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c |  60 +++++--------
>  include/drm/drm_dp_mst_helper.h         |  65 ++------------
>  5 files changed, 146 insertions(+), 98 deletions(-)
>
> --
> 2.19.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel