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. Unfortuntely, 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 v2: * Fix commit message - 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 Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@redhat.com Cc: Jerry Zuo Jerry.Zuo@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Juston Li juston.li@intel.com
squash! drm/dp_mst: Introduce new refcounting scheme for mstbs and ports
squash! drm/dp_mst: Introduce new refcounting scheme for mstbs and ports
* s/)-1/) - 1/g - checkpatch
Signed-off-by: Lyude Paul lyude@redhat.com --- .../gpu/dp-mst/topology-figure-1.dot | 52 ++ .../gpu/dp-mst/topology-figure-2.dot | 56 ++ .../gpu/dp-mst/topology-figure-3.dot | 59 +++ Documentation/gpu/drm-kms-helpers.rst | 26 +- drivers/gpu/drm/drm_dp_mst_topology.c | 482 +++++++++++++++--- include/drm/drm_dp_mst_helper.h | 32 +- 6 files changed, 629 insertions(+), 78 deletions(-) create mode 100644 Documentation/gpu/dp-mst/topology-figure-1.dot create mode 100644 Documentation/gpu/dp-mst/topology-figure-2.dot create mode 100644 Documentation/gpu/dp-mst/topology-figure-3.dot
diff --git a/Documentation/gpu/dp-mst/topology-figure-1.dot b/Documentation/gpu/dp-mst/topology-figure-1.dot new file mode 100644 index 000000000000..157e17c7e0b0 --- /dev/null +++ b/Documentation/gpu/dp-mst/topology-figure-1.dot @@ -0,0 +1,52 @@ +digraph T { + /* Make sure our payloads are always drawn below the driver node */ + subgraph cluster_driver { + fillcolor = grey; + style = filled; + driver -> {payload1, payload2} [dir=none]; + } + + /* Driver malloc references */ + edge [style=dashed]; + driver -> port1; + driver -> port2; + driver -> port3:e; + driver -> port4; + + payload1:s -> port1:e; + payload2:s -> port3:e; + edge [style=""]; + + subgraph cluster_topology { + label="Topology Manager"; + labelloc=bottom; + + /* Topology references */ + mstb1 -> {port1, port2}; + port1 -> mstb2; + port2 -> mstb3 -> {port3, port4}; + port3 -> mstb4; + + /* Malloc references */ + edge [style=dashed;dir=back]; + mstb1 -> {port1, port2}; + port1 -> mstb2; + port2 -> mstb3 -> {port3, port4}; + port3 -> mstb4; + } + + driver [label="DRM driver";style=filled;shape=box;fillcolor=lightblue]; + + payload1 [label="Payload #1";style=filled;shape=box;fillcolor=lightblue]; + payload2 [label="Payload #2";style=filled;shape=box;fillcolor=lightblue]; + + mstb1 [label="MSTB #1";style=filled;fillcolor=palegreen;shape=oval]; + mstb2 [label="MSTB #2";style=filled;fillcolor=palegreen;shape=oval]; + mstb3 [label="MSTB #3";style=filled;fillcolor=palegreen;shape=oval]; + mstb4 [label="MSTB #4";style=filled;fillcolor=palegreen;shape=oval]; + + port1 [label="Port #1";shape=oval]; + port2 [label="Port #2";shape=oval]; + port3 [label="Port #3";shape=oval]; + port4 [label="Port #4";shape=oval]; +} diff --git a/Documentation/gpu/dp-mst/topology-figure-2.dot b/Documentation/gpu/dp-mst/topology-figure-2.dot new file mode 100644 index 000000000000..4243dd1737cb --- /dev/null +++ b/Documentation/gpu/dp-mst/topology-figure-2.dot @@ -0,0 +1,56 @@ +digraph T { + /* Make sure our payloads are always drawn below the driver node */ + subgraph cluster_driver { + fillcolor = grey; + style = filled; + driver -> {payload1, payload2} [dir=none]; + } + + /* Driver malloc references */ + edge [style=dashed]; + driver -> port1; + driver -> port2; + driver -> port3:e; + driver -> port4 [color=red]; + + payload1:s -> port1:e; + payload2:s -> port3:e; + edge [style=""]; + + subgraph cluster_topology { + label="Topology Manager"; + labelloc=bottom; + + /* Topology references */ + mstb1 -> {port1, port2}; + port1 -> mstb2; + edge [color=red]; + port2 -> mstb3 -> {port3, port4}; + port3 -> mstb4; + edge [color=""]; + + /* Malloc references */ + edge [style=dashed;dir=back]; + mstb1 -> {port1, port2}; + port1 -> mstb2; + port2 -> mstb3 -> port3; + edge [color=red]; + mstb3 -> port4; + port3 -> mstb4; + } + + mstb1 [label="MSTB #1";style=filled;fillcolor=palegreen]; + mstb2 [label="MSTB #2";style=filled;fillcolor=palegreen]; + mstb3 [label="MSTB #3";style=filled;fillcolor=palegreen]; + mstb4 [label="MSTB #4";style=filled;fillcolor=grey]; + + port1 [label="Port #1"]; + port2 [label="Port #2"]; + port3 [label="Port #3"]; + port4 [label="Port #4";style=filled;fillcolor=grey]; + + driver [label="DRM driver";style=filled;shape=box;fillcolor=lightblue]; + + payload1 [label="Payload #1";style=filled;shape=box;fillcolor=lightblue]; + payload2 [label="Payload #2";style=filled;shape=box;fillcolor=lightblue]; +} diff --git a/Documentation/gpu/dp-mst/topology-figure-3.dot b/Documentation/gpu/dp-mst/topology-figure-3.dot new file mode 100644 index 000000000000..6cd78d06778b --- /dev/null +++ b/Documentation/gpu/dp-mst/topology-figure-3.dot @@ -0,0 +1,59 @@ +digraph T { + /* Make sure our payloads are always drawn below the driver node */ + subgraph cluster_driver { + fillcolor = grey; + style = filled; + edge [dir=none]; + driver -> payload1; + driver -> payload2 [penwidth=3]; + edge [dir=""]; + } + + /* Driver malloc references */ + edge [style=dashed]; + driver -> port1; + driver -> port2; + driver -> port3:e; + driver -> port4 [color=grey]; + payload1:s -> port1:e; + payload2:s -> port3:e [penwidth=3]; + edge [style=""]; + + subgraph cluster_topology { + label="Topology Manager"; + labelloc=bottom; + + /* Topology references */ + mstb1 -> {port1, port2}; + port1 -> mstb2; + edge [color=grey]; + port2 -> mstb3 -> {port3, port4}; + port3 -> mstb4; + edge [color=""]; + + /* Malloc references */ + edge [style=dashed;dir=back]; + mstb1 -> {port1, port2}; + port1 -> mstb2; + port2 -> mstb3 [penwidth=3]; + mstb3 -> port3 [penwidth=3]; + edge [color=grey]; + mstb3 -> port4; + port3 -> mstb4; + } + + mstb1 [label="MSTB #1";style=filled;fillcolor=palegreen]; + mstb2 [label="MSTB #2";style=filled;fillcolor=palegreen]; + mstb3 [label="MSTB #3";style=filled;fillcolor=palegreen;penwidth=3]; + mstb4 [label="MSTB #4";style=filled;fillcolor=grey]; + + port1 [label="Port #1"]; + port2 [label="Port #2";penwidth=5]; + port3 [label="Port #3";penwidth=3]; + port4 [label="Port #4";style=filled;fillcolor=grey]; + + driver [label="DRM driver";style=filled;shape=box;fillcolor=lightblue]; + + payload1 [label="Payload #1";style=filled;shape=box;fillcolor=lightblue]; + payload2 [label="Payload #2";style=filled;shape=box;fillcolor=lightblue;penwidth=3]; +} diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index b422eb8edf16..7a3fc569bc68 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -208,18 +208,40 @@ Display Port Dual Mode Adaptor Helper Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_dp_dual_mode_helper.c :export:
-Display Port MST Helper Functions Reference -=========================================== +Display Port MST Helpers +======================== + +Overview +--------
.. kernel-doc:: drivers/gpu/drm/drm_dp_mst_topology.c :doc: dp mst helper
+.. kernel-doc:: drivers/gpu/drm/drm_dp_mst_topology.c + :doc: Branch device and port refcounting + +Functions Reference +------------------- + .. kernel-doc:: include/drm/drm_dp_mst_helper.h :internal:
.. kernel-doc:: drivers/gpu/drm/drm_dp_mst_topology.c :export:
+Topology Lifetime Internals +--------------------------- + +These functions aren't exported to drivers, but are documented here to help make +the MST topology helpers easier to understand + +.. kernel-doc:: drivers/gpu/drm/drm_dp_mst_topology.c + :functions: drm_dp_mst_topology_try_get_mstb drm_dp_mst_topology_get_mstb + drm_dp_mst_topology_put_mstb + drm_dp_mst_topology_try_get_port drm_dp_mst_topology_get_port + drm_dp_mst_topology_put_port + drm_dp_mst_get_mstb_malloc drm_dp_mst_put_mstb_malloc + MIPI DSI Helper Functions Reference ===================================
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 6f9b211069a7..c0dc20fbd55a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -850,46 +850,211 @@ static struct drm_dp_mst_branch *drm_dp_add_mst_branch_device(u8 lct, u8 *rad) if (lct > 1) memcpy(mstb->rad, rad, lct / 2); INIT_LIST_HEAD(&mstb->ports); - kref_init(&mstb->kref); + kref_init(&mstb->topology_kref); + kref_init(&mstb->malloc_kref); return mstb; }
-static void drm_dp_free_mst_port(struct kref *kref); - static void drm_dp_free_mst_branch_device(struct kref *kref) { - struct drm_dp_mst_branch *mstb = container_of(kref, struct drm_dp_mst_branch, kref); - if (mstb->port_parent) { - if (list_empty(&mstb->port_parent->next)) - kref_put(&mstb->port_parent->kref, drm_dp_free_mst_port); - } + struct drm_dp_mst_branch *mstb = + container_of(kref, struct drm_dp_mst_branch, malloc_kref); + + if (mstb->port_parent) + drm_dp_mst_put_port_malloc(mstb->port_parent); + kfree(mstb); }
+/** + * DOC: Branch device and port refcounting + * + * Topology refcount overview + * ~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * The refcounting schemes for &struct drm_dp_mst_branch and &struct + * drm_dp_mst_port are somewhat unusual. Both ports and branch devices have + * two different kinds of refcounts: topology refcounts, and malloc refcounts. + * + * Topology refcounts are not exposed to drivers, and are handled internally + * by the DP MST helpers. The helpers use them in order to prevent the + * in-memory topology state from being changed in the middle of critical + * operations like changing the internal state of payload allocations. This + * means each branch and port will be considered to be connected to the rest + * of the topology until it's topology refcount reaches zero. Additionally, + * for ports this means that their associated &struct drm_connector will stay + * registered with userspace until the port's refcount reaches 0. + * + * Malloc refcount overview + * ~~~~~~~~~~~~~~~~~~~~~~~~ + * + * Malloc references are used to keep a &struct drm_dp_mst_port or &struct + * drm_dp_mst_branch allocated even after all of its topology references have + * been dropped, so that the driver or MST helpers can safely access each + * branch's last known state before it was disconnected from the topology. + * When the malloc refcount of a port or branch reaches 0, the memory + * allocation containing the &struct drm_dp_mst_branch or &struct + * drm_dp_mst_port respectively will be freed. + * + * For &struct drm_dp_mst_branch, malloc refcounts are not currently exposed + * to drivers. As of writing this documentation, there are no drivers that + * have a usecase for accessing &struct drm_dp_mst_branch outside of the MST + * helpers. Exposing this API to drivers in a race-free manner would take more + * tweaking of the refcounting scheme, however patches are welcome provided + * there is a legitimate driver usecase for this. + * + * Refcount relationships in a topology + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * Let's take a look at why the relationship between topology and malloc + * refcounts is designed the way it is. + * + * .. kernel-figure:: dp-mst/topology-figure-1.dot + * + * An example of topology and malloc refs in a DP MST topology with two + * active payloads. Topology refcount increments are indicated by solid + * lines, and malloc refcount increments are indicated by dashed lines. + * Each starts from the branch which incremented the refcount, and ends at + * the branch to which the refcount belongs to. + * + * As you can see in figure 1, every branch increments the topology refcount + * of it's children, and increments the malloc refcount of it's parent. + * Additionally, every payload increments the malloc refcount of it's assigned + * port by 1. + * + * So, what would happen if MSTB #3 from the above figure was unplugged from + * the system, but the driver hadn't yet removed payload #2 from port #3? The + * topology would start to look like figure 2. + * + * .. kernel-figure:: dp-mst/topology-figure-2.dot + * + * Ports and branch devices which have been released from memory are + * colored grey, and references which have been removed are colored red. + * + * Whenever a port or branch device's topology refcount reaches zero, it will + * decrement the topology refcounts of all its children, the malloc refcount + * of its parent, and finally its own malloc refcount. For MSTB #4 and port + * #4, this means they both have been disconnected from the topology and freed + * from memory. But, because payload #2 is still holding a reference to port + * #3, port #3 is removed from the topology but it's &struct drm_dp_mst_port + * is still accessible from memory. This also means port #3 has not yet + * decremented the malloc refcount of MSTB #3, so it's &struct + * drm_dp_mst_branch will also stay allocated in memory until port #3's + * malloc refcount reaches 0. + * + * This relationship is necessary because in order to release payload #2, we + * need to be able to figure out the last relative of port #3 that's still + * connected to the topology. In this case, we would travel up the topology as + * shown in figure 3. + * + * .. kernel-figure:: dp-mst/topology-figure-3.dot + * + * And finally, remove payload #2 by communicating with port #2 through + * sideband transactions. + */ + +/** + * drm_dp_mst_get_mstb_malloc() - Increment the malloc refcount of a branch + * device + * @mstb: The &struct drm_dp_mst_branch to increment the malloc refcount of + * + * Increments &drm_dp_mst_branch.malloc_kref. When + * &drm_dp_mst_branch.malloc_kref reaches 0, the memory allocation for @mstb + * will be released and @mstb may no longer be used. + * + * See also: drm_dp_mst_put_mstb_malloc() + */ +static void +drm_dp_mst_get_mstb_malloc(struct drm_dp_mst_branch *mstb) +{ + kref_get(&mstb->malloc_kref); + DRM_DEBUG("mstb %p (%d)\n", mstb, kref_read(&mstb->malloc_kref)); +} + +/** + * drm_dp_mst_put_mstb_malloc() - Decrement the malloc refcount of a branch + * device + * @mstb: The &struct drm_dp_mst_branch to decrement the malloc refcount of + * + * Decrements &drm_dp_mst_branch.malloc_kref. When + * &drm_dp_mst_branch.malloc_kref reaches 0, the memory allocation for @mstb + * will be released and @mstb may no longer be used. + * + * See also: drm_dp_mst_get_mstb_malloc() + */ +static void +drm_dp_mst_put_mstb_malloc(struct drm_dp_mst_branch *mstb) +{ + DRM_DEBUG("mstb %p (%d)\n", mstb, kref_read(&mstb->malloc_kref) - 1); + kref_put(&mstb->malloc_kref, drm_dp_free_mst_branch_device); +} + +static void drm_dp_free_mst_port(struct kref *kref) +{ + struct drm_dp_mst_port *port = + container_of(kref, struct drm_dp_mst_port, malloc_kref); + + drm_dp_mst_put_mstb_malloc(port->parent); + kfree(port); +} + +/** + * drm_dp_mst_get_port_malloc() - Increment the malloc refcount of an MST port + * @port: The &struct drm_dp_mst_port to increment the malloc refcount of + * + * Increments &drm_dp_mst_port.malloc_kref. When &drm_dp_mst_port.malloc_kref + * reaches 0, the memory allocation for @port will be released and @port may + * no longer be used. + * + * Because @port could potentially be freed at any time by the DP MST helpers + * if &drm_dp_mst_port.malloc_kref reaches 0, including during a call to this + * function, drivers that which to make use of &struct drm_dp_mst_port should + * ensure that they grab at least one main malloc reference to their MST ports + * in &drm_dp_mst_topology_cbs.add_connector. This callback is called before + * there is any chance for &drm_dp_mst_port.malloc_kref to reach 0. + * + * See also: drm_dp_mst_put_port_malloc() + */ +void +drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port) +{ + kref_get(&port->malloc_kref); + DRM_DEBUG("port %p (%d)\n", port, kref_read(&port->malloc_kref)); +} +EXPORT_SYMBOL(drm_dp_mst_get_port_malloc); + +/** + * drm_dp_mst_put_port_malloc() - Decrement the malloc refcount of an MST port + * @port: The &struct drm_dp_mst_port to decrement the malloc refcount of + * + * Decrements &drm_dp_mst_port.malloc_kref. When &drm_dp_mst_port.malloc_kref + * reaches 0, the memory allocation for @port will be released and @port may + * no longer be used. + * + * See also: drm_dp_mst_get_port_malloc() + */ +void +drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port) +{ + DRM_DEBUG("port %p (%d)\n", port, kref_read(&port->malloc_kref) - 1); + kref_put(&port->malloc_kref, drm_dp_free_mst_port); +} +EXPORT_SYMBOL(drm_dp_mst_put_port_malloc); + 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, 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;
- /* - * init kref again to be used by ports to remove mst branch when it is - * not needed anymore - */ - kref_init(kref); - - if (mstb->port_parent && list_empty(&mstb->port_parent->next)) - kref_get(&mstb->port_parent->kref); - - /* - * destroy all ports - don't need lock - * as there are no more references to the mst branch - * device at this point. - */ + 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); @@ -908,14 +1073,83 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref) if (wake_tx) wake_up_all(&mstb->mgr->tx_waitq);
- kref_put(kref, drm_dp_free_mst_branch_device); + drm_dp_mst_put_mstb_malloc(mstb); +} + +/** + * drm_dp_mst_topology_try_get_mstb() - Increment the topology refcount of a + * branch device unless its zero + * @mstb: &struct drm_dp_mst_branch to increment the topology refcount of + * + * Attempts to grab a topology reference to @mstb, if it hasn't yet been + * removed from the topology (e.g. &drm_dp_mst_branch.topology_kref has + * reached 0). Holding a topology reference implies that a malloc reference + * will be held to @mstb as long as the user holds the topology reference. + * + * Care should be taken to ensure that the user has at least one malloc + * reference to @mstb. If you already have a topology reference to @mstb, you + * should use drm_dp_mst_topology_get_mstb() instead. + * + * See also: + * drm_dp_mst_topology_get_mstb() + * drm_dp_mst_topology_put_mstb() + * + * Returns: + * * 1: A topology reference was grabbed successfully + * * 0: @port is no longer in the topology, no reference was grabbed + */ +static int __must_check +drm_dp_mst_topology_try_get_mstb(struct drm_dp_mst_branch *mstb) +{ + int ret = kref_get_unless_zero(&mstb->topology_kref); + + if (ret) + DRM_DEBUG("mstb %p (%d)\n", mstb, + kref_read(&mstb->topology_kref)); + + return ret; }
-static void drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb) +/** + * drm_dp_mst_topology_get_mstb() - Increment the topology refcount of a + * branch device + * @mstb: The &struct drm_dp_mst_branch to increment the topology refcount of + * + * Increments &drm_dp_mst_branch.topology_refcount without checking whether or + * not it's already reached 0. This is only valid to use in scenarios where + * you are already guaranteed to have at least one active topology reference + * to @mstb. Otherwise, drm_dp_mst_topology_try_get_mstb() must be used. + * + * See also: + * drm_dp_mst_topology_try_get_mstb() + * drm_dp_mst_topology_put_mstb() + */ +static void drm_dp_mst_topology_get_mstb(struct drm_dp_mst_branch *mstb) { - kref_put(&mstb->kref, drm_dp_destroy_mst_branch_device); + WARN_ON(kref_read(&mstb->topology_kref) == 0); + kref_get(&mstb->topology_kref); + DRM_DEBUG("mstb %p (%d)\n", mstb, kref_read(&mstb->topology_kref)); }
+/** + * drm_dp_mst_topology_put_mstb() - release a topology reference to a branch + * device + * @mstb: The &struct drm_dp_mst_branch to release the topology reference from + * + * Releases a topology reference from @mstb by decrementing + * &drm_dp_mst_branch.topology_kref. + * + * See also: + * drm_dp_mst_topology_try_get_mstb() + * drm_dp_mst_topology_get_mstb() + */ +static void +drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb) +{ + DRM_DEBUG("mstb %p (%d)\n", + mstb, kref_read(&mstb->topology_kref) - 1); + 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) { @@ -937,7 +1171,8 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
static void drm_dp_destroy_port(struct kref *kref) { - struct drm_dp_mst_port *port = container_of(kref, struct drm_dp_mst_port, kref); + struct drm_dp_mst_port *port = + container_of(kref, struct drm_dp_mst_port, topology_kref); struct drm_dp_mst_topology_mgr *mgr = port->mgr;
if (!port->input) { @@ -956,7 +1191,6 @@ static void drm_dp_destroy_port(struct kref *kref) * from an EDID retrieval */
mutex_lock(&mgr->destroy_connector_lock); - kref_get(&port->parent->kref); list_add(&port->next, &mgr->destroy_connector_list); mutex_unlock(&mgr->destroy_connector_lock); schedule_work(&mgr->destroy_connector_work); @@ -967,12 +1201,79 @@ static void drm_dp_destroy_port(struct kref *kref) drm_dp_port_teardown_pdt(port, port->pdt); port->pdt = DP_PEER_DEVICE_NONE; } - kfree(port); + drm_dp_mst_put_port_malloc(port); }
+/** + * drm_dp_mst_topology_try_get_port() - Increment the topology refcount of a + * port unless its zero + * @port: &struct drm_dp_mst_port to increment the topology refcount of + * + * Attempts to grab a topology reference to @port, if it hasn't yet been + * removed from the topology (e.g. &drm_dp_mst_port.topology_kref has reached + * 0). Holding a topology reference implies that a malloc reference will be + * held to @port as long as the user holds the topology reference. + * + * Care should be taken to ensure that the user has at least one malloc + * reference to @port. If you already have a topology reference to @port, you + * should use drm_dp_mst_topology_get_port() instead. + * + * See also: + * drm_dp_mst_topology_get_port() + * drm_dp_mst_topology_put_port() + * + * Returns: + * * 1: A topology reference was grabbed successfully + * * 0: @port is no longer in the topology, no reference was grabbed + */ +static int __must_check +drm_dp_mst_topology_try_get_port(struct drm_dp_mst_port *port) +{ + int ret = kref_get_unless_zero(&port->topology_kref); + + if (ret) + DRM_DEBUG("port %p (%d)\n", port, + kref_read(&port->topology_kref)); + + return ret; +} + +/** + * drm_dp_mst_topology_get_port() - Increment the topology refcount of a port + * @port: The &struct drm_dp_mst_port to increment the topology refcount of + * + * Increments &drm_dp_mst_port.topology_refcount without checking whether or + * not it's already reached 0. This is only valid to use in scenarios where + * you are already guaranteed to have at least one active topology reference + * to @port. Otherwise, drm_dp_mst_topology_try_get_port() must be used. + * + * See also: + * drm_dp_mst_topology_try_get_port() + * drm_dp_mst_topology_put_port() + */ +static void drm_dp_mst_topology_get_port(struct drm_dp_mst_port *port) +{ + WARN_ON(kref_read(&port->topology_kref) == 0); + kref_get(&port->topology_kref); + DRM_DEBUG("port %p (%d)\n", port, kref_read(&port->topology_kref)); +} + +/** + * drm_dp_mst_topology_put_port() - release a topology reference to a port + * @port: The &struct drm_dp_mst_port to release the topology reference from + * + * Releases a topology reference from @port by decrementing + * &drm_dp_mst_port.topology_kref. + * + * See also: + * drm_dp_mst_topology_try_get_port() + * drm_dp_mst_topology_get_port() + */ static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port) { - kref_put(&port->kref, drm_dp_destroy_port); + DRM_DEBUG("port %p (%d)\n", + port, kref_read(&port->topology_kref) - 1); + kref_put(&port->topology_kref, drm_dp_destroy_port); }
static struct drm_dp_mst_branch * @@ -981,10 +1282,10 @@ drm_dp_mst_topology_get_mstb_validated_locked(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_port *port; struct drm_dp_mst_branch *rmstb; - if (to_find == mstb) { - kref_get(&mstb->kref); + + if (to_find == mstb) return mstb; - } + list_for_each_entry(port, &mstb->ports, next) { if (port->mstb) { rmstb = drm_dp_mst_topology_get_mstb_validated_locked( @@ -1001,25 +1302,32 @@ drm_dp_mst_topology_get_mstb_validated(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) { struct drm_dp_mst_branch *rmstb = NULL; + mutex_lock(&mgr->lock); - if (mgr->mst_primary) + if (mgr->mst_primary) { rmstb = drm_dp_mst_topology_get_mstb_validated_locked( mgr->mst_primary, mstb); + + if (rmstb && !drm_dp_mst_topology_try_get_mstb(rmstb)) + rmstb = NULL; + } mutex_unlock(&mgr->lock); return rmstb; }
-static struct drm_dp_mst_port *drm_dp_mst_get_port_ref_locked(struct drm_dp_mst_branch *mstb, struct drm_dp_mst_port *to_find) +static struct drm_dp_mst_port * +drm_dp_mst_topology_get_port_validated_locked(struct drm_dp_mst_branch *mstb, + struct drm_dp_mst_port *to_find) { struct drm_dp_mst_port *port, *mport;
list_for_each_entry(port, &mstb->ports, next) { - if (port == to_find) { - kref_get(&port->kref); + if (port == to_find) return port; - } + if (port->mstb) { - mport = drm_dp_mst_get_port_ref_locked(port->mstb, to_find); + mport = drm_dp_mst_topology_get_port_validated_locked( + port->mstb, to_find); if (mport) return mport; } @@ -1032,9 +1340,15 @@ drm_dp_mst_topology_get_port_validated(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { struct drm_dp_mst_port *rport = NULL; + mutex_lock(&mgr->lock); - if (mgr->mst_primary) - rport = drm_dp_mst_get_port_ref_locked(mgr->mst_primary, port); + if (mgr->mst_primary) { + rport = drm_dp_mst_topology_get_port_validated_locked( + mgr->mst_primary, port); + + if (rport && !drm_dp_mst_topology_try_get_port(rport)) + rport = NULL; + } mutex_unlock(&mgr->lock); return rport; } @@ -1042,11 +1356,12 @@ drm_dp_mst_topology_get_port_validated(struct drm_dp_mst_topology_mgr *mgr, static struct drm_dp_mst_port *drm_dp_get_port(struct drm_dp_mst_branch *mstb, u8 port_num) { struct drm_dp_mst_port *port; + int ret;
list_for_each_entry(port, &mstb->ports, next) { if (port->port_num == port_num) { - kref_get(&port->kref); - return port; + ret = drm_dp_mst_topology_try_get_port(port); + return ret ? port : NULL; } }
@@ -1095,6 +1410,11 @@ static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port) if (port->mstb) { port->mstb->mgr = port->mgr; port->mstb->port_parent = port; + /* + * Make sure this port's memory allocation stays + * around until it's child MSTB releases it + */ + drm_dp_mst_get_port_malloc(port);
send_link = true; } @@ -1155,17 +1475,26 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, bool created = false; int old_pdt = 0; int old_ddps = 0; + port = drm_dp_get_port(mstb, port_msg->port_number); if (!port) { port = kzalloc(sizeof(*port), GFP_KERNEL); if (!port) return; - kref_init(&port->kref); + kref_init(&port->topology_kref); + kref_init(&port->malloc_kref); port->parent = mstb; port->port_num = port_msg->port_number; port->mgr = mstb->mgr; port->aux.name = "DPMST"; port->aux.dev = dev->dev; + + /* + * Make sure the memory allocation for our parent branch stays + * around until our own memory allocation is released + */ + drm_dp_mst_get_mstb_malloc(mstb); + created = true; } else { old_pdt = port->pdt; @@ -1185,7 +1514,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, for this list */ if (created) { mutex_lock(&mstb->mgr->lock); - kref_get(&port->kref); + drm_dp_mst_topology_get_port(port); list_add(&port->next, &mstb->ports); mutex_unlock(&mstb->mgr->lock); } @@ -1210,8 +1539,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, if (created && !port->input) { char proppath[255];
- build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); - port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath); + 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); @@ -1221,6 +1553,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, drm_dp_mst_topology_put_port(port); goto out; } + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || port->pdt == DP_PEER_DEVICE_SST_SINK) && port->port_num >= DP_MST_LOGICAL_PORT_0) { @@ -1278,7 +1611,7 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ { struct drm_dp_mst_branch *mstb; struct drm_dp_mst_port *port; - int i; + int i, ret; /* find the port by iterating down */
mutex_lock(&mgr->lock); @@ -1303,7 +1636,9 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ } } } - kref_get(&mstb->kref); + ret = drm_dp_mst_topology_try_get_mstb(mstb); + if (!ret) + mstb = NULL; out: mutex_unlock(&mgr->lock); return mstb; @@ -1333,19 +1668,22 @@ static struct drm_dp_mst_branch *get_mst_branch_device_by_guid_helper( return NULL; }
-static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device_by_guid( - struct drm_dp_mst_topology_mgr *mgr, - uint8_t *guid) +static struct drm_dp_mst_branch * +drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr, + uint8_t *guid) { struct drm_dp_mst_branch *mstb; + int ret;
/* find the port by iterating down */ mutex_lock(&mgr->lock);
mstb = get_mst_branch_device_by_guid_helper(mgr->mst_primary, guid); - - if (mstb) - kref_get(&mstb->kref); + if (mstb) { + ret = drm_dp_mst_topology_try_get_mstb(mstb); + if (!ret) + mstb = NULL; + }
mutex_unlock(&mgr->lock); return mstb; @@ -1384,11 +1722,14 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) { struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work); struct drm_dp_mst_branch *mstb; + int ret;
mutex_lock(&mgr->lock); mstb = mgr->mst_primary; if (mstb) { - kref_get(&mstb->kref); + ret = drm_dp_mst_topology_try_get_mstb(mstb); + if (!ret) + mstb = NULL; } mutex_unlock(&mgr->lock); if (mstb) { @@ -1716,8 +2057,10 @@ static struct drm_dp_mst_branch *drm_dp_get_last_connected_port_and_mstb(struct
if (found_port) { rmstb = found_port->parent; - kref_get(&rmstb->kref); - *port_num = found_port->port_num; + if (drm_dp_mst_topology_try_get_mstb(rmstb)) + *port_num = found_port->port_num; + else + rmstb = NULL; } } mutex_unlock(&mgr->lock); @@ -1742,7 +2085,9 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, port_num = port->port_num; mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent); if (!mstb) { - mstb = drm_dp_get_last_connected_port_and_mstb(mgr, port->parent, &port_num); + mstb = drm_dp_get_last_connected_port_and_mstb(mgr, + port->parent, + &port_num);
if (!mstb) { drm_dp_mst_topology_put_port(port); @@ -2168,7 +2513,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
/* give this the main reference */ mgr->mst_primary = mstb; - kref_get(&mgr->mst_primary->kref); + drm_dp_mst_topology_get_mstb(mgr->mst_primary);
ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC); @@ -2743,11 +3088,11 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots); if (ret) { 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), ret); goto out; } DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", - pbn, port->vcpi.num_slots); + pbn, port->vcpi.num_slots);
drm_dp_mst_topology_put_port(port); return true; @@ -2791,7 +3136,8 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); * @mgr: manager for this port * @port: unverified port to deallocate vcpi for */ -void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) +void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port) { port = drm_dp_mst_topology_get_port_validated(mgr, port); if (!port) @@ -3086,13 +3432,6 @@ static void drm_dp_tx_work(struct work_struct *work) mutex_unlock(&mgr->qlock); }
-static void drm_dp_free_mst_port(struct kref *kref) -{ - struct drm_dp_mst_port *port = container_of(kref, struct drm_dp_mst_port, kref); - kref_put(&port->parent->kref, drm_dp_free_mst_branch_device); - kfree(port); -} - static void drm_dp_destroy_connector_work(struct work_struct *work) { struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work); @@ -3113,7 +3452,6 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) list_del(&port->next); mutex_unlock(&mgr->destroy_connector_lock);
- kref_init(&port->kref); INIT_LIST_HEAD(&port->next);
mgr->cbs->destroy_connector(mgr, port->connector); @@ -3127,7 +3465,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); }
- kref_put(&port->kref, drm_dp_free_mst_port); + drm_dp_mst_put_port_malloc(port); send_hotplug = true; } if (send_hotplug) 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