gpiolib: remove extra_checks

RFC PATCH In review Bartosz Golaszewski 4 participants 15 messages Dec 19, 2023 – Jan 17, 2024 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-12-19 20:11 UTC #0 lore raw

RFC PATCH 1/1 gpiolib: remove extra_checks

From: Bartosz Golaszewski <[email protected]>

extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

drivers/gpio/gpiolib.c +9 ~~-22~~

index c9ca809b55de..837e9919bf07 100644
@@ -47,19 +47,6 @@
  * GPIOs can sometimes cost only an instruction or two per bit.
  */
 
-
-/* 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;

@@ -2351,7 +2338,7 @@ void gpiod_free(struct gpio_desc *desc)
 		return;
 
 	if (!gpiod_free_commit(desc))
-		WARN_ON(extra_checks);
+		WARN_ON(1);
 
 	module_put(desc->gdev->owner);
 	gpio_device_put(desc->gdev);

@@ -3729,7 +3716,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);
 }

@@ -3748,7 +3735,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)

@@ -3779,7 +3766,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,

@@ -3805,7 +3792,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,

@@ -3826,7 +3813,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);
 }

@@ -3844,7 +3831,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);
 }

@@ -3867,7 +3854,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,

@@ -3909,7 +3896,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,

Signature · 1 line

2.40.1

Linus Walleij

Reviewed-by on v1 1/1

2023-12-20 11:35 UTC #1 lore raw

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>

I see you pinged me to fix this and I didn't get around to look into it even, but you fixed it just as fine yourself :D

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

2023-12-20 14:03 UTC #2 lore raw

In reply to Bartosz Golaszewski

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.

Huh?!

What then CONFIG_DEBUG_GPIO is about?

Quoted 3 lines

The
overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.

...

I agree on might_sleep() changes, but WARN_ON(), see above why.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-12-20 14:08 UTC #3 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.
Huh?!
What then CONFIG_DEBUG_GPIO is about?

Ah, right, it's set in Makefile. I didn't notice before.

Quoted 9 lines

Quoted 4 lines
The
overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
...
I agree on might_sleep() changes, but WARN_ON(), see above why.

See above where?

Bart

Quoted 5 lines

--
With Best Regards,
Andy Shevchenko

Linus Walleij

2023-12-20 15:28 UTC #4 lore raw

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.
Huh?!
What then CONFIG_DEBUG_GPIO is about?

Yeah that is some helper DBrownell added because like me he could never figure out how to pass -DDEBUG to a single file on the command line and besides gpiolib is several files. I added the same to pinctrl to get core debug messages.

I guess Bartosz means extra_checks is == a non-standard DEBUG define.

Yours,
Linus Walleij

Bartosz Golaszewski

author

2023-12-21 09:26 UTC #5 lore raw

Quoted 19 lines · Linus Walleij

Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.
Huh?!
What then CONFIG_DEBUG_GPIO is about?
Yeah that is some helper DBrownell added because like me he could never figure out how to pass -DDEBUG to a single file on the command line and besides gpiolib is several files. I added the same to pinctrl to get core debug messages.

Quoted 5 lines

I guess Bartosz means extra_checks is == a non-standard DEBUG define.
Yours,
Linus Walleij

I think this patch is still correct. Defining DEBUG makes sense to enable dev_dbg() messages. CONFIG_DEBUG_GPIO is used by one driver to enable code that can lead to undefined behavior (should it maybe be #if 0?). So the Makefile bit and the Kconfig option can stay but I'd love to see extra_checks gone.

Bart

Andy Shevchenko

2023-12-21 12:52 UTC #6 lore raw

Quoted 14 lines · Bartosz Golaszewski

Quoted 13 lines · Linus Walleij
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.
Huh?!
What then CONFIG_DEBUG_GPIO is about?

Quoted 7 lines

Quoted 7 lines
Yeah that is some helper DBrownell added because like me he could never figure out how to pass -DDEBUG to a single file on the command line and besides gpiolib is several files. I added the same to pinctrl to get core debug messages.
I guess Bartosz means extra_checks is == a non-standard DEBUG define.

I agree on this statement.

Quoted 2 lines

Defining DEBUG makes sense to
enable dev_dbg() messages.

Exactly!

Quoted 1 line

CONFIG_DEBUG_GPIO is used by one driver

By all drivers which are using pr_debug() / dev_dbg().
I am using it a lot in my development process (actually I have it enabled in all my kernel configurations).

Quoted 2 lines

to enable code that can lead to undefined behavior (should it maybe be #if 0?).

I don't know what you are talking about here.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-12-21 13:00 UTC #7 lore raw

Quoted 29 lines · Andy Shevchenko

Quoted 14 lines · Bartosz Golaszewski
Quoted 13 lines · Linus Walleij
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on
a non-standard DEBUG define one needs to add to the source file.
Huh?!
What then CONFIG_DEBUG_GPIO is about?
Quoted 7 lines
Quoted 7 lines
Yeah that is some helper DBrownell added because like me he could never figure out how to pass -DDEBUG to a single file on the command line and besides gpiolib is several files. I added the same to pinctrl to get core debug messages.
I guess Bartosz means extra_checks is == a non-standard DEBUG define.
I agree on this statement.
Quoted 2 lines
Defining DEBUG makes sense to
enable dev_dbg() messages.

Quoted 8 lines

Exactly!
Quoted 1 line
CONFIG_DEBUG_GPIO is used by one driver
By all drivers which are using pr_debug() / dev_dbg().
I am using it a lot in my development process (actually I have it enabled in all my kernel configurations).

I'm not saying we should remove it. It'll stay defined in the Makefile and remain seamless for debug messages. I just want to get rid of that ugly extra_checks variable which has very little impact.

Quoted 5 lines

Quoted 2 lines
to enable code that can lead to undefined behavior (should it maybe be #if 0?).
I don't know what you are talking about here.

I'm talking about drivers/gpio/gpio-tps65219.c and its usage of CONFIG_DEBUG_GPIO.

Bart

Quoted 5 lines

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2023-12-21 16:13 UTC #8 lore raw

Quoted 4 lines · Bartosz Golaszewski

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Bartosz Golaszewski
On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:

...

Quoted 7 lines

Quoted 7 lines
Quoted 2 lines
Defining DEBUG makes sense to
enable dev_dbg() messages.
Exactly!
Quoted 1 line
CONFIG_DEBUG_GPIO is used by one driver

Quoted 7 lines

Quoted 3 lines
By all drivers which are using pr_debug() / dev_dbg().
I am using it a lot in my development process (actually I have it enabled in all my kernel configurations).
I'm not saying we should remove it. It'll stay defined in the Makefile and remain seamless for debug messages. I just want to get rid of that ugly extra_checks variable which has very little impact.

I agree that extra_checks is unusual (or as Linus put it "non-standard") thingy. And I agree that removal is for good.

My question here solely about that WARN_ON(). Do we need it always be enabled or not?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
to enable code that can lead to undefined behavior (should it maybe be #if 0?).
I don't know what you are talking about here.
I'm talking about drivers/gpio/gpio-tps65219.c and its usage of CONFIG_DEBUG_GPIO.

Oh, that one should probably be

#if 0
	...

#endif

	if (0) {
		...
	}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-12-21 18:50 UTC #9 lore raw

Quoted 24 lines · Andy Shevchenko

Quoted 4 lines · Bartosz Golaszewski
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Bartosz Golaszewski
On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
...
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Defining DEBUG makes sense to
enable dev_dbg() messages.
Exactly!
Quoted 1 line
CONFIG_DEBUG_GPIO is used by one driver
Quoted 7 lines
Quoted 3 lines
By all drivers which are using pr_debug() / dev_dbg().
I am using it a lot in my development process (actually I have it enabled in all my kernel configurations).
I'm not saying we should remove it. It'll stay defined in the Makefile and remain seamless for debug messages. I just want to get rid of that ugly extra_checks variable which has very little impact.

Quoted 6 lines

I agree that extra_checks is unusual (or as Linus put it "non-standard") thingy. And I agree that removal is for good.
My question here solely about that WARN_ON(). Do we need it always be enabled or not?

I think it makes sense. If you're freeing a non-requested descriptor then you clearly are doing something wrong and the system should yell.

Bart

Quoted 25 lines

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
to enable code that can lead to undefined behavior (should it maybe be #if 0?).
I don't know what you are talking about here.
I'm talking about drivers/gpio/gpio-tps65219.c and its usage of CONFIG_DEBUG_GPIO.
Oh, that one should probably be
#if 0
        ...
#endif
or
        if (0) {
                ...
        }
--
With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-12-27 14:59 UTC #10 lore raw

In reply to Bartosz Golaszewski

Quoted 10 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>
---

Patch applied.

Bart

Guenter Roeck

2024-01-16 18:23 UTC #11 lore raw

In reply to Bartosz Golaszewski

Hi,

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>

This patch triggers (exposes) the following backtrace.

BUG: sleeping function called from invalid context at drivers/gpio/gpiolib.c:3738

in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 7, name: kworker/0:0
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
3 locks held by kworker/0:0/7:
#0: c181b3a4 ((wq_completion)events_freezable){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #1: c883df28 ((work_completion)(&(&host->detect)->work)){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #2: c24e1720 (&host->lock){-...}-{2:2}, at: sdhci_check_ro+0x14/0xd4

irq event stamp: 2916
hardirqs last  enabled at (2915): [<c0b18838>] _raw_spin_unlock_irqrestore+0x70/0x84
hardirqs last disabled at (2916): [<c0b1853c>] _raw_spin_lock_irqsave+0x74/0x78
softirqs last  enabled at (2360): [<c00098a4>] __do_softirq+0x28c/0x4b0
softirqs last disabled at (2347): [<c0022774>] __irq_exit_rcu+0x15c/0x1a4
CPU: 0 PID: 7 Comm: kworker/0:0 Tainted: G                 N 6.7.0-09928-g052d534373b7 #1
Hardware name: Freescale i.MX25 (Device Tree Support)
Workqueue: events_freezable mmc_rescan
 unwind_backtrace from show_stack+0x10/0x18
 show_stack from dump_stack_lvl+0x34/0x54
 dump_stack_lvl from __might_resched+0x188/0x274
 __might_resched from gpiod_get_value_cansleep+0x14/0x60
 gpiod_get_value_cansleep from mmc_gpio_get_ro+0x20/0x30
 mmc_gpio_get_ro from esdhc_pltfm_get_ro+0x20/0x48
 esdhc_pltfm_get_ro from sdhci_check_ro+0x44/0xd4
 sdhci_check_ro from mmc_sd_setup_card+0x2a8/0x47c
 mmc_sd_setup_card from mmc_sd_init_card+0xfc/0x93c
 mmc_sd_init_card from mmc_attach_sd+0xd8/0x180
 mmc_attach_sd from mmc_rescan+0x2ac/0x30c
 mmc_rescan from process_scheduled_works+0x2e4/0x644
 process_scheduled_works from worker_thread+0x188/0x418
 worker_thread from kthread+0x11c/0x144
 kthread from ret_from_fork+0x14/0x38

This is with the imx25-pdk qemu emulation when booting from mmc/sd card. It isn't really surprising since sdhci_check_ro() calls the gpio code under spin_lock_irqsave(). No idea how to fix that, so I won't even try.

Bisect log attached for reference.

Guenter

---
# bad: [052d534373b7ed33712a63d5e17b2b6cdbce84fd] Merge tag 'exfat-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat
# good: [70d201a40823acba23899342d62bc2644051ad2e] Merge tag 'f2fs-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs
git bisect start 'HEAD' '70d201a40823'
# good: [b6e1b708176846248c87318786d22465ac96dd2c] drm/xe: Remove uninitialized variable from warning
git bisect good b6e1b708176846248c87318786d22465ac96dd2c
# good: [7912a6391f3ee7eb9f9a69227a209d502679bc0c] Merge tag 'sound-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good 7912a6391f3ee7eb9f9a69227a209d502679bc0c
# bad: [a3cc31e75185f9b1ad8dc45eac77f8de788dc410] Merge tag 'libnvdimm-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
git bisect bad a3cc31e75185f9b1ad8dc45eac77f8de788dc410
# bad: [576db73424305036a6aa9e40daf7109742fbb1df] Merge tag 'gpio-updates-for-v6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux
git bisect bad 576db73424305036a6aa9e40daf7109742fbb1df
# good: [61f4c3e6711477b8a347ca5fe89e5e6613e0a147] Merge tag 'linux-watchdog-6.8-rc1' of git://www.linux-watchdog.org/linux-watchdog
git bisect good 61f4c3e6711477b8a347ca5fe89e5e6613e0a147
# good: [12b7f4ddfcb66dafed432cf4a987f5b40179c0f1] Merge tag 'device_is_big_endian-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core into gpio/for-next git bisect good 12b7f4ddfcb66dafed432cf4a987f5b40179c0f1
# good: [ede7511e7c22c9542a699ddff9f32de74e0bb972] gpiolib: cdev: include overflow.h
git bisect good ede7511e7c22c9542a699ddff9f32de74e0bb972
# bad: [f34fd6ee1be84c6e64574e9eb58f89d32c7f98a4] gpio: dwapb: Use generic request, free and set_config
git bisect bad f34fd6ee1be84c6e64574e9eb58f89d32c7f98a4
# good: [7dd1871e5049bbd40ee78ac94b1678ba5caf2486] gpio: tps65219: don't use CONFIG_DEBUG_GPIO
git bisect good 7dd1871e5049bbd40ee78ac94b1678ba5caf2486
# bad: [0338f6a6fb659f083eca7dd5967bb668d14707f8] gpiolib: drop tabs from local variable declarations
git bisect bad 0338f6a6fb659f083eca7dd5967bb668d14707f8
# bad: [5d5dfc50e5689d5b09de4a323f84c28a6700d156] gpiolib: remove extra_checks
git bisect bad 5d5dfc50e5689d5b09de4a323f84c28a6700d156

# first bad commit: [5d5dfc50e5689d5b09de4a323f84c28a6700d156] gpiolib: remove extra_checks

Bartosz Golaszewski

author

2024-01-16 21:41 UTC #12 lore raw

Quoted 23 lines · Guenter Roeck

Hi,
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>
This patch triggers (exposes) the following backtrace.
BUG: sleeping function called from invalid context at drivers/gpio/gpiolib.c:3738
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 7, name: kworker/0:0
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
3 locks held by kworker/0:0/7:
#0: c181b3a4 ((wq_completion)events_freezable){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #1: c883df28 ((work_completion)(&(&host->detect)->work)){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #2: c24e1720 (&host->lock){-...}-{2:2}, at: sdhci_check_ro+0x14/0xd4

Quoted 13 lines

irq event stamp: 2916
hardirqs last  enabled at (2915): [<c0b18838>] _raw_spin_unlock_irqrestore+0x70/0x84
hardirqs last disabled at (2916): [<c0b1853c>] _raw_spin_lock_irqsave+0x74/0x78
softirqs last  enabled at (2360): [<c00098a4>] __do_softirq+0x28c/0x4b0
softirqs last disabled at (2347): [<c0022774>] __irq_exit_rcu+0x15c/0x1a4
CPU: 0 PID: 7 Comm: kworker/0:0 Tainted: G                 N 6.7.0-09928-g052d534373b7 #1
Hardware name: Freescale i.MX25 (Device Tree Support)
Workqueue: events_freezable mmc_rescan
 unwind_backtrace from show_stack+0x10/0x18
 show_stack from dump_stack_lvl+0x34/0x54
 dump_stack_lvl from __might_resched+0x188/0x274
 __might_resched from gpiod_get_value_cansleep+0x14/0x60
 gpiod_get_value_cansleep from mmc_gpio_get_ro+0x20/0x30

When getting GPIO value with a spinlock taken the driver *must* use the non-sleeping variant of this function: gpiod_get_value(). If the underlying driver can sleep then the developer seriously borked. The API contract has always been this way so I wouldn't treat it as a regression.

I'd start with checking if replacing this with gpiod_get_value() helps. Possibly even do:

if (in_atomic())

    gpiod_get_value();
else
    gpiod_get_value_cansleep();

Bartosz

Quoted 46 lines

mmc_gpio_get_ro from esdhc_pltfm_get_ro+0x20/0x48 esdhc_pltfm_get_ro from sdhci_check_ro+0x44/0xd4 sdhci_check_ro from mmc_sd_setup_card+0x2a8/0x47c mmc_sd_setup_card from mmc_sd_init_card+0xfc/0x93c mmc_sd_init_card from mmc_attach_sd+0xd8/0x180 mmc_attach_sd from mmc_rescan+0x2ac/0x30c mmc_rescan from process_scheduled_works+0x2e4/0x644 process_scheduled_works from worker_thread+0x188/0x418 worker_thread from kthread+0x11c/0x144 kthread from ret_from_fork+0x14/0x38
This is with the imx25-pdk qemu emulation when booting from mmc/sd card. It isn't really surprising since sdhci_check_ro() calls the gpio code under spin_lock_irqsave(). No idea how to fix that, so I won't even try.
Bisect log attached for reference.
Guenter
---
# bad: [052d534373b7ed33712a63d5e17b2b6cdbce84fd] Merge tag 'exfat-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat
# good: [70d201a40823acba23899342d62bc2644051ad2e] Merge tag 'f2fs-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs
git bisect start 'HEAD' '70d201a40823'
# good: [b6e1b708176846248c87318786d22465ac96dd2c] drm/xe: Remove uninitialized variable from warning
git bisect good b6e1b708176846248c87318786d22465ac96dd2c
# good: [7912a6391f3ee7eb9f9a69227a209d502679bc0c] Merge tag 'sound-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good 7912a6391f3ee7eb9f9a69227a209d502679bc0c
# bad: [a3cc31e75185f9b1ad8dc45eac77f8de788dc410] Merge tag 'libnvdimm-for-6.8' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
git bisect bad a3cc31e75185f9b1ad8dc45eac77f8de788dc410
# bad: [576db73424305036a6aa9e40daf7109742fbb1df] Merge tag 'gpio-updates-for-v6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux
git bisect bad 576db73424305036a6aa9e40daf7109742fbb1df
# good: [61f4c3e6711477b8a347ca5fe89e5e6613e0a147] Merge tag 'linux-watchdog-6.8-rc1' of git://www.linux-watchdog.org/linux-watchdog
git bisect good 61f4c3e6711477b8a347ca5fe89e5e6613e0a147
# good: [12b7f4ddfcb66dafed432cf4a987f5b40179c0f1] Merge tag 'device_is_big_endian-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core into gpio/for-next git bisect good 12b7f4ddfcb66dafed432cf4a987f5b40179c0f1
# good: [ede7511e7c22c9542a699ddff9f32de74e0bb972] gpiolib: cdev: include overflow.h
git bisect good ede7511e7c22c9542a699ddff9f32de74e0bb972
# bad: [f34fd6ee1be84c6e64574e9eb58f89d32c7f98a4] gpio: dwapb: Use generic request, free and set_config
git bisect bad f34fd6ee1be84c6e64574e9eb58f89d32c7f98a4
# good: [7dd1871e5049bbd40ee78ac94b1678ba5caf2486] gpio: tps65219: don't use CONFIG_DEBUG_GPIO
git bisect good 7dd1871e5049bbd40ee78ac94b1678ba5caf2486
# bad: [0338f6a6fb659f083eca7dd5967bb668d14707f8] gpiolib: drop tabs from local variable declarations
git bisect bad 0338f6a6fb659f083eca7dd5967bb668d14707f8
# bad: [5d5dfc50e5689d5b09de4a323f84c28a6700d156] gpiolib: remove extra_checks
git bisect bad 5d5dfc50e5689d5b09de4a323f84c28a6700d156
# first bad commit: [5d5dfc50e5689d5b09de4a323f84c28a6700d156] gpiolib: remove extra_checks

Guenter Roeck

2024-01-16 22:34 UTC #13 lore raw

Quoted 38 lines · Bartosz Golaszewski

Quoted 23 lines · Guenter Roeck
Hi,
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>
This patch triggers (exposes) the following backtrace.
BUG: sleeping function called from invalid context at drivers/gpio/gpiolib.c:3738
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 7, name: kworker/0:0
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
3 locks held by kworker/0:0/7:
#0: c181b3a4 ((wq_completion)events_freezable){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #1: c883df28 ((work_completion)(&(&host->detect)->work)){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #2: c24e1720 (&host->lock){-...}-{2:2}, at: sdhci_check_ro+0x14/0xd4
Quoted 13 lines
irq event stamp: 2916
hardirqs last  enabled at (2915): [<c0b18838>] _raw_spin_unlock_irqrestore+0x70/0x84
hardirqs last disabled at (2916): [<c0b1853c>] _raw_spin_lock_irqsave+0x74/0x78
softirqs last  enabled at (2360): [<c00098a4>] __do_softirq+0x28c/0x4b0
softirqs last disabled at (2347): [<c0022774>] __irq_exit_rcu+0x15c/0x1a4
CPU: 0 PID: 7 Comm: kworker/0:0 Tainted: G                 N 6.7.0-09928-g052d534373b7 #1
Hardware name: Freescale i.MX25 (Device Tree Support)
Workqueue: events_freezable mmc_rescan
  unwind_backtrace from show_stack+0x10/0x18
  show_stack from dump_stack_lvl+0x34/0x54
  dump_stack_lvl from __might_resched+0x188/0x274
  __might_resched from gpiod_get_value_cansleep+0x14/0x60
  gpiod_get_value_cansleep from mmc_gpio_get_ro+0x20/0x30

Quoted 6 lines

When getting GPIO value with a spinlock taken the driver *must* use the non-sleeping variant of this function: gpiod_get_value(). If the underlying driver can sleep then the developer seriously borked. The API contract has always been this way so I wouldn't treat it as a regression.

I said

"This patch triggers (exposes) the following backtrace"

and

"It isn't really surprising since sdhci_check_ro() calls the gpio code under
spin_lock_irqsave().
"

I didn't (intend to) claim that this would be a regression. It was supposed to be a report. My apologies if it came along the wrong way.

Guenter

Bartosz Golaszewski

author

2024-01-17 08:51 UTC #14 lore raw

Quoted 51 lines · Guenter Roeck

Quoted 38 lines · Bartosz Golaszewski
Quoted 23 lines · Guenter Roeck
Hi,
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
extra_checks is only used in a few places. It also depends on a non-standard DEBUG define one needs to add to the source file. The overhead of removing it should be minimal (we already use pure might_sleep() in the code anyway) so drop it.
Signed-off-by Bartosz Golaszewski <[email protected]>
This patch triggers (exposes) the following backtrace.
BUG: sleeping function called from invalid context at drivers/gpio/gpiolib.c:3738
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 7, name: kworker/0:0
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
3 locks held by kworker/0:0/7:
#0: c181b3a4 ((wq_completion)events_freezable){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #1: c883df28 ((work_completion)(&(&host->detect)->work)){+.+.}-{0:0}, at: process_scheduled_works+0x23c/0x644 #2: c24e1720 (&host->lock){-...}-{2:2}, at: sdhci_check_ro+0x14/0xd4
Quoted 13 lines
irq event stamp: 2916
hardirqs last  enabled at (2915): [<c0b18838>] _raw_spin_unlock_irqrestore+0x70/0x84
hardirqs last disabled at (2916): [<c0b1853c>] _raw_spin_lock_irqsave+0x74/0x78
softirqs last  enabled at (2360): [<c00098a4>] __do_softirq+0x28c/0x4b0
softirqs last disabled at (2347): [<c0022774>] __irq_exit_rcu+0x15c/0x1a4
CPU: 0 PID: 7 Comm: kworker/0:0 Tainted: G                 N 6.7.0-09928-g052d534373b7 #1
Hardware name: Freescale i.MX25 (Device Tree Support)
Workqueue: events_freezable mmc_rescan
  unwind_backtrace from show_stack+0x10/0x18
  show_stack from dump_stack_lvl+0x34/0x54
  dump_stack_lvl from __might_resched+0x188/0x274
  __might_resched from gpiod_get_value_cansleep+0x14/0x60
  gpiod_get_value_cansleep from mmc_gpio_get_ro+0x20/0x30
Quoted 6 lines
When getting GPIO value with a spinlock taken the driver *must* use the non-sleeping variant of this function: gpiod_get_value(). If the underlying driver can sleep then the developer seriously borked. The API contract has always been this way so I wouldn't treat it as a regression.
I said
"This patch triggers (exposes) the following backtrace"

Quoted 9 lines

and
"It isn't really surprising since sdhci_check_ro() calls the gpio code under
spin_lock_irqsave().
"
I didn't (intend to) claim that this would be a regression. It was supposed to be a report. My apologies if it came along the wrong way.

No worries, I'm just stating that before someone wants a revert. This has been a bug all along in MMC code.

Bartosz