diff mbox series

gpio: Drop CONFIG_DEBUG_GPIO

Message ID 20220310150905.1.Ie0a005d7a763d501e03b7abe8ee968ca99d23282@changeid
State New
Headers show
Series gpio: Drop CONFIG_DEBUG_GPIO | expand

Commit Message

Brian Norris March 10, 2022, 11:09 p.m. UTC
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(-)

Comments

Bartosz Golaszewski March 14, 2022, 3 p.m. UTC | #1
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
Linus Walleij March 14, 2022, 10:22 p.m. UTC | #2
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
Brian Norris March 14, 2022, 10:34 p.m. UTC | #3
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
Geert Uytterhoeven March 22, 2022, 1:14 p.m. UTC | #4
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
Andy Shevchenko March 22, 2022, 2:39 p.m. UTC | #5
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?
Andy Shevchenko March 22, 2022, 2:59 p.m. UTC | #6
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.
Brian Norris March 22, 2022, 4:31 p.m. UTC | #7
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
Linus Walleij March 23, 2022, 8:44 p.m. UTC | #8
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 mbox series

Patch

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,