lkml.org 
[lkml]   [2010]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
    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


    \
     
     \ /
      Last update: 2010-02-09 10:25    [W:0.045 / U:31.596 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site