linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 1/2] core: Add generic object registry implementation
@ 2014-11-04 16:29 Thierry Reding
  2014-11-04 16:29 ` [RFC 2/2] drm/panel: Use generic object registry Thierry Reding
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Thierry Reding @ 2014-11-04 16:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

From: Thierry Reding <treding@nvidia.com>

Add a generic implementation of an object registry. This targets drivers
and subsystems that provide auxiliary objects that other drivers need to
look up. The goal is to put the difficult parts (keep object references,
module usage count, ...) into core code so that individual subsystems do
not have to deal with them.

The intention is for subsystems to instantiate a struct registry and use
a struct registry_record embedded into a subsystem-specific structure to
provide a subsystem-specific API around that.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/base/Makefile    |   2 +-
 drivers/base/registry.c  | 147 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/registry.h |  62 ++++++++++++++++++++
 3 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/registry.c
 create mode 100644 include/linux/registry.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 53c3fe1aeb29..250262d1af2c 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o
+			   topology.o container.o property.o registry.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/registry.c b/drivers/base/registry.c
new file mode 100644
index 000000000000..9f510f6237b7
--- /dev/null
+++ b/drivers/base/registry.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (C) 2014, NVIDIA Corporation.  All rights reserved.
+ *
+ * This file is released under the GPL v2.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/registry.h>
+
+static inline struct registry_record *to_registry_record(struct kref *kref)
+{
+	return container_of(kref, struct registry_record, kref);
+}
+
+static void registry_record_release(struct kref *kref)
+{
+	struct registry_record *record = to_registry_record(kref);
+
+	record->release(record);
+}
+
+/**
+ * registry_record_init - initialize a registry record
+ * @record: record to initialize
+ *
+ * Sets up internal fields of the registry record so that it can subsequently
+ * be added to a registry.
+ */
+void registry_record_init(struct registry_record *record)
+{
+	INIT_LIST_HEAD(&record->list);
+	kref_init(&record->kref);
+}
+
+/**
+ * registry_record_ref - reference on the registry record
+ * @record: record to reference
+ *
+ * Increases the reference count on the record and returns a pointer to it.
+ *
+ * Return: A pointer to the record on success or NULL on failure.
+ */
+struct registry_record *registry_record_ref(struct registry_record *record)
+{
+	if (!record)
+		return NULL;
+
+	/*
+	 * Refuse to give out any more references if the module owning the
+	 * record is being removed.
+	 */
+	if (!try_module_get(record->owner))
+		return NULL;
+
+	kref_get(&record->kref);
+
+	return record;
+}
+
+/**
+ * registry_record_unref - drop a reference to a registry record
+ * @record: record to unreference
+ *
+ * Decreases the reference count on the record. When the reference count
+ * reaches zero the record will be destroyed.
+ */
+void registry_record_unref(struct registry_record *record)
+{
+	if (record) {
+		/*
+		 * Keep a copy of the module owner since the record may
+		 * disappear during the kref_put().
+		 */
+		struct module *owner = record->owner;
+
+		kref_put(&record->kref, registry_record_release);
+		module_put(owner);
+	}
+}
+
+/**
+ * registry_add - add a record to a registry
+ * @registry: registry to add the record to
+ * @record: record to add
+ *
+ * Tries to increase the reference count of the module owning the registry. If
+ * successful adds the new record to the registry.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int registry_add(struct registry *registry, struct registry_record *record)
+{
+	if (!try_module_get(registry->owner))
+		return -ENODEV;
+
+	mutex_lock(&registry->lock);
+	list_add_tail(&record->list, &registry->list);
+	mutex_unlock(&registry->lock);
+
+	return 0;
+}
+
+/**
+ * registry_remove - remove a record from a registry
+ * @registry: registry to remove the record from
+ * @record: record to remove
+ *
+ * Decreases the reference count on the module owning the registry.
+ */
+void registry_remove(struct registry *registry,
+		     struct registry_record *record)
+{
+	mutex_lock(&registry->lock);
+	list_del_init(&record->list);
+	mutex_unlock(&registry->lock);
+
+	module_put(registry->owner);
+}
+
+#ifdef CONFIG_OF
+/**
+ * registry_find_by_of_node - look up an object by device node in a registry
+ * @registry: registry to search
+ * @np: device node to match on
+ *
+ * Return: A pointer to the record matching @np or NULL if no such record was
+ * found.
+ */
+struct registry_record *registry_find_by_of_node(struct registry *registry,
+						 struct device_node *np)
+{
+	struct registry_record *record;
+
+	mutex_lock(&registry->lock);
+
+	list_for_each_entry(record, &registry->list, list)
+		if (record->dev->of_node == np)
+			goto out;
+
+	record = NULL;
+
+out:
+	mutex_unlock(&registry->lock);
+	return record;
+}
+#endif
diff --git a/include/linux/registry.h b/include/linux/registry.h
new file mode 100644
index 000000000000..a807f4124736
--- /dev/null
+++ b/include/linux/registry.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2014, NVIDIA Corporation.  All rights reserved.
+ *
+ * This file is released under the GPL v2.
+ */
+
+#ifndef __LINUX_REGISTRY_H
+#define __LINUX_REGISTRY_H
+
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+struct device;
+struct device_node;
+struct module;
+
+struct registry;
+
+/**
+ * struct registry_record - registry record object
+ * @list: entry in registry for this record
+ * @owner: owner module
+ * @kref: reference count
+ * @dev: parent device
+ * @release: callback to destroy a record when no reference are left
+ */
+struct registry_record {
+	struct list_head list;
+	struct module *owner;
+	struct kref kref;
+	struct device *dev;
+
+	void (*release)(struct registry_record *record);
+};
+
+void registry_record_init(struct registry_record *record);
+struct registry_record *registry_record_ref(struct registry_record *record);
+void registry_record_unref(struct registry_record *record);
+
+/**
+ * struct registry - generic object registry
+ * @list: list head of objects
+ * @owner: owner module
+ * @lock: lock for object list
+ */
+struct registry {
+	struct list_head list;
+	struct module *owner;
+	struct mutex lock;
+};
+
+int registry_add(struct registry *registry, struct registry_record *record);
+void registry_remove(struct registry *registry,
+		     struct registry_record *record);
+
+#ifdef CONFIG_OF
+struct registry_record *registry_find_by_of_node(struct registry *registry,
+						 struct device_node *np);
+#endif
+
+#endif
-- 
2.1.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC 2/2] drm/panel: Use generic object registry
  2014-11-04 16:29 [RFC 1/2] core: Add generic object registry implementation Thierry Reding
@ 2014-11-04 16:29 ` Thierry Reding
  2014-11-04 16:38 ` [RFC 1/2] core: Add generic object registry implementation Greg Kroah-Hartman
  2014-11-05 12:36 ` Andrzej Hajda
  2 siblings, 0 replies; 13+ messages in thread
From: Thierry Reding @ 2014-11-04 16:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

From: Thierry Reding <treding@nvidia.com>

Convert to the new generic object registry and introduce proper object
and module reference counting. This should make panel registration and
removal a lot more robust.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_panel.c | 100 ++++++++++++++++++++++++++++++++++----------
 include/drm/drm_panel.h     |  13 ++++--
 2 files changed, 86 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 3dfe3c886502..c1b58bd421a3 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -23,13 +23,11 @@
 
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/registry.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
 
-static DEFINE_MUTEX(panel_lock);
-static LIST_HEAD(panel_list);
-
 /**
  * DOC: drm panel
  *
@@ -38,6 +36,33 @@ static LIST_HEAD(panel_list);
  * drivers.
  */
 
+/*
+ * DRM panel registry
+ */
+static struct registry panels = {
+	.lock = __MUTEX_INITIALIZER(panels.lock),
+	.list = LIST_HEAD_INIT(panels.list),
+	.owner = THIS_MODULE,
+};
+
+static inline struct drm_panel *to_drm_panel(struct registry_record *record)
+{
+	return container_of(record, struct drm_panel, record);
+}
+
+static void drm_panel_release(struct registry_record *record)
+{
+	struct drm_panel *panel = to_drm_panel(record);
+
+	/*
+	 * The .release() callback is optional because drivers may not need
+	 * to manually release any resources (e.g. if they've used devm_*()
+	 * helper functions).
+	 */
+	if (panel->funcs && panel->funcs->release)
+		panel->funcs->release(panel);
+}
+
 /**
  * drm_panel_init - initialize a panel
  * @panel: DRM panel
@@ -47,11 +72,48 @@ static LIST_HEAD(panel_list);
  */
 void drm_panel_init(struct drm_panel *panel)
 {
-	INIT_LIST_HEAD(&panel->list);
+	registry_record_init(&panel->record);
+	panel->record.release = drm_panel_release;
 }
 EXPORT_SYMBOL(drm_panel_init);
 
 /**
+ * drm_panel_ref - acquire a reference to a panel
+ * @panel: DRM panel
+ *
+ * Increases the reference on a panel and returns a pointer to it.
+ *
+ * Return: A reference to the panel on success or NULL on failure.
+ */
+struct drm_panel *drm_panel_ref(struct drm_panel *panel)
+{
+	if (panel) {
+		struct registry_record *record;
+
+		record = registry_record_ref(&panel->record);
+		if (!record)
+			panel = NULL;
+	}
+
+	return panel;
+}
+EXPORT_SYMBOL(drm_panel_ref);
+
+/**
+ * drm_panel_unref - release a reference to a panel
+ * @panel: DRM panel
+ *
+ * Decreases the reference count on a panel. If the reference count reaches 0
+ * the panel is destroyed.
+ */
+void drm_panel_unref(struct drm_panel *panel)
+{
+	if (panel)
+		registry_record_unref(&panel->record);
+}
+EXPORT_SYMBOL(drm_panel_unref);
+
+/**
  * drm_panel_add - add a panel to the global registry
  * @panel: panel to add
  *
@@ -62,11 +124,10 @@ EXPORT_SYMBOL(drm_panel_init);
  */
 int drm_panel_add(struct drm_panel *panel)
 {
-	mutex_lock(&panel_lock);
-	list_add_tail(&panel->list, &panel_list);
-	mutex_unlock(&panel_lock);
+	panel->record.owner = panel->dev->driver->owner;
+	panel->record.dev = panel->dev;
 
-	return 0;
+	return registry_add(&panels, &panel->record);
 }
 EXPORT_SYMBOL(drm_panel_add);
 
@@ -74,13 +135,12 @@ EXPORT_SYMBOL(drm_panel_add);
  * drm_panel_remove - remove a panel from the global registry
  * @panel: DRM panel
  *
- * Removes a panel from the global registry.
+ * Removes a panel from the global registry. References to the object can
+ * still exist, but drivers won't be able to look the panel up again.
  */
 void drm_panel_remove(struct drm_panel *panel)
 {
-	mutex_lock(&panel_lock);
-	list_del_init(&panel->list);
-	mutex_unlock(&panel_lock);
+	registry_remove(&panels, &panel->record);
 }
 EXPORT_SYMBOL(drm_panel_remove);
 
@@ -132,25 +192,19 @@ EXPORT_SYMBOL(drm_panel_detach);
  * @np: device tree node of the panel
  *
  * Searches the set of registered panels for one that matches the given device
- * tree node. If a matching panel is found, return a pointer to it.
+ * tree node. If a matching panel is found, return a reference to it.
  *
  * Return: A pointer to the panel registered for the specified device tree
  * node or NULL if no panel matching the device tree node can be found.
  */
 struct drm_panel *of_drm_find_panel(struct device_node *np)
 {
-	struct drm_panel *panel;
-
-	mutex_lock(&panel_lock);
+	struct registry_record *record;
 
-	list_for_each_entry(panel, &panel_list, list) {
-		if (panel->dev->of_node == np) {
-			mutex_unlock(&panel_lock);
-			return panel;
-		}
-	}
+	record = registry_find_by_of_node(&panels, np);
+	if (record)
+		return container_of(record, struct drm_panel, record);
 
-	mutex_unlock(&panel_lock);
 	return NULL;
 }
 EXPORT_SYMBOL(of_drm_find_panel);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index b88b4dcd698b..117e221dbc08 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -24,7 +24,8 @@
 #ifndef __DRM_PANEL_H__
 #define __DRM_PANEL_H__
 
-#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/registry.h>
 
 struct drm_connector;
 struct drm_device;
@@ -32,6 +33,7 @@ struct drm_panel;
 
 /**
  * struct drm_panel_funcs - perform operations on a given panel
+ * @release: called when the final reference is dropped
  * @disable: disable panel (turn off back light, etc.)
  * @unprepare: turn off panel
  * @prepare: turn on panel and perform set up
@@ -63,6 +65,7 @@ struct drm_panel;
  * the panel. This is the job of the .unprepare() function.
  */
 struct drm_panel_funcs {
+	void (*release)(struct drm_panel *panel);
 	int (*disable)(struct drm_panel *panel);
 	int (*unprepare)(struct drm_panel *panel);
 	int (*prepare)(struct drm_panel *panel);
@@ -72,20 +75,20 @@ struct drm_panel_funcs {
 
 /**
  * struct drm_panel - DRM panel object
+ * @record: registry record
  * @drm: DRM device owning the panel
  * @connector: DRM connector that the panel is attached to
  * @dev: parent device of the panel
  * @funcs: operations that can be performed on the panel
- * @list: panel entry in registry
  */
 struct drm_panel {
+	struct registry_record record;
+
 	struct drm_device *drm;
 	struct drm_connector *connector;
 	struct device *dev;
 
 	const struct drm_panel_funcs *funcs;
-
-	struct list_head list;
 };
 
 /**
@@ -180,6 +183,8 @@ static inline int drm_panel_get_modes(struct drm_panel *panel)
 }
 
 void drm_panel_init(struct drm_panel *panel);
+struct drm_panel *drm_panel_ref(struct drm_panel *panel);
+void drm_panel_unref(struct drm_panel *panel);
 
 int drm_panel_add(struct drm_panel *panel);
 void drm_panel_remove(struct drm_panel *panel);
-- 
2.1.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-04 16:29 [RFC 1/2] core: Add generic object registry implementation Thierry Reding
  2014-11-04 16:29 ` [RFC 2/2] drm/panel: Use generic object registry Thierry Reding
@ 2014-11-04 16:38 ` Greg Kroah-Hartman
  2014-11-05  9:13   ` Thierry Reding
  2014-11-05 12:36 ` Andrzej Hajda
  2 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2014-11-04 16:38 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Add a generic implementation of an object registry. This targets drivers
> and subsystems that provide auxiliary objects that other drivers need to
> look up. The goal is to put the difficult parts (keep object references,
> module usage count, ...) into core code so that individual subsystems do
> not have to deal with them.
> 
> The intention is for subsystems to instantiate a struct registry and use
> a struct registry_record embedded into a subsystem-specific structure to
> provide a subsystem-specific API around that.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/base/Makefile    |   2 +-
>  drivers/base/registry.c  | 147 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/registry.h |  62 ++++++++++++++++++++
>  3 files changed, 210 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/registry.c
>  create mode 100644 include/linux/registry.h
> 
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 53c3fe1aeb29..250262d1af2c 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
>  			   attribute_container.o transport_class.o \
> -			   topology.o container.o property.o
> +			   topology.o container.o property.o registry.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
>  obj-y			+= power/
> diff --git a/drivers/base/registry.c b/drivers/base/registry.c
> new file mode 100644
> index 000000000000..9f510f6237b7
> --- /dev/null
> +++ b/drivers/base/registry.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014, NVIDIA Corporation.  All rights reserved.
> + *
> + * This file is released under the GPL v2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/registry.h>
> +
> +static inline struct registry_record *to_registry_record(struct kref *kref)
> +{
> +	return container_of(kref, struct registry_record, kref);
> +}
> +
> +static void registry_record_release(struct kref *kref)
> +{
> +	struct registry_record *record = to_registry_record(kref);
> +
> +	record->release(record);
> +}
> +
> +/**
> + * registry_record_init - initialize a registry record
> + * @record: record to initialize
> + *
> + * Sets up internal fields of the registry record so that it can subsequently
> + * be added to a registry.
> + */
> +void registry_record_init(struct registry_record *record)
> +{
> +	INIT_LIST_HEAD(&record->list);
> +	kref_init(&record->kref);
> +}
> +
> +/**
> + * registry_record_ref - reference on the registry record
> + * @record: record to reference
> + *
> + * Increases the reference count on the record and returns a pointer to it.
> + *
> + * Return: A pointer to the record on success or NULL on failure.
> + */
> +struct registry_record *registry_record_ref(struct registry_record *record)
> +{
> +	if (!record)
> +		return NULL;
> +
> +	/*
> +	 * Refuse to give out any more references if the module owning the
> +	 * record is being removed.
> +	 */
> +	if (!try_module_get(record->owner))
> +		return NULL;
> +
> +	kref_get(&record->kref);

You "protect" from a module being removed, but not from someone else
releasing the kref at the same time.  Where is the lock that prevents
this from happening?

And do we really care about module reference counts here?

> +
> +	return record;
> +}
> +
> +/**
> + * registry_record_unref - drop a reference to a registry record
> + * @record: record to unreference
> + *
> + * Decreases the reference count on the record. When the reference count
> + * reaches zero the record will be destroyed.
> + */
> +void registry_record_unref(struct registry_record *record)
> +{
> +	if (record) {
> +		/*
> +		 * Keep a copy of the module owner since the record may
> +		 * disappear during the kref_put().
> +		 */
> +		struct module *owner = record->owner;
> +
> +		kref_put(&record->kref, registry_record_release);
> +		module_put(owner);
> +	}
> +}
> +
> +/**
> + * registry_add - add a record to a registry
> + * @registry: registry to add the record to
> + * @record: record to add
> + *
> + * Tries to increase the reference count of the module owning the registry. If
> + * successful adds the new record to the registry.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int registry_add(struct registry *registry, struct registry_record *record)
> +{
> +	if (!try_module_get(registry->owner))
> +		return -ENODEV;
> +
> +	mutex_lock(&registry->lock);
> +	list_add_tail(&record->list, &registry->list);
> +	mutex_unlock(&registry->lock);

No incrementing of the reference of the record at all?

You seem to be using 2 reference counts for the record / registry, a
module count, and a kref count, which can cause confusion...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-04 16:38 ` [RFC 1/2] core: Add generic object registry implementation Greg Kroah-Hartman
@ 2014-11-05  9:13   ` Thierry Reding
  2014-11-06  2:18     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2014-11-05  9:13 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

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

On Tue, Nov 04, 2014 at 08:38:45AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote:
[...]
> > diff --git a/drivers/base/registry.c b/drivers/base/registry.c
[...]
> > +/**
> > + * registry_record_ref - reference on the registry record
> > + * @record: record to reference
> > + *
> > + * Increases the reference count on the record and returns a pointer to it.
> > + *
> > + * Return: A pointer to the record on success or NULL on failure.
> > + */
> > +struct registry_record *registry_record_ref(struct registry_record *record)
> > +{
> > +	if (!record)
> > +		return NULL;
> > +
> > +	/*
> > +	 * Refuse to give out any more references if the module owning the
> > +	 * record is being removed.
> > +	 */
> > +	if (!try_module_get(record->owner))
> > +		return NULL;
> > +
> > +	kref_get(&record->kref);
> 
> You "protect" from a module being removed, but not from someone else
> releasing the kref at the same time.  Where is the lock that prevents
> this from happening?

You're right, we need a record lock to serialize ref/unref.

> And do we really care about module reference counts here?

We need this so that the code of the .release() callback stays in
memory as long as there are any references.

Also we need the module reference count for registries because they
would typically be statically allocated and go away along with a module
when it is unloaded.

> > +/**
> > + * registry_add - add a record to a registry
> > + * @registry: registry to add the record to
> > + * @record: record to add
> > + *
> > + * Tries to increase the reference count of the module owning the registry. If
> > + * successful adds the new record to the registry.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int registry_add(struct registry *registry, struct registry_record *record)
> > +{
> > +	if (!try_module_get(registry->owner))
> > +		return -ENODEV;
> > +
> > +	mutex_lock(&registry->lock);
> > +	list_add_tail(&record->list, &registry->list);
> > +	mutex_unlock(&registry->lock);
> 
> No incrementing of the reference of the record at all?

I'm not sure if we really need one. Drivers will have to remove the
record from the registry before dropping their reference. I guess we
could throw in another kref_get() here (and a kref_put() in
registry_remove()) for good measure to prevent breakage with buggy
drivers.

> You seem to be using 2 reference counts for the record / registry, a
> module count, and a kref count, which can cause confusion...

There is one reference count (kref actually) per registry record and one
module count for the record owner and the registry owner.

Can you elaborate what you think is confusing? Perhaps I can add more
kerneldoc to clarify.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-04 16:29 [RFC 1/2] core: Add generic object registry implementation Thierry Reding
  2014-11-04 16:29 ` [RFC 2/2] drm/panel: Use generic object registry Thierry Reding
  2014-11-04 16:38 ` [RFC 1/2] core: Add generic object registry implementation Greg Kroah-Hartman
@ 2014-11-05 12:36 ` Andrzej Hajda
  2014-11-05 14:04   ` Thierry Reding
  2 siblings, 1 reply; 13+ messages in thread
From: Andrzej Hajda @ 2014-11-05 12:36 UTC (permalink / raw)
  To: Thierry Reding, Greg Kroah-Hartman; +Cc: Daniel Vetter, linux-kernel, dri-devel

On 11/04/2014 05:29 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Add a generic implementation of an object registry. This targets drivers
> and subsystems that provide auxiliary objects that other drivers need to
> look up. The goal is to put the difficult parts (keep object references,
> module usage count, ...) into core code so that individual subsystems do
> not have to deal with them.
> 
> The intention is for subsystems to instantiate a struct registry and use
> a struct registry_record embedded into a subsystem-specific structure to
> provide a subsystem-specific API around that.


As I understand you want to use this registry for panels and bridges.
Could you explain the idea and describe example scenario when these
refcountings are useful. I guess it should be when panel attached to
drmdrv want to disappear.

Real lifetime of panel is limited by probe/remove callbacks of panel
driver, do you want to prolong it behind these limits?
Do you want to have zombie panels, without hardware they abstract? For
what purpose?
What do you want to do with panel ops? Do they need support both life
states?

Anyway implementation currently seems to be broken, you try to refcount
objects which are usually embedded in driver priv data, which disappears
during remove callback of the driver.

Regards
Andrzej

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/base/Makefile    |   2 +-
>  drivers/base/registry.c  | 147 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/registry.h |  62 ++++++++++++++++++++
>  3 files changed, 210 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/registry.c
>  create mode 100644 include/linux/registry.h
> 
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 53c3fe1aeb29..250262d1af2c 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
>  			   attribute_container.o transport_class.o \
> -			   topology.o container.o property.o
> +			   topology.o container.o property.o registry.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
>  obj-y			+= power/
> diff --git a/drivers/base/registry.c b/drivers/base/registry.c
> new file mode 100644
> index 000000000000..9f510f6237b7
> --- /dev/null
> +++ b/drivers/base/registry.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014, NVIDIA Corporation.  All rights reserved.
> + *
> + * This file is released under the GPL v2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/registry.h>
> +
> +static inline struct registry_record *to_registry_record(struct kref *kref)
> +{
> +	return container_of(kref, struct registry_record, kref);
> +}
> +
> +static void registry_record_release(struct kref *kref)
> +{
> +	struct registry_record *record = to_registry_record(kref);
> +
> +	record->release(record);
> +}
> +
> +/**
> + * registry_record_init - initialize a registry record
> + * @record: record to initialize
> + *
> + * Sets up internal fields of the registry record so that it can subsequently
> + * be added to a registry.
> + */
> +void registry_record_init(struct registry_record *record)
> +{
> +	INIT_LIST_HEAD(&record->list);
> +	kref_init(&record->kref);
> +}
> +
> +/**
> + * registry_record_ref - reference on the registry record
> + * @record: record to reference
> + *
> + * Increases the reference count on the record and returns a pointer to it.
> + *
> + * Return: A pointer to the record on success or NULL on failure.
> + */
> +struct registry_record *registry_record_ref(struct registry_record *record)
> +{
> +	if (!record)
> +		return NULL;
> +
> +	/*
> +	 * Refuse to give out any more references if the module owning the
> +	 * record is being removed.
> +	 */
> +	if (!try_module_get(record->owner))
> +		return NULL;
> +
> +	kref_get(&record->kref);
> +
> +	return record;
> +}
> +
> +/**
> + * registry_record_unref - drop a reference to a registry record
> + * @record: record to unreference
> + *
> + * Decreases the reference count on the record. When the reference count
> + * reaches zero the record will be destroyed.
> + */
> +void registry_record_unref(struct registry_record *record)
> +{
> +	if (record) {
> +		/*
> +		 * Keep a copy of the module owner since the record may
> +		 * disappear during the kref_put().
> +		 */
> +		struct module *owner = record->owner;
> +
> +		kref_put(&record->kref, registry_record_release);
> +		module_put(owner);
> +	}
> +}
> +
> +/**
> + * registry_add - add a record to a registry
> + * @registry: registry to add the record to
> + * @record: record to add
> + *
> + * Tries to increase the reference count of the module owning the registry. If
> + * successful adds the new record to the registry.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int registry_add(struct registry *registry, struct registry_record *record)
> +{
> +	if (!try_module_get(registry->owner))
> +		return -ENODEV;
> +
> +	mutex_lock(&registry->lock);
> +	list_add_tail(&record->list, &registry->list);
> +	mutex_unlock(&registry->lock);
> +
> +	return 0;
> +}
> +
> +/**
> + * registry_remove - remove a record from a registry
> + * @registry: registry to remove the record from
> + * @record: record to remove
> + *
> + * Decreases the reference count on the module owning the registry.
> + */
> +void registry_remove(struct registry *registry,
> +		     struct registry_record *record)
> +{
> +	mutex_lock(&registry->lock);
> +	list_del_init(&record->list);
> +	mutex_unlock(&registry->lock);
> +
> +	module_put(registry->owner);
> +}
> +
> +#ifdef CONFIG_OF
> +/**
> + * registry_find_by_of_node - look up an object by device node in a registry
> + * @registry: registry to search
> + * @np: device node to match on
> + *
> + * Return: A pointer to the record matching @np or NULL if no such record was
> + * found.
> + */
> +struct registry_record *registry_find_by_of_node(struct registry *registry,
> +						 struct device_node *np)
> +{
> +	struct registry_record *record;
> +
> +	mutex_lock(&registry->lock);
> +
> +	list_for_each_entry(record, &registry->list, list)
> +		if (record->dev->of_node == np)
> +			goto out;
> +
> +	record = NULL;
> +
> +out:
> +	mutex_unlock(&registry->lock);
> +	return record;
> +}
> +#endif
> diff --git a/include/linux/registry.h b/include/linux/registry.h
> new file mode 100644
> index 000000000000..a807f4124736
> --- /dev/null
> +++ b/include/linux/registry.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2014, NVIDIA Corporation.  All rights reserved.
> + *
> + * This file is released under the GPL v2.
> + */
> +
> +#ifndef __LINUX_REGISTRY_H
> +#define __LINUX_REGISTRY_H
> +
> +#include <linux/kref.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +struct device;
> +struct device_node;
> +struct module;
> +
> +struct registry;
> +
> +/**
> + * struct registry_record - registry record object
> + * @list: entry in registry for this record
> + * @owner: owner module
> + * @kref: reference count
> + * @dev: parent device
> + * @release: callback to destroy a record when no reference are left
> + */
> +struct registry_record {
> +	struct list_head list;
> +	struct module *owner;
> +	struct kref kref;
> +	struct device *dev;
> +
> +	void (*release)(struct registry_record *record);
> +};
> +
> +void registry_record_init(struct registry_record *record);
> +struct registry_record *registry_record_ref(struct registry_record *record);
> +void registry_record_unref(struct registry_record *record);
> +
> +/**
> + * struct registry - generic object registry
> + * @list: list head of objects
> + * @owner: owner module
> + * @lock: lock for object list
> + */
> +struct registry {
> +	struct list_head list;
> +	struct module *owner;
> +	struct mutex lock;
> +};
> +
> +int registry_add(struct registry *registry, struct registry_record *record);
> +void registry_remove(struct registry *registry,
> +		     struct registry_record *record);
> +
> +#ifdef CONFIG_OF
> +struct registry_record *registry_find_by_of_node(struct registry *registry,
> +						 struct device_node *np);
> +#endif
> +
> +#endif
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-05 12:36 ` Andrzej Hajda
@ 2014-11-05 14:04   ` Thierry Reding
  2014-11-05 16:00     ` Andrzej Hajda
  0 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2014-11-05 14:04 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Greg Kroah-Hartman, Daniel Vetter, David Herrmann, linux-kernel,
	dri-devel

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

On Wed, Nov 05, 2014 at 01:36:24PM +0100, Andrzej Hajda wrote:
> On 11/04/2014 05:29 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Add a generic implementation of an object registry. This targets drivers
> > and subsystems that provide auxiliary objects that other drivers need to
> > look up. The goal is to put the difficult parts (keep object references,
> > module usage count, ...) into core code so that individual subsystems do
> > not have to deal with them.
> > 
> > The intention is for subsystems to instantiate a struct registry and use
> > a struct registry_record embedded into a subsystem-specific structure to
> > provide a subsystem-specific API around that.
> 
> 
> As I understand you want to use this registry for panels and bridges.
> Could you explain the idea and describe example scenario when these
> refcountings are useful. I guess it should be when panel attached to
> drmdrv want to disappear.

Correct. When a panel driver is unloaded it frees memory associated with
the panel. The goal of this registry is for the panel object to stay
around until all references are gone.

> Real lifetime of panel is limited by probe/remove callbacks of panel
> driver, do you want to prolong it behind these limits?

Yes.

> Do you want to have zombie panels, without hardware they abstract? For
> what purpose?

So that display drivers don't try to access objects that have been
freed.

> What do you want to do with panel ops? Do they need support both life
> states?

That's a slightly separate issue for which David Herrmann had a solution
in mind. As I understand it, the idea would be for these objects to gain
revoke support. The goal is that once the underlying object disappears,
calling any of the operations involved would fail (with something like a
-ENODEV). It's a little more complicated than that because the device
could disappear right in the middle of such an operation, but that's
precisely what revoke should allow us to do. Readding David for
visibility.

> Anyway implementation currently seems to be broken,

DRM panels are currently completely broken. If you remove the driver for
a panel the display driver that uses this panel will simply crash the
next time it tries to do anything with the panel. This type of registry
is the first step in trying to fix this.

>                                                     you try to refcount
> objects which are usually embedded in driver priv data, which disappears
> during remove callback of the driver.

A second step in fixing this will be to convert drivers to no longer
free the panel but rather drop their reference to the panel that they've
registered. This will make sure that memory doesn't get freed until all
references are gone.

Note that this means that drivers will also need to be converted not to
use devm_* helpers since that conflicts with the reference counted life-
times of these record objects.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-05 14:04   ` Thierry Reding
@ 2014-11-05 16:00     ` Andrzej Hajda
  2014-11-06  9:48       ` Thierry Reding
  0 siblings, 1 reply; 13+ messages in thread
From: Andrzej Hajda @ 2014-11-05 16:00 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Greg Kroah-Hartman, linux-kernel, dri-devel, Daniel Vetter

On 11/05/2014 03:04 PM, Thierry Reding wrote:
> On Wed, Nov 05, 2014 at 01:36:24PM +0100, Andrzej Hajda wrote:
>> On 11/04/2014 05:29 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Add a generic implementation of an object registry. This targets drivers
>>> and subsystems that provide auxiliary objects that other drivers need to
>>> look up. The goal is to put the difficult parts (keep object references,
>>> module usage count, ...) into core code so that individual subsystems do
>>> not have to deal with them.
>>>
>>> The intention is for subsystems to instantiate a struct registry and use
>>> a struct registry_record embedded into a subsystem-specific structure to
>>> provide a subsystem-specific API around that.
>>
>>
>> As I understand you want to use this registry for panels and bridges.
>> Could you explain the idea and describe example scenario when these
>> refcountings are useful. I guess it should be when panel attached to
>> drmdrv want to disappear.
> 
> Correct. When a panel driver is unloaded it frees memory associated with
> the panel. The goal of this registry is for the panel object to stay
> around until all references are gone.
> 
>> Real lifetime of panel is limited by probe/remove callbacks of panel
>> driver, do you want to prolong it behind these limits?
> 
> Yes.
> 
>> Do you want to have zombie panels, without hardware they abstract? For
>> what purpose?
> 
> So that display drivers don't try to access objects that have been
> freed.

Why do not just release panel references from drm_dev, I have
successfully implemented dsi panels this way, thanks to dsi bus specific
attach/detach callbacks and drm hotplug mechansim.

My point is we do not need to make the whole tricky double refcounting,
with total redesign of panels, revoke, zombies, etc.... It is enough to
have just hot plug/unplug callbacks. This is why I have proposed few
months ago interface_tracker framework. It can add hot(un)plug
capability in a generic way to any framework.

Regards
Andrzej


> 
>> What do you want to do with panel ops? Do they need support both life
>> states?
> 
> That's a slightly separate issue for which David Herrmann had a solution
> in mind. As I understand it, the idea would be for these objects to gain
> revoke support. The goal is that once the underlying object disappears,
> calling any of the operations involved would fail (with something like a
> -ENODEV). It's a little more complicated than that because the device
> could disappear right in the middle of such an operation, but that's
> precisely what revoke should allow us to do. Readding David for
> visibility.
> 
>> Anyway implementation currently seems to be broken,
> 
> DRM panels are currently completely broken. If you remove the driver for
> a panel the display driver that uses this panel will simply crash the
> next time it tries to do anything with the panel. This type of registry
> is the first step in trying to fix this.
> 
>>                                                     you try to refcount
>> objects which are usually embedded in driver priv data, which disappears
>> during remove callback of the driver.
> 
> A second step in fixing this will be to convert drivers to no longer
> free the panel but rather drop their reference to the panel that they've
> registered. This will make sure that memory doesn't get freed until all
> references are gone.
> 
> Note that this means that drivers will also need to be converted not to
> use devm_* helpers since that conflicts with the reference counted life-
> times of these record objects.
> 
> Thierry
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-05  9:13   ` Thierry Reding
@ 2014-11-06  2:18     ` Greg Kroah-Hartman
  2014-11-06 10:25       ` Thierry Reding
  0 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2014-11-06  2:18 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

On Wed, Nov 05, 2014 at 10:13:53AM +0100, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 08:38:45AM -0800, Greg Kroah-Hartman wrote:
> > On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote:
> [...]
> > > diff --git a/drivers/base/registry.c b/drivers/base/registry.c
> [...]
> > > +/**
> > > + * registry_record_ref - reference on the registry record
> > > + * @record: record to reference
> > > + *
> > > + * Increases the reference count on the record and returns a pointer to it.
> > > + *
> > > + * Return: A pointer to the record on success or NULL on failure.
> > > + */
> > > +struct registry_record *registry_record_ref(struct registry_record *record)
> > > +{
> > > +	if (!record)
> > > +		return NULL;
> > > +
> > > +	/*
> > > +	 * Refuse to give out any more references if the module owning the
> > > +	 * record is being removed.
> > > +	 */
> > > +	if (!try_module_get(record->owner))
> > > +		return NULL;
> > > +
> > > +	kref_get(&record->kref);
> > 
> > You "protect" from a module being removed, but not from someone else
> > releasing the kref at the same time.  Where is the lock that prevents
> > this from happening?
> 
> You're right, we need a record lock to serialize ref/unref.
> 
> > And do we really care about module reference counts here?
> 
> We need this so that the code of the .release() callback stays in
> memory as long as there are any references.
> 
> Also we need the module reference count for registries because they
> would typically be statically allocated and go away along with a module
> when it is unloaded.

Never use a 'static' variable as a dynamic one with a kref, that's just
asking for trouble.

> > > +/**
> > > + * registry_add - add a record to a registry
> > > + * @registry: registry to add the record to
> > > + * @record: record to add
> > > + *
> > > + * Tries to increase the reference count of the module owning the registry. If
> > > + * successful adds the new record to the registry.
> > > + *
> > > + * Return: 0 on success or a negative error code on failure.
> > > + */
> > > +int registry_add(struct registry *registry, struct registry_record *record)
> > > +{
> > > +	if (!try_module_get(registry->owner))
> > > +		return -ENODEV;
> > > +
> > > +	mutex_lock(&registry->lock);
> > > +	list_add_tail(&record->list, &registry->list);
> > > +	mutex_unlock(&registry->lock);
> > 
> > No incrementing of the reference of the record at all?
> 
> I'm not sure if we really need one. Drivers will have to remove the
> record from the registry before dropping their reference. I guess we
> could throw in another kref_get() here (and a kref_put() in
> registry_remove()) for good measure to prevent breakage with buggy
> drivers.

Or at least warn people that they need to clean stuff up properly if
they do not, otherwise they will get it wrong.  You need to make it very
hard to get wrong.

> > You seem to be using 2 reference counts for the record / registry, a
> > module count, and a kref count, which can cause confusion...
> 
> There is one reference count (kref actually) per registry record and one
> module count for the record owner and the registry owner.

But both counts are in the same structure, which is a mess.

> Can you elaborate what you think is confusing? Perhaps I can add more
> kerneldoc to clarify.

You have 2 references in the same structure, with different lifecycles,
causing confusion, and odds are, bugs...

Sure, document it better if you want, but I think something needs to be
done differently if at all possible.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-05 16:00     ` Andrzej Hajda
@ 2014-11-06  9:48       ` Thierry Reding
  2014-11-07  9:10         ` Andrzej Hajda
  0 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2014-11-06  9:48 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: Greg Kroah-Hartman, linux-kernel, dri-devel, Daniel Vetter

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

On Wed, Nov 05, 2014 at 05:00:47PM +0100, Andrzej Hajda wrote:
> On 11/05/2014 03:04 PM, Thierry Reding wrote:
> > On Wed, Nov 05, 2014 at 01:36:24PM +0100, Andrzej Hajda wrote:
> >> On 11/04/2014 05:29 PM, Thierry Reding wrote:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> Add a generic implementation of an object registry. This targets drivers
> >>> and subsystems that provide auxiliary objects that other drivers need to
> >>> look up. The goal is to put the difficult parts (keep object references,
> >>> module usage count, ...) into core code so that individual subsystems do
> >>> not have to deal with them.
> >>>
> >>> The intention is for subsystems to instantiate a struct registry and use
> >>> a struct registry_record embedded into a subsystem-specific structure to
> >>> provide a subsystem-specific API around that.
> >>
> >>
> >> As I understand you want to use this registry for panels and bridges.
> >> Could you explain the idea and describe example scenario when these
> >> refcountings are useful. I guess it should be when panel attached to
> >> drmdrv want to disappear.
> > 
> > Correct. When a panel driver is unloaded it frees memory associated with
> > the panel. The goal of this registry is for the panel object to stay
> > around until all references are gone.
> > 
> >> Real lifetime of panel is limited by probe/remove callbacks of panel
> >> driver, do you want to prolong it behind these limits?
> > 
> > Yes.
> > 
> >> Do you want to have zombie panels, without hardware they abstract? For
> >> what purpose?
> > 
> > So that display drivers don't try to access objects that have been
> > freed.
> 
> Why do not just release panel references from drm_dev, I have
> successfully implemented dsi panels this way, thanks to dsi bus specific
> attach/detach callbacks and drm hotplug mechansim.

Like you say yourself, that's something that work only for DSI. Any
other type of panel can't do this.

> My point is we do not need to make the whole tricky double refcounting,

There's no double refcounting. We have no refcounting at all at the
moment.

> with total redesign of panels, revoke, zombies, etc.... It is enough to

It's not a total redesign. It just makes it more mature and implements
features that I think are useful (and needed) but that were left out for
the sake of simplicity. Now it turns out that this is actually quite
fragile and easy to get wrong.

> have just hot plug/unplug callbacks. This is why I have proposed few
> months ago interface_tracker framework. It can add hot(un)plug
> capability in a generic way to any framework.

That's something that this object registry could easily implement as
well. But instead of passing around void * and type IDs as in the
interface tracker it could deal with real objects for proper type-
safety.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-06  2:18     ` Greg Kroah-Hartman
@ 2014-11-06 10:25       ` Thierry Reding
  2014-11-06 16:13         ` Thierry Reding
  0 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2014-11-06 10:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

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

On Wed, Nov 05, 2014 at 06:18:15PM -0800, Greg Kroah-Hartman wrote:
> On Wed, Nov 05, 2014 at 10:13:53AM +0100, Thierry Reding wrote:
> > On Tue, Nov 04, 2014 at 08:38:45AM -0800, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote:
> > [...]
> > > > diff --git a/drivers/base/registry.c b/drivers/base/registry.c
> > [...]
> > > > +/**
> > > > + * registry_record_ref - reference on the registry record
> > > > + * @record: record to reference
> > > > + *
> > > > + * Increases the reference count on the record and returns a pointer to it.
> > > > + *
> > > > + * Return: A pointer to the record on success or NULL on failure.
> > > > + */
> > > > +struct registry_record *registry_record_ref(struct registry_record *record)
> > > > +{
> > > > +	if (!record)
> > > > +		return NULL;
> > > > +
> > > > +	/*
> > > > +	 * Refuse to give out any more references if the module owning the
> > > > +	 * record is being removed.
> > > > +	 */
> > > > +	if (!try_module_get(record->owner))
> > > > +		return NULL;
> > > > +
> > > > +	kref_get(&record->kref);
> > > 
> > > You "protect" from a module being removed, but not from someone else
> > > releasing the kref at the same time.  Where is the lock that prevents
> > > this from happening?
> > 
> > You're right, we need a record lock to serialize ref/unref.
> > 
> > > And do we really care about module reference counts here?
> > 
> > We need this so that the code of the .release() callback stays in
> > memory as long as there are any references.
> > 
> > Also we need the module reference count for registries because they
> > would typically be statically allocated and go away along with a module
> > when it is unloaded.
> 
> Never use a 'static' variable as a dynamic one with a kref, that's just
> asking for trouble.

The registry itself isn't reference counted, precisely because it is
meant to stay around as long as the subsystem stays around. It also has
the nice benefit that it can be statically initialized and therefore we
don't have to worry about initcall ordering or any of that jazz.

> > > > +/**
> > > > + * registry_add - add a record to a registry
> > > > + * @registry: registry to add the record to
> > > > + * @record: record to add
> > > > + *
> > > > + * Tries to increase the reference count of the module owning the registry. If
> > > > + * successful adds the new record to the registry.
> > > > + *
> > > > + * Return: 0 on success or a negative error code on failure.
> > > > + */
> > > > +int registry_add(struct registry *registry, struct registry_record *record)
> > > > +{
> > > > +	if (!try_module_get(registry->owner))
> > > > +		return -ENODEV;
> > > > +
> > > > +	mutex_lock(&registry->lock);
> > > > +	list_add_tail(&record->list, &registry->list);
> > > > +	mutex_unlock(&registry->lock);
> > > 
> > > No incrementing of the reference of the record at all?
> > 
> > I'm not sure if we really need one. Drivers will have to remove the
> > record from the registry before dropping their reference. I guess we
> > could throw in another kref_get() here (and a kref_put() in
> > registry_remove()) for good measure to prevent breakage with buggy
> > drivers.
> 
> Or at least warn people that they need to clean stuff up properly if
> they do not, otherwise they will get it wrong.  You need to make it very
> hard to get wrong.

Perhaps a WARN_ON() if a record is still in the registry when the last
reference is dropped would do the trick? Something like the following
perhaps?

	static void registry_record_release(struct kref *kref)
	{
		struct registry_record *record = to_registry_record(kref);

		/*
		 * Drivers must remove the device from the registry before dropping
		 * the last reference. Try to detect this by warning if a record's
		 * last reference goes away but it is still registered.
		 */
		if (WARN_ON(!list_empty(&record->list)))
			list_del_init(&record->list);

		record->release(record);
	}

Unfortunately we don't have a pointer to the registry around at this
point, so we can't do proper locking. In that case perhaps the WARN is
good enough. Alternatively we could keep a pointer to the registry in
each record.

One other alternative would be to not remove the entry at all. That will
likely cause a crash later on but in that case the WARN should be an
indication about what went wrong. Or maybe this should be BUG_ON?

> > > You seem to be using 2 reference counts for the record / registry, a
> > > module count, and a kref count, which can cause confusion...
> > 
> > There is one reference count (kref actually) per registry record and one
> > module count for the record owner and the registry owner.
> 
> But both counts are in the same structure, which is a mess.

The refcount makes sure that the data stays around, but the module count
is needed to keep the code around, which will be necessary because the
record owner will typically have code that's called when the last
reference goes away.

> > Can you elaborate what you think is confusing? Perhaps I can add more
> > kerneldoc to clarify.
> 
> You have 2 references in the same structure, with different lifecycles,
> causing confusion, and odds are, bugs...

The module reference count really belongs to the driver that creates the
records. We just keep a pointer to the module in the record since for
each record reference we need to make sure that we have one module
reference.

> Sure, document it better if you want, but I think something needs to be
> done differently if at all possible.

try_module_get() is the only way I know of that ensures that the code of
a module stays around. Everytime we give out a new reference to a record
we also need to increment the module reference count accordingly to make
sure the underlying code doesn't go away all of a sudden.

I guess that's not entirely accurate. The module reference count doesn't
have to be increment for every record reference, it only needs to track
each record. So the try_module_get() and module_put() could move into
registry_add() and registry_get(), respectively. But the ->owner field
would still be in the record structure.

Would that alleviate your concerns?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-06 10:25       ` Thierry Reding
@ 2014-11-06 16:13         ` Thierry Reding
  2014-11-07 16:31           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2014-11-06 16:13 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

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

On Thu, Nov 06, 2014 at 11:25:32AM +0100, Thierry Reding wrote:
> On Wed, Nov 05, 2014 at 06:18:15PM -0800, Greg Kroah-Hartman wrote:
[...]
> > Sure, document it better if you want, but I think something needs to be
> > done differently if at all possible.
> 
> try_module_get() is the only way I know of that ensures that the code of
> a module stays around. Everytime we give out a new reference to a record
> we also need to increment the module reference count accordingly to make
> sure the underlying code doesn't go away all of a sudden.
> 
> I guess that's not entirely accurate. The module reference count doesn't
> have to be increment for every record reference, it only needs to track
> each record. So the try_module_get() and module_put() could move into
> registry_add() and registry_get(), respectively. But the ->owner field
> would still be in the record structure.

On further thought I don't think this will work either. Given that the
record can be removed from the registry while somebody else still has a
reference to it, the module owning the record must stay around as long
as there's a reference to the record.

Maybe the module reference count needs to be incremented when the record
is initialized and decremented when the record is released.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-06  9:48       ` Thierry Reding
@ 2014-11-07  9:10         ` Andrzej Hajda
  0 siblings, 0 replies; 13+ messages in thread
From: Andrzej Hajda @ 2014-11-07  9:10 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Greg Kroah-Hartman, linux-kernel, dri-devel, Daniel Vetter

On 11/06/2014 10:48 AM, Thierry Reding wrote:
> On Wed, Nov 05, 2014 at 05:00:47PM +0100, Andrzej Hajda wrote:
>> On 11/05/2014 03:04 PM, Thierry Reding wrote:
>>> On Wed, Nov 05, 2014 at 01:36:24PM +0100, Andrzej Hajda wrote:
>>>> On 11/04/2014 05:29 PM, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Add a generic implementation of an object registry. This targets drivers
>>>>> and subsystems that provide auxiliary objects that other drivers need to
>>>>> look up. The goal is to put the difficult parts (keep object references,
>>>>> module usage count, ...) into core code so that individual subsystems do
>>>>> not have to deal with them.
>>>>>
>>>>> The intention is for subsystems to instantiate a struct registry and use
>>>>> a struct registry_record embedded into a subsystem-specific structure to
>>>>> provide a subsystem-specific API around that.
>>>>
>>>>
>>>> As I understand you want to use this registry for panels and bridges.
>>>> Could you explain the idea and describe example scenario when these
>>>> refcountings are useful. I guess it should be when panel attached to
>>>> drmdrv want to disappear.
>>>
>>> Correct. When a panel driver is unloaded it frees memory associated with
>>> the panel. The goal of this registry is for the panel object to stay
>>> around until all references are gone.
>>>
>>>> Real lifetime of panel is limited by probe/remove callbacks of panel
>>>> driver, do you want to prolong it behind these limits?
>>>
>>> Yes.
>>>
>>>> Do you want to have zombie panels, without hardware they abstract? For
>>>> what purpose?
>>>
>>> So that display drivers don't try to access objects that have been
>>> freed.
>>
>> Why do not just release panel references from drm_dev, I have
>> successfully implemented dsi panels this way, thanks to dsi bus specific
>> attach/detach callbacks and drm hotplug mechansim.
> 
> Like you say yourself, that's something that work only for DSI. Any
> other type of panel can't do this.

But it means that if we want to make panels safe we just need add
registration/deregistration notifications to panels, nothing more.


> 
>> My point is we do not need to make the whole tricky double refcounting,
> 
> There's no double refcounting. We have no refcounting at all at the
> moment.

For me registry_record.kref and try_module_get sounds like refcounting.

> 
>> with total redesign of panels, revoke, zombies, etc.... It is enough to
> 
> It's not a total redesign. It just makes it more mature and implements
> features that I think are useful (and needed) but that were left out for
> the sake of simplicity. Now it turns out that this is actually quite
> fragile and easy to get wrong.

And I try to convince you we can still keep simplicity and make it safe.

> 
>> have just hot plug/unplug callbacks. This is why I have proposed few
>> months ago interface_tracker framework. It can add hot(un)plug
>> capability in a generic way to any framework.
> 
> That's something that this object registry could easily implement as
> well. But instead of passing around void * and type IDs as in the
> interface tracker it could deal with real objects for proper type-
> safety.

It is not a problem to add type-safe helpers to interface tracker.

Regards
Andrzej

> 
> Thierry
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC 1/2] core: Add generic object registry implementation
  2014-11-06 16:13         ` Thierry Reding
@ 2014-11-07 16:31           ` Greg Kroah-Hartman
  0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2014-11-07 16:31 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, David Herrmann, dri-devel, linux-kernel

On Thu, Nov 06, 2014 at 05:13:24PM +0100, Thierry Reding wrote:
> On Thu, Nov 06, 2014 at 11:25:32AM +0100, Thierry Reding wrote:
> > On Wed, Nov 05, 2014 at 06:18:15PM -0800, Greg Kroah-Hartman wrote:
> [...]
> > > Sure, document it better if you want, but I think something needs to be
> > > done differently if at all possible.
> > 
> > try_module_get() is the only way I know of that ensures that the code of
> > a module stays around. Everytime we give out a new reference to a record
> > we also need to increment the module reference count accordingly to make
> > sure the underlying code doesn't go away all of a sudden.
> > 
> > I guess that's not entirely accurate. The module reference count doesn't
> > have to be increment for every record reference, it only needs to track
> > each record. So the try_module_get() and module_put() could move into
> > registry_add() and registry_get(), respectively. But the ->owner field
> > would still be in the record structure.
> 
> On further thought I don't think this will work either. Given that the
> record can be removed from the registry while somebody else still has a
> reference to it, the module owning the record must stay around as long
> as there's a reference to the record.
> 
> Maybe the module reference count needs to be incremented when the record
> is initialized and decremented when the record is released.

The module reference count will never work as it is racy (you can't have
the module itself do the incrementing, which is what is happening here).

I'd argue that the module reference isn't needed at all, because if the
module shuts down and wants to be removed, it will have properly cleaned
up all of its data structures already, right?  So why use it?

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-11-07 16:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-04 16:29 [RFC 1/2] core: Add generic object registry implementation Thierry Reding
2014-11-04 16:29 ` [RFC 2/2] drm/panel: Use generic object registry Thierry Reding
2014-11-04 16:38 ` [RFC 1/2] core: Add generic object registry implementation Greg Kroah-Hartman
2014-11-05  9:13   ` Thierry Reding
2014-11-06  2:18     ` Greg Kroah-Hartman
2014-11-06 10:25       ` Thierry Reding
2014-11-06 16:13         ` Thierry Reding
2014-11-07 16:31           ` Greg Kroah-Hartman
2014-11-05 12:36 ` Andrzej Hajda
2014-11-05 14:04   ` Thierry Reding
2014-11-05 16:00     ` Andrzej Hajda
2014-11-06  9:48       ` Thierry Reding
2014-11-07  9:10         ` Andrzej Hajda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).