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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> 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.

5) Handle the disappearing next_bridge problem: probe() function gets
a pointer to the next bridge, but then for some reasons (e.g. because
of the other device being removed or because of some probe deferral)
the next_bridge driver gets unbdound and the next_bridge becomes
unusable before a call to drm_bridge_attach().

>
> 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);

Ah... And devm-cleanup will just drop a reference to that data,
freeing it when all refs are cleaned? Nice idea.

>         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.

As I wrote previously, I think drm_bridge_attach() might be too late for that.
It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
should increment the refcount, possibly adding a devres action to put
the reference.

> 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.

-- 
With best wishes
Dmitry




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux