Re: [RFC 1/2] core: Add generic object registry implementation

[Andrzej Hajda]

On 11/04/2014 05:29 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/