| Message ID | 20250325-b4-panel-refcounting-v1-1-4e2bf5d19c5d@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panel: Panel Refcounting infrastructure | expand |
Hello Anusha, On Tue, 25 Mar 2025 13:24:08 -0400 Anusha Srivatsa <asrivats@redhat.com> 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. > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> [...] > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > + const struct drm_panel_funcs *funcs, > + int connector_type); > + > +/** > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel ^^ "a refcounted panel" > + * @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 > + * @connector_type: connector type of the driver I'd say it's the connector type in the hardware, rather than of the driver (the driver follows what is in the hardware. Maybe you can just copy the description present in the drm_panel_init kdoc: * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to * the panel interface (must NOT be DRM_MODE_CONNECTOR_Unknown) Other than that it looks good! Luca
On Tue, Mar 25, 2025 at 01:24:08PM -0400, Anusha Srivatsa 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. > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > --- > drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ > include/drm/drm_panel.h | 22 ++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index c627e42a7ce70459f50eb5095fffc806ca45dabf..bdeab5710ee324dc1742fbc77582250960556308 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -355,6 +355,31 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) > } > EXPORT_SYMBOL(of_drm_find_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; > + > + if (!funcs) { > + dev_warn(dev, "Missing funcs pointer\n"); > + return ERR_PTR(-EINVAL); > + } > + > + container = devm_kzalloc(dev, size, GFP_KERNEL); > + if (!container) > + return ERR_PTR(-ENOMEM); > + > + panel = container + offset; > + panel->funcs = funcs; > + > + drm_panel_init(panel, dev, funcs, connector_type); > + > + return container; > +} > +EXPORT_SYMBOL(__devm_drm_panel_alloc); > + > /** > * of_drm_get_panel_orientation - look up the orientation of the panel through > * the "rotation" binding from a device tree node > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index a9c042c8dea1a82ef979c7a68204e0b55483fc28..63fb1dbe15a0556e7484bc18737a6b1f4c208b0c 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -28,6 +28,7 @@ > #include <linux/errno.h> > #include <linux/list.h> > #include <linux/mutex.h> > +#include <linux/kref.h> > > struct backlight_device; > struct dentry; > @@ -268,6 +269,27 @@ struct drm_panel { > bool enabled; > }; > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > + const struct drm_panel_funcs *funcs, > + int connector_type); > + > +/** > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel > + * @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 > + * @connector_type: connector type of the driver > + * The returned refcount is initialised to 1 There's not returned refcount. What is returned is a pointer to the container structure. You should mention that the reference count is initialized to 1, and will be given back automatically through a devm action. Iirc, Luca had a similar mention in his series, if you need inspiration. > + * Returns: > + * Pointer to new panel, or ERR_PTR on failure. It doesn't return a pointer to the new panel, but to the structure containing the panel. Maxime
On Wed, Mar 26, 2025 at 10:22:59AM +0100, Luca Ceresoli wrote: > Hello Anusha, > > On Tue, 25 Mar 2025 13:24:08 -0400 > Anusha Srivatsa <asrivats@redhat.com> 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. > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > [...] > > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > > + const struct drm_panel_funcs *funcs, > > + int connector_type); > > + > > +/** > > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel > ^^ > "a refcounted panel" > > > + * @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 > > + * @connector_type: connector type of the driver > > I'd say it's the connector type in the hardware, rather than of the > driver (the driver follows what is in the hardware. Maybe you can just > copy the description present in the drm_panel_init kdoc: > > * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to > * the panel interface (must NOT be DRM_MODE_CONNECTOR_Unknown) > > Other than that it looks good! Heh, Unknown is fine, but you're right for the rest. I'd use the drm_panel_init doc for that field actually. Maxime
On Wed, Mar 26, 2025 at 11:26 AM Maxime Ripard <mripard@kernel.org> wrote: > On Wed, Mar 26, 2025 at 10:22:59AM +0100, Luca Ceresoli wrote: > > Hello Anusha, > > > > On Tue, 25 Mar 2025 13:24:08 -0400 > > Anusha Srivatsa <asrivats@redhat.com> 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. > > > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > > > [...] > > > > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t > offset, > > > + const struct drm_panel_funcs *funcs, > > > + int connector_type); > > > + > > > +/** > > > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel > > ^^ > > "a refcounted panel" > > > > > + * @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 > > > + * @connector_type: connector type of the driver > > > > I'd say it's the connector type in the hardware, rather than of the > > driver (the driver follows what is in the hardware. Maybe you can just > > copy the description present in the drm_panel_init kdoc: > > > > * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) > corresponding to > > * the panel interface (must NOT be DRM_MODE_CONNECTOR_Unknown) > > > > Other than that it looks good! > > Heh, Unknown is fine, but you're right for the rest. I'd use the > drm_panel_init doc for that field actually. > > Will make this change in the next iteration, Thanks Luca and Maxime Anusha > Maxime >
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index c627e42a7ce70459f50eb5095fffc806ca45dabf..bdeab5710ee324dc1742fbc77582250960556308 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -355,6 +355,31 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) } EXPORT_SYMBOL(of_drm_find_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; + + if (!funcs) { + dev_warn(dev, "Missing funcs pointer\n"); + return ERR_PTR(-EINVAL); + } + + container = devm_kzalloc(dev, size, GFP_KERNEL); + if (!container) + return ERR_PTR(-ENOMEM); + + panel = container + offset; + panel->funcs = funcs; + + drm_panel_init(panel, dev, funcs, connector_type); + + return container; +} +EXPORT_SYMBOL(__devm_drm_panel_alloc); + /** * of_drm_get_panel_orientation - look up the orientation of the panel through * the "rotation" binding from a device tree node diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index a9c042c8dea1a82ef979c7a68204e0b55483fc28..63fb1dbe15a0556e7484bc18737a6b1f4c208b0c 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -28,6 +28,7 @@ #include <linux/errno.h> #include <linux/list.h> #include <linux/mutex.h> +#include <linux/kref.h> struct backlight_device; struct dentry; @@ -268,6 +269,27 @@ struct drm_panel { bool enabled; }; +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, + const struct drm_panel_funcs *funcs, + int connector_type); + +/** + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel + * @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 + * @connector_type: connector type of the driver + * The returned refcount is initialised to 1 + * + * Returns: + * Pointer to new panel, or ERR_PTR on failure. + */ +#define devm_drm_panel_alloc(dev, type, member, funcs, connector_type) \ + ((type *)__devm_drm_panel_alloc(dev, sizeof(type), \ + offsetof(type, member), funcs, \ + connector_type)) + void drm_panel_init(struct drm_panel *panel, struct device *dev, const struct drm_panel_funcs *funcs, int connector_type);
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. Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> --- drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ include/drm/drm_panel.h | 22 ++++++++++++++++++++++ 2 files changed, 47 insertions(+)