| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Dec 2014 11:08:10 -0600
From: Benoit Parrot <bparrot@...com>
To: Alexandre Courbot <gnurou@...il.com>
CC: Linus Walleij <linus.walleij@...aro.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Jiri Prchal <jiri.prchal@...ignal.cz>,
Pantelis Antoniou <panto@...oniou-consulting.com>
Subject: Re: [Patch v4 1/2] gpio: add GPIO hogging mechanism
Alexandre Courbot <gnurou@...il.com> wrote on Wed [2014-Dec-17 17:41:51 +0900]:
> Looks ok to me. I have a few nits below, but otherwise I am happy with
> how it turned out:
>
> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
I'll add this to the next version.
>
> Thanks for your patience with this!
>
> On Sat, Dec 13, 2014 at 7:07 AM, Benoit Parrot <bparrot@...com> wrote:
> > Based on Boris Brezillion's work this is a reworked patch
> > of his initial GPIO hogging mechanism.
> > This patch provides a way to initally configure specific GPIO
>
> s/initally/initially
>
> > when the gpio controller is probed.
>
> GPIO in capitals please.
>
> >
> > The actual DT scanning to collect the GPIO specific data is performed
> > as part of the gpiochip_add().
>
> d/the
>
> >
> > The purpose of this is to allows specific GPIOs to be configured
>
> s/allows/allow
>
> > without any driver specific code.
> > This particularly useful because board design are getting
>
> This is particularly
>
> > increasingly complex and given SoC pins can now have upward
> > of
>
> s/upward of/up to?
well i meant more than 10 I think in some cases we have 13.
>
> > 10 mux values a lot of connections are now dependent on
>
> seems like a comma is missing here.
>
> > external IO muxes to switch various modes and combination.
>
> not sure what a combination is in that context.
>
> >
> > Specific drivers should not necessarily need to be aware of
> > what accounts to a specific board implementation. This board level
> > "description" should be best kept as part of the dts file.
> >
> > Signed-off-by: Benoit Parrot <bparrot@...com>
> > ---
> > Changes since v3:
> > * Relocated the non-DT "hog" function to gpiolib.c.
> > * Rename some of the function to be clearer and remove _ prefixes.
> > * Replace the gpiod_request/gpiod_put usage with
> > gpiochip_request_own_desc/free_own_desc version instead.
> > * Refactor some of the logic to better handle error condition/reporting
> > * Renamed the "direction" DT properties to "state".
> >
> > drivers/gpio/gpiolib-of.c | 119 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/gpio/gpiolib.c | 128 ++++++++++++++++++++++++++++++++++++++--------
> > drivers/gpio/gpiolib.h | 3 ++
> > 3 files changed, 230 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 604dbe6..5422216 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -22,6 +22,7 @@
> > #include <linux/of_gpio.h>
> > #include <linux/pinctrl/pinctrl.h>
> > #include <linux/slab.h>
> > +#include <linux/gpio/machine.h>
> >
> > #include "gpiolib.h"
> >
> > @@ -111,6 +112,122 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
> > EXPORT_SYMBOL(of_get_named_gpio_flags);
> >
> > /**
> > + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
> > + * @np: device node to get GPIO from
> > + * @name: GPIO line name
> > + * @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.
> > + */
> > +
> > +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> > + const char **name,
> > + enum gpio_lookup_flags *lflags,
> > + enum gpiod_flags *dflags)
> > +{
> > + struct device_node *chip_np;
> > + enum of_gpio_flags xlate_flags;
> > + struct gpio_desc *desc;
> > + const char *dir_val;
> > + struct gg_data gg_data = {
> > + .flags = &xlate_flags,
> > + .out_gpio = NULL,
>
> out_gpio will be set to NULL if you don't specify it, so you don't
> need this last line.
ok
>
> > + };
> > + u32 tmp;
> > + int i, ret;
> > +
> > + chip_np = np->parent;
> > + if (!chip_np)
> > + return ERR_PTR(-EINVAL);
> > +
> > + xlate_flags = 0;
> > + *lflags = 0;
> > + *dflags = 0;
> > +
> > + ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + if (tmp > MAX_PHANDLE_ARGS)
> > + return ERR_PTR(-EINVAL);
> > +
> > + gg_data.gpiospec.args_count = tmp;
> > + gg_data.gpiospec.np = chip_np;
> > + for (i = 0; i < tmp; i++) {
> > + ret = of_property_read_u32_index(np, "gpios", i,
> > + &gg_data.gpiospec.args[i]);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + }
> > +
> > + gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> > + if (!gg_data.out_gpio) {
> > + if (np->parent == np)
> > + return ERR_PTR(-ENXIO);
> > + else
> > + return ERR_PTR(-EPROBE_DEFER);
> > + }
> > +
> > + if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> > + *lflags |= GPIO_ACTIVE_LOW;
> > +
> > + if (of_property_read_string(np, "state", &dir_val)) {
> > + pr_warn("%s: GPIO:%d (%s): no hogging state specified, bailing out\n",
> > + __func__, desc_to_gpio(gg_data.out_gpio), np->name);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (!strcmp(dir_val, "input"))
> > + *dflags |= GPIOD_IN;
> > + else if (!strcmp(dir_val, "output-low"))
> > + *dflags |= GPIOD_OUT_LOW;
> > + else if (!strcmp(dir_val, "output-high"))
> > + *dflags |= GPIOD_OUT_HIGH;
> > + else {
> > + pr_warn("%s: GPIO:%d (%s): invalid state '%s' requested, bailing out\n",
> > + __func__, desc_to_gpio(gg_data.out_gpio),
> > + np->name, dir_val);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (name && of_property_read_string(np, "line-name", name))
> > + *name = np->name;
> > +
> > + desc = gg_data.out_gpio;
> > +
> > + return desc;
> > +}
> > +
> > +/**
> > + * of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested
> > + * @chip: gpio chip to act on
> > + *
> > + * This is only used by of_gpiochip_add to request/set GPIO initial
> > + * configuration.
> > + */
> > +static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> > +{
> > + struct gpio_desc *desc = NULL;
> > + struct device_node *np;
> > + const char *name;
> > + enum gpio_lookup_flags lflags;
> > + enum gpiod_flags dflags;
> > +
> > + for_each_child_of_node(chip->dev->of_node, np) {
As I was backporting this to 3.14 for my own purpose.
I encountered a NULL pointer dereference here.
In 3.14 chip->dev (which is optional) is NULL at least on my platform.
I had to change the above line to:
for_each_child_of_node(chip->of_node, np) {
Would you recommend the same change here?
> > + if (!of_property_read_bool(np, "gpio-hog"))
> > + continue;
> > +
> > + desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
> > + if (IS_ERR(desc))
> > + continue;
> > +
> > + if (gpiod_hog(desc, name, lflags, dflags))
> > + continue;
> > + }
> > +}
> > +
> > +/**
> > * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags
> > * @gc: pointer to the gpio_chip structure
> > * @np: device node of the GPIO chip
> > @@ -302,6 +419,8 @@ void of_gpiochip_add(struct gpio_chip *chip)
> >
> > of_gpiochip_add_pin_range(chip);
> > of_node_get(chip->of_node);
> > +
> > + of_gpiochip_scan_hogs(chip);
> > }
> >
> > void of_gpiochip_remove(struct gpio_chip *chip)
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 58659db..5527dbb 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -301,6 +301,7 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
> >
> > /* Forward-declaration */
> > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
> > +static void gpiochip_free_hogs(struct gpio_chip *chip);
> >
> > /**
> > * gpiochip_remove() - unregister a gpio_chip
> > @@ -319,6 +320,7 @@ void gpiochip_remove(struct gpio_chip *chip)
> >
> > gpiochip_irqchip_remove(chip);
> > gpiochip_remove_pin_ranges(chip);
> > + gpiochip_free_hogs(chip);
> > of_gpiochip_remove(chip);
> >
> > for (id = 0; id < chip->ngpio; id++) {
> > @@ -849,6 +851,7 @@ static bool __gpiod_free(struct gpio_desc *desc)
> > clear_bit(FLAG_REQUESTED, &desc->flags);
> > clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > + clear_bit(FLAG_IS_HOGGED, &desc->flags);
> > ret = true;
> > }
> >
> > @@ -1653,6 +1656,47 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev,
> > }
> > EXPORT_SYMBOL_GPL(__gpiod_get_optional);
> >
> > +
> > +/**
> > + * __gpiod_set_helper - helper function to configure a given GPIO
> > + * @desc: gpio whose value will be assigned
> > + * @con_id: function within the GPIO consumer
> > + * @lflags: gpio_lookup_flags - returned from of_find_gpio() or
> > + * of_get_gpio_hog()
> > + * @dflags: gpiod_flags - optional GPIO initialization flags
> > + *
> > + * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> > + * requested function and/or index, or another IS_ERR() code if an error
> > + * occurred while trying to acquire the GPIO.
> > + */
> > +static int __gpiod_set_helper(struct gpio_desc *desc, const char *con_id,
> > + unsigned long lflags, enum gpiod_flags dflags)
> > +{
> > + int status;
> > +
> > + if (lflags & GPIO_ACTIVE_LOW)
> > + set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > + if (lflags & GPIO_OPEN_DRAIN)
> > + set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > + if (lflags & GPIO_OPEN_SOURCE)
> > + set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > +
> > + /* No particular flag request, return here... */
> > + if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> > + pr_debug("no flags found for %s\n", con_id);
> > + return 0;
> > + }
> > +
> > + /* Process flags */
> > + if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
> > + status = gpiod_direction_output(desc,
> > + dflags & GPIOD_FLAGS_BIT_DIR_VAL);
> > + else
> > + status = gpiod_direction_input(desc);
> > +
> > + return status;
> > +}
> > +
> > /**
> > * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> > * @dev: GPIO consumer, can be NULL for system-global GPIOs
> > @@ -1702,30 +1746,12 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev,
> > }
> >
> > status = gpiod_request(desc, con_id);
> > -
> > if (status < 0)
> > return ERR_PTR(status);
> >
> > - if (lookupflags & GPIO_ACTIVE_LOW)
> > - set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > - if (lookupflags & GPIO_OPEN_DRAIN)
> > - set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > - if (lookupflags & GPIO_OPEN_SOURCE)
> > - set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > -
> > - /* No particular flag request, return here... */
> > - if (!(flags & GPIOD_FLAGS_BIT_DIR_SET))
> > - return desc;
> > -
> > - /* Process flags */
> > - if (flags & GPIOD_FLAGS_BIT_DIR_OUT)
> > - status = gpiod_direction_output(desc,
> > - flags & GPIOD_FLAGS_BIT_DIR_VAL);
> > - else
> > - status = gpiod_direction_input(desc);
> > -
> > + status = __gpiod_set_helper(desc, con_id, lookupflags, flags);
> > if (status < 0) {
> > - dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
> > + pr_debug("setup of GPIO %s failed\n", con_id);
> > gpiod_put(desc);
> > return ERR_PTR(status);
> > }
> > @@ -1819,6 +1845,68 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> > EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >
> > /**
> > + * gpiod_hog - Hog the specified GPIO desc given the provided flags
> > + * @desc: gpio whose value will be assigned
> > + * @name: gpio line name
> > + * @lflags: gpio_lookup_flags - returned from of_find_gpio() or
> > + * of_get_gpio_hog()
> > + * @dflags: gpiod_flags - optional GPIO initialization flags
> > + *
> > + */
> > +int gpiod_hog(struct gpio_desc *desc, const char *name,
> > + unsigned long lflags, enum gpiod_flags dflags)
> > +{
> > + struct gpio_chip *chip;
> > + struct gpio_desc *local_desc;
> > + int hwnum;
> > + int status;
> > +
> > + chip = gpiod_to_chip(desc);
> > + hwnum = gpio_chip_hwgpio(desc);
> > +
> > + local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>
> This looks somehow funny because local_desc will be strictly equal to
> desc, but there is nothing you can do here. The DT GPIO lookup process
> should not return a desc, but a (gpiochip*, hwnum) tuple to begin
> with, so let's keep it that way. I made a note to myself to fix this.
Yeah it does, I was almost tempted to add a check to make sure local_desc
would be identical to desc. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists