[Nouveau] [PATCH v5 06/20] drm/dp_mst: Introduce new refcounting scheme for mstbs and ports
Daniel Vetter
daniel at ffwll.ch
Thu Jan 10 09:12:12 UTC 2019
On Tue, Jan 08, 2019 at 07:35:03PM -0500, 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. 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 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 at redhat.com>
> Reviewed-by: Harry Wentland <harry.wentland at amd.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: David Airlie <airlied at redhat.com>
> Cc: Jerry Zuo <Jerry.Zuo at amd.com>
> Cc: Juston Li <juston.li at intel.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 | 457 +++++++++++++++---
> include/drm/drm_dp_mst_helper.h | 32 +-
> 6 files changed, 613 insertions(+), 69 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 074e985093ca..c53cf7eb1dbc 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.
I had to take a double take on this one here. Maybe add ", i.e. the
arrows point the same way as the C pointers used to reference a
structure."
> + *
> + * As you can see in figure 1, every branch increments the topology refcount
Maybe s/figure 1/above figure/, the output doesn't have numbered figures
(and if it did, we'd need an rst reference).
> + * 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);
> }
>
> -static void drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *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)
> {
> - kref_put(&mstb->kref, drm_dp_destroy_mst_branch_device);
> + 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;
> }
>
> +/**
> + * 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)
> +{
> + 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);
> }
> @@ -1284,7 +1613,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);
> @@ -1309,7 +1638,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;
> @@ -1344,14 +1675,17 @@ 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;
> @@ -1390,11 +1724,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) {
> @@ -1722,8 +2059,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);
> @@ -2176,7 +2515,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);
> @@ -3096,13 +3435,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);
> @@ -3123,7 +3455,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);
> @@ -3137,7 +3468,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);
> +
_Really_ nice documentation, and the graphs look pretty!
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Aside: We're lacking kerneldoc for drm_dp_mst_topology_cbs. A follow-up to
document a bit what drivers are expected to do in each callback would be
neat.
-Daniel
> #endif
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Nouveau
mailing list