| 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
| ||
|
Message-ID: <AANLkTi=yZZgh0v+-_Cw0JudAxxTZuBWeVCEZ-dBtENPw@mail.gmail.com>
Date: Thu, 17 Feb 2011 15:33:42 +0800
From: Eric Miao <eric.y.miao@...il.com>
To: Peter Tyser <ptyser@...-inc.com>
Cc: linux-kernel@...r.kernel.org, Alek Du <alek.du@...el.com>,
Samuel Ortiz <sameo@...ux.intel.com>,
David Brownell <dbrownell@...rs.sourceforge.net>,
"Uwe Kleine-K?nig" <u.kleine-koenig@...gutronix.de>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Joe Perches <joe@...ches.com>,
Alan Cox <alan@...rguk.ukuu.org.uk>,
Grant Likely <grant.likely@...retlab.ca>
Subject: Re: [PATCH v2 1/4] gpiolib: Add "unknown" direction support
On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <ptyser@...-inc.com> wrote:
> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
> regardless of their true state. This resulted in all GPIO output pins
> initially being incorrectly identified as "input" in the GPIO sysfs.
>
> Since the direction of GPIOs is not known prior to having their
> direction set, instead set the default direction to "unknown" to prevent
> user confusion. A pin with an "unknown" direction can not be written or
> read via sysfs; it must first be configured as an input or output before
> it can be used.
>
Hrm... that's why I don't like the original definition of gpio_request()
which is vague on the pin configurations. The pin configuration should
be clear upon requesting, otherwise it's a potential issue.
Anyway, this unknown state looks to be a good mitigation to this
underlying problem. I'm good with it.
> While we're playing with the direction flag in/out defines, rename them to
> a more descriptive FLAG_DIR_* format.
>
> Cc: Alek Du <alek.du@...el.com>
> Cc: Samuel Ortiz <sameo@...ux.intel.com>
> Cc: David Brownell <dbrownell@...rs.sourceforge.net>
> Cc: Eric Miao <eric.y.miao@...il.com>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@...gutronix.de>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Joe Perches <joe@...ches.com>
> Cc: Alan Cox <alan@...rguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@...retlab.ca>
> Signed-off-by: Peter Tyser <ptyser@...-inc.com>
> ---
> Change since V1:
> - This patch is new and is based on feedback from Alan to support a new
> "unknown" direction.
> - Rename FLAG_IS_OUT to FLAG_DIR_OUT, and add FLAG_DIR_IN
>
> drivers/gpio/gpiolib.c | 82 +++++++++++++++++++++++++++--------------------
> 1 files changed, 47 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 649550e..0113c10 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -49,13 +49,14 @@ struct gpio_desc {
> unsigned long flags;
> /* flag symbols are bit numbers */
> #define FLAG_REQUESTED 0
> -#define FLAG_IS_OUT 1
> -#define FLAG_RESERVED 2
> -#define FLAG_EXPORT 3 /* protected by sysfs_lock */
> -#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
> -#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
> -#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
> -#define FLAG_ACTIVE_LOW 7 /* sysfs value has active low */
> +#define FLAG_DIR_OUT 1 /* pin is an output */
> +#define FLAG_DIR_IN 2 /* pin is an input */
> +#define FLAG_RESERVED 3
> +#define FLAG_EXPORT 4 /* protected by sysfs_lock */
> +#define FLAG_SYSFS 5 /* exported via /sys/class/gpio/control */
> +#define FLAG_TRIG_FALL 6 /* trigger on falling edge */
> +#define FLAG_TRIG_RISE 7 /* trigger on rising edge */
> +#define FLAG_ACTIVE_LOW 8 /* sysfs value has active low */
>
> #define ID_SHIFT 16 /* add new flags before this one */
>
> @@ -220,15 +221,22 @@ static ssize_t gpio_direction_show(struct device *dev,
> {
> const struct gpio_desc *desc = dev_get_drvdata(dev);
> ssize_t status;
> + const char *dir_str;
>
> mutex_lock(&sysfs_lock);
>
> - if (!test_bit(FLAG_EXPORT, &desc->flags))
> + if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> status = -EIO;
> - else
> - status = sprintf(buf, "%s\n",
> - test_bit(FLAG_IS_OUT, &desc->flags)
> - ? "out" : "in");
> + } else {
> + if (test_bit(FLAG_DIR_OUT, &desc->flags))
> + dir_str = "out";
> + else if (test_bit(FLAG_DIR_IN, &desc->flags))
> + dir_str = "in";
> + else
> + dir_str = "unknown";
> +
> + status = sprintf(buf, "%s\n", dir_str);
> + }
>
> mutex_unlock(&sysfs_lock);
> return status;
> @@ -272,6 +280,9 @@ static ssize_t gpio_value_show(struct device *dev,
>
> if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> status = -EIO;
> + } else if ((!test_bit(FLAG_DIR_OUT, &desc->flags)) &&
> + (!test_bit(FLAG_DIR_IN, &desc->flags))) {
> + status = -EPERM;
> } else {
> int value;
>
> @@ -297,7 +308,7 @@ static ssize_t gpio_value_store(struct device *dev,
>
> if (!test_bit(FLAG_EXPORT, &desc->flags))
> status = -EIO;
> - else if (!test_bit(FLAG_IS_OUT, &desc->flags))
> + else if (!test_bit(FLAG_DIR_OUT, &desc->flags))
> status = -EPERM;
> else {
> long value;
> @@ -741,7 +752,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
>
> if (!status && gpio_to_irq(gpio) >= 0
> && (direction_may_change
> - || !test_bit(FLAG_IS_OUT,
> + || test_bit(FLAG_DIR_IN,
> &desc->flags)))
> status = device_create_file(dev,
> &dev_attr_edge);
> @@ -1059,22 +1070,10 @@ int gpiochip_add(struct gpio_chip *chip)
> break;
> }
> }
> - if (status == 0) {
> - for (id = base; id < base + chip->ngpio; id++) {
> + if (status == 0)
> + for (id = base; id < base + chip->ngpio; id++)
> gpio_desc[id].chip = chip;
>
> - /* REVISIT: most hardware initializes GPIOs as
> - * inputs (often with pullups enabled) so power
> - * usage is minimized. Linux code should set the
> - * gpio direction first thing; but until it does,
> - * we may expose the wrong direction in sysfs.
> - */
> - gpio_desc[id].flags = !chip->direction_input
> - ? (1 << FLAG_IS_OUT)
> - : 0;
> - }
> - }
> -
> of_gpiochip_add(chip);
>
> unlock:
> @@ -1402,8 +1401,10 @@ int gpio_direction_input(unsigned gpio)
> }
>
> status = chip->direction_input(chip, gpio);
> - if (status == 0)
> - clear_bit(FLAG_IS_OUT, &desc->flags);
> + if (status == 0) {
> + clear_bit(FLAG_DIR_OUT, &desc->flags);
> + set_bit(FLAG_DIR_IN, &desc->flags);
> + }
> lose:
> return status;
> fail:
> @@ -1455,8 +1456,10 @@ int gpio_direction_output(unsigned gpio, int value)
> }
>
> status = chip->direction_output(chip, gpio, value);
> - if (status == 0)
> - set_bit(FLAG_IS_OUT, &desc->flags);
> + if (status == 0) {
> + clear_bit(FLAG_DIR_IN, &desc->flags);
> + set_bit(FLAG_DIR_OUT, &desc->flags);
> + }
> lose:
> return status;
> fail:
> @@ -1643,21 +1646,27 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> unsigned i;
> unsigned gpio = chip->base;
> struct gpio_desc *gdesc = &gpio_desc[gpio];
> - int is_out;
> + const char *dir_str;
> + int unknown = 0;
>
> for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
> if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
> continue;
>
> - is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> - seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> - gpio, gdesc->label,
> - is_out ? "out" : "in ",
> - chip->get
> - ? (chip->get(chip, i) ? "hi" : "lo")
> - : "? ");
> + if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
> + dir_str = "in ";
> + } else if (test_bit(FLAG_DIR_OUT, &gdesc->flags)) {
> + dir_str = "out";
> + } else {
> + dir_str = "? ";
> + unknown = 1;
> + }
> +
> + seq_printf(s, " gpio-%-3d (%-20.20s) %s %s", gpio, gdesc->label,
> + dir_str, (chip->get && !unknown) ?
> + (chip->get(chip, i) ? "hi" : "lo") : "?");
>
> - if (!is_out) {
> + if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
> int irq = gpio_to_irq(gpio);
> struct irq_desc *desc = irq_to_desc(irq);
>
> --
> 1.7.0.4
>
>
Powered by blists - more mailing lists