On 2019-01-04 7:14 p.m., Lyude Paul wrote: > 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@xxxxxxxxxx> > Cc: Daniel Vetter <daniel@xxxxxxxx> > Cc: David Airlie <airlied@xxxxxxxxxx> > Cc: Jerry Zuo <Jerry.Zuo@xxxxxxx> > Cc: Harry Wentland <harry.wentland@xxxxxxx> > Cc: Juston Li <juston.li@xxxxxxxxx> > > 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 > Are the auto squash things and the string replacement command here intentional? > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx> Couple of small comments, otherwise this change looks correct. Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx> > --- > .../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. > + */ > + Love the doc with graphs and detailed explanation of why there are now two refcounts. > +/** > + * 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 /s/which/wish/g > + * 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); > I usually find it better to leave stylistic changes separate from functional changes. It makes maintainer's lives easier. Harry > 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 > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx