Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3455 bytes --]

Hi,

Most of these comments affect your earlier patches, but let's work on
the API-level view.

On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:
> + * When using refcounted mode, the driver should allocate ``struct
> + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> + * transfer lifecycle management to the DRM bridge core, and implement a
> + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> + * example::
> + *
> + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> + *     {
> + *         kfree(container_of(bridge, struct my_bridge, bridge));
> + *     }
> + *
> + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> + *         .destroy = my_bridge_destroy,
> + *         ...
> + *     };
> + *
> + *     static int my_bridge_probe(...)
> + *     {
> + *         struct my_bridge *mybr;
> + *         int err;
> + *
> + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> + *         if (!mybr)
> + *             return -ENOMEM;
> + *
> + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> + *         if (err)
> + *             return err;
> + *
> + *         ...
> + *         drm_bridge_add();
> + *         ...
> + *     }
> + *
> + *     static void my_bridge_remove()
> + *     {
> + *         struct my_bridge *mybr = ...;
> + *         drm_bridge_remove(&mybr->bridge);
> + *         // ... NO kfree here!
> + *     }

I'm a bit worried there, since that API is pretty difficult to get
right, and we don't have anything to catch bad patterns.

Let's take a step back. What we're trying to solve here is:

  1) We want to avoid any dangling pointers to a bridge if the bridge
     device is removed.

  2) To do so, we need to switch to reference counted allocations and
     pointers.

  3) Most bridges structures are allocated through devm_kzalloc, and they
     one that aren't are freed at remove time anyway, so the allocated
     structure will be gone when the device is removed.

  4) To properly track users, each user that will use a drm_bridge needs
     to take a reference.

AFAIU, the destroy introduction and the on-purpose omission of kfree in
remove is to solve 3.

Introducing a function that allocates the drm_bridge container struct
(like drmm_encoder_alloc for example), take a reference, register a devm
kfree action, and return the pointer to the driver structure would solve
that too pretty nicely.

So, something like:


struct driver_priv {
       struct drm_bridge bridge;

       ...
}

static int driver_probe(...)
{
	struct driver_priv *priv;
	struct drm_bridge *bridge;

        ....

	priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);
	if (IS_ERR(priv))
	   return ERR_PTR(priv);
	bridge = &priv->bridge;

	...

	drm_bridge_add(bridge);
}

Would work just as well.

I also don't think we need explicit (at least for the common case)
drm_bridge_get and drm_bridge_put calls for bridge users.
drm_bridge_attach and drm_bridge_detach can get/put the reference
directly.

And we'll also need some flag in drm_bridge to indicate that the device
is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
because now your bridge driver sticks around for much longer than your
device so the expectation that your device managed resources (clocks,
registers, etc.) are always going to be around.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]