| 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: Sun, 21 Jun 2015 16:21:17 +0900
From: Alexandre Courbot <gnurou@...il.com>
To: Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Benoit Parrot <bparrot@...com>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
Linux-SH <linux-sh@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFC] gpio: Retry deferred GPIO hogging on pin range change
On Tue, Jun 16, 2015 at 9:36 PM, Geert Uytterhoeven
<geert+renesas@...der.be> wrote:
> If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
> case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
> doesn't work:
>
> requesting hog GPIO lcd0 (chip r8a7740_pfc, offset 176) failed
>
> The actual error code is -517 == -EPROBE_DEFER.
>
> The problem is that PFC+GPIO registration is handled in multiple steps:
> 1. pinctrl_register(),
> 2. gpiochip_add(),
> 3. gpiochip_add_pin_range().
>
> Configuration of the hogs is handled in gpiochip_add():
>
> gpiochip_add
> of_gpiochip_add
> of_gpiochip_scan_hogs
> gpiod_hog
> gpiochip_request_own_desc
> __gpiod_request
> chip->request
> pinctrl_request_gpio
> pinctrl_get_device_gpio_range
>
> However, at this point the GPIO controller hasn't been added to
> pinctrldev_list yet, so the range can't be found, and the operation fails
> with -EPROBE_DEFER.
>
> - Exchanging the order of the calls to gpiochip_add() and
> gpiochip_add_pin_range() is not an option, as the latter depends on
> initialization done by the former.
> - Just moving the call of of_gpiochip_scan_hogs() from gpiochip_add()
> to gpiochip_add_pin_range() is also not an option, as the latter is
> optional, and thus not used by all drivers.
>
> Hence if of_gpiochip_scan_hogs() fails with -EPROBE_DEFER, call it
> again every time the pin range is changed, until it succeeded.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Questions:
> - Is there a better solution to handle this?
I do not understand the issue well enough to propose a better
solution, but I really hope there is one. This turns GPIO into a
slightly bigger yarn mess that what it already is and does not help
understanding how probing is performed. Yielding in the middle of
adding hogs and re-trying later sounds like a recipe for issues like
hogs being added several times.
So I am not really fond of this, to be honest. The GPIO hogging
mechanism is still quite new, so there is certainly a way to fix such
issues by addressing the fundamental cause instead of duct taping it?
>
> - Should the pin ranges be configured by passing an array of data to
> gpiochip_add() instead of having calls to gpiochip_add_pin_range()?
> That would require changing all drivers.
>
> - What happens if you have multiple hogs in multiple ranges?
> The first hog(s) may be configured multiple times. Is that a problem?
>
> - In one of the threads that discussed the GPIO hogging mechanism, Maxime
> Ripard said: "Our pinctrl driver is also our GPIO driver, so they both
> share the same node."
> Maxime: Did you try GPIO hogging? Did it work?
> If yes, which driver are you using? What's different compared to sh-pfc?
> If no, did you get it to work?
>
> Thanks!
> ---
> drivers/gpio/gpiolib-of.c | 21 +++++++++++++++++----
> drivers/gpio/gpiolib.c | 1 +
> include/linux/gpio/driver.h | 1 +
> include/linux/of_gpio.h | 2 ++
> 4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 9a0ec48a47375d18..90dd02b19f75c27c 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -205,13 +205,14 @@ static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> * This is only used by of_gpiochip_add to request/set GPIO initial
> * configuration.
> */
> -static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> +static int 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;
> + int error;
>
> for_each_child_of_node(chip->of_node, np) {
> if (!of_property_read_bool(np, "gpio-hog"))
> @@ -221,9 +222,12 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> if (IS_ERR(desc))
> continue;
>
> - if (gpiod_hog(desc, name, lflags, dflags))
> - continue;
> + error = gpiod_hog(desc, name, lflags, dflags);
> + if (error == -EPROBE_DEFER)
> + return error;
> }
> +
> + return 0;
> }
>
> /**
> @@ -416,8 +420,17 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
> }
> }
>
> +void of_gpiochip_pin_range_changed(struct gpio_chip *chip)
> +{
> + if (chip->hog_error) {
> + /* Retry */
> + chip->hog_error = of_gpiochip_scan_hogs(chip);
> + }
> +}
> +
> #else
> static void of_gpiochip_add_pin_range(struct gpio_chip *chip) {}
> +void of_gpiochip_pin_range_changed(struct gpio_chip *chip) {}
> #endif
>
> void of_gpiochip_add(struct gpio_chip *chip)
> @@ -436,7 +449,7 @@ 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);
> + chip->hog_error = 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 957ede5664cfe168..b0fe7a459d8835bb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -759,6 +759,7 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
>
> list_add_tail(&pin_range->node, &chip->pin_ranges);
>
> + of_gpiochip_pin_range_changed(chip);
> return 0;
> }
> EXPORT_SYMBOL_GPL(gpiochip_add_pin_range);
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index c8393cd4d44f2d87..9396b68dced2c5b1 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -146,6 +146,7 @@ struct gpio_chip {
> * corresponding pins for gpio usage.
> */
> struct list_head pin_ranges;
> + int hog_error;
> #endif
> };
>
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 69dbe312b11b23f6..34421f17f4712d0b 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -54,6 +54,7 @@ extern int of_mm_gpiochip_add(struct device_node *np,
> struct of_mm_gpio_chip *mm_gc);
> extern void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc);
>
> +extern void of_gpiochip_pin_range_changed(struct gpio_chip *chip);
> extern void of_gpiochip_add(struct gpio_chip *gc);
> extern void of_gpiochip_remove(struct gpio_chip *gc);
> extern int of_gpio_simple_xlate(struct gpio_chip *gc,
> @@ -76,6 +77,7 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
> return -ENOSYS;
> }
>
> +static inline void of_gpiochip_pin_range_changed(struct gpio_chip *chip) { }
> static inline void of_gpiochip_add(struct gpio_chip *gc) { }
> static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
>
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists