[PATCH RFC 0/3] Implement refcounting 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.
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: cbouat...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Anton Vorontsov
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 <avoro...@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
Anton Vorontsov
initialize other fields (for example node->data_lock).
Signed-off-by: Anton Vorontsov <avoro...@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
Michal Simek
> 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
--
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
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.
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
Michal Simek
> 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
Grant Likely
<avoro...@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 <avoro...@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(-)
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.
Grant Likely
> 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 <avoro...@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.
Grant Likely
> 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.
I'm not convinced the changes take the best approach, so I'm not okay
with it yet.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
Anton Vorontsov
[...]
> >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: cbouat...@gmail.com
irc://irc.freenode.net/bd2
Anton Vorontsov
[...]
> 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: cbouat...@gmail.com
irc://irc.freenode.net/bd2
Grant Likely
<avoro...@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.
Anton Vorontsov
[...]
> 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: cbouat...@gmail.com
irc://irc.freenode.net/bd2