On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote: > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <[email protected]> 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().
Oh, right. We need to plumb it in drm_of_find_bridge somehow too.
> > 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.
Yup.
> > 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.
We probably need both. drm_bridge_attach adds the bridge pointer to the
encoder bridge_chain list, so if we had something like
bridge = drm_of_find_bridge();
drm_bridge_attach(encoder, bridge);
drm_bridge_put(bridge);
We could have a dangling pointer.
Maxime
signature.asc
Description: PGP signature
