Hi Maxime, Dmitry,
thanks both for the useful review!
On Mon, 6 Jan 2025 14:24:00 +0200
Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
> 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.
I like the idea. It's basically a macro wrapping the calls to kzalloc()
+ drm_bridge_init() that I proposed in this series. I had thought about
such an idea initially but I haven't seen such a macro in
drm_connector.h I didn't follow the idea.
I don't love the _alloc name though because it will be doing much more
than allocating. What about devm_drm_bridge_new()?
I understand _alloc is coherent with the drmm_encoder_alloc() and I
could survive that... but what about renaming that one to
drmm_encoder_new()?
Or maybe _create instead of _new, because _new is used for atomic
states, in opposition to _old.
> > 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.
Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
drm_bridge.c will need to be sprinkled with them I guess.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
