Hi Jani,
On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote:
> On Tue, 29 Apr 2025, Maxime Ripard <mrip...@kernel.org> wrote:
> > Hi Jani,
> >
> > On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> >> On Mon, 31 Mar 2025, Anusha Srivatsa <asriv...@redhat.com> wrote:
> >> > Allocate panel via reference counting. Add _get() and _put() helper
> >> > functions to ensure panel allocations are refcounted. Avoid use after
> >> > free by ensuring panel pointer is valid and can be usable till the last
> >> > reference is put.
> >> >
> >> > Reviewed-by: Luca Ceresoli <luca.ceres...@bootlin.com>
> >> > Reviewed-by: Maxime Ripard <mrip...@kernel.org>
> >> > Signed-off-by: Anusha Srivatsa <asriv...@redhat.com>
> >> >
> >> > ---
> >> > v4: Add refcounting documentation in this patch (Maxime)
> >> >
> >> > v3: Add include in this patch (Luca)
> >> >
> >> > v2: Export drm_panel_put/get() (Maxime)
> >> > - Change commit log with better workding (Luca, Maxime)
> >> > - Change drm_panel_put() to return void (Luca)
> >> > - Code Cleanups - add return in documentation, replace bridge to
> >> > panel (Luca)
> >> > ---
> >> > drivers/gpu/drm/drm_panel.c | 64
> >> > ++++++++++++++++++++++++++++++++++++++++++++-
> >> > include/drm/drm_panel.h | 19 ++++++++++++++
> >> > 2 files changed, 82 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> >> > index
> >> > bdeab5710ee324dc1742fbc77582250960556308..7b17531d85a4dc3031709919564d2e4d8332f748
> >> > 100644
> >> > --- a/drivers/gpu/drm/drm_panel.c
> >> > +++ b/drivers/gpu/drm/drm_panel.c
> >> > @@ -355,24 +355,86 @@ struct drm_panel *of_drm_find_panel(const struct
> >> > device_node *np)
> >> > }
> >> > EXPORT_SYMBOL(of_drm_find_panel);
> >> >
> >> > +static void __drm_panel_free(struct kref *kref)
> >> > +{
> >> > + struct drm_panel *panel = container_of(kref, struct drm_panel,
> >> > refcount);
> >> > +
> >> > + kfree(panel->container);
> >> > +}
> >> > +
> >> > +/**
> >> > + * drm_panel_get - Acquire a panel reference
> >> > + * @panel: DRM panel
> >> > + *
> >> > + * This function increments the panel's refcount.
> >> > + * Returns:
> >> > + * Pointer to @panel
> >> > + */
> >> > +struct drm_panel *drm_panel_get(struct drm_panel *panel)
> >> > +{
> >> > + if (!panel)
> >> > + return panel;
> >> > +
> >> > + kref_get(&panel->refcount);
> >> > +
> >> > + return panel;
> >> > +}
> >> > +EXPORT_SYMBOL(drm_panel_get);
> >> > +
> >> > +/**
> >> > + * drm_panel_put - Release a panel reference
> >> > + * @panel: DRM panel
> >> > + *
> >> > + * This function decrements the panel's reference count and frees the
> >> > + * object if the reference count drops to zero.
> >> > + */
> >> > +void drm_panel_put(struct drm_panel *panel)
> >> > +{
> >> > + if (panel)
> >> > + kref_put(&panel->refcount, __drm_panel_free);
> >> > +}
> >> > +EXPORT_SYMBOL(drm_panel_put);
> >> > +
> >> > +/**
> >> > + * drm_panel_put_void - wrapper to drm_panel_put() taking a void pointer
> >> > + *
> >> > + * @data: pointer to @struct drm_panel, cast to a void pointer
> >> > + *
> >> > + * Wrapper of drm_panel_put() to be used when a function taking a void
> >> > + * pointer is needed, for example as a devm action.
> >> > + */
> >> > +static void drm_panel_put_void(void *data)
> >> > +{
> >> > + struct drm_panel *panel = (struct drm_panel *)data;
> >> > +
> >> > + drm_panel_put(panel);
> >> > +}
> >> > +
> >> > void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t
> >> > offset,
> >> > const struct drm_panel_funcs *funcs,
> >> > int connector_type)
> >> > {
> >> > void *container;
> >> > struct drm_panel *panel;
> >> > + int err;
> >> >
> >> > if (!funcs) {
> >> > dev_warn(dev, "Missing funcs pointer\n");
> >> > return ERR_PTR(-EINVAL);
> >> > }
> >> >
> >> > - container = devm_kzalloc(dev, size, GFP_KERNEL);
> >> > + container = kzalloc(size, GFP_KERNEL);
> >> > if (!container)
> >> > return ERR_PTR(-ENOMEM);
> >> >
> >> > panel = container + offset;
> >> > + panel->container = container;
> >> > panel->funcs = funcs;
> >> > + kref_init(&panel->refcount);
> >>
> >> Hi Anusha, this should be done in drm_panel_init() instead.
> >>
> >> There are many users of drm_panel that don't use devm_drm_panel_alloc()
> >> but allocate separately, and call drm_panel_init() only.
> >
> > That wouldn't really work, because then drivers would have allocated the
> > panel with devm_kzalloc and thus the structure would be freed when the
> > device is removed, no matter the reference counting state.
> >
> >> They'll all have refcount set to 0 instead of 1 like kref_init() does.
> >>
> >> This means all subsequent get/put pairs on such panels will lead to
> >> __drm_panel_free() being called! But through a lucky coincidence, that
> >> will be a nop because panel->container is also not initialized...
> >>
> >> I'm sorry to say, the drm refcounting interface is quite broken for such
> >> use cases.
> >
> > The plan is to convert all panel drivers to that function, and Anusha
> > already sent series to do. It still needs a bit of work, but it should
> > land soon-ish.
> >
> > For the transitional period though, it's not clear to me what you think
> > is broken at the moment, and / or what should be fixed.
> >
> > Would you prefer an explicit check on container not being 0, with a
> > comment?
>
> I'm looking at what it would take to add drm_panel support to i915 so
> that you could have drm_panel_followers on it. There are gaps of course,
> but initially it would mean allocating and freeing drm_panel ourselves,
> not via devm_drm_panel_alloc() nor devm_kzalloc(), because none of the
> other stuff is allocated that way. drm_panel would just sit as a
> sub-struct inside struct intel_panel, which is a sub-struct of struct
> intel_connector, which has its own allocation...I'm not entirely sure why you would need to allocate it from i915? The drm_panel structure is only meant to be allocated by panel drivers, and afaik no panel interface controller is allocating it. > But basically in its current state, the refcounting would not be > reliable for that use case. I guess with panel->container being NULL > nothing happens, but the idea that the refcount drops back to 0 after a > get/put is a bit scary. > > Anyway, I think there should be no harm in moving the kref init to > drm_panel_init(), right? I mean, there is because the plan so far was to remove drm_panel_init() :) Maxime
signature.asc
Description: PGP signature
