| Message ID | 20250327-b4-panel-refcounting-v2-1-b5f5ca551f95@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panel: Panel Refcounting infrastructure | expand |
On Thu, Mar 27, 2025 at 11:58 AM Maxime Ripard <mripard@kernel.org> wrote: > On Thu, Mar 27, 2025 at 10:55:39AM -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. > > > > v2: Better documentation for connector_type field - follow drm_panel_init > > documentation. (Luca) > > - Clarify the refcount initialisation in comments.(Maxime) > > - Correct the documentation of the return type (Maxime) > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > --- > > drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ > > include/drm/drm_panel.h | 23 +++++++++++++++++++++++ > > 2 files changed, 48 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..53251c6b11d78149ede3dad41ffa6a88f3c3c58b > 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,28 @@ 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: the connector type (DRM_MODE_CONNECTOR_*) > corresponding to > > + * the panel interface > > + * Returns: > > + * Pointer to container structure embedding the panel, ERR_PTR on > failure. > > + * The reference count is initialised to 1 and is automatically given > back > > + * by devm action. > > Sorry, I noticed after the facts, but this can't be in the Returns > section, it needs to be in the main one. > > Maxime, Not really following you. Are you suggesting this explanation needs to be in the helper documentation instead of in returns? Anusha > Maxime >
On Thu, 27 Mar 2025 10:55:39 -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. > > [ ... ] Reviewed-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime
On Thu, Mar 27, 2025 at 10:55:39AM -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. > > v2: Better documentation for connector_type field - follow drm_panel_init > documentation. (Luca) > - Clarify the refcount initialisation in comments.(Maxime) > - Correct the documentation of the return type (Maxime) > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > --- > drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ > include/drm/drm_panel.h | 23 +++++++++++++++++++++++ > 2 files changed, 48 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..53251c6b11d78149ede3dad41ffa6a88f3c3c58b 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,28 @@ 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: the connector type (DRM_MODE_CONNECTOR_*) corresponding to > + * the panel interface > + * Returns: > + * Pointer to container structure embedding the panel, ERR_PTR on failure. > + * The reference count is initialised to 1 and is automatically given back > + * by devm action. Sorry, I noticed after the facts, but this can't be in the Returns section, it needs to be in the main one. Maxime
Hello Anusha, Thanks for your continued effort. I have a few minor comments. Nothing big, but since Maxime requested a change you'll have to send a new iteration, so find my comments below. On Thu, 27 Mar 2025 10:55:39 -0400 Anusha Srivatsa <asrivats@redhat.com> wrote: [...] > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index a9c042c8dea1a82ef979c7a68204e0b55483fc28..53251c6b11d78149ede3dad41ffa6a88f3c3c58b 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> Minor nit: you don't need this include in patch 1. You should move it to patch 2 where it is actually used. > @@ -268,6 +269,28 @@ 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 ^^ A typo here is certainly not a huge problem, but I think I had already reported this should be "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: the connector type (DRM_MODE_CONNECTOR_*) corresponding to > + * the panel interface > + * Returns: > + * Pointer to container structure embedding the panel, ERR_PTR on failure. > + * The reference count is initialised to 1 and is automatically given back > + * by devm action. > + */ In addition to Maxime's comment: I think it's a common practice to have an empty line after the last @argument and also before the "Returns:" line, to improve readability Luca
On Thu, Mar 27, 2025 at 11:33:15AM -0400, Anusha Srivatsa wrote: > On Thu, Mar 27, 2025 at 11:58 AM Maxime Ripard <mripard@kernel.org> wrote: > > > On Thu, Mar 27, 2025 at 10:55:39AM -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. > > > > > > v2: Better documentation for connector_type field - follow drm_panel_init > > > documentation. (Luca) > > > - Clarify the refcount initialisation in comments.(Maxime) > > > - Correct the documentation of the return type (Maxime) > > > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > > --- > > > drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ > > > include/drm/drm_panel.h | 23 +++++++++++++++++++++++ > > > 2 files changed, 48 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..53251c6b11d78149ede3dad41ffa6a88f3c3c58b > > 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,28 @@ 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: the connector type (DRM_MODE_CONNECTOR_*) > > corresponding to > > > + * the panel interface > > > + * Returns: > > > + * Pointer to container structure embedding the panel, ERR_PTR on > > failure. > > > + * The reference count is initialised to 1 and is automatically given > > back > > > + * by devm action. > > > > Sorry, I noticed after the facts, but this can't be in the Returns > > section, it needs to be in the main one. > > Maxime, Not really following you. Are you suggesting this explanation > needs to be in the helper documentation instead of in returns? This is a general documentation thing, so it needs to be in the main documentation section, thus between the argumnts and returned values one. Maxime
On Fri, Mar 28, 2025 at 4:34 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > Hello Anusha, > > Thanks for your continued effort. > > I have a few minor comments. Nothing big, but since Maxime requested a > change you'll have to send a new iteration, so find my comments below. > > On Thu, 27 Mar 2025 10:55:39 -0400 > Anusha Srivatsa <asrivats@redhat.com> wrote: > > [...] > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index > a9c042c8dea1a82ef979c7a68204e0b55483fc28..53251c6b11d78149ede3dad41ffa6a88f3c3c58b > 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> > > Minor nit: you don't need this include in patch 1. You should move it > to patch 2 where it is actually used. > > > @@ -268,6 +269,28 @@ 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 > ^^ > A typo here is certainly not a huge problem, but I think I had already > reported this should be "a refcounted panel". > > Yeah you had and I thought I had taken care of it. WIll change this. > + * @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: the connector type (DRM_MODE_CONNECTOR_*) > corresponding to > > + * the panel interface > > + * Returns: > > + * Pointer to container structure embedding the panel, ERR_PTR on > failure. > > + * The reference count is initialised to 1 and is automatically given > back > > + * by devm action. > > + */ > > In addition to Maxime's comment: I think it's a common practice to have > an empty line after the last @argument and also before the "Returns:" > line, to improve readability > > On it! Thanks, Anusha > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > >
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..53251c6b11d78149ede3dad41ffa6a88f3c3c58b 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,28 @@ 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: the connector type (DRM_MODE_CONNECTOR_*) corresponding to + * the panel interface + * Returns: + * Pointer to container structure embedding the panel, ERR_PTR on failure. + * The reference count is initialised to 1 and is automatically given back + * by devm action. + */ +#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. v2: Better documentation for connector_type field - follow drm_panel_init documentation. (Luca) - Clarify the refcount initialisation in comments.(Maxime) - Correct the documentation of the return type (Maxime) Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> --- drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++ include/drm/drm_panel.h | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+)