| Message ID | 20220310150905.1.Ie0a005d7a763d501e03b7abe8ee968ca99d23282@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Drop CONFIG_DEBUG_GPIO | expand |
On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <briannorris@chromium.org> wrote: > > CONFIG_DEBUG_GPIO has existed since the introduction of gpiolib, but its > Kconfig description and motivation seem to have been off-base for quite > some time. > > Description: it says nothing about enabling extra printk()s. But -DDEBUG > does just that; it turns on every dev_dbg()/pr_debug() that would > otherwise be silent. > > Purpose: might_sleep() and WARN_ON() should have very low overhead, and > anyway, there's a separate CONFIG_DEBUG_ATOMIC_SLEEP for the > might_sleep() overhead. > > Additionally, the conflated purpose (extra debug checks, and extra > printing) makes for a mixed bag for users. In particular, some drivers > can be extra-spammy with -DDEBUG -- e.g., with the Rockchip GPIO driver > getting moved out of drivers/pinctrl/ in commit 936ee2675eee > ("gpio/rockchip: add driver for rockchip gpio"), now some dev_dbg() > calls are enabled in its IRQ handler. > > Altogether, it seems like CONFIG_DEBUG_GPIO isn't serving any good > purpose and should just be removed. It can be supplanted by dynamic > debug (which post-dates gpiolib) and atomic-debug facilities. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > I like it. It's true we don't see many of those DEBUG constructs anymore nowadays and overhead for might_sleep() and WARN_ON() is negligible. Applied, thanks! Bart
On Mon, Mar 14, 2022 at 4:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <briannorris@chromium.org> wrote: > > > > CONFIG_DEBUG_GPIO has existed since the introduction of gpiolib, but its > > Kconfig description and motivation seem to have been off-base for quite > > some time. > > > > Description: it says nothing about enabling extra printk()s. But -DDEBUG > > does just that; it turns on every dev_dbg()/pr_debug() that would > > otherwise be silent. > > > > Purpose: might_sleep() and WARN_ON() should have very low overhead, and > > anyway, there's a separate CONFIG_DEBUG_ATOMIC_SLEEP for the > > might_sleep() overhead. > > > > Additionally, the conflated purpose (extra debug checks, and extra > > printing) makes for a mixed bag for users. In particular, some drivers > > can be extra-spammy with -DDEBUG -- e.g., with the Rockchip GPIO driver > > getting moved out of drivers/pinctrl/ in commit 936ee2675eee > > ("gpio/rockchip: add driver for rockchip gpio"), now some dev_dbg() > > calls are enabled in its IRQ handler. > > > > Altogether, it seems like CONFIG_DEBUG_GPIO isn't serving any good > > purpose and should just be removed. It can be supplanted by dynamic > > debug (which post-dates gpiolib) and atomic-debug facilities. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > > > I like it. It's true we don't see many of those DEBUG constructs > anymore nowadays and overhead for might_sleep() and WARN_ON() is > negligible. I agree. I have something similar for pinctrl, maybe that needs to go too. Yours, Linus Walleij
On Mon, Mar 14, 2022 at 3:23 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Mar 14, 2022 at 4:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > I like it. It's true we don't see many of those DEBUG constructs > > anymore nowadays and overhead for might_sleep() and WARN_ON() is > > negligible. > > I agree. I have something similar for pinctrl, maybe that needs to > go too. Huh, yeah, CONFIG_DEBUG_PINCTRL does look awfully similar, and I just didn't notice because we don't happen to have it enabled for Chromium kernels. We happen to have CONFIG_DEBUG_GPIO enabled though, and the "new" rockchip-gpio log messages triggered me :) I guess one difference is that CONFIG_DEBUG_PINCTRL is almost exclusively (aside from some renesas drivers?) about extra logging and less about interesting checks that one might want to enable in more general settings. So it's a clearer call to make that people generally want it disabled. In other words, a -DDEBUG construct in itself isn't necessarily terrible (even if it's a little redundant with dynamic debug?), if it's not conflated with stuff that might be more generally useful. Regards, Brian
Hi Brian, On Tue, Mar 15, 2022 at 1:19 AM Brian Norris <briannorris@chromium.org> wrote: > On Mon, Mar 14, 2022 at 3:23 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 14, 2022 at 4:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > I like it. It's true we don't see many of those DEBUG constructs > > > anymore nowadays and overhead for might_sleep() and WARN_ON() is > > > negligible. > > > > I agree. I have something similar for pinctrl, maybe that needs to > > go too. > > Huh, yeah, CONFIG_DEBUG_PINCTRL does look awfully similar, and I just > didn't notice because we don't happen to have it enabled for Chromium > kernels. We happen to have CONFIG_DEBUG_GPIO enabled though, and the > "new" rockchip-gpio log messages triggered me :) > > I guess one difference is that CONFIG_DEBUG_PINCTRL is almost > exclusively (aside from some renesas drivers?) about extra logging and > less about interesting checks that one might want to enable in more > general settings. So it's a clearer call to make that people generally > want it disabled. For the Renesas pinctrl drivers, it enables sanity checks on the pin control tables, which you definitely do not want in your production kernel. 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
On Mon, Mar 14, 2022 at 6:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <briannorris@chromium.org> wrote: > I like it. It's true we don't see many of those DEBUG constructs > anymore nowadays and overhead for might_sleep() and WARN_ON() is > negligible. I don't like the part about removing -DDEBUG at all. > Applied, thanks! Shall I send a partial revert?
On Tue, Mar 22, 2022 at 4:49 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <briannorris@chromium.org> wrote: > > > > ... > > > > > Description: it says nothing about enabling extra printk()s. But -DDEBUG > > > does just that; it turns on every dev_dbg()/pr_debug() that would > > > otherwise be silent. > > > > Which is what some and I are using a lot during development. > > > > AFAIK this: https://www.kernel.org/doc/local/pr_debug.txt is the right > way to do it? Yes. But it means we need to have a separate option on a per driver (or group of drivers) basis. I don't think it's a good idea right now. > https://www.kernel.org/doc/local/pr_debug.txt > > This doesn't mention adding Kconfig options just to enable debug messages. > > > ... > > > > > -ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG > > > - > > > > NAK to this change. > > > > I'm not against enabling might_sleep() unconditionally. > > > > These are already controlled by CONFIG_DEBUG_ATOMIC_SLEEP, no? Or > maybe I can't parse that double negation. The part of the patch that converts might_sleep_if():s is fine with me.
On Tue, Mar 22, 2022 at 8:00 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Mar 22, 2022 at 4:49 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <briannorris@chromium.org> wrote: > > > > > > ... > > > > > > > Description: it says nothing about enabling extra printk()s. But -DDEBUG > > > > does just that; it turns on every dev_dbg()/pr_debug() that would > > > > otherwise be silent. > > > > > > Which is what some and I are using a lot during development. Well, we could fix that part by updating the documentation, so users know what they're getting themselves into. I'm also curious: does dynamic debug not suit you? https://www.kernel.org/doc/html/v4.19/admin-guide/dynamic-debug-howto.html TBH, I never remember its syntax, and it seems very easy to get wrong, so I often throw in #define's myself, if I want it foolproof. But I'm curious others thoughts too. > > AFAIK this: https://www.kernel.org/doc/local/pr_debug.txt is the right > > way to do it? > > Yes. But it means we need to have a separate option on a per driver > (or group of drivers) basis. I don't think it's a good idea right now. I'm not sure I understand this thought; isn't this the opposite of what you're arguing above? (That drivers/gpio/ deserves its own Kconfig option for enabling (non-dynamic) debug prints?) > > https://www.kernel.org/doc/local/pr_debug.txt > > > > This doesn't mention adding Kconfig options just to enable debug messages. > > > > > ... > > > > > > > -ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG > > > > - > > > > > > NAK to this change. > > > > > > I'm not against enabling might_sleep() unconditionally. > > > > > > > These are already controlled by CONFIG_DEBUG_ATOMIC_SLEEP, no? Or > > maybe I can't parse that double negation. > > The part of the patch that converts might_sleep_if():s is fine with me. I'm fine with that approach (keep CONFIG_DEBUG_GPIO *only* as a print-verbosity/DDEBUG control), even if I think it's a bit odd. My main point in the patch is differentiating debug checks (that I want; that are silent-by-default; that have their own Kconfig knobs) from debug prints (that are noisy by default; that I don't want). So if you convince Bartosz and/or Linus, you can get an Ack from me for a partial revert. Regards, Brian
On Tue, Mar 22, 2022 at 5:31 PM Brian Norris <briannorris@chromium.org> wrote: > I'm also curious: does dynamic debug not suit you? > https://www.kernel.org/doc/html/v4.19/admin-guide/dynamic-debug-howto.html > TBH, I never remember its syntax, and it seems very easy to get wrong, > so I often throw in #define's myself, if I want it foolproof. But I'm > curious others thoughts too. Dynamic debug almost always assume that the system comes up so you can go in and enable it manually. What about problems during boot. Or if the system doesn't even get to userspace? GPIO and pin control can be critical system resources and the platform may not boot completely or mount root as a result of some problem you're debugging. True, there are ways to pass arguments also on the command line argument when the kernel boots. Figuring out how the command line should look to enable -DDEBUG at boot time on say drivers/pinctrl/intel/* is a pretty horrific exercise. Add to that using the boot loader interactively to type that long argument in for every reboot. In all such scenarios what people do is go into the Makefile and add -DDEBUG to the CFLAGS as a hack instead, because it is faster and persistent. Adding that as an option in KConfig achieves the same thing, just easier than hacking the Makefile. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index ad99b96f6d79..ef91cc3066f5 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -47,17 +47,6 @@ config GPIOLIB_IRQCHIP select IRQ_DOMAIN bool -config DEBUG_GPIO - bool "Debug GPIO calls" - depends on DEBUG_KERNEL - help - Say Y here to add some extra checks and diagnostics to GPIO calls. - These checks help ensure that GPIOs have been properly initialized - before they are used, and that sleeping calls are not made from - non-sleeping contexts. They can make bitbanged serial protocols - slower. The diagnostics help catch the type of setup errors - that are most common when setting up new platforms or boards. - config GPIO_SYSFS bool "/sys/class/gpio/... (sysfs interface)" if EXPERT depends on SYSFS diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 30141fec12be..f3ddac590ffe 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -1,8 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 # generic gpio support: platform drivers, dedicated expander chips, etc -ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG - obj-$(CONFIG_GPIOLIB) += gpiolib.o obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 56975a6d7c9e..1e1a193987fd 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -40,18 +40,6 @@ */ -/* When debugging, extend minimal trust to callers and platform code. - * Also emit diagnostic messages that may help initial bringup, when - * board setup or driver bugs are most common. - * - * Otherwise, minimize overhead in what may be bitbanging codepaths. - */ -#ifdef DEBUG -#define extra_checks 1 -#else -#define extra_checks 0 -#endif - /* Device and char device-related information */ static DEFINE_IDA(gpio_ida); static dev_t gpio_devt; @@ -2046,7 +2034,7 @@ void gpiod_free(struct gpio_desc *desc) module_put(desc->gdev->owner); put_device(&desc->gdev->dev); } else { - WARN_ON(extra_checks); + WARN_ON(1); } } @@ -3338,7 +3326,7 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent); */ int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc) { - might_sleep_if(extra_checks); + might_sleep(); VALIDATE_DESC(desc); return gpiod_get_raw_value_commit(desc); } @@ -3357,7 +3345,7 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc) { int value; - might_sleep_if(extra_checks); + might_sleep(); VALIDATE_DESC(desc); value = gpiod_get_raw_value_commit(desc); if (value < 0) @@ -3388,7 +3376,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size, struct gpio_array *array_info, unsigned long *value_bitmap) { - might_sleep_if(extra_checks); + might_sleep(); if (!desc_array) return -EINVAL; return gpiod_get_array_value_complex(true, true, array_size, @@ -3414,7 +3402,7 @@ int gpiod_get_array_value_cansleep(unsigned int array_size, struct gpio_array *array_info, unsigned long *value_bitmap) { - might_sleep_if(extra_checks); + might_sleep(); if (!desc_array) return -EINVAL; return gpiod_get_array_value_complex(false, true, array_size, @@ -3435,7 +3423,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_array_value_cansleep); */ void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value) { - might_sleep_if(extra_checks); + might_sleep(); VALIDATE_DESC_VOID(desc); gpiod_set_raw_value_commit(desc, value); } @@ -3453,7 +3441,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_raw_value_cansleep); */ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) { - might_sleep_if(extra_checks); + might_sleep(); VALIDATE_DESC_VOID(desc); gpiod_set_value_nocheck(desc, value); } @@ -3476,7 +3464,7 @@ int gpiod_set_raw_array_value_cansleep(unsigned int array_size, struct gpio_array *array_info, unsigned long *value_bitmap) { - might_sleep_if(extra_checks); + might_sleep(); if (!desc_array) return -EINVAL; return gpiod_set_array_value_complex(true, true, array_size, desc_array, @@ -3518,7 +3506,7 @@ int gpiod_set_array_value_cansleep(unsigned int array_size, struct gpio_array *array_info, unsigned long *value_bitmap) { - might_sleep_if(extra_checks); + might_sleep(); if (!desc_array) return -EINVAL; return gpiod_set_array_value_complex(false, true, array_size,
CONFIG_DEBUG_GPIO has existed since the introduction of gpiolib, but its Kconfig description and motivation seem to have been off-base for quite some time. Description: it says nothing about enabling extra printk()s. But -DDEBUG does just that; it turns on every dev_dbg()/pr_debug() that would otherwise be silent. Purpose: might_sleep() and WARN_ON() should have very low overhead, and anyway, there's a separate CONFIG_DEBUG_ATOMIC_SLEEP for the might_sleep() overhead. Additionally, the conflated purpose (extra debug checks, and extra printing) makes for a mixed bag for users. In particular, some drivers can be extra-spammy with -DDEBUG -- e.g., with the Rockchip GPIO driver getting moved out of drivers/pinctrl/ in commit 936ee2675eee ("gpio/rockchip: add driver for rockchip gpio"), now some dev_dbg() calls are enabled in its IRQ handler. Altogether, it seems like CONFIG_DEBUG_GPIO isn't serving any good purpose and should just be removed. It can be supplanted by dynamic debug (which post-dates gpiolib) and atomic-debug facilities. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/gpio/Kconfig | 11 ----------- drivers/gpio/Makefile | 2 -- drivers/gpio/gpiolib.c | 30 +++++++++--------------------- 3 files changed, 9 insertions(+), 34 deletions(-)