summaryrefslogtreecommitdiff
path: root/include/drm
diff options
context:
space:
mode:
authorLyude Paul <lyude@redhat.com>2019-01-11 03:53:29 +0300
committerLyude Paul <lyude@redhat.com>2019-01-11 04:12:19 +0300
commitebcc0e6b509108b4a67daa4c55809a05ab7f4b77 (patch)
tree014d480572e9f7209710cae1e3595c19ca14b2fc /include/drm
parentd0757afd00d71dca98268d09884dc6248743d8ce (diff)
downloadlinux-ebcc0e6b509108b4a67daa4c55809a05ab7f4b77.tar.xz
drm/dp_mst: Introduce new refcounting scheme for mstbs and ports
The current way of handling refcounting in the DP MST helpers is really confusing and probably just plain wrong because it's been hacked up many times over the years without anyone actually going over the code and seeing if things could be simplified. To the best of my understanding, the current scheme works like this: drm_dp_mst_port and drm_dp_mst_branch both have a single refcount. When this refcount hits 0 for either of the two, they're removed from the topology state, but not immediately freed. Both ports and branch devices will reinitialize their kref once it's hit 0 before actually destroying themselves. The intended purpose behind this is so that we can avoid problems like not being able to free a remote payload that might still be active, due to us having removed all of the port/branch device structures in memory, as per: commit 91a25e463130 ("drm/dp/mst: deallocate payload on port destruction") Which may have worked, but then it caused use-after-free errors. Being new to MST at the time, I tried fixing it; commit 263efde31f97 ("drm/dp/mst: Get validated port ref in drm_dp_update_payload_part1()") But, that was broken: both drm_dp_mst_port and drm_dp_mst_branch structs are validated in almost every DP MST helper function. Simply put, this means we go through the topology and try to see if the given drm_dp_mst_branch or drm_dp_mst_port is still attached to something before trying to use it in order to avoid dereferencing freed memory (something that has happened a LOT in the past with this library). Because of this it doesn't actually matter whether or not we keep keep the ports and branches around in memory as that's not enough, because any function that validates the branches and ports passed to it will still reject them anyway since they're no longer in the topology structure. So, use-after-free errors were fixed but payload deallocation was completely broken. Two years later, AMD informed me about this issue and I attempted to come up with a temporary fix, pending a long-overdue cleanup of this library: commit c54c7374ff44 ("drm/dp_mst: Skip validating ports during destruction, just ref") But then that introduced use-after-free errors, so I quickly reverted it: commit 9765635b3075 ("Revert "drm/dp_mst: Skip validating ports during destruction, just ref"") And in the process, learned that there is just no simple fix for this: the design is just broken. Unfortunately, the usage of these helpers are quite broken as well. Some drivers like i915 have been smart enough to avoid accessing any kind of information from MST port structures, but others like nouveau have assumed, understandably so, that drm_dp_mst_port structures are normal and can just be accessed at any time without worrying about use-after-free errors. After a lot of discussion, me and Daniel Vetter came up with a better idea to replace all of this. To summarize, since this is documented far more indepth in the documentation this patch introduces, we make it so that drm_dp_mst_port and drm_dp_mst_branch structures have two different classes of refcounts: topology_kref, and malloc_kref. topology_kref corresponds to the lifetime of the given drm_dp_mst_port or drm_dp_mst_branch in it's given topology. Once it hits zero, any associated connectors are removed and the branch or port can no longer be validated. malloc_kref corresponds to the lifetime of the memory allocation for the actual structure, and will always be non-zero so long as the topology_kref is non-zero. This gives us a way to allow callers to hold onto port and branch device structures past their topology lifetime, and dramatically simplifies the lifetimes of both structures. This also finally fixes the port deallocation problem, properly. Additionally: since this now means that we can keep ports and branch devices allocated in memory for however long we need, we no longer need a significant amount of the port validation that we currently do. Additionally, there is one last scenario that this fixes, which couldn't have been fixed properly beforehand: - CPU1 unrefs port from topology (refcount 1->0) - CPU2 refs port in topology(refcount 0->1) Since we now can guarantee memory safety for ports and branches as-needed, we also can make our main reference counting functions fix this problem by using kref_get_unless_zero() internally so that topology refcounts can only ever reach 0 once. Changes since v4: * Change the kernel-figure summary for dp-mst/topology-figure-1.dot a bit - danvet * Remove figure numbers - danvet Changes since v3: * Remove rebase detritus - danvet * Split out purely style changes into separate patches - hwentlan Changes since v2: * Fix commit message - checkpatch * s/)-1/) - 1/g - checkpatch Changes since v1: * Remove forward declarations - danvet * Move "Branch device and port refcounting" section from documentation into kernel-doc comments - danvet * Export internal topology lifetime functions into their own section in the kernel-docs - danvet * s/@/&/g for struct references in kernel-docs - danvet * Drop the "when they are no longer being used" bits from the kernel docs - danvet * Modify diagrams to show how the DRM driver interacts with the topology and payloads - danvet * Make suggested documentation changes for drm_dp_mst_topology_get_mstb() and drm_dp_mst_topology_get_port() - danvet * Better explain the relationship between malloc refs and topology krefs in the documentation for drm_dp_mst_topology_get_port() and drm_dp_mst_topology_get_mstb() - danvet * Fix "See also" in drm_dp_mst_topology_get_mstb() - danvet * Rename drm_dp_mst_topology_get_(port|mstb)() -> drm_dp_mst_topology_try_get_(port|mstb)() and drm_dp_mst_topology_ref_(port|mstb)() -> drm_dp_mst_topology_get_(port|mstb)() - danvet * s/should/must in docs - danvet * WARN_ON(refcount == 0) in topology_get_(mstb|port) - danvet * Move kdocs for mstb/port structs inline - danvet * Split drm_dp_get_last_connected_port_and_mstb() changes into their own commit - danvet Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Reviewed-by: Daniel Vetter <daniel@ffwll.ch> Cc: David Airlie <airlied@redhat.com> Cc: Jerry Zuo <Jerry.Zuo@amd.com> Cc: Juston Li <juston.li@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-7-lyude@redhat.com
Diffstat (limited to 'include/drm')
-rw-r--r--include/drm/drm_dp_mst_helper.h32
1 files changed, 28 insertions, 4 deletions
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 371cc2816477..8eca5f29242c 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -44,7 +44,6 @@ struct drm_dp_vcpi {
/**
* 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.
@@ -67,7 +66,18 @@ struct drm_dp_vcpi {
* in the MST topology.
*/
struct drm_dp_mst_port {
- struct kref kref;
+ /**
+ * @topology_kref: refcount for this port's lifetime in the topology,
+ * only the DP MST helpers should need to touch this
+ */
+ struct kref topology_kref;
+
+ /**
+ * @malloc_kref: refcount for the memory allocation containing this
+ * structure. See drm_dp_mst_get_port_malloc() and
+ * drm_dp_mst_put_port_malloc().
+ */
+ struct kref malloc_kref;
u8 port_num;
bool input;
@@ -102,7 +112,6 @@ struct drm_dp_mst_port {
/**
* struct drm_dp_mst_branch - MST branch device.
- * @kref: reference count for this port.
* @rad: Relative Address to talk to this branch device.
* @lct: Link count total to talk to this branch device.
* @num_ports: number of ports on the branch.
@@ -121,7 +130,19 @@ struct drm_dp_mst_port {
* to downstream port of parent branches.
*/
struct drm_dp_mst_branch {
- struct kref kref;
+ /**
+ * @topology_kref: refcount for this branch device's lifetime in the
+ * topology, only the DP MST helpers should need to touch this
+ */
+ struct kref topology_kref;
+
+ /**
+ * @malloc_kref: refcount for the memory allocation containing this
+ * structure. See drm_dp_mst_get_mstb_malloc() and
+ * drm_dp_mst_put_mstb_malloc().
+ */
+ struct kref malloc_kref;
+
u8 rad[8];
u8 lct;
int num_ports;
@@ -626,4 +647,7 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port, bool power_up);
+void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
+void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
+
#endif