Hello Anusha,
On Wed, 12 Mar 2025 20:54:42 -0400
Anusha Srivatsa <asrivats@xxxxxxxxxx> wrote:
> Introduce reference counted allocations for panels to avoid
> use-after-free. The patch adds the macro devm_drm_bridge_alloc()
> to allocate a new refcounted panel. Followed the documentation for
> drmm_encoder_alloc() and devm_drm_dev_alloc and other similar
> implementations for this purpose.
>
> Also adding drm_panel_get() and drm_panel_put() to suitably
> increment and decrement the refcount
>
> Signed-off-by: Anusha Srivatsa <asrivats@xxxxxxxxxx>
I'm very happy to see the very first step of the panel rework mentioned
by Maxime see the light! :-)
This patch looks mostly good to me, and the similarity with my bridge
refcounting work is by itself reassuring.
I have a few notes, one is relevant and the others are minor details,
see below.
In the Subject line: s/allocatons/allocations/
[...]
> +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
> + const struct drm_panel_funcs *funcs)
> +{
> + void *container;
> + struct drm_panel *panel;
> + int err;
> +
> + if (!funcs) {
> + dev_warn(dev, "Missing funcs pointer\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + container = kzalloc(size, GFP_KERNEL);
> + if (!container)
> + return ERR_PTR(-ENOMEM);
> +
> + panel = container + offset;
> + panel->container_offset = offset;
> + panel->funcs = funcs;
> + kref_init(&panel->refcount);
> +
> + err = devm_add_action_or_reset(dev, drm_panel_put_void, panel);
> + if (err)
> + return ERR_PTR(err);
> +
> + drm_panel_init(panel, dev, funcs, panel->connector_type);
panel->connector_type here is uninitialized. You are passing
panel->connector_type to drm_panel_init(), which will then copy it into
panel->connector_type itself.
> +
> + /**
> + * @container_offset: Offset of this struct within the container
> + * struct embedding it. Used for refcounted panels to free the
> + * embeddeing struct when the refcount drops to zero.
> + */
> + size_t container_offset;
While storing the offset obviously works, and that's what I had
implemented in my latest bridge refcounting series, after some
discussion with Maxime we agreed storing a container pointer instead of
the offset is cleaner. I think it would be good here as well.
See: https://lore.kernel.org/lkml/20250227-macho-convivial-tody-cea7dc@houat/
> +/**
> + * drm_panel_get - Acquire a panel reference
> + * @panel: DRM panel
> + *
> + * This function increments the panel's refcount.
> + *
> + */
> +static inline void drm_panel_get(struct drm_panel *panel)
> +{
> +
Remove empty line.
> +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
> + const struct drm_panel_funcs *funcs);
> +
> +/**
> + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel
s/an/a/ -- same typo as in my bridge series so I'm fixing it in my
series as well :)
> + * @dev: struct device of the panel device
> + * @type: the type of the struct which contains struct &drm_panel
> + * @member: the name of the &drm_panel within @type
> + * @funcs: callbacks for this panel
> + *
> + * The returned refcount is initialised to 1
In my opinion it is important to clarify that the caller does not have
to explicitly call drm_panel_put() on the returned pointer, because
devm will do it. Without clarifying, a user might think they need to,
and that would result in an extra put, which would be a bug.
Adapting from [0], that would be:
* The returned refcount is initialized to 1. This reference will be
* automatically dropped via devm (by calling drm_panel_put()) when @dev
* is removed.
[0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-14-9d6f2c9c3058@xxxxxxxxxxx/
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
