linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RFC 00/15] Resource tracking/allocation framework
@ 2014-12-10 15:48 Andrzej Hajda
  2014-12-10 15:48 ` [RFC 01/15] drivers/base: add track framework Andrzej Hajda
                   ` (15 more replies)
  0 siblings, 16 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

Many kernel frameworks that uses provider/consumer schema suffer from
few issues:
1. They have broken driver unbinding handling. When device driver of the provider
  is unbound from the device, its consumers stay with reference to non-existing
  objects, as a result we can observe oopses, crashes, hangs.
  Frameworks tries to handle it by using:
  - module refcounting (try_module_get/module_put) - as the name says it
    protects only from module unloading, not driver unbinding. Additionally
    it can cause circular dependencies of modules and inability to unload them.
  - ghost providers - when driver is unbound provider is replaced with phony one,
    which returns errors on every access. It also does not solve the issue -
    consumer device can hang if the hardware represented by the provider is off.
2. Consumer does not know when required resource becomes available. Currently it
  is solved by deferred probing. The main problem with this solution is that it
  delays deferred device probe to late_initcall, for some devices it is
  unacceptable/undesirable.
3. There are drivers which can work without specific resource, but if
  the resource becomes available/unavailable it can do some additional stuff.
  An example of such driver is DRM driver (more precisely drm_connector) -
  it can start without attached drm_panel, but if the panel becomes available it
  can react by generating HPD event and start using it. Delaying drm
  initialization due to lack of panel should be avoided - drm can still be
  usable without it, for example it can still use HDMI monitor.
  Currently it can be handled by periodic polling drm_panel framework for
  the panel, but it is rather an workaround, not a real solution.

The main problem with the 1st issue is that drivers cannot be protected from
unbinding. So there is no way to prevent removal of resources they provide.
The only solution I see is to provide notifications to consumers about incoming
removal of resources they are using, so they can react appropriately.
Symmetrically we can add notifications about appearance of specific resources,
this way the 2nd issue can be solved.
And finally with both notifications we can solve the 3rd issue.

In the 1st patch I propose generic framework providing such notifications, named
track - propositions for better name are welcome.
Its main feature is that callbacks are serialized but are not called under lock,
it allows two important things:
- avoid additional locks in the consumers to protect data access from different
  callbacks,
- call the framework from within the callback, so complex dependencies can be
  modelled this way, without worries about deadlocks due to framework.

The 2nd patch contains restrack framework which uses internally track framework
to track and automatically allocate different resources. In short it can replace
all resource allocations with single call + callback. Of course it is just
a bonus. The most important thing is that it solves all described issues:
- it can properly handle provider unbind/re-bind,
- it avoids late init due to deferred probing,
- it allows to track optional resources.

Simple example taken from restrack commit message:

    static int lcd_probe(...)
    {
        struct restrack *rtrack;

    (...initialization w/o resource allocation ...)

        rtrack = devm_restrack_register(dev, lcd_callback,
                regulator_bulk_restrack_desc(&ctx->supplies[0]),
                regulator_bulk_restrack_desc(&ctx->supplies[1]),
                clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
                clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
                phy_restrack_desc(&ctx->phy, "dsim"),
        );

        return PTR_ERR_OR_NULL(rtrack);
    }

    void lcd_callback(struct device *dev, int ret)
    {
        struct lcd_ctx *ctx = dev_get_drvdata(dev);

        if (ret == 0)
                drm_panel_add(&ctx->panel);
        else if (ret == -EPROBE_DEFER)
                drm_panel_remove(&ctx->panel);
        else
                dev_err(dev, "restrack error %d\n", ret);
    }

For other examples look at patches 11, 13, 15.

Patches 3,4,6,8,9 adds restrack support to various frameworks. Restrack support
is added only to resources exposed via Device Tree. Adding support for non-DT
resources should be also possible, but I guess it can be more complicated as
resource lookup mechanism is more fuzzy. Anyway I can work on it if necessary.
Moreover these patches in some cases adds redundant code for DT lookup,
this redundancy can be removed in final version of the patchset.
Also another frameworks may need similar patches.

Patch 11 converts exynos-dsi driver to restrack API. It solves issues of
provider unbind (1st issue) and late init due to deferred probing (2nd issue).

Patch 13 shows how to deal with optional resources (3rd issue).

Patch 15 is a simple example how to deal with resources depending on other
resources. In this particular case ld9040 lcd panel driver requires presence
of regulators and gpio prior to expose drm_panel resource.

Patches 5 and 7 adds helper functions for DT node lookup of resource providers.

Patches 10,12,14 are just fixes or cleanups.

The patchset is based on exynos-drm-next [1] due to fact I have developed it
on exynos platforms. I can rebase it on other branch, if necessary.

[1]: https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/log/?h=exynos-drm-next

Regards
Andrzej


Andrzej Hajda (15):
  drivers/base: add track framework
  drivers/base: add restrack framework
  drm/panel: add restrack support
  regulator: add restrack support
  gpio: move DT parsing code to separate functions
  gpio: add restrack support
  clk: add DT parsing function
  clk: add restrack support
  phy: add restrack support
  drm/exynos/dsi: simplify hotplug code
  drm/exynos/dsi: convert to restrack API
  drm/exynos/dpi: use common of_graph functions
  drm/exynos/dpi: convert to restrack API
  drm/panel/ld9040: do not power off panel on removal
  drm/panel/ld9040: convert to restrack API

 drivers/base/Makefile                    |   2 +-
 drivers/base/restrack.c                  | 344 +++++++++++++++++++++++++++++++
 drivers/base/track.c                     | 241 ++++++++++++++++++++++
 drivers/clk/clk.c                        |   6 +
 drivers/clk/clkdev.c                     |  97 +++++++++
 drivers/gpio/gpiolib-of.c                |  59 +++---
 drivers/gpio/gpiolib.c                   | 114 +++++++++-
 drivers/gpio/gpiolib.h                   |   4 +-
 drivers/gpu/drm/drm_panel.c              |  97 +++++++++
 drivers/gpu/drm/exynos/exynos_drm_dpi.c  | 193 +++++++----------
 drivers/gpu/drm/exynos/exynos_drm_dsi.c  | 113 +++++-----
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   7 +
 drivers/gpu/drm/panel/panel-ld9040.c     |  42 ++--
 drivers/phy/phy-core.c                   |  90 ++++++++
 drivers/regulator/core.c                 |  77 +++++++
 include/drm/drm_panel.h                  |   4 +
 include/linux/clk.h                      |   3 +
 include/linux/gpio/consumer.h            |   4 +
 include/linux/phy/phy.h                  |   3 +
 include/linux/regulator/consumer.h       |  10 +
 include/linux/restrack.h                 | 143 +++++++++++++
 include/linux/track.h                    | 148 +++++++++++++
 22 files changed, 1567 insertions(+), 234 deletions(-)
 create mode 100644 drivers/base/restrack.c
 create mode 100644 drivers/base/track.c
 create mode 100644 include/linux/restrack.h
 create mode 100644 include/linux/track.h

-- 
1.9.1

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

* [RFC 01/15] drivers/base: add track framework
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-12 16:36   ` Mark Brown
  2014-12-10 15:48 ` [RFC 02/15] drivers/base: add restrack framework Andrzej Hajda
                   ` (14 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

track is a generic framework for safe tracking presence of any kernel objects
having an id. There are no special requirements about type of object or its
id. Id shall be unique.
Typical usage of the framework by consumer looks as follow:
1. Consumer registers notifier callback for objects with given id.
2. If the object is present consumer is notified immediately, otherwise it
   will be notified immediately after object creation.
3. If the object is about to be removed notification is sent to the consumer
   just before removal.
4. When consumer do not need the object anymore it unregisters its notifier
   callback. If the object is present during notifier unregistration
   notification about removal of the object is sent to the consumer.

All notification callbacks are serialized so the consumer do not need use
additional mechanisms to prevent callback races. Consumer should assume that
object is valid only between up and down notification callbacks inclusive.

Framework usage by object provider is simple:
1. When object becomes available it notifies framework about it.
2. When object becomes unavailable it notifies framework about it.

Provider should take care of calling notifications synchronously in proper
order.

The framework does not uses locks during notification calls, so it is safe
to call framework functions from the callbacks. This feature allows to model
complex object dependencies without deadlock risk.

Some framework functions can sleep so the framework should be used in process
context.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/Makefile |   2 +-
 drivers/base/track.c  | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/track.h | 148 +++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/track.c
 create mode 100644 include/linux/track.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 6922cd6..4edff7d 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
+			   topology.o container.o track.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/track.c b/drivers/base/track.c
new file mode 100644
index 0000000..8cf0492
--- /dev/null
+++ b/drivers/base/track.c
@@ -0,0 +1,241 @@
+/*
+ * Generic framework for tracking named object
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ * Andrzej Hajda <a.hajda@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * track is a generic framework for safe tracking presence of any kernel objects
+ * having an id. There are no special requirements about type of object or its
+ * id. Id shall be unique.
+ * Typical usage of the framework by consumer looks as follow:
+ * 1. Consumer registers notifier callback for objects with given id.
+ * 2. If the object is present consumer is notified immediately, otherwise it
+ *    will be notified immediately after object creation.
+ * 3. If the object is about to be removed notification is sent to the consumer
+ *    just before removal.
+ * 4. When consumer do not need the object anymore it unregisters its notifier
+ *    callback. If the object is present during notifier unregistration
+ *    notification about removal of the object is sent to the consumer.
+ *
+ * All notification callbacks are serialized so the consumer do not need use
+ * additional mechanisms to prevent callback races. Consumer should assume that
+ * object is valid only between up and down notification callbacks inclusive.
+ *
+ * Framework usage by object provider is simple:
+ * 1. When object becomes available it notifies framework about it.
+ * 2. When object becomes unavailable it notifies framework about it.
+ *
+ * Provider should take care of calling notifications synchronously in proper
+ * order.
+ *
+ * The framework does not uses locks during notification calls, so it is safe
+ * to call framework functions from the callbacks. This feature allows to model
+ * complex object dependencies without deadlock risk.
+ *
+ * Some framework functions can sleep so the framework should be used in process
+ * context.
+ *
+ */
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/track.h>
+
+struct track_task {
+	struct list_head list;
+	enum track_task_id task_id;
+
+	unsigned long type;
+	const void *id;
+	union {
+		void *data;
+		struct track_block *itb;
+	};
+};
+
+struct track_node {
+	struct list_head list;
+	struct list_head itb_head;
+	struct track_block *itb_next;
+	const void *id;
+	unsigned long type;
+	void *data;
+	bool up;
+};
+
+static struct track_node *
+track_get_node(struct tracker *track, unsigned long type, const void *id,
+		bool create)
+{
+	struct track_node *node;
+
+	list_for_each_entry(node, &track->nodes, list) {
+		if (node->type == type && node->id == id)
+			return node;
+	}
+
+	if (!create)
+		return NULL;
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return NULL;
+
+	node->id = id;
+	node->type = type;
+	node->up = false;
+	INIT_LIST_HEAD(&node->itb_head);
+	list_add(&node->list, &track->nodes);
+
+	return node;
+}
+
+static void track_process_task(struct tracker *track, struct track_task *task)
+{
+	struct track_block *itb;
+	struct track_node *node;
+
+	switch (task->task_id) {
+	case track_task_register:
+		node = track_get_node(track, task->type, task->id, true);
+		list_add_tail(&task->itb->list, &node->itb_head);
+
+		if (node->up)
+			task->itb->callback(task->itb, node->data, true);
+		return;
+	case track_task_unregister:
+		node = track_get_node(track, task->type, task->id, false);
+		if (WARN_ON(!node))
+			return;
+
+		if (node->up)
+			task->itb->callback(task->itb, node->data, false);
+
+		if (node->itb_next == task->itb)
+			node->itb_next = list_next_entry(node->itb_next, list);
+		list_del(&task->itb->list);
+
+		if (list_empty(&node->itb_head) && !node->up) {
+			list_del(&node->list);
+			kfree(node);
+		}
+		return;
+	case track_task_up:
+		node = track_get_node(track, task->type, task->id, true);
+
+		node->up = true;
+		node->data = task->data;
+		list_for_each_entry_safe(itb, node->itb_next, &node->itb_head,
+					 list)
+			itb->callback(itb, node->data, true);
+		return;
+	case track_task_down:
+		node = track_get_node(track, task->type, task->id, false);
+
+		if (WARN_ON(!node))
+			return;
+
+		WARN_ON(!node->up);
+
+		if (list_empty(&node->itb_head)) {
+			list_del(&node->list);
+			kfree(node);
+			return;
+		}
+
+		node->up = false;
+		node->data = task->data;
+
+		list_for_each_entry_safe(itb, node->itb_next, &node->itb_head,
+					 list)
+			itb->callback(itb, node->data, false);
+	}
+}
+
+static int track_process_queue(struct tracker *track)
+{
+	struct track_task *task, *ptask = NULL;
+	unsigned long flags;
+	bool empty;
+
+	/* Queue non-emptiness is used as a sentinel to prevent processing
+	 * by multiple threads, so we cannot delete entry from the queue
+	 * until it is processed.
+	 */
+	while (true) {
+		spin_lock_irqsave(&track->queue_lock, flags);
+
+		if (ptask)
+			list_del(&ptask->list);
+		task = list_first_entry(&track->queue,
+					struct track_task, list);
+
+		empty = list_empty(&track->queue);
+		if (empty)
+			complete_all(&track->queue_empty);
+
+		spin_unlock_irqrestore(&track->queue_lock, flags);
+
+		kfree(ptask);
+
+		if (empty)
+			break;
+
+		track_process_task(track, task);
+		ptask = task;
+	}
+
+	return 0;
+}
+
+int track_add_task(struct tracker *track, enum track_task_id task_id,
+		   unsigned long type, const void *id, void *data, bool sync)
+{
+	struct track_task *task;
+	unsigned long flags;
+	bool empty, owner;
+
+	task = kmalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+
+	task->task_id = task_id;
+	task->id = id;
+	task->type = type;
+	task->data = data;
+
+	spin_lock_irqsave(&track->queue_lock, flags);
+
+	empty = list_empty(&track->queue);
+	if (empty) {
+		reinit_completion(&track->queue_empty);
+		track->queue_owner = current;
+	} else {
+		owner = (track->queue_owner == current);
+	}
+
+	if (empty || !owner)
+		list_add(&task->list, &track->queue);
+
+	spin_unlock_irqrestore(&track->queue_lock, flags);
+
+	pr_debug("%s:%d: task=%c type=%ld id=%p\n", __func__, __LINE__,
+		 "ru+-"[task_id], type, id);
+
+	if (empty)
+		return track_process_queue(track);
+
+	if (owner) {
+		track_process_task(track, task);
+		kfree(task);
+		return 0;
+	}
+
+	if (sync)
+		wait_for_completion(&track->queue_empty);
+
+	return 0;
+}
diff --git a/include/linux/track.h b/include/linux/track.h
new file mode 100644
index 0000000..b1eed60
--- /dev/null
+++ b/include/linux/track.h
@@ -0,0 +1,148 @@
+#ifndef TRACK_H
+#define TRACK_H
+
+#include <linux/completion.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+struct list_head;
+struct track_block;
+struct tracker;
+
+typedef void (*track_fn_t)(struct track_block *itb, void *data, bool on);
+
+/**
+ * struct track_block - structure used by track consumers to register callback
+ * @callback: Pointer to notification function provided by consumer
+ * @list: field used by track core to chain blocks
+ *
+ * struct track_block is passed to the framework during callback registration,
+ * it should be valid until callback unregistration.
+ * Usually it can be embedded in drivers private data, so callback function can
+ * get pointer to private data using container_of mechanism.
+ *
+ */
+struct track_block {
+	track_fn_t callback;
+	struct list_head list;
+};
+
+enum track_task_id {
+	track_task_register,
+	track_task_unregister,
+	track_task_up,
+	track_task_down,
+};
+
+/**
+ * struct tracker - the main structure for tracking objects
+ * @queue: queue of tasks to do
+ * @queue_lock: lock protecting @queue access
+ * @queue_empty: completion fired when queue becomes empty
+ * @queue_owner: id of task processing queue
+ * @nodes: list head of track_nodes
+ *
+ * struct tracker - is the main structure used to track and synchronize
+ * framework functions and callbacks. There can be many independent trackers in
+ * the system.
+ */
+struct tracker {
+	struct list_head queue;
+	spinlock_t queue_lock;
+	struct completion queue_empty;
+	struct task_struct *queue_owner;
+	struct list_head nodes;
+};
+
+/**
+ * DEFINE_TRACKER - define the main structure for object tracking
+ * @x: name of the structure
+ *
+ * This macro defines and initializes tracker structure.
+ */
+#define DEFINE_TRACKER(x) \
+	struct tracker x = { \
+		.queue = LIST_HEAD_INIT(x.queue), \
+		.queue_lock = __SPIN_LOCK_UNLOCKED(x.queue_lock), \
+		.queue_empty = COMPLETION_INITIALIZER(x.queue_empty), \
+		.nodes = LIST_HEAD_INIT(x.nodes), \
+	}
+
+/* internal function, exposed only to allow inline public functions */
+extern int track_add_task(struct tracker *track, enum track_task_id task_id,
+		unsigned long type, const void *id, void *data, bool sync);
+
+/**
+ * track_register - register track callback
+ * @track: pointer to tracker main structure
+ * @itb: block containing registered callback
+ * @type: tracked object type, defined by user
+ * @id: object id
+ *
+ * track_register registers callback which will receive notifications about
+ * presence of object identified by pair (@type, @id). If during registration
+ * tracked object is present, notification callback will be sent immediately.
+ */
+static inline int track_register(struct tracker *track, struct track_block *itb,
+		unsigned long type, const void *id)
+{
+	return track_add_task(track, track_task_register, type, id, itb,
+				false);
+}
+
+/**
+ * track_unregister - unregister track callback
+ * @track: pointer to tracker main structure
+ * @itb: block containing registered callback
+ * @type: tracked object type
+ * @id: object id
+ *
+ * track_unregister unregisters previously registered callback. If during
+ * unregistration tracked object is present, notification is send immediately.
+ */
+static inline int track_unregister(struct tracker *track,
+		struct track_block *itb, unsigned long type, const void *id)
+{
+	return track_add_task(track, track_task_unregister, type, id, itb,
+				true);
+}
+
+/**
+ * track_up - notifies about object appearance
+ * @track: pointer to tracker main structure
+ * @type: tracked object type
+ * @id: object id
+ * @data: optional data pointer, usually it should be object pointer
+ *
+ * track_up sends notification about object appearance to all callbacks
+ * registered to track this object. It should be called after the object is
+ * fully created. It is up to provider to synchronize calls to track_up and
+ * track_down for given object.
+ */
+static inline int track_up(struct tracker *track, unsigned long type,
+		const void *id, void *data)
+{
+	return track_add_task(track, track_task_up, type, id, data, false);
+}
+
+/**
+ * track_down - notifies about object disappearance
+ * @track: pointer to tracker main structure
+ * @type: tracked object type
+ * @id: object id
+ * @data: optional data pointer, usually it should be object pointer
+ *
+ * track_up sends notification about object disappearance to all callbacks
+ * registered to track this object. It should be called before the object is
+ * destroyed. It is up to provider to synchronize calls to track_up and
+ * track_down for given object.
+ */
+static inline int track_down(struct tracker *track, unsigned long type,
+		const void *id, void *data)
+{
+	return track_add_task(track, track_task_down, type, id, data,
+				true);
+}
+
+#endif /* TRACK_H */
-- 
1.9.1

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

* [RFC 02/15] drivers/base: add restrack framework
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
  2014-12-10 15:48 ` [RFC 01/15] drivers/base: add track framework Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-12 16:52   ` Mark Brown
  2014-12-10 15:48 ` [RFC 03/15] drm/panel: add restrack support Andrzej Hajda
                   ` (13 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

restrack framework allows tracking presence of resources with dynamic life
time. Typical example of such resources are all resources provided by device
drivers, for example clocks, phys, regulators. Since drivers can be bound and
unbound dynamically and unconditionally, resources provided by such drivers
can appear and disappear at any time.

To use restrack in consumer user should call one of *restrack_register
functions. In the function arguments consumer should provide callback and
description of resources which should be tracked. Each resource description
should contain pointer to variable where restrack should store address of
allocated resource and parameters describing specific resource, for example
in case of clock it should be clock name and in case of gpio it should be
gpio name and flags.
The callback should have two arguments:
- dev - device for which callback have been registered,
- ret - return code.
If callback is called with ret == 0 it means all tracked resources are
present, allocated and provided resource pointers are set accordingly.
In case ret == -EPROBE_DEFER it means all resources are present but at least
one of the resources are to be removed after return form the callback.

Simplified example of framework usage on LCD panel driver.

static int lcd_probe(...)
{
	struct restrack *rtrack;

	(...initialization w/o resource allocation ...)

	rtrack = devm_restrack_register(dev, lcd_callback,
		regulator_bulk_restrack_desc(&ctx->supplies[0]),
		regulator_bulk_restrack_desc(&ctx->supplies[1]),
		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
		phy_restrack_desc(&ctx->phy, "dsim"),
	);

	return PTR_ERR_OR_NULL(rtrack);
}

void lcd_callback(struct device *dev, int ret)
{
	struct lcd_ctx *ctx = dev_get_drvdata(dev);

	if (ret == 0)
		drm_panel_add(&ctx->panel);
	else if (ret == -EPROBE_DEFER)
		drm_panel_remove(&ctx->panel);
	else
		dev_err(dev, "restrack error %d\n", ret);
}

Please note few things:
1. drm_panel_add calls restrack_up and drm_panel_remove calls restrack_down.
   It is OK to call restrack framework from the callback.
2. In lcd_callback if ret is 0 or -EDEFER_PROBE all resources are valid, ie
   driver for example can call clk_prepare_enable(ctx->pll_clk).
3. No mutexes are needed to protect lcd_ctx in lcd_callback call.
4. All resources are freed by restrack_unregister, which in this case is
   called by devres framework.

To add restrack support to specific framework following things should be
defined:
- structure describing resource with embedded restrack_desc structure,
- at least one exported allocator of such structure,
- few simple operations according to description of struct restrack_ops,
- notifications about adding/removal of the resource.
For details please look at implementations.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/Makefile    |   2 +-
 drivers/base/restrack.c  | 344 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/restrack.h | 137 +++++++++++++++++++
 3 files changed, 482 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/restrack.c
 create mode 100644 include/linux/restrack.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4edff7d..cf9a21e 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 track.o
+			   topology.o container.o track.o restrack.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/restrack.c b/drivers/base/restrack.c
new file mode 100644
index 0000000..e16d8ed
--- /dev/null
+++ b/drivers/base/restrack.c
@@ -0,0 +1,344 @@
+/*
+ * Resource tracking framework
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ * Andrzej Hajda <a.hajda@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * restrack framework allows to track presence of resources with dynamic life
+ * time. Typical example of such resources are all resources provided by device
+ * drivers, for example clocks, phys, regulators. Since drivers can be bound and
+ * unbound dynamically and unconditionally, resources provided by such drivers
+ * can appear and disappear at any time.
+ *
+ * To use restrack in consumer user should call one of *restrack_register
+ * functions. In the function arguments consumer should provide callback and
+ * description of resources which should be tracked. Each resource description
+ * should contain pointer to variable where restrack should store address of
+ * allocated resource and parameters describing specific resource, for example
+ * in case of clock it should be clock name and in case of gpio it should be
+ * gpio name and flags.
+ * The callback should have two arguments:
+ * - dev - device for which callback have been registered,
+ * - ret - return code.
+ * If callback is called with ret == 0 it means all tracked resources are
+ * present, allocated and provided resource pointers are set accordingly.
+ * In case ret == -EPROBE_DEFER it means all resources are present but at least
+ * one of the resources are to be removed after return form the callback.
+ *
+ * Simplified example of framework usage on LCD panel driver.
+ *
+ * static int lcd_probe(...)
+ * {
+ *	struct restrack *rtrack;
+ *
+ *	(...initialization w/o resource allocation ...)
+ *
+ *	rtrack = devm_restrack_register(dev, lcd_callback,
+ *		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+ *		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+ *		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
+ *		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
+ *		phy_restrack_desc(&ctx->phy, "dsim"),
+ *	);
+ *
+ *	return PTR_ERR_OR_NULL(rtrack);
+ * }
+ *
+ * void lcd_callback(struct device *dev, int ret)
+ * {
+ *	struct lcd_ctx *ctx = dev_get_drvdata(dev);
+ *
+ *	if (ret == 0)
+ *		drm_panel_add(&ctx->panel);
+ *	else if (ret == -EPROBE_DEFER)
+ *		drm_panel_remove(&ctx->panel);
+ *	else
+ *		dev_err(dev, "restrack error %d\n", ret);
+ * }
+ *
+ * Please note few things:
+ * 1. drm_panel_add calls restrack_up and drm_panel_remove calls restrack_down.
+ *    It is OK to call restrack framework from the callback.
+ * 2. In lcd_callback if ret is 0 or -EDEFER_PROBE all resources are valid, ie
+ *    driver for example can call clk_prepare_enable(ctx->pll_clk).
+ * 3. No mutexes are needed to protect lcd_ctx in lcd_callback call.
+ * 4. All resources are freed by restrack_unregister, which in this case is
+ *    called by devres framework.
+ *
+ * To add restrack support to specific framework following things should be
+ * defined:
+ * - structure describing resource with embedded restrack_desc structure,
+ * - at least one exported allocator of such structure,
+ * - few simple operations according to description of struct restrack_ops,
+ * - notifications about adding/removal of the resource.
+ * For details please look at existing implementations.
+ *
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/restrack.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/track.h>
+
+static DEFINE_TRACKER(restrack);
+
+struct restrack_ctx {
+	struct device *dev;
+	unsigned int count;
+	unsigned int get_count;
+	restrack_fn_t callback;
+	struct restrack_desc *desc[0];
+};
+
+/**
+ * restrack_up - notify that resource provider is up
+ * @type: resource type
+ * @id: resource id
+ * @data: @type dependent cookie, usually pointer to resource provider
+ */
+int restrack_up(unsigned long type, const void *id, void *data)
+{
+	return track_up(&restrack, type, id, data);
+}
+
+/**
+ * restrack_down - notify that resource provider is down
+ * @type: resource type
+ * @id: resource id
+ * @data: @type dependent cookie, usually pointer to resource provider
+ */
+int restrack_down(unsigned long type, const void *id, void *data)
+{
+	return track_down(&restrack, type, id, data);
+}
+
+static void restrack_itb_cb(struct track_block *itb, void *data, bool on)
+{
+	struct restrack_desc *desc;
+	struct restrack_ctx *ctx;
+	int i;
+
+	desc = container_of(itb, struct restrack_desc, itb);
+	ctx = desc->ctx;
+
+	for (i = 0; i < ctx->count; ++i) {
+		struct restrack_desc *d = ctx->desc[i];
+
+		if ((d->if_id != desc->if_id) ||
+		    (d->ops->if_type != desc->ops->if_type))
+			continue;
+
+		pr_debug("%s:%d: if_type=%ld on=%d res=%d/%d\n", __func__,
+			 __LINE__, d->ops->if_type, on, i, ctx->count);
+
+		if (on) {
+			if (!d->status)
+				continue;
+
+			d->status = d->ops->if_up(ctx->dev, d, data);
+
+			if (!d->status)
+				++ctx->get_count;
+
+			if (d->status == -EPROBE_DEFER)
+				continue;
+
+			if (d->status || ctx->get_count == ctx->count)
+				ctx->callback(ctx->dev, d->status);
+		} else {
+			if (!d->status) {
+				if (ctx->get_count-- == ctx->count)
+					ctx->callback(ctx->dev, -EPROBE_DEFER);
+				d->ops->if_down(ctx->dev, d, data);
+			}
+			d->status = -EPROBE_DEFER;
+		}
+	}
+}
+
+/* check if the same interface is present in previous restrack descriptors */
+static bool restrack_find_prev_if(struct restrack_ctx *ctx, int n)
+{
+	struct restrack_desc *desc = ctx->desc[n];
+	int i;
+
+	for (i = 0; i < n; ++i) {
+		struct restrack_desc *d = ctx->desc[i];
+
+		if (d->if_id == desc->if_id &&
+		    d->ops->if_type == desc->ops->if_type)
+			return true;
+	}
+
+	return false;
+}
+
+/**
+ * restrack_unregister - unregister restrack callback
+ * @ctx: restrack context, previously allocated by __restrack_register
+ */
+
+void restrack_unregister(struct restrack_ctx *ctx)
+{
+	int i;
+
+	i = ctx->count;
+	while (i-- > 0) {
+		struct restrack_desc *desc = ctx->desc[i];
+
+		if (IS_ERR(desc))
+			continue;
+
+		if (restrack_find_prev_if(ctx, i))
+			continue;
+
+		track_unregister(&restrack, &desc->itb, desc->ops->if_type,
+				 desc->if_id);
+	}
+
+	i = ctx->count;
+	while (i-- > 0) {
+		struct restrack_desc *desc = ctx->desc[i];
+
+		if (IS_ERR(desc))
+			continue;
+
+		if (desc->ops->destroy)
+			desc->ops->destroy(ctx->dev, desc);
+		else
+			kfree(desc);
+	}
+
+	kfree(ctx);
+}
+
+/**
+ * __restrack_register - register resource tracker callback
+ * @dev: consumer device
+ * @callback: callback which will be called when:
+ *	- all tracked resources become available, with @ret = 0
+ *	- one of the resources becomes unavailable, with @ret = -EPROBE_DEFER,
+ *	- resource allocation errors, with @ret equal to this error
+ * @count: number of resource descriptors to track
+ * @descs: list of pointers to resource descriptors to track
+ *
+ * It can be called as follows:
+ *	struct restrack_desc *descriptors[] = {
+ *		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+ *		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+ *		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
+ *		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
+ *		phy_restrack_desc(&ctx->phy, "dsim"),
+ *	};
+ *	rtrack = __restrack_register(dev, callback,
+ *			ARRAY_SIZE(descriptors), descriptors);
+ */
+struct restrack_ctx *__restrack_register(struct device *dev, restrack_fn_t cb,
+		int count, struct restrack_desc **descriptors)
+{
+	struct restrack_ctx *ctx;
+	int ret, i;
+
+	ctx = kzalloc(sizeof(*ctx) + count * sizeof(ctx->desc[0]),
+			 GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+
+	ctx->dev = dev;
+	ctx->count = count;
+	ctx->callback = cb;
+	memcpy(ctx->desc, descriptors, count * sizeof(*descriptors));
+
+	for (i = 0; i < count; ++i) {
+		struct restrack_desc *desc = descriptors[i];
+
+		if (IS_ERR(desc)) {
+			ret = PTR_ERR(desc);
+			goto err_free;
+		}
+		INIT_LIST_HEAD(&desc->itb.list);
+		desc->itb.callback = restrack_itb_cb;
+		desc->ctx = ctx;
+		desc->status = -EPROBE_DEFER;
+		ret = desc->ops->init(dev, desc);
+		if (ret)
+			goto err_free;
+	}
+
+	/* Callbacks should be registered after all fields are initialized,
+	 * otherwise callback could access partially initialized data. */
+	for (i = 0; i < count; ++i) {
+		struct restrack_desc *desc = descriptors[i];
+		const struct restrack_ops *ops = desc->ops;
+
+		/* do not track the same interface twice */
+		if (restrack_find_prev_if(ctx, i))
+			continue;
+
+		ret = track_register(&restrack, &desc->itb, ops->if_type,
+				     desc->if_id);
+		if (ret)
+			goto err_free;
+	}
+
+	return ctx;
+
+err_free:
+	restrack_unregister(ctx);
+
+	return ERR_PTR(ret);
+}
+
+static void devm_restrack_release(struct device *dev, void *res)
+{
+	struct restrack_ctx **ptr = res;
+
+	if (!IS_ERR(*ptr))
+		restrack_unregister(*ptr);
+}
+
+/**
+ * __devm_restrack_register - devm version of __restrack_register
+ * @dev: consumer device
+ * @callback: callback which will be called when:
+ *	- all tracked resources become available, with @ret = 0
+ *	- one of the resources becomes unavailable, with @ret = -EPROBE_DEFER,
+ *	- resource allocation errors, with @ret equal to this error
+ * @count: number of resource descriptors to track
+ * @descs: list of pointers to resource descriptors to track
+ *
+ * It can be called as follows:
+ *	struct restrack_desc *descriptors[] = {
+ *		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+ *		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+ *		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
+ *		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
+ *		phy_restrack_desc(&ctx->phy, "dsim"),
+ *	};
+ *	rtrack = __devm_restrack_register(dev, callback,
+ *			ARRAY_SIZE(descriptors), descriptors);
+ */
+struct restrack_ctx *__devm_restrack_register(struct device *dev,
+		restrack_fn_t cb, int count, struct restrack_desc **descs)
+{
+	struct restrack_ctx **ptr;
+
+	ptr = devres_alloc(devm_restrack_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	/* It should be added before __restrack_register to keep correct order
+	 * of resources - restrack callback can also use devres framework.
+	 */
+	devres_add(dev, ptr);
+
+	*ptr = __restrack_register(dev, cb, count, descs);
+
+	return *ptr;
+}
+EXPORT_SYMBOL_GPL(__devm_restrack_register);
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
new file mode 100644
index 0000000..6cf8144f
--- /dev/null
+++ b/include/linux/restrack.h
@@ -0,0 +1,137 @@
+#ifndef RESTRACK_H
+#define RESTRACK_H
+
+#include <linux/track.h>
+
+struct device;
+struct restrack_ctx;
+struct restrack_desc;
+
+int restrack_up(unsigned long type, const void *id, void *data);
+int restrack_down(unsigned long type, const void *id, void *data);
+
+/**
+ * struct restrack_ops - Resource tracker operations for specific resource
+ * @if_type: interface type of provider associated with given resource,
+ *	multiple restrack_ops per if_type are allowed
+ * @init: called by restrack_register to initialize device depended fields
+ *	of the resource descriptor
+ * @destroy: destroy resource descriptor
+ * @if_up: called when provider of given resource becomes available, usually
+ *	it should allocate the resource and fill the variable which was
+ *	provided by restrack_register.
+ * @if_down: called before provider of given resource becomes unavailable. It
+ *	should release the resource and set associated variable to
+ *	-EPROBE_DEFER.
+ *
+ * struct restrack_ops provides set of operations to handle resource specific
+ * operations. It translates resource names specific to consumer device to
+ * track interface id exposed by providers and translates back track
+ * notifications to callback convenient for device driver.
+ * @desc parameter provided in all callbacks is a pointer to common
+ * restrack_desc structure embedded in bigger structure which is usually
+ * allocated by resource specific allocator function.
+ * Callbacks should return 0 on success or error code.
+ */
+struct restrack_ops {
+	unsigned long if_type;
+	int (*init)(struct device *dev, struct restrack_desc *desc);
+	void (*destroy)(struct device *dev, struct restrack_desc *desc);
+	int (*if_up)(struct device *dev, struct restrack_desc *desc,
+		     void *data);
+	void (*if_down)(struct device *dev, struct restrack_desc *desc,
+			void *data);
+};
+
+#define restrack_desc_to_rd(_rd, _var) container_of(_var, typeof(*rd), desc)
+
+#define RESTRACK_DESC_ALLOC(_rd, _ops, _ptr, _name) \
+({ \
+	_rd = kzalloc(sizeof(*_rd), GFP_KERNEL); \
+	if (_rd) { \
+		*_ptr = ERR_PTR(-EPROBE_DEFER); \
+		_rd->ptr = _ptr; \
+		_rd->name = _name; \
+		_rd->desc.ops = &_ops; \
+	} \
+})
+
+/**
+ * struct restrack_desc - resource descriptor
+ * @ops: operations associated with the resource, filled by resource descriptor
+ *	 allocator
+ * @if_id: interface id associated with the resource, filled by @init operation
+ * @itb: iftrack block used by restrack core
+ * @ctx: restrack context used by restrack core
+ * @status: status of the descriptor, used by restrack core
+ */
+struct restrack_desc {
+	const struct restrack_ops *ops;
+	void *if_id;
+	struct track_block itb;
+	struct restrack_ctx *ctx;
+	int status;
+};
+
+typedef void (*restrack_fn_t)(struct device *dev, int ret);
+
+void restrack_unregister(struct restrack_ctx *ctx);
+struct restrack_ctx *__restrack_register(struct device *dev, restrack_fn_t cb,
+		int count, struct restrack_desc **descriptors);
+struct restrack_ctx *__devm_restrack_register(struct device *dev,
+		restrack_fn_t cb, int count,
+		struct restrack_desc **descriptors);
+
+/**
+ * restrack_register - register resource tracking callback
+ * @dev: consumer device
+ * @callback: callback which will be called when:
+ *	- all tracked resources become available, with @ret = 0
+ *	- one of the resources becomes unavailable, with @ret = -EPROBE_DEFER,
+ *	- resource allocation errors, with @ret equal to this error
+ * @descs: list of pointers to resource descriptors to track
+ *
+ * restrack_register is a wrapper macro around @__restrack_register. It can be
+ * called as follows:
+ *	ctx->rtrack = restrack_register(dev, callback,
+ *		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+ *		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+ *		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
+ *		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
+ *		phy_restrack_desc(&ctx->phy, "dsim"),
+ *	);
+ */
+#define restrack_register(dev, callback, descs...) \
+({ \
+	struct restrack_desc *desc[] = { descs }; \
+\
+	__restrack_register(dev, callback, ARRAY_SIZE(desc), desc); \
+})
+
+/**
+ * devm_restrack_register - devm version of restrack_register
+ * @dev: consumer device
+ * @callback: callback which will be called when:
+ *	- all tracked resources become available, with @ret = 0
+ *	- one of the resources becomes unavailable, with @ret = -EPROBE_DEFER,
+ *	- resource allocation errors, with @ret equal to this error
+ * @descs: list of pointers to resource descriptors to track
+ *
+ * restrack_register is a wrapper macro around @__restrack_register. It can be
+ * called as follows:
+ *	rtrack = devm_restrack_register(dev, callback,
+ *		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+ *		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+ *		clk_restrack_desc(&ctx->pll_clk, "pll_clk"),
+ *		clk_restrack_desc(&ctx->bus_clk, "bus_clk"),
+ *		phy_restrack_desc(&ctx->phy, "dsim"),
+ *	);
+ */
+#define devm_restrack_register(dev, callback, descs...) \
+({ \
+	struct restrack_desc *desc[] = { descs }; \
+\
+	__devm_restrack_register(dev, callback, ARRAY_SIZE(desc), desc); \
+})
+
+#endif /* RESTRACK_H */
-- 
1.9.1

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

* [RFC 03/15] drm/panel: add restrack support
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
  2014-12-10 15:48 ` [RFC 01/15] drivers/base: add track framework Andrzej Hajda
  2014-12-10 15:48 ` [RFC 02/15] drivers/base: add restrack framework Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 04/15] regulator: " Andrzej Hajda
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

The patch adds Device Tree restrack support to drm/panel framework.
As panels supports only Device Tree based lookup all panels can be
converted to restrack.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/drm_panel.c | 97 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_panel.h     |  4 ++
 include/linux/restrack.h    |  2 +
 3 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e..db04696 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -22,7 +22,9 @@
  */
 
 #include <linux/err.h>
+#include <linux/of_graph.h>
 #include <linux/module.h>
+#include <linux/restrack.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
@@ -41,6 +43,7 @@ int drm_panel_add(struct drm_panel *panel)
 	mutex_lock(&panel_lock);
 	list_add_tail(&panel->list, &panel_list);
 	mutex_unlock(&panel_lock);
+	restrack_up(RESTRACK_TYPE_DRM_PANEL, panel->dev->of_node, panel);
 
 	return 0;
 }
@@ -48,6 +51,7 @@ EXPORT_SYMBOL(drm_panel_add);
 
 void drm_panel_remove(struct drm_panel *panel)
 {
+	restrack_down(RESTRACK_TYPE_DRM_PANEL, panel->dev->of_node, panel);
 	mutex_lock(&panel_lock);
 	list_del_init(&panel->list);
 	mutex_unlock(&panel_lock);
@@ -93,6 +97,99 @@ struct drm_panel *of_drm_find_panel(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_drm_find_panel);
+
+struct drm_panel_restrack_desc {
+	struct drm_panel **ptr;
+	const char *name;
+	int port;
+	struct restrack_desc desc;
+};
+
+static int drm_panel_restrack_init(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct drm_panel_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+	struct device_node *np;
+
+	if (rd->name) {
+		desc->if_id = of_parse_phandle(dev->of_node, rd->name, 0);
+		goto end;
+	}
+
+	np = NULL;
+	while ((np = of_graph_get_next_endpoint(dev->of_node, np))) {
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(np, &ep);
+		if (ep.port != rd->port)
+			continue;
+		desc->if_id = of_graph_get_remote_port_parent(np);
+		of_node_put(np);
+		break;
+	}
+
+end:
+	return desc->if_id ? 0 : -EINVAL;
+}
+
+static void drm_panel_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct drm_panel_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int drm_panel_restrack_ifup(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct drm_panel_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = data;
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void drm_panel_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct drm_panel_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops drm_panel_restrack_ops = {
+	.if_type = RESTRACK_TYPE_DRM_PANEL,
+	.init = drm_panel_restrack_init,
+	.destroy = drm_panel_restrack_destroy,
+	.if_up = drm_panel_restrack_ifup,
+	.if_down = drm_panel_restrack_ifdown,
+};
+
+/**
+ * drm_panel_restrack_desc - drm_panel resource descriptor allocator
+ * @panel: pointer to variable which will be set to drm_panel handle
+ * @prop_name: property name containing phandle to the panel node, it can be
+ *	       NULL if driver uses only of_graph
+ * @port: of_graph port number in case of_graph is used
+ *
+ * The function creates resource description for panel, which shall be used by
+ * *restrack_register functions.
+ */
+struct restrack_desc *drm_panel_restrack_desc(struct drm_panel **panel,
+		const char *prop_name, int port)
+{
+	struct drm_panel_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, drm_panel_restrack_ops, panel, prop_name);
+	if (!rd)
+		return ERR_PTR(-ENOMEM);
+
+	rd->port = port;
+	return &rd->desc;
+}
+EXPORT_SYMBOL_GPL(drm_panel_restrack_desc);
+
 #endif
 
 MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 1fbcc96..46eb88e 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -130,6 +130,10 @@ int drm_panel_detach(struct drm_panel *panel);
 
 #ifdef CONFIG_OF
 struct drm_panel *of_drm_find_panel(struct device_node *np);
+
+struct restrack_desc;
+struct restrack_desc *drm_panel_restrack_desc(struct drm_panel **panel,
+		const char *prop_name, int port);
 #else
 static inline struct drm_panel *of_drm_find_panel(struct device_node *np)
 {
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index 6cf8144f..af5b617 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -3,6 +3,8 @@
 
 #include <linux/track.h>
 
+#define RESTRACK_TYPE_DRM_PANEL 1
+
 struct device;
 struct restrack_ctx;
 struct restrack_desc;
-- 
1.9.1

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

* [RFC 04/15] regulator: add restrack support
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (2 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 03/15] drm/panel: add restrack support Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 16:07   ` Mark Brown
  2014-12-10 15:48 ` [RFC 05/15] gpio: move DT parsing code to separate functions Andrzej Hajda
                   ` (11 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Regulators supports various methods of lookup.
The patch adds restrack support only to DT based regulators.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/regulator/core.c           | 77 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h | 10 +++++
 include/linux/restrack.h           |  1 +
 3 files changed, 88 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd87c0c..5641e85 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -32,6 +32,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/module.h>
+#include <linux/restrack.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/regulator.h>
@@ -3733,6 +3734,10 @@ add_dev:
 	rdev_init_debugfs(rdev);
 out:
 	mutex_unlock(&regulator_list_mutex);
+
+	if (rdev->dev.of_node)
+		restrack_up(RESTRACK_TYPE_REGULATOR, rdev->dev.of_node, rdev);
+
 	return rdev;
 
 unset_supplies:
@@ -3767,6 +3772,9 @@ void regulator_unregister(struct regulator_dev *rdev)
 	if (rdev == NULL)
 		return;
 
+	if (rdev->dev.of_node)
+		restrack_down(RESTRACK_TYPE_REGULATOR, rdev->dev.of_node, rdev);
+
 	if (rdev->supply) {
 		while (rdev->use_count--)
 			regulator_disable(rdev->supply);
@@ -3971,6 +3979,75 @@ static const struct file_operations supply_map_fops = {
 #endif
 };
 
+struct regulator_restrack_desc {
+	struct regulator **ptr;
+	const char *name;
+	struct restrack_desc desc;
+};
+
+static int regulator_restrack_init(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	desc->if_id = of_get_regulator(dev, rd->name);
+	return PTR_ERR_OR_ZERO(desc->if_id);
+}
+
+static void regulator_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int regulator_restrack_ifup(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = regulator_get(dev, rd->name);
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void regulator_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	regulator_put(*rd->ptr);
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops regulator_restrack_ops = {
+	.if_type = RESTRACK_TYPE_REGULATOR,
+	.init = regulator_restrack_init,
+	.destroy = regulator_restrack_destroy,
+	.if_up = regulator_restrack_ifup,
+	.if_down = regulator_restrack_ifdown,
+};
+
+/**
+ * regulator_restrack_desc - regulator resource descriptor allocator
+ * @regulator: pointer to variable which will be set to regulator handle
+ * @name: name of regulator
+ *
+ * The function creates resource description for regulator, which shall be used
+ * by *restrack_register functions.
+ */
+struct restrack_desc *regulator_restrack_desc(struct regulator **regulator,
+		const char *name)
+{
+	struct regulator_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, regulator_restrack_ops, regulator, name);
+
+	return rd ? &rd->desc : ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(regulator_restrack_desc);
+
 static int __init regulator_init(void)
 {
 	int ret;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index f540b14..69e71ebb 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -551,4 +551,14 @@ static inline int regulator_is_supported_voltage_tol(struct regulator *regulator
 					      target_uV + tol_uV);
 }
 
+struct restrack_desc;
+struct restrack_desc *regulator_restrack_desc(struct regulator **regulator,
+		const char *supply);
+
+static inline struct restrack_desc *
+regulator_bulk_restrack_desc(struct regulator_bulk_data *data)
+{
+	return regulator_restrack_desc(&data->consumer, data->supply);
+}
+
 #endif
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index af5b617..4e4eec6 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -4,6 +4,7 @@
 #include <linux/track.h>
 
 #define RESTRACK_TYPE_DRM_PANEL 1
+#define RESTRACK_TYPE_REGULATOR 2
 
 struct device;
 struct restrack_ctx;
-- 
1.9.1

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

* [RFC 05/15] gpio: move DT parsing code to separate functions
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (3 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 04/15] regulator: " Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2015-01-13  6:17   ` Linus Walleij
  2014-12-10 15:48 ` [RFC 06/15] gpio: add restrack support Andrzej Hajda
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

The patch moves Device Tree parsing logic to separate
function.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpio/gpiolib-of.c | 59 +++++++++++++++++++++--------------------------
 drivers/gpio/gpiolib.c    | 33 +++++++++++++++++++-------
 drivers/gpio/gpiolib.h    |  4 ++--
 3 files changed, 53 insertions(+), 43 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 604dbe6..4707727 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -28,7 +28,7 @@
 /* Private data structure for of_gpiochip_find_and_xlate */
 struct gg_data {
 	enum of_gpio_flags *flags;
-	struct of_phandle_args gpiospec;
+	struct of_phandle_args *gpiospec;
 
 	struct gpio_desc *out_gpio;
 };
@@ -39,12 +39,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
 	struct gg_data *gg_data = data;
 	int ret;
 
-	if ((gc->of_node != gg_data->gpiospec.np) ||
-	    (gc->of_gpio_n_cells != gg_data->gpiospec.args_count) ||
+	if ((gc->of_node != gg_data->gpiospec->np) ||
+	    (gc->of_gpio_n_cells != gg_data->gpiospec->args_count) ||
 	    (!gc->of_xlate))
 		return false;
 
-	ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
+	ret = gc->of_xlate(gc, gg_data->gpiospec, gg_data->flags);
 	if (ret < 0)
 		return false;
 
@@ -52,61 +52,54 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
 	return true;
 }
 
-/**
- * of_get_named_gpiod_flags() - Get a GPIO descriptor and flags for GPIO API
- * @np:		device node to get GPIO from
- * @propname:	property name containing gpio specifier(s)
- * @index:	index of the GPIO
- * @flags:	a flags pointer to fill in
- *
- * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
- * value on the error condition. If @flags is not NULL the function also fills
- * in flags for the GPIO.
- */
-struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
-		     const char *propname, int index, enum of_gpio_flags *flags)
+struct gpio_desc *of_get_gpiod_by_spec(struct of_phandle_args *spec,
+				       enum of_gpio_flags *flags)
 {
 	/* Return -EPROBE_DEFER to support probe() functions to be called
 	 * later when the GPIO actually becomes available
 	 */
 	struct gg_data gg_data = {
 		.flags = flags,
-		.out_gpio = ERR_PTR(-EPROBE_DEFER)
+		.out_gpio = ERR_PTR(-EPROBE_DEFER),
+		.gpiospec = spec
 	};
-	int ret;
 
 	/* .of_xlate might decide to not fill in the flags, so clear it. */
 	if (flags)
 		*flags = 0;
 
-	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells", index,
-					 &gg_data.gpiospec);
-	if (ret) {
-		pr_debug("%s: can't parse '%s' property of node '%s[%d]'\n",
-			__func__, propname, np->full_name, index);
-		return ERR_PTR(ret);
-	}
-
 	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 
-	of_node_put(gg_data.gpiospec.np);
-	pr_debug("%s: parsed '%s' property of node '%s[%d]' - status (%d)\n",
-		 __func__, propname, np->full_name, index,
+	pr_debug("%s: parsed property of node '%s[%d]' - status (%d)\n",
+		 __func__, spec->np->full_name, spec->args[0],
 		 PTR_ERR_OR_ZERO(gg_data.out_gpio));
+
 	return gg_data.out_gpio;
 }
 
 int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
 			    int index, enum of_gpio_flags *flags)
 {
+	struct of_phandle_args spec;
 	struct gpio_desc *desc;
+	int ret;
+
+	ret = of_parse_phandle_with_args(np, list_name, "#gpio-cells", index,
+					 &spec);
+	if (ret) {
+		pr_debug("%s: can't parse '%s' property of node '%s[%d]'\n",
+			__func__, list_name, np->full_name, index);
+		return ret;
+	}
 
-	desc = of_get_named_gpiod_flags(np, list_name, index, flags);
+	desc = of_get_gpiod_by_spec(&spec, flags);
 
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
-	else
-		return desc_to_gpio(desc);
+
+	of_node_put(spec.np);
+
+	return desc_to_gpio(desc);
 }
 EXPORT_SYMBOL(of_get_named_gpio_flags);
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e8e98ca..78fcec9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1470,15 +1470,13 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table)
 	mutex_unlock(&gpio_lookup_lock);
 }
 
-static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
-				      unsigned int idx,
-				      enum gpio_lookup_flags *flags)
+static int of_get_gpiod_spec(struct device *dev, const char *con_id,
+			     unsigned int idx, struct of_phandle_args *spec)
 {
 	static const char *suffixes[] = { "gpios", "gpio" };
 	char prop_name[32]; /* 32 is max size of property name */
-	enum of_gpio_flags of_flags;
-	struct gpio_desc *desc;
 	unsigned int i;
+	int ret;
 
 	for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
 		if (con_id)
@@ -1486,12 +1484,31 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 		else
 			snprintf(prop_name, 32, "%s", suffixes[i]);
 
-		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
-						&of_flags);
-		if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
+		ret = of_parse_phandle_with_args(dev->of_node, prop_name,
+						 "#gpio-cells", idx, spec);
+		if (!ret)
 			break;
 	}
 
+	return ret;
+}
+
+static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
+				      unsigned int idx,
+				      enum gpio_lookup_flags *flags)
+{
+	enum of_gpio_flags of_flags;
+	struct of_phandle_args spec;
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = of_get_gpiod_spec(dev, con_id, idx, &spec);
+	if (ret)
+		return ERR_PTR(ret);
+
+	desc = of_get_gpiod_by_spec(&spec, &of_flags);
+	of_node_put(spec.np);
+
 	if (IS_ERR(desc))
 		return desc;
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9db2b6a..3c0c1ad 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -54,8 +54,8 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
 }
 #endif
 
-struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
-		   const char *list_name, int index, enum of_gpio_flags *flags);
+struct gpio_desc *of_get_gpiod_by_spec(struct of_phandle_args *spec,
+				       enum of_gpio_flags *flags);
 
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
 
-- 
1.9.1

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

* [RFC 06/15] gpio: add restrack support
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (4 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 05/15] gpio: move DT parsing code to separate functions Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 07/15] clk: add DT parsing function Andrzej Hajda
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

GPIO supports different methods of lookup.
The patch adds restrack support only to DT based GPIOs.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpio/gpiolib.c        | 81 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h |  4 +++
 include/linux/restrack.h      |  1 +
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 78fcec9..5d85e20 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -15,6 +15,7 @@
 #include <linux/acpi.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
+#include <linux/restrack.h>
 
 #include "gpiolib.h"
 
@@ -278,6 +279,8 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (status)
 		goto fail;
 
+	restrack_up(RESTRACK_TYPE_GPIO, chip->of_node, chip);
+
 	status = gpiochip_export(chip);
 	if (status)
 		goto fail;
@@ -313,6 +316,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 	unsigned long	flags;
 	unsigned	id;
 
+	restrack_down(RESTRACK_TYPE_GPIO, chip->of_node, chip);
 	acpi_gpiochip_remove(chip);
 
 	spin_lock_irqsave(&gpio_lock, flags);
@@ -1770,6 +1774,83 @@ void gpiod_put(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
+struct gpiod_restrack_desc {
+	struct gpio_desc **ptr;
+	const char *name;
+	enum gpiod_flags flags;
+	struct of_phandle_args spec;
+	struct restrack_desc desc;
+};
+
+static int gpiod_restrack_init(struct device *dev, struct restrack_desc *desc)
+{
+	struct gpiod_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+	int ret;
+
+	ret = of_get_gpiod_spec(dev, rd->name, 0, &rd->spec);
+	if (!ret)
+		desc->if_id = rd->spec.np;
+	return ret;
+}
+
+static void gpiod_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct gpiod_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int gpiod_restrack_ifup(struct device *dev, struct restrack_desc *desc,
+		void *data)
+{
+	struct gpiod_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = gpiod_get(dev, rd->name, rd->flags);
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void gpiod_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct gpiod_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	gpiod_put(*rd->ptr);
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops gpiod_restrack_ops = {
+	.if_type = RESTRACK_TYPE_GPIO,
+	.init = gpiod_restrack_init,
+	.destroy = gpiod_restrack_destroy,
+	.if_up = gpiod_restrack_ifup,
+	.if_down = gpiod_restrack_ifdown,
+};
+
+/**
+ * gpiod_restrack_desc - gpio resource descriptor allocator
+ * @gpiod: pointer to variable which will be set to gpiod handle
+ * @name: name of gpio
+ * @flags: gpiod flags
+ *
+ * The function creates resource description for gpio, which shall be used
+ * by *restrack_register functions.
+ */
+struct restrack_desc *gpiod_restrack_desc(struct gpio_desc **gpiod,
+		const char *con_id, enum gpiod_flags flags)
+{
+	struct gpiod_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, gpiod_restrack_ops, gpiod, con_id);
+	if (!rd)
+		return ERR_PTR(-ENOMEM);
+
+	rd->flags = flags;
+	return &rd->desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_restrack_desc);
+
 #ifdef CONFIG_DEBUG_FS
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 12f146f..55f2e4e 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -94,6 +94,10 @@ int gpiod_to_irq(const struct gpio_desc *desc);
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
 
+struct restrack_desc;
+struct restrack_desc *gpiod_restrack_desc(struct gpio_desc **gpiod,
+		const char *con_id, enum gpiod_flags flags);
+
 #else /* CONFIG_GPIOLIB */
 
 static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index 4e4eec6..e1aded0 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -5,6 +5,7 @@
 
 #define RESTRACK_TYPE_DRM_PANEL 1
 #define RESTRACK_TYPE_REGULATOR 2
+#define RESTRACK_TYPE_GPIO 3
 
 struct device;
 struct restrack_ctx;
-- 
1.9.1

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

* [RFC 07/15] clk: add DT parsing function
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (5 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 06/15] gpio: add restrack support Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 08/15] clk: add restrack support Andrzej Hajda
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

The patch adds function for parsing Device Tree to get
clock specifier. The function could be ultimately used
by clock core.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/clk/clkdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index da4bda8..bd22750 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -120,6 +120,29 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 	return clk;
 }
 EXPORT_SYMBOL(of_clk_get_by_name);
+
+static int of_get_clock_spec(struct device *dev, const char *name,
+		struct of_phandle_args *spec)
+{
+	struct device_node *np = dev->of_node;
+
+	while (np) {
+		int idx = 0;
+
+		if (name)
+			idx = of_property_match_string(np, "clock-names", name);
+
+		if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells",
+						idx, spec))
+			return 0;
+
+		np = np->parent;
+		if (np && !of_get_property(np, "clock-ranges", NULL))
+			break;
+	}
+
+	return -ENOENT;
+}
 #endif
 
 /*
-- 
1.9.1

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

* [RFC 08/15] clk: add restrack support
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (6 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 07/15] clk: add DT parsing function Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 09/15] phy: " Andrzej Hajda
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Clocks supports different methods of lookup.
The patch adds restrack support only to DT based clocks.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/clk/clk.c        |  6 ++++
 drivers/clk/clkdev.c     | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      |  3 ++
 include/linux/restrack.h |  1 +
 4 files changed, 84 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4896ae9..2c50204 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -16,6 +16,7 @@
 #include <linux/spinlock.h>
 #include <linux/err.h>
 #include <linux/list.h>
+#include <linux/restrack.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/device.h>
@@ -2476,6 +2477,9 @@ int of_clk_add_provider(struct device_node *np,
 	ret = of_clk_set_defaults(np, true);
 	if (ret < 0)
 		of_clk_del_provider(np);
+	else
+		restrack_up(RESTRACK_TYPE_CLOCK, cp->node, cp);
+
 
 	return ret;
 }
@@ -2489,6 +2493,8 @@ void of_clk_del_provider(struct device_node *np)
 {
 	struct of_clk_provider *cp;
 
+	restrack_down(RESTRACK_TYPE_CLOCK, np, NULL);
+
 	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(cp, &of_clk_providers, link) {
 		if (cp->node == np) {
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bd22750..2532ea1 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -20,6 +20,7 @@
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/of.h>
+#include <linux/restrack.h>
 
 #include "clk.h"
 
@@ -143,6 +144,79 @@ static int of_get_clock_spec(struct device *dev, const char *name,
 
 	return -ENOENT;
 }
+
+struct clk_restrack_desc {
+	struct clk **ptr;
+	const char *name;
+	struct of_phandle_args spec;
+	struct restrack_desc desc;
+};
+
+static int clk_restrack_init(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct clk_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+	int ret;
+
+	ret = of_get_clock_spec(dev, rd->name, &rd->spec);
+	if (!ret)
+		desc->if_id = rd->spec.np;
+	return ret;
+}
+
+static void clk_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct clk_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int clk_restrack_ifup(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct clk_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = of_clk_get_by_clkspec(&rd->spec);
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void clk_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct clk_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	clk_put(*rd->ptr);
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops clk_restrack_ops = {
+	.if_type = RESTRACK_TYPE_CLOCK,
+	.init = clk_restrack_init,
+	.destroy = clk_restrack_destroy,
+	.if_up = clk_restrack_ifup,
+	.if_down = clk_restrack_ifdown,
+};
+
+/**
+ * clk_restrack_desc - clock resource descriptor allocator
+ * @clk: pointer to variable which will be set to clock handle
+ * @name: name of clock
+ *
+ * The function creates resource description for clock, which shall be used
+ * by *restrack_register functions.
+ */
+struct restrack_desc *clk_restrack_desc(struct clk **clock, const char *name)
+{
+	struct clk_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, clk_restrack_ops, clock, name);
+
+	return rd ? &rd->desc : ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(clk_restrack_desc);
+
 #endif
 
 /*
diff --git a/include/linux/clk.h b/include/linux/clk.h
index c7f258a..28bad95 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -428,6 +428,9 @@ struct of_phandle_args;
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
+
+struct restrack_desc;
+struct restrack_desc *clk_restrack_desc(struct clk **clock, const char *name);
 #else
 static inline struct clk *of_clk_get(struct device_node *np, int index)
 {
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index e1aded0..6707dce 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -6,6 +6,7 @@
 #define RESTRACK_TYPE_DRM_PANEL 1
 #define RESTRACK_TYPE_REGULATOR 2
 #define RESTRACK_TYPE_GPIO 3
+#define RESTRACK_TYPE_CLOCK 4
 
 struct device;
 struct restrack_ctx;
-- 
1.9.1

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

* [RFC 09/15] phy: add restrack support
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (7 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 08/15] clk: add restrack support Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 10/15] drm/exynos/dsi: simplify hotplug code Andrzej Hajda
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

PHYs supports different methods of lookup.
The patch adds restrack support only to DT based PHYs.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/phy/phy-core.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h  |  3 ++
 include/linux/restrack.h |  1 +
 3 files changed, 94 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index ff5eec5..449ec4e 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -22,6 +22,7 @@
 #include <linux/idr.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/restrack.h>
 
 static struct class *phy_class;
 static DEFINE_MUTEX(phy_provider_mutex);
@@ -754,6 +755,8 @@ struct phy_provider *__of_phy_provider_register(struct device *dev,
 	list_add_tail(&phy_provider->list, &phy_provider_list);
 	mutex_unlock(&phy_provider_mutex);
 
+	restrack_up(RESTRACK_TYPE_PHY, dev->of_node, phy_provider);
+
 	return phy_provider;
 }
 EXPORT_SYMBOL_GPL(__of_phy_provider_register);
@@ -804,6 +807,9 @@ void of_phy_provider_unregister(struct phy_provider *phy_provider)
 	if (IS_ERR(phy_provider))
 		return;
 
+	restrack_down(RESTRACK_TYPE_PHY, phy_provider->dev->of_node,
+		      phy_provider);
+
 	mutex_lock(&phy_provider_mutex);
 	list_del(&phy_provider->list);
 	kfree(phy_provider);
@@ -828,6 +834,90 @@ void devm_of_phy_provider_unregister(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_of_phy_provider_unregister);
 
+int of_get_phy_spec(struct device *dev, const char *name,
+		struct of_phandle_args *spec)
+{
+	struct device_node *np = dev->of_node;
+	int idx;
+
+	idx = of_property_match_string(np, "phy-names", name);
+
+	return of_parse_phandle_with_args(np, "phys", "#phy-cells", idx, spec);
+}
+
+struct phy_restrack_desc {
+	struct phy **ptr;
+	const char *name;
+	struct of_phandle_args spec;
+	struct restrack_desc desc;
+};
+
+static int phy_restrack_init(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct phy_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+	int ret;
+
+	ret = of_get_phy_spec(dev, rd->name, &rd->spec);
+	if (!ret)
+		desc->if_id = rd->spec.np;
+	return ret;
+}
+
+static void phy_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct phy_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int phy_restrack_ifup(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct phy_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+	struct phy_provider *phy_provider = data;
+
+	*rd->ptr = phy_provider->of_xlate(phy_provider->dev, &rd->spec);
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void phy_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct phy_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops phy_restrack_ops = {
+	.if_type = RESTRACK_TYPE_PHY,
+	.init = phy_restrack_init,
+	.destroy = phy_restrack_destroy,
+	.if_up = phy_restrack_ifup,
+	.if_down = phy_restrack_ifdown,
+};
+
+/**
+ * phy_restrack_desc - phy resource descriptor allocator
+ * @phy: pointer to variable which will be set to phy handle
+ * @name: name of phy
+ *
+ * The function creates resource description for phy, which shall be used
+ * by *restrack_register functions.
+ */
+struct restrack_desc *phy_restrack_desc(struct phy **phy, const char *name)
+{
+	struct phy_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, phy_restrack_ops, phy, name);
+
+	return rd ? &rd->desc : ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(phy_restrack_desc);
+
+
 /**
  * phy_release() - release the phy
  * @dev: the dev member within phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 8cb6f81..14d176c 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -174,6 +174,9 @@ struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
 void of_phy_provider_unregister(struct phy_provider *phy_provider);
 void devm_of_phy_provider_unregister(struct device *dev,
 	struct phy_provider *phy_provider);
+
+struct restrack_desc;
+struct restrack_desc *phy_restrack_desc(struct phy **phy, const char *name);
 #else
 static inline int phy_pm_runtime_get(struct phy *phy)
 {
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index 6707dce..411d791 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -7,6 +7,7 @@
 #define RESTRACK_TYPE_REGULATOR 2
 #define RESTRACK_TYPE_GPIO 3
 #define RESTRACK_TYPE_CLOCK 4
+#define RESTRACK_TYPE_PHY 5
 
 struct device;
 struct restrack_ctx;
-- 
1.9.1

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

* [RFC 10/15] drm/exynos/dsi: simplify hotplug code
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (8 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 09/15] phy: " Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 11/15] drm/exynos/dsi: convert to restrack API Andrzej Hajda
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Exynos DSI driver uses DSI bus attach/detach callbacks to implement
panel hotplug mechanism. The patch moves panel attachment code
from .detect callback to DSI bus callbacks. It makes the code
simpler and more straightforward.
The patch removes also redundant and lock unprotected dpms_off call
from unbind code.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 61 ++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 05fe93d..8201d79 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -271,7 +271,6 @@ struct exynos_dsi {
 	struct exynos_drm_display display;
 	struct mipi_dsi_host dsi_host;
 	struct drm_connector connector;
-	struct device_node *panel_node;
 	struct drm_panel *panel;
 	struct device *dev;
 
@@ -1154,10 +1153,11 @@ static int exynos_dsi_init(struct exynos_dsi *dsi)
 
 static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
 {
+	struct device_node *panel_node = dsi->panel->dev->of_node;
 	int ret;
 	int te_gpio_irq;
 
-	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
+	dsi->te_gpio = of_get_named_gpio(panel_node, "te-gpios", 0);
 	if (!gpio_is_valid(dsi->te_gpio)) {
 		dev_err(dsi->dev, "no te-gpios specified\n");
 		ret = dsi->te_gpio;
@@ -1198,11 +1198,25 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
+	struct drm_device *drm_dev = dsi->connector.dev;
+	bool changed = false;
 
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	dsi->panel_node = device->dev.of_node;
+
+	mutex_lock(&drm_dev->mode_config.mutex);
+
+	dsi->panel = of_drm_find_panel(device->dev.of_node);
+	if (dsi->panel) {
+		drm_panel_attach(dsi->panel, &dsi->connector);
+		if (drm_dev->mode_config.poll_enabled) {
+			dsi->connector.status = connector_status_connected;
+			changed = true;
+		}
+	}
+
+	mutex_unlock(&drm_dev->mode_config.mutex);
 
 	/*
 	 * This is a temporary solution and should be made by more generic way.
@@ -1217,8 +1231,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 			return ret;
 	}
 
-	if (dsi->connector.dev)
-		drm_helper_hpd_irq_event(dsi->connector.dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(drm_dev);
 
 	return 0;
 }
@@ -1227,13 +1241,29 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
+	struct drm_device *drm_dev = dsi->connector.dev;
+	struct exynos_drm_display *display = dev_get_drvdata(dsi->dev);
+	bool changed = false;
 
 	exynos_dsi_unregister_te_irq(dsi);
 
-	dsi->panel_node = NULL;
+	mutex_lock(&drm_dev->mode_config.mutex);
+
+	display->ops->dpms(display, DRM_MODE_DPMS_OFF);
+
+	if (dsi->panel) {
+		drm_panel_detach(dsi->panel);
+		dsi->panel = NULL;
+		if (drm_dev->mode_config.poll_enabled) {
+			dsi->connector.status = connector_status_disconnected;
+			changed = true;
+		}
+	}
+
+	mutex_unlock(&drm_dev->mode_config.mutex);
 
-	if (dsi->connector.dev)
-		drm_helper_hpd_irq_event(dsi->connector.dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(drm_dev);
 
 	return 0;
 }
@@ -1430,19 +1460,6 @@ exynos_dsi_detect(struct drm_connector *connector, bool force)
 {
 	struct exynos_dsi *dsi = connector_to_dsi(connector);
 
-	if (!dsi->panel) {
-		dsi->panel = of_drm_find_panel(dsi->panel_node);
-		if (dsi->panel)
-			drm_panel_attach(dsi->panel, &dsi->connector);
-	} else if (!dsi->panel_node) {
-		struct exynos_drm_display *display;
-
-		display = platform_get_drvdata(to_platform_device(dsi->dev));
-		exynos_dsi_dpms(display, DRM_MODE_DPMS_OFF);
-		drm_panel_detach(dsi->panel);
-		dsi->panel = NULL;
-	}
-
 	if (dsi->panel)
 		return connector_status_connected;
 
@@ -1665,8 +1682,6 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 	struct exynos_drm_display *display = dev_get_drvdata(dev);
 	struct exynos_dsi *dsi = display_to_dsi(display);
 
-	exynos_dsi_dpms(display, DRM_MODE_DPMS_OFF);
-
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
 
-- 
1.9.1

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

* [RFC 11/15] drm/exynos/dsi: convert to restrack API
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (9 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 10/15] drm/exynos/dsi: simplify hotplug code Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 12/15] drm/exynos/dpi: use common of_graph functions Andrzej Hajda
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Convert exynos_dsi driver to use restrack API.
As a result driver have following advantages:
- correctly handles removal of resources,
- do not need to defer probing, so as a result the whole drm system
  initialization will not be postponed to late initcall, unless other
  components delays it,
- simplified initialization.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 52 +++++++++++++--------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 8201d79..53ac467 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -23,7 +23,7 @@
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
-
+#include <linux/restrack.h>
 #include <video/mipi_display.h>
 #include <video/videomode.h>
 
@@ -1690,9 +1690,18 @@ static const struct component_ops exynos_dsi_component_ops = {
 	.unbind	= exynos_dsi_unbind,
 };
 
+void exynos_dsi_restrack_cb(struct device *dev, int ret)
+{
+	if (ret)
+		component_del(dev, &exynos_dsi_component_ops);
+	else
+		component_add(dev, &exynos_dsi_component_ops);
+}
+
 static int exynos_dsi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct restrack_ctx *rtrack;
 	struct resource *res;
 	struct exynos_dsi *dsi;
 	int ret;
@@ -1728,26 +1737,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	dsi->supplies[0].supply = "vddcore";
 	dsi->supplies[1].supply = "vddio";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dsi->supplies),
-				      dsi->supplies);
-	if (ret) {
-		dev_info(dev, "failed to get regulators: %d\n", ret);
-		return -EPROBE_DEFER;
-	}
-
-	dsi->pll_clk = devm_clk_get(dev, "pll_clk");
-	if (IS_ERR(dsi->pll_clk)) {
-		dev_info(dev, "failed to get dsi pll input clock\n");
-		ret = PTR_ERR(dsi->pll_clk);
-		goto err_del_component;
-	}
-
-	dsi->bus_clk = devm_clk_get(dev, "bus_clk");
-	if (IS_ERR(dsi->bus_clk)) {
-		dev_info(dev, "failed to get dsi bus clock\n");
-		ret = PTR_ERR(dsi->bus_clk);
-		goto err_del_component;
-	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dsi->reg_base = devm_ioremap_resource(dev, res);
@@ -1757,13 +1746,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
-	dsi->phy = devm_phy_get(dev, "dsim");
-	if (IS_ERR(dsi->phy)) {
-		dev_info(dev, "failed to get dsim phy\n");
-		ret = PTR_ERR(dsi->phy);
-		goto err_del_component;
-	}
-
 	dsi->irq = platform_get_irq(pdev, 0);
 	if (dsi->irq < 0) {
 		dev_err(dev, "failed to request dsi irq resource\n");
@@ -1782,11 +1764,20 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dsi->display);
 
-	ret = component_add(dev, &exynos_dsi_component_ops);
+	rtrack = devm_restrack_register(dsi->dev, exynos_dsi_restrack_cb,
+		regulator_bulk_restrack_desc(&dsi->supplies[0]),
+		regulator_bulk_restrack_desc(&dsi->supplies[1]),
+		clk_restrack_desc(&dsi->pll_clk, "pll_clk"),
+		clk_restrack_desc(&dsi->bus_clk, "bus_clk"),
+		phy_restrack_desc(&dsi->phy, "dsim"),
+	);
+
+	ret = PTR_ERR_OR_ZERO(rtrack);
+
 	if (ret)
 		goto err_del_component;
 
-	return ret;
+	return 0;
 
 err_del_component:
 	exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CONNECTOR);
@@ -1795,7 +1786,6 @@ err_del_component:
 
 static int exynos_dsi_remove(struct platform_device *pdev)
 {
-	component_del(&pdev->dev, &exynos_dsi_component_ops);
 	exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR);
 
 	return 0;
-- 
1.9.1

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

* [RFC 12/15] drm/exynos/dpi: use common of_graph functions
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (10 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 11/15] drm/exynos/dsi: convert to restrack API Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 13/15] drm/exynos/dpi: convert to restrack API Andrzej Hajda
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

The patch removes private of_graph functions and uses common ones.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c | 117 +++++++-------------------------
 1 file changed, 24 insertions(+), 93 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 37678cf..91a29e1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -14,6 +14,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
 
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 
 #include <video/of_videomode.h>
@@ -21,6 +22,15 @@
 
 #include "exynos_drm_drv.h"
 
+/* possible of_graph port nodes used by device */
+enum {
+	FIMD_PORT_IN0,
+	FIMD_PORT_IN1,
+	FIMD_PORT_IN2,
+	FIMD_PORT_RGB,
+	FIMD_PORT_WRB,
+};
+
 struct exynos_dpi {
 	struct exynos_drm_display display;
 	struct device *dev;
@@ -171,102 +181,12 @@ static struct exynos_drm_display_ops exynos_dpi_display_ops = {
 	.dpms = exynos_dpi_dpms
 };
 
-/* of_* functions will be removed after merge of of_graph patches */
-static struct device_node *
-of_get_child_by_name_reg(struct device_node *parent, const char *name, u32 reg)
-{
-	struct device_node *np;
-
-	for_each_child_of_node(parent, np) {
-		u32 r;
-
-		if (!np->name || of_node_cmp(np->name, name))
-			continue;
-
-		if (of_property_read_u32(np, "reg", &r) < 0)
-			r = 0;
-
-		if (reg == r)
-			break;
-	}
-
-	return np;
-}
-
-static struct device_node *of_graph_get_port_by_reg(struct device_node *parent,
-						    u32 reg)
-{
-	struct device_node *ports, *port;
-
-	ports = of_get_child_by_name(parent, "ports");
-	if (ports)
-		parent = ports;
-
-	port = of_get_child_by_name_reg(parent, "port", reg);
-
-	of_node_put(ports);
-
-	return port;
-}
-
-static struct device_node *
-of_graph_get_endpoint_by_reg(struct device_node *port, u32 reg)
-{
-	return of_get_child_by_name_reg(port, "endpoint", reg);
-}
-
-static struct device_node *
-of_graph_get_remote_port_parent(const struct device_node *node)
-{
-	struct device_node *np;
-	unsigned int depth;
-
-	np = of_parse_phandle(node, "remote-endpoint", 0);
-
-	/* Walk 3 levels up only if there is 'ports' node. */
-	for (depth = 3; depth && np; depth--) {
-		np = of_get_next_parent(np);
-		if (depth == 2 && of_node_cmp(np->name, "ports"))
-			break;
-	}
-	return np;
-}
-
-enum {
-	FIMD_PORT_IN0,
-	FIMD_PORT_IN1,
-	FIMD_PORT_IN2,
-	FIMD_PORT_RGB,
-	FIMD_PORT_WRB,
-};
-
-static struct device_node *exynos_dpi_of_find_panel_node(struct device *dev)
-{
-	struct device_node *np, *ep;
-
-	np = of_graph_get_port_by_reg(dev->of_node, FIMD_PORT_RGB);
-	if (!np)
-		return NULL;
-
-	ep = of_graph_get_endpoint_by_reg(np, 0);
-	of_node_put(np);
-	if (!ep)
-		return NULL;
-
-	np = of_graph_get_remote_port_parent(ep);
-	of_node_put(ep);
-
-	return np;
-}
-
 static int exynos_dpi_parse_dt(struct exynos_dpi *ctx)
 {
 	struct device *dev = ctx->dev;
 	struct device_node *dn = dev->of_node;
 	struct device_node *np;
 
-	ctx->panel_node = exynos_dpi_of_find_panel_node(dev);
-
 	np = of_get_child_by_name(dn, "display-timings");
 	if (np) {
 		struct videomode *vm;
@@ -289,10 +209,21 @@ static int exynos_dpi_parse_dt(struct exynos_dpi *ctx)
 		return 0;
 	}
 
-	if (!ctx->panel_node)
-		return -EINVAL;
+	np = NULL;
+	while ((np = of_graph_get_next_endpoint(dev->of_node, np))) {
+		struct of_endpoint ep;
 
-	return 0;
+		of_graph_parse_endpoint(np, &ep);
+		if (ep.port != FIMD_PORT_RGB)
+			continue;
+
+		ctx->panel_node = of_graph_get_remote_port_parent(np);
+		of_node_put(np);
+
+		return ctx->panel_node ? 0 : -EINVAL;
+	}
+
+	return -EINVAL;
 }
 
 struct exynos_drm_display *exynos_dpi_probe(struct device *dev)
-- 
1.9.1

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

* [RFC 13/15] drm/exynos/dpi: convert to restrack API
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (11 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 12/15] drm/exynos/dpi: use common of_graph functions Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 14/15] drm/panel/ld9040: do not power off panel on removal Andrzej Hajda
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

exynos_dpi implements drm encoder and connector. Currently its probe
was defered if the associated panel was not yet present. It is inefficient
behavior - it could start normally with connector status set to disconnected
and notify drm about status change when panel appears/disapeears.
Unfortunately drm_panel API does not provide such notifications.
restrack solves the issue above.
After converting to restrack driver have following advantages:
- correctly handles removal of resources,
- do not need to defer probing, so as a result the whole drm system
  initialization will not be postponed to late initcall, unless other
  components delays it,
- it starts/stops tracking panel presence only when necessary.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c  | 80 +++++++++++++++++++++++---------
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |  7 +++
 2 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 91a29e1..75ee578 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -13,9 +13,9 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
-
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
+#include <linux/restrack.h>
 
 #include <video/of_videomode.h>
 #include <video/videomode.h>
@@ -34,7 +34,6 @@ enum {
 struct exynos_dpi {
 	struct exynos_drm_display display;
 	struct device *dev;
-	struct device_node *panel_node;
 
 	struct drm_panel *panel;
 	struct drm_connector connector;
@@ -42,10 +41,13 @@ struct exynos_dpi {
 
 	struct videomode *vm;
 	int dpms_mode;
+	struct restrack_ctx *rtrack;
 };
 
 #define connector_to_dpi(c) container_of(c, struct exynos_dpi, connector)
 
+struct exynos_drm_display *fimd_dev_to_display(struct device *dev);
+
 static inline struct exynos_dpi *display_to_dpi(struct exynos_drm_display *d)
 {
 	return container_of(d, struct exynos_dpi, display);
@@ -56,14 +58,18 @@ exynos_dpi_detect(struct drm_connector *connector, bool force)
 {
 	struct exynos_dpi *ctx = connector_to_dpi(connector);
 
-	if (ctx->panel && !ctx->panel->connector)
-		drm_panel_attach(ctx->panel, &ctx->connector);
+	if (!ctx->vm && IS_ERR(ctx->panel))
+		return connector_status_disconnected;
 
 	return connector_status_connected;
 }
 
 static void exynos_dpi_connector_destroy(struct drm_connector *connector)
 {
+	struct exynos_dpi *ctx = connector_to_dpi(connector);
+
+	if (!IS_ERR(ctx->rtrack))
+		restrack_unregister(ctx->rtrack);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 }
@@ -94,7 +100,7 @@ static int exynos_dpi_get_modes(struct drm_connector *connector)
 		return 1;
 	}
 
-	if (ctx->panel)
+	if (!IS_ERR(ctx->panel))
 		return ctx->panel->funcs->get_modes(ctx->panel);
 
 	return 0;
@@ -113,6 +119,38 @@ static struct drm_connector_helper_funcs exynos_dpi_connector_helper_funcs = {
 	.best_encoder = exynos_dpi_best_encoder,
 };
 
+static void exynos_dpi_dpms(struct exynos_drm_display *display, int mode);
+
+void exynos_dpi_notifier(struct device *dev, int ret)
+{
+	struct exynos_drm_display *display = fimd_dev_to_display(dev);
+	struct exynos_dpi *ctx = display_to_dpi(display);
+	struct drm_connector *connector = &ctx->connector;
+	struct drm_device *drm_dev = connector->dev;
+	bool poll_enabled = drm_dev->mode_config.poll_enabled;
+
+	mutex_lock(&drm_dev->mode_config.mutex);
+
+	if (ret == 0) {
+		drm_panel_attach(ctx->panel, connector);
+	} else if (ret == -EPROBE_DEFER) {
+		exynos_dpi_dpms(&ctx->display, DRM_MODE_DPMS_OFF);
+		drm_panel_detach(ctx->panel);
+		ctx->panel = ERR_PTR(-EPROBE_DEFER);
+	} else {
+		dev_err(dev, "restrack error %d\n", ret);
+		poll_enabled = false;
+	}
+
+	if (poll_enabled)
+		connector->status = exynos_dpi_detect(connector, true);
+
+	mutex_unlock(&drm_dev->mode_config.mutex);
+
+	if (poll_enabled)
+		drm_kms_helper_hotplug_event(drm_dev);
+}
+
 static int exynos_dpi_create_connector(struct exynos_drm_display *display,
 				       struct drm_encoder *encoder)
 {
@@ -136,12 +174,23 @@ static int exynos_dpi_create_connector(struct exynos_drm_display *display,
 	drm_connector_register(connector);
 	drm_mode_connector_attach_encoder(connector, encoder);
 
+	if (ctx->vm)
+		return 0;
+
+	ctx->rtrack = restrack_register(ctx->dev, exynos_dpi_notifier,
+		drm_panel_restrack_desc(&ctx->panel, NULL, FIMD_PORT_RGB)
+	);
+
+	if (IS_ERR(ctx->rtrack))
+		DRM_ERROR("error installing panel tracker: %ld\n",
+			  PTR_ERR(ctx->rtrack));
+
 	return 0;
 }
 
 static void exynos_dpi_poweron(struct exynos_dpi *ctx)
 {
-	if (ctx->panel) {
+	if (!IS_ERR(ctx->panel)) {
 		drm_panel_prepare(ctx->panel);
 		drm_panel_enable(ctx->panel);
 	}
@@ -149,7 +198,7 @@ static void exynos_dpi_poweron(struct exynos_dpi *ctx)
 
 static void exynos_dpi_poweroff(struct exynos_dpi *ctx)
 {
-	if (ctx->panel) {
+	if (!IS_ERR(ctx->panel)) {
 		drm_panel_disable(ctx->panel);
 		drm_panel_unprepare(ctx->panel);
 	}
@@ -217,10 +266,9 @@ static int exynos_dpi_parse_dt(struct exynos_dpi *ctx)
 		if (ep.port != FIMD_PORT_RGB)
 			continue;
 
-		ctx->panel_node = of_graph_get_remote_port_parent(np);
 		of_node_put(np);
 
-		return ctx->panel_node ? 0 : -EINVAL;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -252,15 +300,6 @@ struct exynos_drm_display *exynos_dpi_probe(struct device *dev)
 		goto err_del_component;
 	}
 
-	if (ctx->panel_node) {
-		ctx->panel = of_drm_find_panel(ctx->panel_node);
-		if (!ctx->panel) {
-			exynos_drm_component_del(dev,
-						EXYNOS_DEVICE_TYPE_CONNECTOR);
-			return ERR_PTR(-EPROBE_DEFER);
-		}
-	}
-
 	return &ctx->display;
 
 err_del_component:
@@ -273,11 +312,6 @@ int exynos_dpi_remove(struct exynos_drm_display *display)
 {
 	struct exynos_dpi *ctx = display_to_dpi(display);
 
-	exynos_dpi_dpms(&ctx->display, DRM_MODE_DPMS_OFF);
-
-	if (ctx->panel)
-		drm_panel_detach(ctx->panel);
-
 	exynos_drm_component_del(ctx->dev, EXYNOS_DEVICE_TYPE_CONNECTOR);
 
 	return 0;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index e5810d1..05cf4e2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -190,6 +190,13 @@ static inline struct fimd_context *mgr_to_fimd(struct exynos_drm_manager *mgr)
 	return container_of(mgr, struct fimd_context, manager);
 }
 
+struct exynos_drm_display *fimd_dev_to_display(struct device *dev)
+{
+	struct fimd_context *ctx = dev_get_drvdata(dev);
+
+	return ctx->display;
+}
+
 static const struct of_device_id fimd_driver_dt_match[] = {
 	{ .compatible = "samsung,s3c6400-fimd",
 	  .data = &s3c64xx_fimd_driver_data },
-- 
1.9.1

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

* [RFC 14/15] drm/panel/ld9040: do not power off panel on removal
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (12 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 13/15] drm/exynos/dpi: convert to restrack API Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 15:48 ` [RFC 15/15] drm/panel/ld9040: convert to restrack API Andrzej Hajda
  2014-12-10 16:16 ` [RFC 00/15] Resource tracking/allocation framework Russell King - ARM Linux
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Panel is powered off already by connector during drm_panel_remove call.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/panel/panel-ld9040.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-ld9040.c b/drivers/gpu/drm/panel/panel-ld9040.c
index 08cf2c5..3337f30 100644
--- a/drivers/gpu/drm/panel/panel-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-ld9040.c
@@ -361,7 +361,6 @@ static int ld9040_remove(struct spi_device *spi)
 {
 	struct ld9040 *ctx = spi_get_drvdata(spi);
 
-	ld9040_power_off(ctx);
 	drm_panel_remove(&ctx->panel);
 
 	return 0;
-- 
1.9.1

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

* [RFC 15/15] drm/panel/ld9040: convert to restrack API
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (13 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 14/15] drm/panel/ld9040: do not power off panel on removal Andrzej Hajda
@ 2014-12-10 15:48 ` Andrzej Hajda
  2014-12-10 16:16 ` [RFC 00/15] Resource tracking/allocation framework Russell King - ARM Linux
  15 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-10 15:48 UTC (permalink / raw)
  To: linux-arm-kernel

Convert ld9040 panel driver to use restrack API.
As a result driver have following advantages:
- correctly handles removal of resources,
- do not need to defer probing, so as a result the panel
  becomes available as soon as possible, in case of deferred
  probing it was late_initcall,
- simplified initialization.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/panel/panel-ld9040.c | 41 ++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-ld9040.c b/drivers/gpu/drm/panel/panel-ld9040.c
index 3337f30..619610e 100644
--- a/drivers/gpu/drm/panel/panel-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-ld9040.c
@@ -17,6 +17,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/restrack.h>
 
 #include <video/mipi_display.h>
 #include <video/of_videomode.h>
@@ -310,9 +311,22 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
 	return 0;
 }
 
+void ld9040_restrack_cb(struct device *dev, int ret)
+{
+	struct ld9040 *ctx = dev_get_drvdata(dev);
+
+	if (ret == 0)
+		drm_panel_add(&ctx->panel);
+	else if (ret == -EPROBE_DEFER)
+		drm_panel_remove(&ctx->panel);
+	else
+		dev_err(dev, "restrack error %d\n", ret);
+}
+
 static int ld9040_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct restrack_ctx *rtrack;
 	struct ld9040 *ctx;
 	int ret;
 
@@ -331,17 +345,6 @@ static int ld9040_probe(struct spi_device *spi)
 
 	ctx->supplies[0].supply = "vdd3";
 	ctx->supplies[1].supply = "vci";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
-				      ctx->supplies);
-	if (ret < 0)
-		return ret;
-
-	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ctx->reset_gpio)) {
-		dev_err(dev, "cannot get reset-gpios %ld\n",
-			PTR_ERR(ctx->reset_gpio));
-		return PTR_ERR(ctx->reset_gpio);
-	}
 
 	spi->bits_per_word = 9;
 	ret = spi_setup(spi);
@@ -354,16 +357,13 @@ static int ld9040_probe(struct spi_device *spi)
 	ctx->panel.dev = dev;
 	ctx->panel.funcs = &ld9040_drm_funcs;
 
-	return drm_panel_add(&ctx->panel);
-}
-
-static int ld9040_remove(struct spi_device *spi)
-{
-	struct ld9040 *ctx = spi_get_drvdata(spi);
-
-	drm_panel_remove(&ctx->panel);
+	rtrack = devm_restrack_register(ctx->dev, ld9040_restrack_cb,
+		regulator_bulk_restrack_desc(&ctx->supplies[0]),
+		regulator_bulk_restrack_desc(&ctx->supplies[1]),
+		gpiod_restrack_desc(&ctx->reset_gpio, "reset", GPIOD_OUT_HIGH),
+	);
 
-	return 0;
+	return PTR_ERR_OR_ZERO(rtrack);
 }
 
 static struct of_device_id ld9040_of_match[] = {
@@ -374,7 +374,6 @@ MODULE_DEVICE_TABLE(of, ld9040_of_match);
 
 static struct spi_driver ld9040_driver = {
 	.probe		= ld9040_probe,
-	.remove		= ld9040_remove,
 	.driver = {
 		.name	= "ld9040",
 		.owner	= THIS_MODULE,
-- 
1.9.1

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

* [RFC 04/15] regulator: add restrack support
  2014-12-10 15:48 ` [RFC 04/15] regulator: " Andrzej Hajda
@ 2014-12-10 16:07   ` Mark Brown
  2014-12-11 10:49     ` Andrzej Hajda
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2014-12-10 16:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:
> Regulators supports various methods of lookup.
> The patch adds restrack support only to DT based regulators.

Why, what does this mean and how might one use it?  I've not looked at
the code since I don't know what it's supposed to accomplish...

One very high level thing is that anything that only works for DT only
seems to be a non-starter, the API should be hiding details of the
firmware interface.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141210/2edabc3e/attachment.sig>

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

* [RFC 00/15] Resource tracking/allocation framework
  2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
                   ` (14 preceding siblings ...)
  2014-12-10 15:48 ` [RFC 15/15] drm/panel/ld9040: convert to restrack API Andrzej Hajda
@ 2014-12-10 16:16 ` Russell King - ARM Linux
       [not found]   ` <CA+tnuKYSDZ0mtzQ_g1QjovJwG01OgPjSXuozEBXQsEZv_2o-=w@mail.gmail.com>
  15 siblings, 1 reply; 31+ messages in thread
From: Russell King - ARM Linux @ 2014-12-10 16:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 04:48:18PM +0100, Andrzej Hajda wrote:
> 3. There are drivers which can work without specific resource, but if
>   the resource becomes available/unavailable it can do some additional stuff.
>   An example of such driver is DRM driver (more precisely drm_connector) -
>   it can start without attached drm_panel, but if the panel becomes available it
>   can react by generating HPD event and start using it.

Bad example, and actually incorrect.  DRM connectors are referenced in
userspace by an IDR number, which can be re-used in the case of a
connector appearing, disappearing, and then a different connector
re-appearing.

DRM really is *not* safe to hotplug like this: DRM is more a card-level
thing, which is why we have the component helpers - which allow us to
merge several devices into one logical card-like device in a generic
manner.

DRM needs a stable picture of the CRTCs, encoders and connectors, which
should _never_ change during the lifetime of the DRM device.  Devices
attached to connectors can be hotplugged, but that's about the limit of
hot-plugging in DRM.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

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

* [RFC 00/15] Resource tracking/allocation framework
       [not found]   ` <CA+tnuKYSDZ0mtzQ_g1QjovJwG01OgPjSXuozEBXQsEZv_2o-=w@mail.gmail.com>
@ 2014-12-10 17:39     ` Russell King - ARM Linux
  0 siblings, 0 replies; 31+ messages in thread
From: Russell King - ARM Linux @ 2014-12-10 17:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 06:23:49PM +0100, A H wrote:
> 10 gru 2014 17:16 "Russell King - ARM Linux" <linux@arm.linux.org.uk>
> napisa?(a):
> >
> > On Wed, Dec 10, 2014 at 04:48:18PM +0100, Andrzej Hajda wrote:
> > > 3. There are drivers which can work without specific resource, but if
> > >   the resource becomes available/unavailable it can do some additional
> stuff.
> > >   An example of such driver is DRM driver (more precisely
> drm_connector) -
> > >   it can start without attached drm_panel, but if the panel becomes
> available it
> > >   can react by generating HPD event and start using it.
> >
> > Bad example, and actually incorrect.  DRM connectors are referenced in
> > userspace by an IDR number, which can be re-used in the case of a
> > connector appearing, disappearing, and then a different connector
> > re-appearing.
> 
> But it is not about reappearing of drm_connector, it is about reappearing
> of drm_panel which is fortunately not a part of drm driver. Connector here
> is only consumer of drm_panel which should have possibility to receive
> notifications on panel (dis-)appearance.

Okay, so that's exactly like a panel being "hotplugged" to the connector,
which should be fine.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

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

* [RFC 04/15] regulator: add restrack support
  2014-12-10 16:07   ` Mark Brown
@ 2014-12-11 10:49     ` Andrzej Hajda
  2014-12-11 12:58       ` Mark Brown
  0 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-11 10:49 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Mark,

On 12/10/2014 05:07 PM, Mark Brown wrote:
> On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:
>> Regulators supports various methods of lookup.
>> The patch adds restrack support only to DT based regulators.
> Why, what does this mean and how might one use it?  I've not looked at
> the code since I don't know what it's supposed to accomplish...

Looking at this patch makes no sense without looking at cover letter
or at the patch adding restrack framework. In short adding restrack support
to regulators will allow to:
- proper handle regulator provider unbind/re-bind, currently it results
in oopses, crashes and hangs,
- avoid late probe due to deferred probing, currently if probe is
deferred, re-probe occurs in late initcall,
- track appearance of resources which can alter behavior of the driver
if present but they are not required,
  I am not sure if there are use cases for it in case of regulators, but
other resources have such use cases,
- as a bonus we can have simpler allocation of various resources, please
look at cover letter for example.

>
> One very high level thing is that anything that only works for DT only
> seems to be a non-starter, the API should be hiding details of the
> firmware interface.

I agree, but as this is RFC, not everything is finished. It seems I have
forgotten to mention it clearly in cover
letter.
Anyway it seems I should adjust patchset and move matching code from
restrack/track core to specific frameworks.
This way any current or future lookup method should be supported.

But the main purpose of this patchset is to get opinions, if the main
ideas are OK. And if the patchset can be
eventually accepted.

Regards
Andrzej

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

* [RFC 04/15] regulator: add restrack support
  2014-12-11 10:49     ` Andrzej Hajda
@ 2014-12-11 12:58       ` Mark Brown
  2014-12-11 13:43         ` Russell King - ARM Linux
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2014-12-11 12:58 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 11, 2014 at 11:49:54AM +0100, Andrzej Hajda wrote:
> On 12/10/2014 05:07 PM, Mark Brown wrote:
> > On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:

> >> Regulators supports various methods of lookup.
> >> The patch adds restrack support only to DT based regulators.

> > Why, what does this mean and how might one use it?  I've not looked at
> > the code since I don't know what it's supposed to accomplish...

> Looking at this patch makes no sense without looking at cover letter
> or at the patch adding restrack framework. In short adding restrack support
> to regulators will allow to:

I did look at the cover letter and glance at the first couple of patches
but I still can't tell what this is really intended to do.  There's a
lot of implementation detail but not a lot of high level overview so
it's not very clear to me how exactly this solves problems or how it is
different to the component framework, it sounds very similar.

I'd expect someone reading the change in the regulator API to have at
least some idea how this fits in with the rest of the API and how to use
it, and probably more importantly I'd expect to be able to understand
why this is DT only.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141211/26ed42f8/attachment-0001.sig>

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

* [RFC 04/15] regulator: add restrack support
  2014-12-11 12:58       ` Mark Brown
@ 2014-12-11 13:43         ` Russell King - ARM Linux
  2014-12-12  8:22           ` Andrzej Hajda
  0 siblings, 1 reply; 31+ messages in thread
From: Russell King - ARM Linux @ 2014-12-11 13:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 11, 2014 at 12:58:37PM +0000, Mark Brown wrote:
> I'd expect someone reading the change in the regulator API to have at
> least some idea how this fits in with the rest of the API and how to use
> it, and probably more importantly I'd expect to be able to understand
> why this is DT only.

Yep.

This is a repetitive problem, and I fully agree with your concern about
stuff which is supposed to be arch-independent being designed with only
DT in mind.

New core kernel features should *not* be designed with only DT in mind -
DT is not the only firmware description language which the kernel
supports.  Folk need to understand that if they design a new arch
independent kernel feature where the sole use case is with DT, that new
feature is probably going to get rejected, especially when it's
something as generic as resource tracking.

The world is not DT only.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

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

* [RFC 04/15] regulator: add restrack support
  2014-12-11 13:43         ` Russell King - ARM Linux
@ 2014-12-12  8:22           ` Andrzej Hajda
  0 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-12  8:22 UTC (permalink / raw)
  To: linux-arm-kernel

On 12/11/2014 02:43 PM, Russell King - ARM Linux wrote:
> On Thu, Dec 11, 2014 at 12:58:37PM +0000, Mark Brown wrote:
>> I'd expect someone reading the change in the regulator API to have at
>> least some idea how this fits in with the rest of the API and how to use
>> it, and probably more importantly I'd expect to be able to understand
>> why this is DT only.
> 
> Yep.
> 
> This is a repetitive problem, and I fully agree with your concern about
> stuff which is supposed to be arch-independent being designed with only
> DT in mind.
> 
> New core kernel features should *not* be designed with only DT in mind -
> DT is not the only firmware description language which the kernel
> supports.  Folk need to understand that if they design a new arch
> independent kernel feature where the sole use case is with DT, that new
> feature is probably going to get rejected, especially when it's
> something as generic as resource tracking.
> 
> The world is not DT only.
> 

OK. I will post next version of patchset with resource/provider lookup
left to frameworks (regulators, clock, etc), this way it will be fully
firmware agnostic. I will add also better description of the framework.

Regards
Andrzej

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

* [RFC 01/15] drivers/base: add track framework
  2014-12-10 15:48 ` [RFC 01/15] drivers/base: add track framework Andrzej Hajda
@ 2014-12-12 16:36   ` Mark Brown
  2014-12-12 23:12     ` AH
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2014-12-12 16:36 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:
> track is a generic framework for safe tracking presence of any kernel objects
> having an id. There are no special requirements about type of object or its
> id. Id shall be unique.

This is pretty confusing, when it talks about "kernel objects" that
sounds like it means struct kobject but in fact there's no connection
with that or any of the other kernel object stuff.  Perhaps it makes
sense but it should be addressed.

> Typical usage of the framework by consumer looks as follow:
> 1. Consumer registers notifier callback for objects with given id.

This is also a custom thing not connected with the notifier mechanism
implemented by struct notifier_block.  Again this should probably be
addressed, even if it's just used internally it seems like there should
be some opportunity for code reuse here.

> +	case track_task_up:
> +		node = track_get_node(track, task->type, task->id, true);
> +
> +		node->up = true;
> +		node->data = task->data;
> +		list_for_each_entry_safe(itb, node->itb_next, &node->itb_head,
> +					 list)
> +			itb->callback(itb, node->data, true);
> +		return;
> +	case track_task_down:

I'm not sure the up and down naming is the most obvious naming for
users.  It's obviously inspired by semaphores but it's not entirely
obvious that this is going to make things clear and meaningful for
someone trying to understand the interface.

> +static int track_process_queue(struct tracker *track)
> +{
> +	struct track_task *task, *ptask = NULL;
> +	unsigned long flags;
> +	bool empty;
> +
> +	/* Queue non-emptiness is used as a sentinel to prevent processing
> +	 * by multiple threads, so we cannot delete entry from the queue
> +	 * until it is processed.
> +	 */
> +	while (true) {
> +		spin_lock_irqsave(&track->queue_lock, flags);
> +
> +		if (ptask)
> +			list_del(&ptask->list);
> +		task = list_first_entry(&track->queue,
> +					struct track_task, list);
> +
> +		empty = list_empty(&track->queue);
> +		if (empty)
> +			complete_all(&track->queue_empty);
> +
> +		spin_unlock_irqrestore(&track->queue_lock, flags);

Here we get a task from the head of the list and drop the lock, leaving
the task on the list...

> +		kfree(ptask);
> +
> +		if (empty)
> +			break;
> +
> +		track_process_task(track, task);

...we then go and do some other stuff, including processing that task,
without the lock or or any other means I can see of excluding other
users before going round and removing the task.  This seems to leave us
vulnerable to double execution.  I *think* this is supposed to be
handled by your comment "Provider should take care of calling
notifications synchronously in proper order" in the changelog but that's
a bit obscure, it's not specific about what the requirements are (or
what the limits are supposed to be on the notification callbacks).

I'm also unclear what is supposed to happen if adding a notification
races with removing the thing being watched.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141212/01edde99/attachment.sig>

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

* [RFC 02/15] drivers/base: add restrack framework
  2014-12-10 15:48 ` [RFC 02/15] drivers/base: add restrack framework Andrzej Hajda
@ 2014-12-12 16:52   ` Mark Brown
  2014-12-15  8:28     ` Andrzej Hajda
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2014-12-12 16:52 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 04:48:20PM +0100, Andrzej Hajda wrote:
> restrack framework allows tracking presence of resources with dynamic life
> time. Typical example of such resources are all resources provided by device

I don't know about anyone else but I'm having a hard time reading the
restrack name, it looks like a misspelling of restack to me.

At a high level my biggest questions here are the relationship between
this and the component code and usability.  The usability concern I have
is that I see no diagnostics or trace here at all.  This means that if a
user looks at their system, sees that the device model claims the driver
for a device bound to the device but can't see any sign of the device
doing anything they don't have any way of telling why that is other than
to look in the driver code, see what resources it was trying to depend
on and then go back to the running system to try to understand which of
those resources hasn't appeared.

> +int restrack_up(unsigned long type, const void *id, void *data)

> +int restrack_down(unsigned long type, const void *id, void *data)

Again I'm not sure that the up and down naming is meaningful in the
context of this interface.

> +static void restrack_itb_cb(struct track_block *itb, void *data, bool on)
> +{

itb_cb?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141212/10951c43/attachment.sig>

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

* [RFC 01/15] drivers/base: add track framework
  2014-12-12 16:36   ` Mark Brown
@ 2014-12-12 23:12     ` AH
  2014-12-15 12:55       ` Mark Brown
  0 siblings, 1 reply; 31+ messages in thread
From: AH @ 2014-12-12 23:12 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Mark,

Thanks for review.

Mark Brown wrote on 12.12.2014 17:36:
> On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:
>> track is a generic framework for safe tracking presence of any kernel objects
>> having an id. There are no special requirements about type of object or its
>> id. Id shall be unique.
>
> This is pretty confusing, when it talks about "kernel objects" that
> sounds like it means struct kobject but in fact there's no connection
> with that or any of the other kernel object stuff.  Perhaps it makes
> sense but it should be addressed.
>

OK

>> Typical usage of the framework by consumer looks as follow:
>> 1. Consumer registers notifier callback for objects with given id.
>
> This is also a custom thing not connected with the notifier mechanism
> implemented by struct notifier_block.  Again this should probably be
> addressed, even if it's just used internally it seems like there should
> be some opportunity for code reuse here.

I though about using notfier_block, but beside the struct itself there 
wouldn't be too much code to reuse. In my previous attempt of this 
framework more arguments were passed to the callback so I have dropped
this idea completely, but now after simplifying the callback it fits better.

>
>> +	case track_task_up:
>> +		node = track_get_node(track, task->type, task->id, true);
>> +
>> +		node->up = true;
>> +		node->data = task->data;
>> +		list_for_each_entry_safe(itb, node->itb_next, &node->itb_head,
>> +					 list)
>> +			itb->callback(itb, node->data, true);
>> +		return;
>> +	case track_task_down:
>
> I'm not sure the up and down naming is the most obvious naming for
> users.  It's obviously inspired by semaphores but it's not entirely
> obvious that this is going to make things clear and meaningful for
> someone trying to understand the interface.

In my 1st attempt I have called the framework interface_tracker, so it 
was named ifup/ifdown after unix commands:) Now it is less obvious.
Finding good names is always painful, anyway I will think about it.

>
>> +static int track_process_queue(struct tracker *track)
>> +{
>> +	struct track_task *task, *ptask = NULL;
>> +	unsigned long flags;
>> +	bool empty;
>> +
>> +	/* Queue non-emptiness is used as a sentinel to prevent processing
>> +	 * by multiple threads, so we cannot delete entry from the queue
>> +	 * until it is processed.
>> +	 */
>> +	while (true) {
>> +		spin_lock_irqsave(&track->queue_lock, flags);
>> +
>> +		if (ptask)
>> +			list_del(&ptask->list);
>> +		task = list_first_entry(&track->queue,
>> +					struct track_task, list);
>> +
>> +		empty = list_empty(&track->queue);
>> +		if (empty)
>> +			complete_all(&track->queue_empty);
>> +
>> +		spin_unlock_irqrestore(&track->queue_lock, flags);
>
> Here we get a task from the head of the list and drop the lock, leaving
> the task on the list...

Yes and it is explained in the comment few lines above.

>
>> +		kfree(ptask);
>> +
>> +		if (empty)
>> +			break;
>> +
>> +		track_process_task(track, task);
>
> ...we then go and do some other stuff, including processing that task,
> without the lock or or any other means I can see of excluding other
> users before going round and removing the task.  This seems to leave us
> vulnerable to double execution.

No, if you look at track_add_task function you will see that the queue 
is processed only if it is initially empty, otherwise the task is only 
added to the queue, so it will be processed after processing earlier tasks.
So the rule is that if someone add task to the queue it checks if the 
queue is empty, in such case it process all tasks from the queue until
the queue becomes empty, even the tasks added by other processed.
This way all tasks are serialized.


> I *think* this is supposed to be
> handled by your comment "Provider should take care of calling
> notifications synchronously in proper order" in the changelog but that's
> a bit obscure, it's not specific about what the requirements are (or
> what the limits are supposed to be on the notification callbacks).

No, this comment is just to avoid advertising object (ie calling 
track_up) that can be just removed by concurrent thread.


>
> I'm also unclear what is supposed to happen if adding a notification
> races with removing the thing being watched.
>

The sequence should be always as follows:
1. create thing, then call track_up(thing).
...
2. call track_down(thing) then remove thing.

If we put 1 into probe and 2 into remove callback of the driver it will 
be safe - we are synchronised by device_lock. But if, for some reason, 
we want to create object after probe we should do own synchronization or 
just put device_lock around 1. The same applies if we want to remove
object earlier. This is the comment above about. I will expand it to 
more verbose explanation.

Regards
Andrzej

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

* [RFC 02/15] drivers/base: add restrack framework
  2014-12-12 16:52   ` Mark Brown
@ 2014-12-15  8:28     ` Andrzej Hajda
  2014-12-15 11:38       ` Mark Brown
  0 siblings, 1 reply; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-15  8:28 UTC (permalink / raw)
  To: linux-arm-kernel

On 12/12/2014 05:52 PM, Mark Brown wrote:
> On Wed, Dec 10, 2014 at 04:48:20PM +0100, Andrzej Hajda wrote:
>> restrack framework allows tracking presence of resources with dynamic life
>> time. Typical example of such resources are all resources provided by device
> I don't know about anyone else but I'm having a hard time reading the
> restrack name, it looks like a misspelling of restack to me.

Any alternative names?

>
> At a high level my biggest questions here are the relationship between
> this and the component code and usability.  The usability concern I have
> is that I see no diagnostics or trace here at all.  This means that if a
> user looks at their system, sees that the device model claims the driver
> for a device bound to the device but can't see any sign of the device
> doing anything they don't have any way of telling why that is other than
> to look in the driver code, see what resources it was trying to depend
> on and then go back to the running system to try to understand which of
> those resources hasn't appeared.

I will move the code for provider matching to frameworks,
so it will be easy to add just dev_info after every failed attempt
of getting resource, including deferring. This is the simplest solution
and it should be similar in verbosity to deferred probing.

Maybe other solution is to provide debug_fs (or device) attribute showing
restrack status per device.

>
>> +int restrack_up(unsigned long type, const void *id, void *data)
>> +int restrack_down(unsigned long type, const void *id, void *data)
> Again I'm not sure that the up and down naming is meaningful in the
> context of this interface.
>
>> +static void restrack_itb_cb(struct track_block *itb, void *data, bool on)
>> +{
> itb_cb?

Ups I forgot to rename few variables from my previous attempt.
itb - stayed for interface tracker block.

Regards
Andrzej

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

* [RFC 02/15] drivers/base: add restrack framework
  2014-12-15  8:28     ` Andrzej Hajda
@ 2014-12-15 11:38       ` Mark Brown
  0 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2014-12-15 11:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Dec 15, 2014 at 09:28:41AM +0100, Andrzej Hajda wrote:
> On 12/12/2014 05:52 PM, Mark Brown wrote:
> > On Wed, Dec 10, 2014 at 04:48:20PM +0100, Andrzej Hajda wrote:

> > I don't know about anyone else but I'm having a hard time reading the
> > restrack name, it looks like a misspelling of restack to me.

> Any alternative names?

Well, even just res_track would help.

> I will move the code for provider matching to frameworks,
> so it will be easy to add just dev_info after every failed attempt
> of getting resource, including deferring. This is the simplest solution
> and it should be similar in verbosity to deferred probing.

> Maybe other solution is to provide debug_fs (or device) attribute showing
> restrack status per device.

I think both are useful - it's often helpful to have a listing of what
resources have actually been registered, for example to help spot typos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141215/1bebb5ca/attachment.sig>

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

* [RFC 01/15] drivers/base: add track framework
  2014-12-12 23:12     ` AH
@ 2014-12-15 12:55       ` Mark Brown
  2014-12-15 14:00         ` Andrzej Hajda
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2014-12-15 12:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Dec 13, 2014 at 12:12:19AM +0100, AH wrote:
> Mark Brown wrote on 12.12.2014 17:36:
> >On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:

> >>+		kfree(ptask);
> >>+
> >>+		if (empty)
> >>+			break;
> >>+
> >>+		track_process_task(track, task);

> >...we then go and do some other stuff, including processing that task,
> >without the lock or or any other means I can see of excluding other
> >users before going round and removing the task.  This seems to leave us
> >vulnerable to double execution.

> No, if you look at track_add_task function you will see that the queue is
> processed only if it is initially empty, otherwise the task is only added to
> the queue, so it will be processed after processing earlier tasks.
> So the rule is that if someone add task to the queue it checks if the queue
> is empty, in such case it process all tasks from the queue until
> the queue becomes empty, even the tasks added by other processed.
> This way all tasks are serialized.

This is all pretty fiddly and seems fragile - if nothing else the code
seems undercommented since the above is only going to be apparent with
following through multiple functions and we're relying on both owner and
list emptiness with more than one place where a task can get processed.

> >I'm also unclear what is supposed to happen if adding a notification
> >races with removing the thing being watched.

> The sequence should be always as follows:
> 1. create thing, then call track_up(thing).
> ...
> 2. call track_down(thing) then remove thing.

> If we put 1 into probe and 2 into remove callback of the driver it will be
> safe - we are synchronised by device_lock. But if, for some reason, we want
> to create object after probe we should do own synchronization or just put
> device_lock around 1. The same applies if we want to remove
> object earlier. This is the comment above about. I will expand it to more
> verbose explanation.

You can't rely on the device lock here since this isn't tied to kobjects
or anything at all - it's a freestanding interface someone could pick up
and use in another context.  Besides, that isn't really my concern - my
concern is what happens if something asks to wait for 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141215/2a78aef9/attachment.sig>

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

* [RFC 01/15] drivers/base: add track framework
  2014-12-15 12:55       ` Mark Brown
@ 2014-12-15 14:00         ` Andrzej Hajda
  0 siblings, 0 replies; 31+ messages in thread
From: Andrzej Hajda @ 2014-12-15 14:00 UTC (permalink / raw)
  To: linux-arm-kernel

On 12/15/2014 01:55 PM, Mark Brown wrote:
> On Sat, Dec 13, 2014 at 12:12:19AM +0100, AH wrote:
>> Mark Brown wrote on 12.12.2014 17:36:
>>> On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:
>>>> +		kfree(ptask);
>>>> +
>>>> +		if (empty)
>>>> +			break;
>>>> +
>>>> +		track_process_task(track, task);
>>> ...we then go and do some other stuff, including processing that task,
>>> without the lock or or any other means I can see of excluding other
>>> users before going round and removing the task.  This seems to leave us
>>> vulnerable to double execution.
>> No, if you look at track_add_task function you will see that the queue is
>> processed only if it is initially empty, otherwise the task is only added to
>> the queue, so it will be processed after processing earlier tasks.
>> So the rule is that if someone add task to the queue it checks if the queue
>> is empty, in such case it process all tasks from the queue until
>> the queue becomes empty, even the tasks added by other processed.
>> This way all tasks are serialized.
> This is all pretty fiddly and seems fragile - if nothing else the code
> seems undercommented since the above is only going to be apparent with
> following through multiple functions and we're relying on both owner and
> list emptiness with more than one place where a task can get processed.

I have changed it already to test queue owner, this way it should
be more clear.

>
>>> I'm also unclear what is supposed to happen if adding a notification
>>> races with removing the thing being watched.
>> The sequence should be always as follows:
>> 1. create thing, then call track_up(thing).
>> ...
>> 2. call track_down(thing) then remove thing.
>> If we put 1 into probe and 2 into remove callback of the driver it will be
>> safe - we are synchronised by device_lock. But if, for some reason, we want
>> to create object after probe we should do own synchronization or just put
>> device_lock around 1. The same applies if we want to remove
>> object earlier. This is the comment above about. I will expand it to more
>> verbose explanation.
> You can't rely on the device lock here since this isn't tied to kobjects
> or anything at all - it's a freestanding interface someone could pick up
> and use in another context.  Besides, that isn't really my concern - my
> concern is what happens if something asks to wait for 
But I do not rely here on device_lock, I just point out that 1 and 2
should be
synchronized and as a common way is to put such things into probe
and remove, device_lock can do the synchronization for us in such case,
so no need for additional synchronization.

And to make everything clear, track_up will not be called by the driver
directly,
it shall be called by respective resource framework functions, for example
by regulator_register and regulator_unregister.

And regarding your initial/real concern, I guess you mean this one:
>> I'm also unclear what is supposed to happen if adding a notification
>> races with removing the thing being watched.
I guess you mean registering notifier and removal of thing it is
supposed to watch.
As all track tasks are serialized these two will be serialized also so
we can have
only two scenarios:
1.
  a) register notifier
  - thing is up, so notifier will be immediately called with info that
the thing is up
  b) remove thing
  - thing will be down, so notifier will be called with info that the
thing will be removed
2.
  a) remove thing
  - notifier is not yet registered, so callback will not be called,
  b) register notifier
  - thing is already removed, so callback will not be called.

I hope this is what you were asking for.

Regards
Andrzej

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

* [RFC 05/15] gpio: move DT parsing code to separate functions
  2014-12-10 15:48 ` [RFC 05/15] gpio: move DT parsing code to separate functions Andrzej Hajda
@ 2015-01-13  6:17   ` Linus Walleij
  0 siblings, 0 replies; 31+ messages in thread
From: Linus Walleij @ 2015-01-13  6:17 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 10, 2014 at 4:48 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:

> The patch moves Device Tree parsing logic to separate
> function.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

I think this patch need to be rebased on the GPIO devel branch, but
looks like it makes things cleaner so can stand on its own merits.
We moves stuff around a bit in the OF code though.

Please remember to send this patch to the linux-gpio list and comaintainer
Alexandre Courbot on reposts. (Cc-tags in the patch are good
for this.)

Yours,
Linus Walleij

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

end of thread, other threads:[~2015-01-13  6:17 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-10 15:48 [RFC 00/15] Resource tracking/allocation framework Andrzej Hajda
2014-12-10 15:48 ` [RFC 01/15] drivers/base: add track framework Andrzej Hajda
2014-12-12 16:36   ` Mark Brown
2014-12-12 23:12     ` AH
2014-12-15 12:55       ` Mark Brown
2014-12-15 14:00         ` Andrzej Hajda
2014-12-10 15:48 ` [RFC 02/15] drivers/base: add restrack framework Andrzej Hajda
2014-12-12 16:52   ` Mark Brown
2014-12-15  8:28     ` Andrzej Hajda
2014-12-15 11:38       ` Mark Brown
2014-12-10 15:48 ` [RFC 03/15] drm/panel: add restrack support Andrzej Hajda
2014-12-10 15:48 ` [RFC 04/15] regulator: " Andrzej Hajda
2014-12-10 16:07   ` Mark Brown
2014-12-11 10:49     ` Andrzej Hajda
2014-12-11 12:58       ` Mark Brown
2014-12-11 13:43         ` Russell King - ARM Linux
2014-12-12  8:22           ` Andrzej Hajda
2014-12-10 15:48 ` [RFC 05/15] gpio: move DT parsing code to separate functions Andrzej Hajda
2015-01-13  6:17   ` Linus Walleij
2014-12-10 15:48 ` [RFC 06/15] gpio: add restrack support Andrzej Hajda
2014-12-10 15:48 ` [RFC 07/15] clk: add DT parsing function Andrzej Hajda
2014-12-10 15:48 ` [RFC 08/15] clk: add restrack support Andrzej Hajda
2014-12-10 15:48 ` [RFC 09/15] phy: " Andrzej Hajda
2014-12-10 15:48 ` [RFC 10/15] drm/exynos/dsi: simplify hotplug code Andrzej Hajda
2014-12-10 15:48 ` [RFC 11/15] drm/exynos/dsi: convert to restrack API Andrzej Hajda
2014-12-10 15:48 ` [RFC 12/15] drm/exynos/dpi: use common of_graph functions Andrzej Hajda
2014-12-10 15:48 ` [RFC 13/15] drm/exynos/dpi: convert to restrack API Andrzej Hajda
2014-12-10 15:48 ` [RFC 14/15] drm/panel/ld9040: do not power off panel on removal Andrzej Hajda
2014-12-10 15:48 ` [RFC 15/15] drm/panel/ld9040: convert to restrack API Andrzej Hajda
2014-12-10 16:16 ` [RFC 00/15] Resource tracking/allocation framework Russell King - ARM Linux
     [not found]   ` <CA+tnuKYSDZ0mtzQ_g1QjovJwG01OgPjSXuozEBXQsEZv_2o-=w@mail.gmail.com>
2014-12-10 17:39     ` Russell King - ARM Linux

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).