Re: [PATCH RFC 1/2] drm/panel: Add new helpers for refcounted panel allocatons

[Luca Ceresoli]

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