Message-ID: <20250414-dangerous-stoic-lemur-5e083c@houat>
Date: Mon, 14 Apr 2025 17:40:46 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Jonathan Corbet <corbet@....net>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Anusha Srivatsa <asrivats@...hat.com>, Paul Kocialkowski <paulk@...-base.io>, 
	Dmitry Baryshkov <lumag@...nel.org>, Hervé Codina <herve.codina@...tlin.com>, 
	Hui Pu <Hui.Pu@...ealthcare.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 1/2] drm/bridge: documentat bridge allocation and
 lifecycle

Hi,

On Wed, Apr 09, 2025 at 04:50:34PM +0200, Luca Ceresoli wrote:
> Document in detail the DRM bridge allocation and refcounting process based
> on the recently introduced devm_drm_bridge_alloc().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>

There's a typo in your commit title.

> ---
> 
> Changes in v7:
>  - remove mention of "legacy mode", we now support only refcounted
>    bridges
>  - rename patch title from "drm/bridge: add documentation of refcounted
>    bridges", we now support only refcounted bridges
> 
> Changes in v6:
>  - update to the new devm_drm_bridge_alloc() API
>  - rewrite and improve various sentences for clarity
>  - fix typos (Randy Dunlap)
> 
> This patch was added in v5.
> ---
>  Documentation/gpu/drm-kms-helpers.rst |  6 +++
>  drivers/gpu/drm/drm_bridge.c          | 73 +++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 5139705089f200b189876a5a61bf2a935cec433a..393cd0e4cb5af3fe98674e7a96c853ffb2556c97 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -151,6 +151,12 @@ Overview
>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>     :doc: overview
>  
> +Bridge allocation and lifecycle
> +-------------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> +   :doc: bridge lifecycle
> +
>  Display Driver Integration
>  --------------------------
>  
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b4c89ec01998b849018ce031c7cd84614e65e710..b7e1ad761dad52bdb2ec09d425e69ee23a18fd36 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -61,6 +61,79 @@
>   * encoder chain.
>   */
>  
> +/**
> + * DOC: bridge lifecycle
> + *
> + * In some use cases such as hot-plugging a DRM bridge device can
> + * physically disappear and reappear at runtime. To handle such cases
> + * without destroying and recreating the entire DRM pipeline, DRM bridge
> + * lifetime is managed using reference counting:

That case doesn't exist yet, so documenting it seems a source of confusion.

> + * - each &struct drm_bridge is reference counted since its allocation
> + * - any code taking a pointer to a bridge has APIs to get a reference and
> + *   put it when done, to ensure the memory allocated for the bridge won't
> + *   be deallocated while there is still a reference to it
> + * - the driver implementing the bridge also holds a reference, but the
> + *   allocated struct can survive the driver in case other references still
> + *   exist
> + * - deallocation is done when the last put happens, dropping the refcount
> + *   to zero
> + *
> + * Usage of refcounted bridges happens in two sides: the bridge *provider*
> + * and the bridge *consumers*. The bridge provider is the driver
> + * implementing the bridge. The bridge consumers are all parts of the
> + * kernel taking a &struct drm_bridge pointer, including other bridges,
> + * encoders and the DRM core.
> + *
> + * For bridge **providers**, the bridge driver declares a driver-specific
> + * struct embedding a &struct drm_bridge. E.g.::
> + *
> + *   struct my_bridge {
> + *       ...
> + *       struct drm_bridge bridge;
> + *       ...
> + *   };
> + *
> + * The driver must allocate and initialize ``struct my_bridge`` using
> + * devm_drm_bridge_alloc(), as in this example::
> + *
> + *     static int my_bridge_probe(...)
> + *     {
> + *         struct device *dev = ...;
> + *         struct my_bridge *mybr;
> + *
> + *         mybr = devm_drm_bridge_alloc(dev, struct my_bridge, bridge, &my_bridge_funcs);
> + *         if (IS_ERR(mybr))
> + *             return PTR_ERR(mybr);
> + *
> + *         // Get resources, initialize my_bridge members...
> + *         drm_bridge_add(&mybr->bridge);
> + *         ...
> + *     }
> + *
> + *     static void my_bridge_remove(...)
> + *     {
> + *         struct my_bridge *mybr = ...;
> + *
> + *         drm_bridge_remove(&mybr->bridge);
> + *         // Free resources
> + *         // ... NO kfree here!
> + *     }

This part is already documented by drm_bridge_add(), so it's not clear
what that section brings to the table either.

> + * Bridge **consumers** need to handle the case of a bridge being removed
> + * while they have a pointer to it. As this can happen at any time, such
> + * code can incur in use-after-free. To avoid that, consumers have to call
> + * drm_bridge_get() when taking a pointer and drm_bridge_put() after they
> + * are done using it. This will extend the allocation lifetime of the
> + * bridge struct until the last reference has been put, potentially a long
> + * time after the bridge device has been removed from the kernel.

And it's kind of the same thing here. You're saying here that every
consumer absolutely needs to call drm_bridge_get() and drm_bridge_put()
on their pointer ...

> + * Functions that return a pointer to a bridge, such as
> + * of_drm_find_bridge(), internally call drm_bridge_get() on the bridge
> + * they are about to return, so users using such functions to get a bridge
> + * pointer only have to take care of calling drm_bridge_put().
> + */

... but that every function that gives you a pointer will take care of
drm_bridge_get already and (will) document that you need to call
drm_bridge_put ?

I guess my larger question is kind of an editorial one. What do you want
people to learn here that isn't in some function documentation already?
At the moment, it looks like a doc that used to be useful but got kind
of deprecated by the documentation you created on all the functions we
merged so far, or a documentation that might be useful at some point but
not quite yet. Either way, it's confusing.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists