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(-)
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@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);
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@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
On Tue, 2018-11-27 at 22:23 +0100, Daniel Vetter wrote:
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@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.
Yeah, that makes more sense now that there's context :P, sgtm.
- 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
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@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; } }
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@redhat.com Cc: stable@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);
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.19.2, v4.18.19, v4.14.81, v4.9.137, v4.4.163, v3.18.125,
v4.19.2: Build OK! v4.18.19: Failed to apply! Possible dependencies: Unable to calculate
v4.14.81: Failed to apply! Possible dependencies: Unable to calculate
v4.9.137: Failed to apply! Possible dependencies: f479c0ba4a17 ("drm/nouveau/kms/nv50: initial support for DP 1.2 multi-stream")
v4.4.163: Failed to apply! Possible dependencies: f479c0ba4a17 ("drm/nouveau/kms/nv50: initial support for DP 1.2 multi-stream")
v3.18.125: Failed to apply! Possible dependencies: f479c0ba4a17 ("drm/nouveau/kms/nv50: initial support for DP 1.2 multi-stream")
How should we proceed with this patch?
-- Thanks, Sasha
Same as the previous commit, but for nv50_mstc_detect() this time.
Signed-off-by: Lyude Paul lyude@redhat.com Cc: stable@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);
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.19.2, v4.18.19, v4.14.81, v4.9.137, v4.4.163, v3.18.125,
v4.19.2: Build OK! v4.18.19: Build OK! v4.14.81: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v4.9.137: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v4.4.163: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v3.18.125: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
How should we proceed with this patch?
-- Thanks, Sasha
On Sat, 2018-11-17 at 12:24 +0000, Sasha Levin wrote:
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.19.2, v4.18.19, v4.14.81, v4.9.137, v4.4.163, v3.18.125,
v4.19.2: Build OK! v4.18.19: Build OK! v4.14.81: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v4.9.137: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v4.4.163: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
v3.18.125: Failed to apply! Possible dependencies: e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
How should we proceed with this patch?
e46368cf77f2 ("drm/nouveau/drm/nouveau: Grab runtime PM ref in nv50_mstc_detect()")
Should also be backported for v4.9+, and then this patch should be applied on top of that.
-- Thanks, Sasha
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@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.
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@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;
Patches 1 and 5 are Acked-by: Karol Herbst kherbst@redhat.com Patches 2-4 are Reviewed-by: Karol Herbst kherbst@redhat.com On Sat, Nov 17, 2018 at 1:21 AM Lyude Paul lyude@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
For the series:
Acked-by: Ben Skeggs bskeggs@redhat.com On Sat, 17 Nov 2018 at 10:21, Lyude Paul lyude@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Ben Skeggs -
Daniel Vetter -
Karol Herbst -
Lyude Paul -
Sasha Levin