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