[Intel-gfx] [PATCH v4 02/16] drm/dp_mst: Introduce new refcounting scheme for mstbs and ports

Wentland, Harry Harry.Wentland at amd.com
Tue Jan 8 19:00:09 UTC 2019



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 at redhat.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: David Airlie <airlied at redhat.com>
> Cc: Jerry Zuo <Jerry.Zuo at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Juston Li <juston.li at 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
> 

Are the auto squash things and the string replacement command here intentional?

> Signed-off-by: Lyude Paul <lyude at redhat.com>

Couple of small comments, otherwise this change looks correct.

Reviewed-by: Harry Wentland <harry.wentland at amd.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.
> + */
> +

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
> 


More information about the Intel-gfx mailing list