[PATCH 0/5] drm/panel: Panel Refcounting infrastructure

[PATCH 0/5] drm/panel: Panel Refcounting infrastructure

[Anusha Srivatsa]

This series adds the infrastructure needed for the refcounting
allocations for panels similar to Luca's efforts with bridges.
Underlying intention and idea is the same - avoid use-after-free
situations in panels. Get reference to panel when in use and put
the reference back (down) when not in use.
Once this gets approved, rest of the drivers will have to be
mass converted to use this API.  All the callers of of_drm_find_panel()
will have to be converted too.

Tried to split the patches as suggested in the RFC series[1].
Also fixed the connector used during panel_init to be the one
passed by driver.

Patch 4 was not suggested or part of my initial work. Added it
after looking at the comments Luca's v8 of the bridge series
received.[2]

[1] -> https://patchwork.freedesktop.org/series/146236/
[2] -> https://patchwork.freedesktop.org/series/146306/#rev2

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
Anusha Srivatsa (5):
      drm/panel: Add new helpers for refcounted panel allocatons
      drm/panel: Add refcount support
      drm/panel: get/put panel reference in drm_panel_add/remove()
      drm/panel: deprecate old-style panel allocation
      drm/panel/panel-simple: Use the new allocation in place of devm_kzalloc()

 drivers/gpu/drm/drm_panel.c          | 96 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/panel/panel-simple.c |  5 +-
 include/drm/drm_panel.h              | 39 +++++++++++++++
 3 files changed, 135 insertions(+), 5 deletions(-)
---
base-commit: c8ba07caaecc622a9922cda49f24790821af8a71
change-id: 20250324-b4-panel-refcounting-40ab56aa34f7

Best regards,
-- 
Anusha Srivatsa <asrivats@redhat.com>

[PATCH 1/5] drm/panel: Add new helpers for refcounted panel allocatons

[Anusha Srivatsa]

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
+ *
+ * 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);

-- 
2.48.1

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

[Luca Ceresoli]

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 2978 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/5] drm/panel: Add refcount support

[Anusha Srivatsa]

Allocate panel via reference counting.
Add _get() and _put() helper functions
to ensure panel allocations are refcounted.
Avoid use after free by ensuring panel is
valid and can be usable till the last reference
is put. This avoids use-after-free

Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
 drivers/gpu/drm/drm_panel.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_panel.h     | 19 ++++++++++++-
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index bdeab5710ee324dc1742fbc77582250960556308..079c3c666a2ddc99a0051d1a3c9ba65d986dd003 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -355,24 +355,87 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
 }
 EXPORT_SYMBOL(of_drm_find_panel);
 
+static void __drm_panel_free(struct kref *kref)
+{
+	struct drm_panel *panel = container_of(kref, struct drm_panel, refcount);
+
+	kfree(panel->container);
+}
+
+/**
+ * drm_panel_get - Acquire a panel reference
+ * @panel: DRM panel
+ *
+ * This function increments the panel's refcount.
+ *
+ */
+struct drm_panel *drm_panel_get(struct drm_panel *panel)
+{
+	if (!panel)
+		return panel;
+
+	kref_get(&panel->refcount);
+
+	return panel;
+}
+
+/**
+ * drm_panel_put - Release a panel reference
+ * @panel: DRM panel
+ *
+ * This function decrements the panel's reference count and frees the
+ * object if the reference count drops to zero.
+ */
+struct drm_panel *drm_panel_put(struct drm_panel *panel)
+{
+	if (!panel)
+		return panel;
+
+	kref_put(&panel->refcount, __drm_panel_free);
+
+	return panel;
+}
+
+/**
+ * drm_bridge_put_void - wrapper to drm_bridge_put() taking a void pointer
+ *
+ * @data: pointer to @struct drm_bridge, cast to a void pointer
+ *
+ * Wrapper of drm_bridge_put() to be used when a function taking a void
+ * pointer is needed, for example as a devm action.
+ */
+static void drm_panel_put_void(void *data)
+{
+	struct drm_panel *panel = (struct drm_panel *)data;
+
+	drm_panel_put(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;
+	int err;
 
 	if (!funcs) {
 		dev_warn(dev, "Missing funcs pointer\n");
 		return ERR_PTR(-EINVAL);
 	}
 
-	container = devm_kzalloc(dev, size, GFP_KERNEL);
+	container = kzalloc(size, GFP_KERNEL);
 	if (!container)
 		return ERR_PTR(-ENOMEM);
 
 	panel = container + offset;
+	panel->container = container;
 	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, connector_type);
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 63fb1dbe15a0556e7484bc18737a6b1f4c208b0c..af81d596f385567a12cf9e08dff9443ce4d97ec0 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -267,6 +267,17 @@ struct drm_panel {
 	 * If true then the panel has been enabled.
 	 */
 	bool enabled;
+
+	/**
+	 * @container: Pointer to the private driver struct embedding this
+	 * @struct drm_panel.
+	 */
+	void *container;
+
+	/**
+	 * @refcount: reference count of users referencing this bridge.
+	 */
+	struct kref refcount;
 };
 
 void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
@@ -280,7 +291,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
  * @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
+ *
+ * The returned refcount is initialised to 1. This  reference will
+ * be automatically dropped via devm (by calling
+ * drm_bridge_put()) when @dev is removed.
  *
  * Returns:
  * Pointer to new panel, or ERR_PTR on failure.
@@ -294,6 +308,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
 		    const struct drm_panel_funcs *funcs,
 		    int connector_type);
 
+struct drm_panel *drm_panel_get(struct drm_panel *panel);
+struct drm_panel *drm_panel_put(struct drm_panel *panel);
+
 void drm_panel_add(struct drm_panel *panel);
 void drm_panel_remove(struct drm_panel *panel);
 

-- 
2.48.1

Re: [PATCH 2/5] drm/panel: Add refcount support

[kernel test robot]

Hi Anusha,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c8ba07caaecc622a9922cda49f24790821af8a71]

url:    https://github.com/intel-lab-lkp/linux/commits/Anusha-Srivatsa/drm-panel-Add-new-helpers-for-refcounted-panel-allocatons/20250326-012651
base:   c8ba07caaecc622a9922cda49f24790821af8a71
patch link:    https://lore.kernel.org/r/20250325-b4-panel-refcounting-v1-2-4e2bf5d19c5d%40redhat.com
patch subject: [PATCH 2/5] drm/panel: Add refcount support
config: s390-randconfig-002-20250326 (https://download.01.org/0day-ci/archive/20250326/202503260820.3wTF0Zap-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250326/202503260820.3wTF0Zap-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503260820.3wTF0Zap-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_panel.c:408: warning: expecting prototype for drm_bridge_put_void(). Prototype was for drm_panel_put_void() instead


vim +408 drivers/gpu/drm/drm_panel.c

   398	
   399	/**
   400	 * drm_bridge_put_void - wrapper to drm_bridge_put() taking a void pointer
   401	 *
   402	 * @data: pointer to @struct drm_bridge, cast to a void pointer
   403	 *
   404	 * Wrapper of drm_bridge_put() to be used when a function taking a void
   405	 * pointer is needed, for example as a devm action.
   406	 */
   407	static void drm_panel_put_void(void *data)
 > 408	{
   409		struct drm_panel *panel = (struct drm_panel *)data;
   410	
   411		drm_panel_put(panel);
   412	}
   413	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/5] drm/panel: Add refcount support

[Luca Ceresoli]

On Tue, 25 Mar 2025 13:24:09 -0400
Anusha Srivatsa <asrivats@redhat.com> wrote:

> Allocate panel via reference counting.
> Add _get() and _put() helper functions
> to ensure panel allocations are refcounted.
> Avoid use after free by ensuring panel is
> valid and can be usable till the last reference
> is put. This avoids use-after-free

"panel is valid and can be usable" is not totally correct. When there
are still references held, you ensure the panel struct is still
_allocated_, not necessarily valid and usable.

Minor nit: you are wrapping at less than 50 columns, which is a bit
tight. I think 75 is the expected value for wrapping.

> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

[...]

> +/**
> + * drm_panel_get - Acquire a panel reference
> + * @panel: DRM panel
> + *
> + * This function increments the panel's refcount.
> + *
> + */

Not sure it's mandatory, but documenting the returned value is good
practice , e.g.:

 * Returns:
 * Pointer to @panel.

> +/**
> + * drm_panel_put - Release a panel reference
> + * @panel: DRM panel
> + *
> + * This function decrements the panel's reference count and frees the
> + * object if the reference count drops to zero.
> + */
> +struct drm_panel *drm_panel_put(struct drm_panel *panel)
> +{
> +	if (!panel)
> +		return panel;
> +
> +	kref_put(&panel->refcount, __drm_panel_free);
> +
> +	return panel;

While this is using the same structure as my bridge work, I now realize
the _put() should probably not return the panel (or bridge) pointer, it
should just be a void function. The reason is the pointer might have
been freed when _put() returns (depending on the refcount) so that
pointer value might be dangling and whoever calls _put() must not use
that pointer anymore.

Other get/put APIs do this, e.g. of_node_get/put().

So I'm going to change drm_bridge_put() to return void, unless there
are good reasons to keep it and that I'm missing.

> @@ -280,7 +291,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
>   * @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
> + *
> + * The returned refcount is initialised to 1. This  reference will
> + * be automatically dropped via devm (by calling
> + * drm_bridge_put()) when @dev is removed.
          ^^^^^^
"panel". Same in a few other places in this patch. Please search in all
this series in case there are more. It's easy to forget about replacing
some of those in the comments. :)

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/5] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Wed, Mar 26, 2025 at 10:23:04AM +0100, Luca Ceresoli wrote:
> On Tue, 25 Mar 2025 13:24:09 -0400
> Anusha Srivatsa <asrivats@redhat.com> wrote:
> 
> > Allocate panel via reference counting.
> > Add _get() and _put() helper functions
> > to ensure panel allocations are refcounted.
> > Avoid use after free by ensuring panel is
> > valid and can be usable till the last reference
> > is put. This avoids use-after-free
> 
> "panel is valid and can be usable" is not totally correct. When there
> are still references held, you ensure the panel struct is still
> _allocated_, not necessarily valid and usable.

I guess "panel pointer is valid" is a better wording indeed.

> > +/**
> > + * drm_panel_put - Release a panel reference
> > + * @panel: DRM panel
> > + *
> > + * This function decrements the panel's reference count and frees the
> > + * object if the reference count drops to zero.
> > + */
> > +struct drm_panel *drm_panel_put(struct drm_panel *panel)
> > +{
> > +	if (!panel)
> > +		return panel;
> > +
> > +	kref_put(&panel->refcount, __drm_panel_free);
> > +
> > +	return panel;
> 
> While this is using the same structure as my bridge work, I now realize
> the _put() should probably not return the panel (or bridge) pointer, it
> should just be a void function. The reason is the pointer might have
> been freed when _put() returns (depending on the refcount) so that
> pointer value might be dangling and whoever calls _put() must not use
> that pointer anymore.
> 
> Other get/put APIs do this, e.g. of_node_get/put().
> 
> So I'm going to change drm_bridge_put() to return void, unless there
> are good reasons to keep it and that I'm missing.

Oh, right, definitely.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/5] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Tue, Mar 25, 2025 at 01:24:09PM -0400, Anusha Srivatsa wrote:
> Allocate panel via reference counting.
> Add _get() and _put() helper functions
> to ensure panel allocations are refcounted.
> Avoid use after free by ensuring panel is
> valid and can be usable till the last reference
> is put. This avoids use-after-free
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
> ---
>  drivers/gpu/drm/drm_panel.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_panel.h     | 19 ++++++++++++-
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index bdeab5710ee324dc1742fbc77582250960556308..079c3c666a2ddc99a0051d1a3c9ba65d986dd003 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -355,24 +355,87 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  }
>  EXPORT_SYMBOL(of_drm_find_panel);
>  
> +static void __drm_panel_free(struct kref *kref)
> +{
> +	struct drm_panel *panel = container_of(kref, struct drm_panel, refcount);
> +
> +	kfree(panel->container);
> +}
> +
> +/**
> + * drm_panel_get - Acquire a panel reference
> + * @panel: DRM panel
> + *
> + * This function increments the panel's refcount.
> + *
> + */
> +struct drm_panel *drm_panel_get(struct drm_panel *panel)
> +{
> +	if (!panel)
> +		return panel;
> +
> +	kref_get(&panel->refcount);
> +
> +	return panel;
> +}

This should be exported

> +/**
> + * drm_panel_put - Release a panel reference
> + * @panel: DRM panel
> + *
> + * This function decrements the panel's reference count and frees the
> + * object if the reference count drops to zero.
> + */
> +struct drm_panel *drm_panel_put(struct drm_panel *panel)
> +{
> +	if (!panel)
> +		return panel;
> +
> +	kref_put(&panel->refcount, __drm_panel_free);
> +
> +	return panel;
> +}

Ditto,

> +/**
> + * drm_bridge_put_void - wrapper to drm_bridge_put() taking a void pointer
> + *
> + * @data: pointer to @struct drm_bridge, cast to a void pointer
> + *
> + * Wrapper of drm_bridge_put() to be used when a function taking a void
> + * pointer is needed, for example as a devm action.
> + */
> +static void drm_panel_put_void(void *data)
> +{
> +	struct drm_panel *panel = (struct drm_panel *)data;
> +
> +	drm_panel_put(panel);
> +}

You can drop the documentation on that one.

Looks good otherwise,
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[Anusha Srivatsa]

Take the panel reference and put it back as required
using the helpers introduced in previous patch.
drm_panel_add() and drm_panel_remove()
add a panel to the global registry and removes a panel
respectively, use get() and put() helpers to keep up
with refcounting.

Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
 drivers/gpu/drm/drm_panel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 079c3c666a2ddc99a0051d1a3c9ba65d986dd003..11a0415bc61f59190ef5eb378d1583c493265e6a 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -79,6 +79,7 @@ EXPORT_SYMBOL(drm_panel_init);
  */
 void drm_panel_add(struct drm_panel *panel)
 {
+	drm_panel_get(panel);
 	mutex_lock(&panel_lock);
 	list_add_tail(&panel->list, &panel_list);
 	mutex_unlock(&panel_lock);
@@ -96,6 +97,7 @@ void drm_panel_remove(struct drm_panel *panel)
 	mutex_lock(&panel_lock);
 	list_del_init(&panel->list);
 	mutex_unlock(&panel_lock);
+	drm_panel_put(panel);
 }
 EXPORT_SYMBOL(drm_panel_remove);
 

-- 
2.48.1

Re: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[kernel test robot]

Hi Anusha,

kernel test robot noticed the following build errors:

[auto build test ERROR on c8ba07caaecc622a9922cda49f24790821af8a71]

url:    https://github.com/intel-lab-lkp/linux/commits/Anusha-Srivatsa/drm-panel-Add-new-helpers-for-refcounted-panel-allocatons/20250326-012651
base:   c8ba07caaecc622a9922cda49f24790821af8a71
patch link:    https://lore.kernel.org/r/20250325-b4-panel-refcounting-v1-3-4e2bf5d19c5d%40redhat.com
patch subject: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()
config: xtensa-randconfig-001-20250326 (https://download.01.org/0day-ci/archive/20250326/202503260710.gDo7vXVR-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250326/202503260710.gDo7vXVR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503260710.gDo7vXVR-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in arch/xtensa/platforms/iss/simdisk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
>> ERROR: modpost: "drm_panel_put" [drivers/gpu/drm/drm.ko] undefined!
>> ERROR: modpost: "drm_panel_get" [drivers/gpu/drm/drm.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[kernel test robot]

Hi Anusha,

kernel test robot noticed the following build errors:

[auto build test ERROR on c8ba07caaecc622a9922cda49f24790821af8a71]

url:    https://github.com/intel-lab-lkp/linux/commits/Anusha-Srivatsa/drm-panel-Add-new-helpers-for-refcounted-panel-allocatons/20250326-012651
base:   c8ba07caaecc622a9922cda49f24790821af8a71
patch link:    https://lore.kernel.org/r/20250325-b4-panel-refcounting-v1-3-4e2bf5d19c5d%40redhat.com
patch subject: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()
config: hexagon-randconfig-002-20250326 (https://download.01.org/0day-ci/archive/20250326/202503261119.h0EEYLFA-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250326/202503261119.h0EEYLFA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503261119.h0EEYLFA-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: drm_panel_get
   >>> referenced by drm_panel.c:82 (drivers/gpu/drm/drm_panel.c:82)
   >>>               drivers/gpu/drm/drm_panel.o:(drm_panel_add) in archive vmlinux.a
   >>> referenced by drm_panel.c:82 (drivers/gpu/drm/drm_panel.c:82)
   >>>               drivers/gpu/drm/drm_panel.o:(drm_panel_add) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: drm_panel_put
   >>> referenced by drm_panel.c:100 (drivers/gpu/drm/drm_panel.c:100)
   >>>               drivers/gpu/drm/drm_panel.o:(drm_panel_remove) in archive vmlinux.a
   >>> referenced by drm_panel.c:100 (drivers/gpu/drm/drm_panel.c:100)
   >>>               drivers/gpu/drm/drm_panel.o:(drm_panel_remove) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[Luca Ceresoli]

On Tue, 25 Mar 2025 13:24:10 -0400
Anusha Srivatsa <asrivats@redhat.com> wrote:

> Take the panel reference and put it back as required
> using the helpers introduced in previous patch.
> drm_panel_add() and drm_panel_remove()
> add a panel to the global registry and removes a panel
> respectively, use get() and put() helpers to keep up
> with refcounting.
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

This is OK but is should not be applied until all panel drivers are
converted to the new devm_drm_panel_alloc() API. Otherwise for
not-yet-converted panel drivers drm_panel_get/put() would operate on an
uninitialized kref. See [0].

# Do not apply until all panel drivers are converted!
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

[0] https://lore.kernel.org/all/20250317155607.68cff522@booty/

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Tue, Mar 25, 2025 at 01:24:10PM -0400, Anusha Srivatsa wrote:
> Take the panel reference and put it back as required
> using the helpers introduced in previous patch.
> drm_panel_add() and drm_panel_remove()
> add a panel to the global registry and removes a panel
> respectively, use get() and put() helpers to keep up
> with refcounting.
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

We should defer merging that patch until we have converted all the panel
drivers to the new allocation function.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/5] drm/panel: get/put panel reference in drm_panel_add/remove()

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

On Wed, Mar 26, 2025 at 11:31 AM Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Mar 25, 2025 at 01:24:10PM -0400, Anusha Srivatsa wrote:
> > Take the panel reference and put it back as required
> > using the helpers introduced in previous patch.
> > drm_panel_add() and drm_panel_remove()
> > add a panel to the global registry and removes a panel
> > respectively, use get() and put() helpers to keep up
> > with refcounting.
> >
> > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
>
> We should defer merging that patch until we have converted all the panel
> drivers to the new allocation function.
>
> Luca, Maxime,

I ll drop this patch in the next iteration and send it once it is ready to
be merged.

Anusha

> Maxime
>

[-- Attachment #2: Type: text/html, Size: 1391 bytes --]

[PATCH 4/5] drm/panel: deprecate old-style panel allocation

[Anusha Srivatsa]

Start moving to the new refcounted allocations using
the new API devm_drm_panel_alloc(). Deprecate any other
allocation.

Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
 drivers/gpu/drm/drm_panel.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 11a0415bc61f59190ef5eb378d1583c493265e6a..5793011f4938a2d4fb9d84a700817bda317af305 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -74,8 +74,10 @@ EXPORT_SYMBOL(drm_panel_init);
  * drm_panel_add - add a panel to the global registry
  * @panel: panel to add
  *
- * Add a panel to the global registry so that it can be looked up by display
- * drivers.
+ * Add a panel to the global registry so that it can be looked
+ * up by display drivers. The panel to be added must have been
+ * allocated by devm_drm_panel_alloc(). Old-style allocation by
+ * kzalloc(), devm_kzalloc() and similar is deprecated.
  */
 void drm_panel_add(struct drm_panel *panel)
 {

-- 
2.48.1

Re: [PATCH 4/5] drm/panel: deprecate old-style panel allocation

[Luca Ceresoli]

On Tue, 25 Mar 2025 13:24:11 -0400
Anusha Srivatsa <asrivats@redhat.com> wrote:

> Start moving to the new refcounted allocations using
> the new API devm_drm_panel_alloc(). Deprecate any other
> allocation.
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/5] drm/panel: deprecate old-style panel allocation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On Tue, Mar 25, 2025 at 01:24:11PM -0400, Anusha Srivatsa wrote:
> Start moving to the new refcounted allocations using
> the new API devm_drm_panel_alloc(). Deprecate any other
> allocation.
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
> ---
>  drivers/gpu/drm/drm_panel.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 11a0415bc61f59190ef5eb378d1583c493265e6a..5793011f4938a2d4fb9d84a700817bda317af305 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -74,8 +74,10 @@ EXPORT_SYMBOL(drm_panel_init);
>   * drm_panel_add - add a panel to the global registry
>   * @panel: panel to add
>   *
> - * Add a panel to the global registry so that it can be looked up by display
> - * drivers.
> + * Add a panel to the global registry so that it can be looked
> + * up by display drivers. The panel to be added must have been
> + * allocated by devm_drm_panel_alloc(). Old-style allocation by
> + * kzalloc(), devm_kzalloc() and similar is deprecated.

It's not that it's deprecated, it's that it's unsafe. Since you already
said that the allocation must be done through devm_drm_panel_alloc(),
there's not much use to mention the old style stuff, I'd just drop the
last sentence.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/5] drm/panel: deprecate old-style panel allocation

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Wed, Mar 26, 2025 at 11:32 AM Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Mar 25, 2025 at 01:24:11PM -0400, Anusha Srivatsa wrote:
> > Start moving to the new refcounted allocations using
> > the new API devm_drm_panel_alloc(). Deprecate any other
> > allocation.
> >
> > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
> > ---
> >  drivers/gpu/drm/drm_panel.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index
> 11a0415bc61f59190ef5eb378d1583c493265e6a..5793011f4938a2d4fb9d84a700817bda317af305
> 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -74,8 +74,10 @@ EXPORT_SYMBOL(drm_panel_init);
> >   * drm_panel_add - add a panel to the global registry
> >   * @panel: panel to add
> >   *
> > - * Add a panel to the global registry so that it can be looked up by
> display
> > - * drivers.
> > + * Add a panel to the global registry so that it can be looked
> > + * up by display drivers. The panel to be added must have been
> > + * allocated by devm_drm_panel_alloc(). Old-style allocation by
> > + * kzalloc(), devm_kzalloc() and similar is deprecated.
>
> It's not that it's deprecated, it's that it's unsafe. Since you already
> said that the allocation must be done through devm_drm_panel_alloc(),
> there's not much use to mention the old style stuff, I'd just drop the
> last sentence.
>
>
Alrighty.

Thanks,
Anusha

> Maxime
>

[-- Attachment #2: Type: text/html, Size: 2275 bytes --]

[PATCH 5/5] drm/panel/panel-simple: Use the new allocation in place of devm_kzalloc()

[Anusha Srivatsa]

Start using the new helper that does the refcounted
allocations.

Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 6ba600f97aa4c8daae577823fcf17ef31b0eb46f..60b845fad4e1b378af52d34dfae0139c4625dc51 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -579,7 +579,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	u32 bus_flags;
 	int err;
 
-	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+	panel = devm_drm_panel_alloc(dev, struct panel_simple, base,
+				     &panel_simple_funcs, desc->connector_type);
 	if (!panel)
 		return -ENOMEM;
 
@@ -694,8 +695,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
-	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
-
 	err = drm_panel_of_backlight(&panel->base);
 	if (err) {
 		dev_err_probe(dev, err, "Could not find backlight\n");

-- 
2.48.1

Re: [PATCH 5/5] drm/panel/panel-simple: Use the new allocation in place of devm_kzalloc()

[Luca Ceresoli]

On Tue, 25 Mar 2025 13:24:12 -0400
Anusha Srivatsa <asrivats@redhat.com> wrote:

> Start using the new helper that does the refcounted
> allocations.
> 
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 6ba600f97aa4c8daae577823fcf17ef31b0eb46f..60b845fad4e1b378af52d34dfae0139c4625dc51 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -579,7 +579,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	u32 bus_flags;
>  	int err;
>  
> -	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> +	panel = devm_drm_panel_alloc(dev, struct panel_simple, base,
> +				     &panel_simple_funcs, desc->connector_type);
>  	if (!panel)
>  		return -ENOMEM;

devm_drm_panel_alloc() returns "Pointer to new panel, or ERR_PTR on
failure.", so you need IS_ERR() to check for an error condition:

  if (IS_ERR(panel))
    return PTR_ERR(Panel);

Otherwise looks good.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com