[PATCH RFC 0/3] Implement refcounting for OF GPIO chips

[PATCH RFC 0/3] Implement refcounting for OF GPIO chips

[Anton Vorontsov]

Hi all,

OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
that of_get_gpio()'s returned GPIO number will be no longer valid, or
worse, it may point to an unexpected GPIO controller.

This scenario is possible:

driver A:               driver B:              driver C:
---------               ---------              ---------
                        gpiochip_add()
gpio = of_get_gpio()
                        gpiochip_remove()
                                               gpiochip_add()
gpio_request(gpio);
gpio_set_value(gpio);

That is, driver A assumes that it is working with GPIO from driver B,
but in practice it may disappear and driver C will take its GPIO base
number, so it will provide the same GPIO numbers.

The above situation is hard to trigger, but the issue is there
nonetheless, and so needs fixing.

Thanks,

p.s. The patches are based on top of
     '[PATCH v2 0/4] OF GPIO integration for I2C/SPI GPIO chips'
     http://lkml.org/lkml/2010/2/5/243

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/3] of platforms: Move common static initialization to of_node_init()

[Anton Vorontsov]

So far of_node_init() just initializes a kref, later we'll have to
initialize other fields (for example node->data_lock).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/microblaze/kernel/prom.c             |    4 ++--
 arch/powerpc/kernel/prom.c                |    4 ++--
 arch/powerpc/platforms/iseries/vio.c      |    5 +++--
 arch/powerpc/platforms/pseries/dlpar.c    |    7 ++++---
 arch/powerpc/platforms/pseries/reconfig.c |    7 ++++---
 arch/powerpc/sysdev/msi_bitmap.c          |    4 ++--
 arch/sparc/kernel/prom_common.c           |    6 +++---
 drivers/of/base.c                         |   10 ++++++++++
 include/linux/of.h                        |    2 ++
 9 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/arch/microblaze/kernel/prom.c b/arch/microblaze/kernel/prom.c
index b817df1..0003453 100644
--- a/arch/microblaze/kernel/prom.c
+++ b/arch/microblaze/kernel/prom.c
@@ -30,6 +30,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq.h>
 #include <linux/lmb.h>
+#include <linux/of.h>
 
 #include <asm/prom.h>
 #include <asm/page.h>
@@ -255,7 +256,7 @@ static unsigned long __init unflatten_dt_node(unsigned long mem,
 	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
 				__alignof__(struct device_node));
 	if (allnextpp) {
-		memset(np, 0, sizeof(*np));
+		of_node_init(np);
 		np->full_name = ((char *)np) + sizeof(struct device_node);
 		if (new_format) {
 			char *p2 = np->full_name;
@@ -287,7 +288,6 @@ static unsigned long __init unflatten_dt_node(unsigned long mem,
 				dad->next->sibling = np;
 			dad->next = np;
 		}
-		kref_init(&np->kref);
 	}
 	while (1) {
 		u32 sz, noff;
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 4ec3008..d8c2528 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -32,6 +32,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq.h>
 #include <linux/lmb.h>
+#include <linux/of.h>
 
 #include <asm/prom.h>
 #include <asm/rtas.h>
@@ -290,7 +291,7 @@ static unsigned long __init unflatten_dt_node(unsigned long mem,
 	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
 				__alignof__(struct device_node));
 	if (allnextpp) {
-		memset(np, 0, sizeof(*np));
+		of_node_init(np);
 		np->full_name = ((char*)np) + sizeof(struct device_node);
 		if (new_format) {
 			char *p = np->full_name;
@@ -321,7 +322,6 @@ static unsigned long __init unflatten_dt_node(unsigned long mem,
 				dad->next->sibling = np;
 			dad->next = np;
 		}
-		kref_init(&np->kref);
 	}
 	while(1) {
 		u32 sz, noff;
diff --git a/arch/powerpc/platforms/iseries/vio.c b/arch/powerpc/platforms/iseries/vio.c
index 2aa8b56..5f91a96 100644
--- a/arch/powerpc/platforms/iseries/vio.c
+++ b/arch/powerpc/platforms/iseries/vio.c
@@ -83,10 +83,12 @@ static void free_property(struct property *np)
 static struct device_node *new_node(const char *path,
 		struct device_node *parent)
 {
-	struct device_node *np = kzalloc(sizeof(*np), GFP_KERNEL);
+	struct device_node *np = kmalloc(sizeof(*np), GFP_KERNEL);
 
 	if (!np)
 		return NULL;
+
+	of_node_init(np);
 	np->full_name = kmalloc(strlen(path) + 1, GFP_KERNEL);
 	if (!np->full_name) {
 		kfree(np);
@@ -94,7 +96,6 @@ static struct device_node *new_node(const char *path,
 	}
 	strcpy(np->full_name, path);
 	of_node_set_flag(np, OF_DYNAMIC);
-	kref_init(&np->kref);
 	np->parent = of_node_get(parent);
 	return np;
 }
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 37bce52..72043ad 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -11,11 +11,11 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/kref.h>
 #include <linux/notifier.h>
 #include <linux/proc_fs.h>
 #include <linux/spinlock.h>
 #include <linux/cpu.h>
+#include <linux/of.h>
 #include "offline_states.h"
 
 #include <asm/prom.h>
@@ -69,10 +69,12 @@ static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa)
 	struct device_node *dn;
 	char *name;
 
-	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+	dn = kmalloc(sizeof(*dn), GFP_KERNEL);
 	if (!dn)
 		return NULL;
 
+	of_node_init(dn);
+
 	/* The configure connector reported name does not contain a
 	 * preceeding '/', so we allocate a buffer large enough to
 	 * prepend this to the full_name.
@@ -242,7 +244,6 @@ int dlpar_attach_node(struct device_node *dn)
 	int rc;
 
 	of_node_set_flag(dn, OF_DYNAMIC);
-	kref_init(&dn->kref);
 	dn->parent = derive_parent(dn->full_name);
 	if (!dn->parent)
 		return -ENOMEM;
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index a2305d2..a0b65b7 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -12,9 +12,9 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/kref.h>
 #include <linux/notifier.h>
 #include <linux/proc_fs.h>
+#include <linux/of.h>
 
 #include <asm/prom.h>
 #include <asm/machdep.h>
@@ -113,10 +113,12 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 	struct device_node *np;
 	int err = -ENOMEM;
 
-	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	np = kmalloc(sizeof(*np), GFP_KERNEL);
 	if (!np)
 		goto out_err;
 
+	of_node_init(np);
+
 	np->full_name = kmalloc(strlen(path) + 1, GFP_KERNEL);
 	if (!np->full_name)
 		goto out_err;
@@ -125,7 +127,6 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 
 	np->properties = proplist;
 	of_node_set_flag(np, OF_DYNAMIC);
-	kref_init(&np->kref);
 
 	np->parent = derive_parent(path);
 	if (IS_ERR(np->parent)) {
diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c
index 5a32cbe..0f259ff 100644
--- a/arch/powerpc/sysdev/msi_bitmap.c
+++ b/arch/powerpc/sysdev/msi_bitmap.c
@@ -10,6 +10,7 @@
 
 #include <linux/kernel.h>
 #include <linux/bitmap.h>
+#include <linux/of.h>
 #include <asm/msi_bitmap.h>
 
 int msi_bitmap_alloc_hwirqs(struct msi_bitmap *bmp, int num)
@@ -199,8 +200,7 @@ void __init test_of_node(void)
 	DECLARE_BITMAP(expected, size);
 
 	/* There should really be a struct device_node allocator */
-	memset(&of_node, 0, sizeof(of_node));
-	kref_init(&of_node.kref);
+	of_node_init(&of_node);
 	of_node.full_name = node_name;
 
 	check(0 == msi_bitmap_alloc(&bmp, size, &of_node));
diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
index d80a65d..dbfad05 100644
--- a/arch/sparc/kernel/prom_common.c
+++ b/arch/sparc/kernel/prom_common.c
@@ -229,11 +229,11 @@ static struct device_node * __init prom_create_node(phandle node,
 		return NULL;
 
 	dp = prom_early_alloc(sizeof(*dp));
-	dp->unique_id = prom_unique_id++;
-	dp->parent = parent;
 
-	kref_init(&dp->kref);
+	of_node_init(dp);
 
+	dp->unique_id = prom_unique_id++;
+	dp->parent = parent;
 	dp->name = get_one_property(node, "name");
 	dp->type = get_one_property(node, "device_type");
 	dp->node = node;
diff --git a/drivers/of/base.c b/drivers/of/base.c
index e6627b2..716d439 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -28,6 +28,16 @@ struct device_node *allnodes;
  */
 DEFINE_RWLOCK(devtree_lock);
 
+/**
+ * of_node_init - Initialize a device node
+ * @n:	Node to initialize
+ */
+void of_node_init(struct device_node *np)
+{
+	memset(np, 0, sizeof(*np));
+	kref_init(&np->kref);
+}
+
 int of_n_addr_cells(struct device_node *np)
 {
 	const int *ip;
diff --git a/include/linux/of.h b/include/linux/of.h
index e7facd8..717d690 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -63,6 +63,8 @@ struct device_node {
 #endif
 };
 
+extern void of_node_init(struct device_node *np);
+
 static inline int of_node_check_flag(struct device_node *n, unsigned long flag)
 {
 	return test_bit(flag, &n->_flags);
-- 
1.6.5.7

Re: [PATCH 1/3] of platforms: Move common static initialization to of_node_init()

[Grant Likely]

On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> So far of_node_init() just initializes a kref, later we'll have to
> initialize other fields (for example node->data_lock).
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0arch/microblaze/kernel/prom.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A04 ++--
> =A0arch/powerpc/kernel/prom.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A04 +=
+--
> =A0arch/powerpc/platforms/iseries/vio.c =A0 =A0 =A0| =A0 =A05 +++--
> =A0arch/powerpc/platforms/pseries/dlpar.c =A0 =A0| =A0 =A07 ++++---
> =A0arch/powerpc/platforms/pseries/reconfig.c | =A0 =A07 ++++---
> =A0arch/powerpc/sysdev/msi_bitmap.c =A0 =A0 =A0 =A0 =A0| =A0 =A04 ++--
> =A0arch/sparc/kernel/prom_common.c =A0 =A0 =A0 =A0 =A0 | =A0 =A06 +++---
> =A0drivers/of/base.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =
=A0 10 ++++++++++
> =A0include/linux/of.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =
=A0 =A02 ++
> =A09 files changed, 32 insertions(+), 17 deletions(-)

unflatten_dt_*() has been merged and moved to drivers/of.  Please
rebase this series on top of my next-devicetree branch in this git
tree:

git://git.secretlab.ca/git/linux-2.6

Otherwise, this looks fine to me.

g.

[PATCH 2/3] of: Introduce safe accessors for node->data

[Anton Vorontsov]

Platform code use node->data to store some private information
associated with a node.

Previously there was no need for any locks and accessors since we were
initializing the data mostly at boot time and never modified it later.

Though, nowadays OF GPIO infrastructure supports GPIO chips detaching,
so to handle this correctly we have to introduce locking for the
node->data field.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/base.c  |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h |    5 +++++
 2 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 716d439..41ed6ba 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -36,8 +36,53 @@ void of_node_init(struct device_node *np)
 {
 	memset(np, 0, sizeof(*np));
 	kref_init(&np->kref);
+	spin_lock_init(&np->data_lock);
 }
 
+/**
+ *	of_node_set_data - Try to set node->data
+ *	@node:	Node
+ *	@data:	Data
+ *
+ *	This function tries to safely set node->data. Returns 0 on success,
+ *	-EBUSY if node->data was already set.
+ */
+int of_node_set_data(struct device_node *np, void *data)
+{
+	int ret = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&np->data_lock, flags);
+	if (np->data)
+		ret = -EBUSY;
+	else
+		np->data = data;
+	spin_unlock_irqrestore(&np->data_lock, flags);
+
+	if (!ret)
+		of_node_get(np);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_node_set_data);
+
+/**
+ *	of_node_release_data_unlocked - Release node->data
+ *	@node:	Node
+ *
+ *	This function releases node->data, so that others could set it.
+ *
+ *	This function doesn't grab any locks, so that a caller can grab the
+ *	lock itself, atomically read the data and decide if it wants to
+ *	release it.
+ */
+void of_node_release_data_unlocked(struct device_node *np)
+{
+	np->data = NULL;
+	of_node_put(np);
+}
+EXPORT_SYMBOL(of_node_release_data_unlocked);
+
 int of_n_addr_cells(struct device_node *np)
 {
 	const int *ip;
diff --git a/include/linux/of.h b/include/linux/of.h
index 717d690..c573ee2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <linux/bitops.h>
 #include <linux/kref.h>
+#include <linux/spinlock.h>
 #include <linux/mod_devicetable.h>
 
 typedef u32 phandle;
@@ -56,6 +57,7 @@ struct device_node {
 	struct	kref kref;
 	unsigned long _flags;
 	void	*data;
+	spinlock_t data_lock;
 #if defined(CONFIG_SPARC)
 	char	*path_component_name;
 	unsigned int unique_id;
@@ -65,6 +67,9 @@ struct device_node {
 
 extern void of_node_init(struct device_node *np);
 
+extern int of_node_set_data(struct device_node *np, void *data);
+extern void of_node_release_data_unlocked(struct device_node *np);
+
 static inline int of_node_check_flag(struct device_node *n, unsigned long flag)
 {
 	return test_bit(flag, &n->_flags);
-- 
1.6.5.7

Re: [PATCH 2/3] of: Introduce safe accessors for node->data

[Grant Likely]

On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Platform code use node->data to store some private information
> associated with a node.
>
> Previously there was no need for any locks and accessors since we were
> initializing the data mostly at boot time and never modified it later.
>
> Though, nowadays OF GPIO infrastructure supports GPIO chips detaching,
> so to handle this correctly we have to introduce locking for the
> node->data field.

I'm not convinced this is needed.  What's wrong with using the
whole-tree devtree_lock?

g.

Re: [PATCH 2/3] of: Introduce safe accessors for node->data

[Anton Vorontsov]

On Tue, Feb 09, 2010 at 10:25:22AM -0700, Grant Likely wrote:
> On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > Platform code use node->data to store some private information
> > associated with a node.
> >
> > Previously there was no need for any locks and accessors since we were
> > initializing the data mostly at boot time and never modified it later.
> >
> > Though, nowadays OF GPIO infrastructure supports GPIO chips detaching,
> > so to handle this correctly we have to introduce locking for the
> > node->data field.
> 
> I'm not convinced this is needed.  What's wrong with using the
> whole-tree devtree_lock?

Why are you concerned? It doesn't add much of any footprint.

$ grep -c { -r arch/powerpc/boot/dts/ | cut -d: -f2 | sort -n | tail -n1
84

So far we have max 84 nodes, so it's a few hundreds of bytes for all
the dev tree.

Anyway, yes, we can use the devtree lock. Though, this will require a
bit more modifications, and I'm not sure if it's a great idea in
general (i.e. using the global lock in contrast to fine grained
locking).

The thing is that you can't use most of the of_ functions when you
hold the devtree lock (IIRC, rwlock has the same restrictions as a
spinlock, so you can't nest these locks).

I can try to rework OF GPIO calls so that they won't require of_
calls when they hold the lock, and let's see how it'll look like.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Anton Vorontsov]

OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
that of_get_gpio()'s returned GPIO number will be no longer valid, or
worse, it may point to an unexpected GPIO controller.

This scenario is possible:

driver A:               driver B:              driver C:
---------               ---------              ---------
                        gpiochip_add()
gpio = of_get_gpio()
                        gpiochip_remove()
                                               gpiochip_add()
gpio_request(gpio);
gpio_set_value(gpio);

That is, driver A assumes that it is working with GPIO from driver B,
but in practice it may disappear and driver C will take its GPIO base
number, so it will provide the same GPIO numbers.

With this patch that situation is no longer possible. Though drivers
will need to learn to put GPIOs back, so that GPIO controllers could
be removed.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/gpio.c       |   82 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/of_gpio.h |    5 +++
 2 files changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 9d8df77..e94c5c8 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -28,6 +28,8 @@
  * Returns GPIO number to use with Linux generic 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.
+ *
+ * Remeber to put the GPIO back using of_put_gpio() call.
  */
 int of_get_gpio_flags(struct device_node *np, int index,
 		      enum of_gpio_flags *flags)
@@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int index,
 		goto err0;
 	}
 
+	spin_lock(&gc->data_lock);
+
 	of_gc = gc->data;
 	if (!of_gc) {
 		pr_debug("%s: gpio controller %s isn't registered\n",
@@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int index,
 		goto err1;
 
 	ret += of_gc->chip->base;
+
+	if (!try_module_get(of_gc->chip->owner)) {
+		ret = -EINVAL;
+		goto err1;
+	}
+
+	of_gc->refcnt++;
 err1:
+	spin_unlock(&gc->data_lock);
+
 	of_node_put(gc);
 err0:
 	pr_debug("%s exited with status %d\n", __func__, ret);
+
 	return ret;
 }
 EXPORT_SYMBOL(of_get_gpio_flags);
 
 /**
+ * of_put_gpio - Put a GPIO back to the OF subsystem
+ * @np:		device node of the GPIO owner
+ * @index:	index of the GPIO
+ */
+static inline void of_put_gpio(struct device_node *np, int index)
+{
+	int ret;
+	struct device_node *gc;
+	struct of_gpio_chip *of_gc;
+
+	ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
+					  &gc, NULL);
+	if (ret) {
+		pr_debug("%s: can't parse gpios property\n", __func__);
+		return;
+	}
+
+	spin_lock(&gc->data_lock);
+
+	of_gc = gc->data;
+	if (!of_gc) {
+		pr_debug("%s: gpio controller %s isn't registered\n",
+			 np->full_name, gc->full_name);
+		goto err;
+	}
+
+	if (of_gc->refcnt)
+		of_gc->refcnt--;
+	else
+		WARN_ON(1);
+
+	module_put(of_gc->chip->owner);
+err:
+	spin_unlock(&gc->data_lock);
+}
+
+/**
  * of_gpio_count - Count GPIOs for a device
  * @np:		device node to count GPIOs for
  *
@@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
 				       struct device_node *np)
 {
 	struct of_gpio_chip *of_gc;
-
-	if (np->data) {
-		WARN_ON(1);
-		return -EBUSY;
-	}
+	int ret;
 
 	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
 	if (!of_gc)
@@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
 	of_gc->gpio_cells = 2;
 	of_gc->xlate = of_gpio_simple_xlate;
 	of_gc->chip = chip;
-	np->data = of_gc;
-	of_node_get(np);
 
-	return 0;
+	ret = of_node_set_data(np, of_gc);
+	if (ret)
+		kfree(of_gc);
+
+	return ret;
 }
 EXPORT_SYMBOL(of_gpiochip_register_simple);
 
@@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct gpio_chip *chip,
 				  struct device_node *np)
 {
 	struct of_gpio_chip *of_gc = np->data;
+	int ret = 0;
 
 	if (!of_gc || of_gc->chip != chip) {
 		WARN_ON(1);
 		return -EINVAL;
 	}
 
-	np->data = NULL;
-	kfree(of_gc);
-	of_node_put(np);
+	spin_lock(&np->data_lock);
 
-	return 0;
+	if (of_gc->refcnt)
+		ret = -EBUSY;
+	else
+		of_node_release_data_unlocked(np);
+
+	spin_unlock(&np->data_lock);
+
+	if (!ret)
+		kfree(of_gc);
+
+	return ret;
 }
 
 static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index c74cb37..aca7ab1 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -38,6 +38,7 @@ enum of_gpio_flags {
 struct of_gpio_chip {
 	struct gpio_chip gc; /* legacy, don't use for a new code */
 	struct gpio_chip *chip;
+	unsigned int refcnt;
 	int gpio_cells;
 	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
 		     const void *gpio_spec, enum of_gpio_flags *flags);
@@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct device_node *np, int index,
 	return -ENOSYS;
 }
 
+static inline void of_put_gpio(struct device_node *np, int index) {}
+
 static inline unsigned int of_gpio_count(struct device_node *np)
 {
 	return 0;
@@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct device_node *np)
  *
  * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
  * value on the error condition.
+ *
+ * Remeber to put the GPIO back using of_put_gpio() call.
  */
 static inline int of_get_gpio(struct device_node *np, int index)
 {
-- 
1.6.5.7

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Michal Simek]

Anton Vorontsov wrote:
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.

I am not able to apply this last patch.

$ git-am < \[PATCH\ 3_3\]\ of_gpio\:\ Introduce\ of_put_gpio\(\)\,\ add\ 
ref\ counting\ for\ OF\ GPIO\ chips.eml
Applying of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO 
chips
error: patch failed: drivers/of/gpio.c:254
error: drivers/of/gpio.c: patch does not apply
Patch failed at 0001.
When you have resolved this problem run "git-am --resolved".
If you would prefer to skip this patch, instead run "git-am --skip".

Below is my git-log which patches I applied.
Please fix it.

Thanks,
Michal

99d5baaa53562252a896080bf426b4ae71a5e55f of: Introduce safe accessors 
for node->data
df6d71af9c0691716ad8a368609a7e26a8dfdaa0 of platforms: Move common 
static initialization to of_node_init()
566969302a9dd70ce8a07c978bd08804ee73d0d6 powerpc/mcu_mpc8349emitx: 
Remove OF GPIO handling stuff
816932ea74919867c1f899758ea4cdd138145453 of/gpio: Implement GPIOLIB 
notifier hooks
3ea035a9e0f446b2b9cd9bacf73395a0c80ae3e9 of/gpio: Implement GPIOLIB 
notifier hooks
b7b96a9ee215c800476d002b06a338c4499f3813 of/gpio: Add support for 
two-stage registration for the of_gpio_chips
15678cc192c03a2b1bbb36da18ff2a3fe2d78897 gpiolib: Introduce chip 
addition/removal notifier
deb0c98c7f6035d47a247e548384517a955314a5 Merge branch 'for-2.6.33' of 
git://linux-nfs.org/~bfields/linux



> 
> This scenario is possible:
> 
> driver A:               driver B:              driver C:
> ---------               ---------              ---------
>                         gpiochip_add()
> gpio = of_get_gpio()
>                         gpiochip_remove()
>                                                gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
> 
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
> 
> With this patch that situation is no longer possible. Though drivers
> will need to learn to put GPIOs back, so that GPIO controllers could
> be removed.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/of/gpio.c       |   82 ++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/of_gpio.h |    5 +++
>  2 files changed, 75 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 9d8df77..e94c5c8 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -28,6 +28,8 @@
>   * Returns GPIO number to use with Linux generic 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.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
>   */
>  int of_get_gpio_flags(struct device_node *np, int index,
>  		      enum of_gpio_flags *flags)
> @@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int index,
>  		goto err0;
>  	}
>  
> +	spin_lock(&gc->data_lock);
> +
>  	of_gc = gc->data;
>  	if (!of_gc) {
>  		pr_debug("%s: gpio controller %s isn't registered\n",
> @@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int index,
>  		goto err1;
>  
>  	ret += of_gc->chip->base;
> +
> +	if (!try_module_get(of_gc->chip->owner)) {
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	of_gc->refcnt++;
>  err1:
> +	spin_unlock(&gc->data_lock);
> +
>  	of_node_put(gc);
>  err0:
>  	pr_debug("%s exited with status %d\n", __func__, ret);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(of_get_gpio_flags);
>  
>  /**
> + * of_put_gpio - Put a GPIO back to the OF subsystem
> + * @np:		device node of the GPIO owner
> + * @index:	index of the GPIO
> + */
> +static inline void of_put_gpio(struct device_node *np, int index)
> +{
> +	int ret;
> +	struct device_node *gc;
> +	struct of_gpio_chip *of_gc;
> +
> +	ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
> +					  &gc, NULL);
> +	if (ret) {
> +		pr_debug("%s: can't parse gpios property\n", __func__);
> +		return;
> +	}
> +
> +	spin_lock(&gc->data_lock);
> +
> +	of_gc = gc->data;
> +	if (!of_gc) {
> +		pr_debug("%s: gpio controller %s isn't registered\n",
> +			 np->full_name, gc->full_name);
> +		goto err;
> +	}
> +
> +	if (of_gc->refcnt)
> +		of_gc->refcnt--;
> +	else
> +		WARN_ON(1);
> +
> +	module_put(of_gc->chip->owner);
> +err:
> +	spin_unlock(&gc->data_lock);
> +}
> +
> +/**
>   * of_gpio_count - Count GPIOs for a device
>   * @np:		device node to count GPIOs for
>   *
> @@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
>  				       struct device_node *np)
>  {
>  	struct of_gpio_chip *of_gc;
> -
> -	if (np->data) {
> -		WARN_ON(1);
> -		return -EBUSY;
> -	}
> +	int ret;
>  
>  	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
>  	if (!of_gc)
> @@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
>  	of_gc->gpio_cells = 2;
>  	of_gc->xlate = of_gpio_simple_xlate;
>  	of_gc->chip = chip;
> -	np->data = of_gc;
> -	of_node_get(np);
>  
> -	return 0;
> +	ret = of_node_set_data(np, of_gc);
> +	if (ret)
> +		kfree(of_gc);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_gpiochip_register_simple);
>  
> @@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct gpio_chip *chip,
>  				  struct device_node *np)
>  {
>  	struct of_gpio_chip *of_gc = np->data;
> +	int ret = 0;
>  
>  	if (!of_gc || of_gc->chip != chip) {
>  		WARN_ON(1);
>  		return -EINVAL;
>  	}
>  
> -	np->data = NULL;
> -	kfree(of_gc);
> -	of_node_put(np);
> +	spin_lock(&np->data_lock);
>  
> -	return 0;
> +	if (of_gc->refcnt)
> +		ret = -EBUSY;
> +	else
> +		of_node_release_data_unlocked(np);
> +
> +	spin_unlock(&np->data_lock);
> +
> +	if (!ret)
> +		kfree(of_gc);
> +
> +	return ret;
>  }
>  
>  static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index c74cb37..aca7ab1 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -38,6 +38,7 @@ enum of_gpio_flags {
>  struct of_gpio_chip {
>  	struct gpio_chip gc; /* legacy, don't use for a new code */
>  	struct gpio_chip *chip;
> +	unsigned int refcnt;
>  	int gpio_cells;
>  	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>  		     const void *gpio_spec, enum of_gpio_flags *flags);
> @@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct device_node *np, int index,
>  	return -ENOSYS;
>  }
>  
> +static inline void of_put_gpio(struct device_node *np, int index) {}
> +
>  static inline unsigned int of_gpio_count(struct device_node *np)
>  {
>  	return 0;
> @@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct device_node *np)
>   *
>   * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
>   * value on the error condition.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
>   */
>  static inline int of_get_gpio(struct device_node *np, int index)
>  {


-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Michal Simek]

Michal Simek wrote:
> Anton Vorontsov wrote:
>> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
>> that of_get_gpio()'s returned GPIO number will be no longer valid, or
>> worse, it may point to an unexpected GPIO controller.
> 
> I am not able to apply this last patch.
> 
> $ git-am < \[PATCH\ 3_3\]\ of_gpio\:\ Introduce\ of_put_gpio\(\)\,\ add\ 
> ref\ counting\ for\ OF\ GPIO\ chips.eml
> Applying of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO 
> chips
> error: patch failed: drivers/of/gpio.c:254
> error: drivers/of/gpio.c: patch does not apply
> Patch failed at 0001.
> When you have resolved this problem run "git-am --resolved".
> If you would prefer to skip this patch, instead run "git-am --skip".
> 
> Below is my git-log which patches I applied.
> Please fix it.

I see where the problem is. I applied Andrews patch too that's why I am 
getting the fault above.
Anyway will be good to fix it. Will test gpio and let you know.

Michal


> 
> Thanks,
> Michal
> 
> 99d5baaa53562252a896080bf426b4ae71a5e55f of: Introduce safe accessors 
> for node->data
> df6d71af9c0691716ad8a368609a7e26a8dfdaa0 of platforms: Move common 
> static initialization to of_node_init()
> 566969302a9dd70ce8a07c978bd08804ee73d0d6 powerpc/mcu_mpc8349emitx: 
> Remove OF GPIO handling stuff
> 816932ea74919867c1f899758ea4cdd138145453 of/gpio: Implement GPIOLIB 
> notifier hooks
> 3ea035a9e0f446b2b9cd9bacf73395a0c80ae3e9 of/gpio: Implement GPIOLIB 
> notifier hooks
> b7b96a9ee215c800476d002b06a338c4499f3813 of/gpio: Add support for 
> two-stage registration for the of_gpio_chips
> 15678cc192c03a2b1bbb36da18ff2a3fe2d78897 gpiolib: Introduce chip 
> addition/removal notifier
> deb0c98c7f6035d47a247e548384517a955314a5 Merge branch 'for-2.6.33' of 
> git://linux-nfs.org/~bfields/linux
> 
> 
> 
>>
>> This scenario is possible:
>>
>> driver A:               driver B:              driver C:
>> ---------               ---------              ---------
>>                         gpiochip_add()
>> gpio = of_get_gpio()
>>                         gpiochip_remove()
>>                                                gpiochip_add()
>> gpio_request(gpio);
>> gpio_set_value(gpio);
>>
>> That is, driver A assumes that it is working with GPIO from driver B,
>> but in practice it may disappear and driver C will take its GPIO base
>> number, so it will provide the same GPIO numbers.
>>
>> With this patch that situation is no longer possible. Though drivers
>> will need to learn to put GPIOs back, so that GPIO controllers could
>> be removed.
>>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>>  drivers/of/gpio.c       |   82 
>> ++++++++++++++++++++++++++++++++++++++++-------
>>  include/linux/of_gpio.h |    5 +++
>>  2 files changed, 75 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
>> index 9d8df77..e94c5c8 100644
>> --- a/drivers/of/gpio.c
>> +++ b/drivers/of/gpio.c
>> @@ -28,6 +28,8 @@
>>   * Returns GPIO number to use with Linux generic 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.
>> + *
>> + * Remeber to put the GPIO back using of_put_gpio() call.
>>   */
>>  int of_get_gpio_flags(struct device_node *np, int index,
>>                enum of_gpio_flags *flags)
>> @@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int 
>> index,
>>          goto err0;
>>      }
>>  
>> +    spin_lock(&gc->data_lock);
>> +
>>      of_gc = gc->data;
>>      if (!of_gc) {
>>          pr_debug("%s: gpio controller %s isn't registered\n",
>> @@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int 
>> index,
>>          goto err1;
>>  
>>      ret += of_gc->chip->base;
>> +
>> +    if (!try_module_get(of_gc->chip->owner)) {
>> +        ret = -EINVAL;
>> +        goto err1;
>> +    }
>> +
>> +    of_gc->refcnt++;
>>  err1:
>> +    spin_unlock(&gc->data_lock);
>> +
>>      of_node_put(gc);
>>  err0:
>>      pr_debug("%s exited with status %d\n", __func__, ret);
>> +
>>      return ret;
>>  }
>>  EXPORT_SYMBOL(of_get_gpio_flags);
>>  
>>  /**
>> + * of_put_gpio - Put a GPIO back to the OF subsystem
>> + * @np:        device node of the GPIO owner
>> + * @index:    index of the GPIO
>> + */
>> +static inline void of_put_gpio(struct device_node *np, int index)
>> +{
>> +    int ret;
>> +    struct device_node *gc;
>> +    struct of_gpio_chip *of_gc;
>> +
>> +    ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
>> +                      &gc, NULL);
>> +    if (ret) {
>> +        pr_debug("%s: can't parse gpios property\n", __func__);
>> +        return;
>> +    }
>> +
>> +    spin_lock(&gc->data_lock);
>> +
>> +    of_gc = gc->data;
>> +    if (!of_gc) {
>> +        pr_debug("%s: gpio controller %s isn't registered\n",
>> +             np->full_name, gc->full_name);
>> +        goto err;
>> +    }
>> +
>> +    if (of_gc->refcnt)
>> +        of_gc->refcnt--;
>> +    else
>> +        WARN_ON(1);
>> +
>> +    module_put(of_gc->chip->owner);
>> +err:
>> +    spin_unlock(&gc->data_lock);
>> +}
>> +
>> +/**
>>   * of_gpio_count - Count GPIOs for a device
>>   * @np:        device node to count GPIOs for
>>   *
>> @@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct 
>> gpio_chip *chip,
>>                         struct device_node *np)
>>  {
>>      struct of_gpio_chip *of_gc;
>> -
>> -    if (np->data) {
>> -        WARN_ON(1);
>> -        return -EBUSY;
>> -    }
>> +    int ret;
>>  
>>      of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
>>      if (!of_gc)
>> @@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct 
>> gpio_chip *chip,
>>      of_gc->gpio_cells = 2;
>>      of_gc->xlate = of_gpio_simple_xlate;
>>      of_gc->chip = chip;
>> -    np->data = of_gc;
>> -    of_node_get(np);
>>  
>> -    return 0;
>> +    ret = of_node_set_data(np, of_gc);
>> +    if (ret)
>> +        kfree(of_gc);
>> +
>> +    return ret;
>>  }
>>  EXPORT_SYMBOL(of_gpiochip_register_simple);
>>  
>> @@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct 
>> gpio_chip *chip,
>>                    struct device_node *np)
>>  {
>>      struct of_gpio_chip *of_gc = np->data;
>> +    int ret = 0;
>>  
>>      if (!of_gc || of_gc->chip != chip) {
>>          WARN_ON(1);
>>          return -EINVAL;
>>      }
>>  
>> -    np->data = NULL;
>> -    kfree(of_gc);
>> -    of_node_put(np);
>> +    spin_lock(&np->data_lock);
>>  
>> -    return 0;
>> +    if (of_gc->refcnt)
>> +        ret = -EBUSY;
>> +    else
>> +        of_node_release_data_unlocked(np);
>> +
>> +    spin_unlock(&np->data_lock);
>> +
>> +    if (!ret)
>> +        kfree(of_gc);
>> +
>> +    return ret;
>>  }
>>  
>>  static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
>> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
>> index c74cb37..aca7ab1 100644
>> --- a/include/linux/of_gpio.h
>> +++ b/include/linux/of_gpio.h
>> @@ -38,6 +38,7 @@ enum of_gpio_flags {
>>  struct of_gpio_chip {
>>      struct gpio_chip gc; /* legacy, don't use for a new code */
>>      struct gpio_chip *chip;
>> +    unsigned int refcnt;
>>      int gpio_cells;
>>      int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>>               const void *gpio_spec, enum of_gpio_flags *flags);
>> @@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct 
>> device_node *np, int index,
>>      return -ENOSYS;
>>  }
>>  
>> +static inline void of_put_gpio(struct device_node *np, int index) {}
>> +
>>  static inline unsigned int of_gpio_count(struct device_node *np)
>>  {
>>      return 0;
>> @@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct 
>> device_node *np)
>>   *
>>   * Returns GPIO number to use with Linux generic GPIO API, or one of 
>> the errno
>>   * value on the error condition.
>> + *
>> + * Remeber to put the GPIO back using of_put_gpio() call.
>>   */
>>  static inline int of_get_gpio(struct device_node *np, int index)
>>  {
> 
> 


-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Grant Likely]

On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.
>
> This scenario is possible:
>
> driver A: =A0 =A0 =A0 =A0 =A0 =A0 =A0 driver B: =A0 =A0 =A0 =A0 =A0 =A0 =
=A0driver C:
> --------- =A0 =A0 =A0 =A0 =A0 =A0 =A0 --------- =A0 =A0 =A0 =A0 =A0 =A0 =
=A0---------
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_add()
> gpio =3D of_get_gpio()
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_remove()
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
>
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
>
> With this patch that situation is no longer possible. Though drivers
> will need to learn to put GPIOs back, so that GPIO controllers could
> be removed.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Rather than having a lock at the device tree data pointer level which
mixes usage with potentially many other drivers; wouldn't it make more
sense to use a mutex at the of_gc subsystem context?

g.

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Anton Vorontsov]

On Tue, Feb 09, 2010 at 10:28:15AM -0700, Grant Likely wrote:
[...]
> Rather than having a lock at the device tree data pointer level which
> mixes usage with potentially many other drivers; wouldn't it make more
> sense to use a mutex at the of_gc subsystem context?

I don't think so.

of_gc = np->data;
lock(of_gc); (or lock(devtree))
<do something with of_gc>

doesn't provide us what we need, i.e. it doesn't guarantee that
np->data (of_gc) is still alive.

And here:

lock(np->data); (or lock(devtree))
of_gc = np->data;
lock(of_gc);
<do something with of_gc>

The second lock becomes useless (unless you also refcount np->data
usage and can drop the devtree/np->data lock, and grab some other
kind of lock, e.g. mutex, but this is silly).

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Grant Likely]

On Tue, Feb 9, 2010 at 12:14 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Feb 09, 2010 at 10:28:15AM -0700, Grant Likely wrote:
> [...]
>> Rather than having a lock at the device tree data pointer level which
>> mixes usage with potentially many other drivers; wouldn't it make more
>> sense to use a mutex at the of_gc subsystem context?
>
> I don't think so.
>
> of_gc = np->data;
> lock(of_gc); (or lock(devtree))
> <do something with of_gc>
>
> doesn't provide us what we need, i.e. it doesn't guarantee that
> np->data (of_gc) is still alive.
>
> And here:
>
> lock(np->data); (or lock(devtree))
> of_gc = np->data;
> lock(of_gc);
> <do something with of_gc>
>
> The second lock becomes useless (unless you also refcount np->data
> usage and can drop the devtree/np->data lock, and grab some other
> kind of lock, e.g. mutex, but this is silly).

Okay, I'm convinced now.  The model is wrong.  struct of_gc does need
to be a member of struct gpio_chip and conditionally compiled against
CONFIG_OF_GPIO.  This locking requirement is just too plain ugly, and
dereferencing the np->data pointer in this way is dangerous (what if
something that is not struct of_gc is stored there).

Put of_gc into struct gpio_chip, and I'll completely support that approach.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Anton Vorontsov]

On Mon, Feb 15, 2010 at 12:49:56PM -0700, Grant Likely wrote:
[...]
> Okay, I'm convinced now.  The model is wrong.  struct of_gc does need
> to be a member of struct gpio_chip and conditionally compiled against
> CONFIG_OF_GPIO.  This locking requirement is just too plain ugly,

And how would you implement of_get_gpio(np, index) then?
You don't have 'struct gpio_chip' in of_get_gpio(), so you can't
get of_gc out of it.

It's possible to lookup all GPIOs (gpio[0..ARCH_NR_GPIOS]) matching
on chip->dev->archdata.node == np, but you're just moving this stuff
into gpiolib, where you'll have to grab global gpio_lock instead of
devtree lock. And just as with np->data_lock or global devtree lock,
you'll have to grab gpio_lock in of_gpio_put(), of_gpio_chip_register
and of_gpio_chip_unregister().

See? Your suggestion doesn't make things better or simpler, instead
it turns the OF GPIO inside out, exposing arch details to the generic
code.

There is really no excuse for the arch-specific (OF) stuff being in
the generic code. Not in the irq subsystem, not in the gpio
subsystem. My implementation actually proves that.

> and
> dereferencing the np->data pointer in this way is dangerous
> (what if
> something that is not struct of_gc is stored there).

Not possible with the safe np->data accessors.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips

[Michal Simek]

Anton Vorontsov wrote:
> Hi all,
> 
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.
> 
> This scenario is possible:
> 
> driver A:               driver B:              driver C:
> ---------               ---------              ---------
>                         gpiochip_add()
> gpio = of_get_gpio()
>                         gpiochip_remove()
>                                                gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
> 
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
> 
> The above situation is hard to trigger, but the issue is there
> nonetheless, and so needs fixing.


I tested xilinx gpio driver, heartbeat trigger and access through sysfs 
and I haven't found any problem. There is only small part of code for 
Microblaze and it is the same with PowerPC. If is Ben OK with it, I am 
ok too.

Thanks,
Michal


> 
> Thanks,
> 
> p.s. The patches are based on top of
>      '[PATCH v2 0/4] OF GPIO integration for I2C/SPI GPIO chips'
>      http://lkml.org/lkml/2010/2/5/243
> 


-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663

Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips

[Grant Likely]

On Tue, Feb 9, 2010 at 2:40 AM, Michal Simek <michal.simek@petalogix.com> w=
rote:
> Anton Vorontsov wrote:
>>
>> Hi all,
>>
>> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
>> that of_get_gpio()'s returned GPIO number will be no longer valid, or
>> worse, it may point to an unexpected GPIO controller.
>>
>> This scenario is possible:
>>
>> driver A: =A0 =A0 =A0 =A0 =A0 =A0 =A0 driver B: =A0 =A0 =A0 =A0 =A0 =A0 =
=A0driver C:
>> --------- =A0 =A0 =A0 =A0 =A0 =A0 =A0 --------- =A0 =A0 =A0 =A0 =A0 =A0 =
=A0---------
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_add()
>> gpio =3D of_get_gpio()
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_remove()
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 gpiochip_add()
>> gpio_request(gpio);
>> gpio_set_value(gpio);
>>
>> That is, driver A assumes that it is working with GPIO from driver B,
>> but in practice it may disappear and driver C will take its GPIO base
>> number, so it will provide the same GPIO numbers.
>>
>> The above situation is hard to trigger, but the issue is there
>> nonetheless, and so needs fixing.
>
>
> I tested xilinx gpio driver, heartbeat trigger and access through sysfs a=
nd
> I haven't found any problem. There is only small part of code for Microbl=
aze
> and it is the same with PowerPC. If is Ben OK with it, I am ok too.

I'm not convinced the changes take the best approach, so I'm not okay
with it yet.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips

[Anton Vorontsov]

On Tue, Feb 09, 2010 at 10:40:53AM +0100, Michal Simek wrote:
[...]
> >The above situation is hard to trigger, but the issue is there
> >nonetheless, and so needs fixing.
> 
> 
> I tested xilinx gpio driver, heartbeat trigger and access through
> sysfs and I haven't found any problem. There is only small part of
> code for Microblaze and it is the same with PowerPC. If is Ben OK
> with it, I am ok too.

Thanks for testing!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2