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 <[email protected]>
---
drivers/gpio/Kconfig | 11 -----------
drivers/gpio/Makefile | 2 --
drivers/gpio/gpiolib.c | 30 +++++++++---------------------
3 files changed, 9 insertions(+), 34 deletions(-)
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,
--
2.35.1.723.g4982287a31-goog
On Mon, Mar 14, 2022 at 3:23 PM Linus Walleij <[email protected]> wrote:
> On Mon, Mar 14, 2022 at 4:00 PM Bartosz Golaszewski <[email protected]> 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
On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <[email protected]> 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 <[email protected]>
> ---
>
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 <[email protected]> wrote:
> On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <[email protected]> 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 <[email protected]>
> > ---
> >
>
> 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
Hi Brian,
On Tue, Mar 15, 2022 at 1:19 AM Brian Norris <[email protected]> wrote:
> On Mon, Mar 14, 2022 at 3:23 PM Linus Walleij <[email protected]> wrote:
> > On Mon, Mar 14, 2022 at 4:00 PM Bartosz Golaszewski <[email protected]> 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 -- [email protected]
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 Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <[email protected]> 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?
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.
Bart
On Mon, Mar 14, 2022 at 6:35 PM Bartosz Golaszewski <[email protected]> wrote:
> On Fri, Mar 11, 2022 at 12:09 AM Brian Norris <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <[email protected]> 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.
...
> -ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG
> -
NAK to this change.
I'm not against enabling might_sleep() unconditionally.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 22, 2022 at 8:00 AM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Mar 22, 2022 at 4:49 PM Bartosz Golaszewski <[email protected]> wrote:
> > On Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <[email protected]> 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 <[email protected]> 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
On Tue, Mar 22, 2022 at 5:31 PM Brian Norris <[email protected]> wrote:
>
> On Tue, Mar 22, 2022 at 8:00 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Mar 22, 2022 at 4:49 PM Bartosz Golaszewski <[email protected]> wrote:
> > > On Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <[email protected]> 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
I'm about to send my PR for v5.18 so I'll remove this one from my
for-next branch as it's not urgent. Let's discuss it during the next
cycle.
Bart
On Tue, Mar 22, 2022 at 4:49 PM Bartosz Golaszewski <[email protected]> wrote:
> On Tue, Mar 22, 2022 at 3:38 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Mar 11, 2022 at 4:55 AM Brian Norris <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko