| Message ID | 1416383487-15993-1-git-send-email-acourbot@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Nov 19, 2014 at 8:51 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: > Replace the ARCH_NR_GPIOS-sized static array of GPIO descriptors by > dynamically-allocated arrays for each GPIO chip. > > This change makes gpio_to_desc() perform in O(n) (where n is the number > of GPIO chips registered) instead of O(1), however since n is rarely > bigger than 1 or 2 no noticeable performance issue is expected. > Besides this provides more incentive for GPIO consumers to move to the > gpiod interface. One could use a O(log(n)) structure to link the GPIO > chips together, but considering the low limit of n the hypothetical > performance benefit is probably not worth the added complexity. > > This patch uses kcalloc() in gpiochip_add(), which removes the ability > to add a chip before kcalloc() can operate. I am not aware of such > cases, but if someone bisects up to this patch then I will be proven > wrong... > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> OK patch applied. Let's see if the world explodes. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, Alexandre, On Fri, Nov 28, 2014 at 11:29 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Nov 19, 2014 at 8:51 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: >> Replace the ARCH_NR_GPIOS-sized static array of GPIO descriptors by >> dynamically-allocated arrays for each GPIO chip. >> >> This change makes gpio_to_desc() perform in O(n) (where n is the number >> of GPIO chips registered) instead of O(1), however since n is rarely >> bigger than 1 or 2 no noticeable performance issue is expected. >> Besides this provides more incentive for GPIO consumers to move to the >> gpiod interface. One could use a O(log(n)) structure to link the GPIO >> chips together, but considering the low limit of n the hypothetical >> performance benefit is probably not worth the added complexity. >> >> This patch uses kcalloc() in gpiochip_add(), which removes the ability >> to add a chip before kcalloc() can operate. I am not aware of such >> cases, but if someone bisects up to this patch then I will be proven >> wrong... >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > OK patch applied. Let's see if the world explodes. This patch changes a return value from -EPROBE_DEFER to -EINVAL in regulator-gpio when a GPIO cannot be found yet, causing probe failures on r8a7791/koelsch: of_get_named_gpiod_flags: can't parse 'enable-gpio' property of node '/regulator@1[0]' of_get_named_gpiod_flags: parsed 'gpios' property of node '/regulator@1[0]' - status (-517) -gpio-0 (?): gpiod_request: status -517 -gpio-regulator regulator@1: Could not obtain regulator setting GPIOs: -517 -platform regulator@1: Driver gpio-regulator requests probe deferral +------------[ cut here ]------------ +WARNING: CPU: 0 PID: 1 at /scratch/geert/linux/linux-renesas/drivers/gpio/gpiolib.c:80 gpio_to_desc+0x94/0xac() +invalid GPIO 0 +Modules linked in: +CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc7-koelsch-05803-ge7a5d822abfc727e #634 +Hardware name: Generic R8A7791 (Flattened Device Tree) +Backtrace: +[<c0012118>] (dump_backtrace) from [<c0012338>] (show_stack+0x18/0x1c) + r6:c04608f7 r5:00000009 r4:00000000 r3:00200040 +[<c0012320>] (show_stack) from [<c0386e48>] (dump_stack+0x78/0x94) +[<c0386dd0>] (dump_stack) from [<c0026b84>] (warn_slowpath_common+0x70/0x94) + r4:eec53d10 r3:c04f3390 +[<c0026b14>] (warn_slowpath_common) from [<c0026be0>] (warn_slowpath_fmt+0x38/0x40) + r8:eed4a600 r7:00000001 r6:00000000 r5:00000000 r4:c04f9430 +[<c0026bac>] (warn_slowpath_fmt) from [<c01dd700>] (gpio_to_desc+0x94/0xac) + r3:00000000 r2:c0460931 +[<c01dd66c>] (gpio_to_desc) from [<c01df200>] (gpio_request_one+0x18/0xc4) + r5:00000000 r4:00000000 +[<c01df1e8>] (gpio_request_one) from [<c01df2d4>] (gpio_request_array+0x28/0x60) + r6:00000004 r5:00000000 r4:eed5a4c0 r3:00000002 +[<c01df2ac>] (gpio_request_array) from [<c0201b20>] (gpio_regulator_probe+0x424/0x5a4) + r7:eed4a610 r6:00000004 r5:eed464d0 r4:eed5a5d0 +[<c02016fc>] (gpio_regulator_probe) from [<c022a81c>] (platform_drv_probe+0x50/0xa0) + r10:00000000 r9:c0509f80 r8:c04fa648 r7:00000000 r6:c04fa648 r5:eed4a610 + r4:ffffffed +[<c022a7cc>] (platform_drv_probe) from [<c0229000>] (driver_probe_device+0xc0/0x204) + r6:00000000 r5:c054ae0c r4:eed4a610 r3:c022a7cc +[<c0228f40>] (driver_probe_device) from [<c0229200>] (__driver_attach+0x70/0x94) + r8:c04ee560 r7:c04fe4b0 r6:c04fa648 r5:eed4a644 r4:eed4a610 r3:00000000 +[<c0229190>] (__driver_attach) from [<c02276bc>] (bus_for_each_dev+0x74/0x98) + r6:c0229190 r5:c04fa648 r4:00000000 r3:00000001 +[<c0227648>] (bus_for_each_dev) from [<c0228b5c>] (driver_attach+0x20/0x28) + r6:eed52300 r5:00000000 r4:c04fa648 +[<c0228b3c>] (driver_attach) from [<c02287f4>] (bus_add_driver+0xe8/0x1d0) +[<c022870c>] (bus_add_driver) from [<c022989c>] (driver_register+0xa4/0xe8) + r7:c04ee560 r6:00000000 r5:c04c0498 r4:c04fa648 +[<c02297f8>] (driver_register) from [<c022a750>] (__platform_driver_register+0x50/0x64) + r5:c04c0498 r4:eed5a800 +[<c022a700>] (__platform_driver_register) from [<c04c04b0>] (gpio_regulator_init+0x18/0x20) +[<c04c0498>] (gpio_regulator_init) from [<c0009778>] (do_one_initcall+0x108/0x1b8) +[<c0009670>] (do_one_initcall) from [<c04a9e10>] (kernel_init_freeable+0x11c/0x1e4) + r9:c0509f80 r8:c0509f80 r7:c04dfcf0 r6:c04d7c2c r5:0000006d r4:00000004 +[<c04a9cf4>] (kernel_init_freeable) from [<c0384538>] (kernel_init+0x10/0xec) + r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0384528 r4:00000000 +[<c0384528>] (kernel_init) from [<c000ee58>] (ret_from_fork+0x14/0x3c) + r4:00000000 r3:eec52000 +---[ end trace e95579bdd1fdbe74 ]--- +gpiod_request: invalid GPIO +gpio-regulator regulator@1: Could not obtain regulator setting GPIOs: -22 +gpio-regulator: probe of regulator@1 failed with error -22 Reverting commit 14e85c0e69d5c7fdbd963edbbec1dc5cdd385200 ("gpio: remove gpio_descs global array") fixes the issue. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 2, 2014 at 7:32 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Linus, Alexandre, > > On Fri, Nov 28, 2014 at 11:29 AM, Linus Walleij > <linus.walleij@linaro.org> wrote: >> On Wed, Nov 19, 2014 at 8:51 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: >>> Replace the ARCH_NR_GPIOS-sized static array of GPIO descriptors by >>> dynamically-allocated arrays for each GPIO chip. >>> >>> This change makes gpio_to_desc() perform in O(n) (where n is the number >>> of GPIO chips registered) instead of O(1), however since n is rarely >>> bigger than 1 or 2 no noticeable performance issue is expected. >>> Besides this provides more incentive for GPIO consumers to move to the >>> gpiod interface. One could use a O(log(n)) structure to link the GPIO >>> chips together, but considering the low limit of n the hypothetical >>> performance benefit is probably not worth the added complexity. >>> >>> This patch uses kcalloc() in gpiochip_add(), which removes the ability >>> to add a chip before kcalloc() can operate. I am not aware of such >>> cases, but if someone bisects up to this patch then I will be proven >>> wrong... >>> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> >> OK patch applied. Let's see if the world explodes. > > This patch changes a return value from -EPROBE_DEFER to -EINVAL in > regulator-gpio when a GPIO cannot be found yet, causing probe failures > on r8a7791/koelsch: Hi Geert, Thanks for signaling this. I think I understand what is going wrong and will send a fixup patch in a few minutes. Cheers, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 2, 2014 at 2:25 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Tue, Dec 2, 2014 at 7:32 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> This patch changes a return value from -EPROBE_DEFER to -EINVAL in >> regulator-gpio when a GPIO cannot be found yet, causing probe failures >> on r8a7791/koelsch: > > Hi Geert, > > Thanks for signaling this. I think I understand what is going wrong > and will send a fixup patch in a few minutes. OK standing by. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index eb739a51e774..5619922ebf44 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -47,8 +47,6 @@ */ DEFINE_SPINLOCK(gpio_lock); -static struct gpio_desc gpio_desc[ARCH_NR_GPIOS]; - #define GPIO_OFFSET_VALID(chip, offset) (offset >= 0 && offset < chip->ngpio) static DEFINE_MUTEX(gpio_lookup_lock); @@ -65,10 +63,22 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label) */ struct gpio_desc *gpio_to_desc(unsigned gpio) { - if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) - return NULL; - else - return &gpio_desc[gpio]; + struct gpio_chip *chip; + unsigned long flags; + + spin_lock_irqsave(&gpio_lock, flags); + + list_for_each_entry(chip, &gpio_chips, list) { + if (chip->base <= gpio && chip->base + chip->ngpio > gpio) { + spin_unlock_irqrestore(&gpio_lock, flags); + return &chip->desc[gpio - chip->base]; + } + } + + spin_unlock_irqrestore(&gpio_lock, flags); + + WARN(1, "invalid GPIO %d\n", gpio); + return NULL; } EXPORT_SYMBOL_GPL(gpio_to_desc); @@ -91,7 +101,7 @@ struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, */ int desc_to_gpio(const struct gpio_desc *desc) { - return desc - &gpio_desc[0]; + return desc->chip->base + (desc - &desc->chip->desc[0]); } EXPORT_SYMBOL_GPL(desc_to_gpio); @@ -206,7 +216,7 @@ static int gpiochip_add_to_list(struct gpio_chip *chip) /** * gpiochip_add() - register a gpio_chip * @chip: the chip to register, with chip->base initialized - * Context: potentially before irqs or kmalloc will work + * Context: potentially before irqs will work * * Returns a negative errno if the chip can't be registered, such as * because the chip->base is invalid or already associated with a @@ -226,12 +236,11 @@ int gpiochip_add(struct gpio_chip *chip) int status = 0; unsigned id; int base = chip->base; + struct gpio_desc *descs; - if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1)) - && base >= 0) { - status = -EINVAL; - goto fail; - } + descs = kcalloc(chip->ngpio, sizeof(descs[0]), GFP_KERNEL); + if (!descs) + return -ENOMEM; spin_lock_irqsave(&gpio_lock, flags); @@ -247,10 +256,8 @@ int gpiochip_add(struct gpio_chip *chip) status = gpiochip_add_to_list(chip); if (status == 0) { - chip->desc = &gpio_desc[chip->base]; - for (id = 0; id < chip->ngpio; id++) { - struct gpio_desc *desc = &chip->desc[id]; + struct gpio_desc *desc = &descs[id]; desc->chip = chip; /* REVISIT: most hardware initializes GPIOs as @@ -266,6 +273,8 @@ int gpiochip_add(struct gpio_chip *chip) } } + chip->desc = descs; + spin_unlock_irqrestore(&gpio_lock, flags); #ifdef CONFIG_PINCTRL @@ -291,6 +300,9 @@ int gpiochip_add(struct gpio_chip *chip) unlock: spin_unlock_irqrestore(&gpio_lock, flags); fail: + kfree(descs); + chip->desc = NULL; + /* failures here can mean systems won't boot... */ pr_err("%s: GPIOs %d..%d (%s) failed to register\n", __func__, chip->base, chip->base + chip->ngpio - 1, @@ -331,6 +343,9 @@ void gpiochip_remove(struct gpio_chip *chip) list_del(&chip->list); spin_unlock_irqrestore(&gpio_lock, flags); gpiochip_unexport(chip); + + kfree(chip->desc); + chip->desc = NULL; } EXPORT_SYMBOL_GPL(gpiochip_remove);
Replace the ARCH_NR_GPIOS-sized static array of GPIO descriptors by dynamically-allocated arrays for each GPIO chip. This change makes gpio_to_desc() perform in O(n) (where n is the number of GPIO chips registered) instead of O(1), however since n is rarely bigger than 1 or 2 no noticeable performance issue is expected. Besides this provides more incentive for GPIO consumers to move to the gpiod interface. One could use a O(log(n)) structure to link the GPIO chips together, but considering the low limit of n the hypothetical performance benefit is probably not worth the added complexity. This patch uses kcalloc() in gpiochip_add(), which removes the ability to add a chip before kcalloc() can operate. I am not aware of such cases, but if someone bisects up to this patch then I will be proven wrong... Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/gpio/gpiolib.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-)