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(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c9ca809b55de..837e9919bf07 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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,
--
2.40.1
On Tue, Dec 19, 2023 at 9:11 PM Bartosz Golaszewski <[email protected]> wrote:
> 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
On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> 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?
> 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.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > 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.
>
> > 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
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > 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
On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > 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
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
On Thu, Dec 21, 2023 at 10:26:03AM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
> > On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > > 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.
I agree on this statement.
> Defining DEBUG makes sense to
> enable dev_dbg() messages.
Exactly!
> 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).
> 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.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 21, 2023 at 1:52 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Dec 21, 2023 at 10:26:03AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
> > > On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > > > 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.
>
> I agree on this statement.
>
> > Defining DEBUG makes sense to
> > enable dev_dbg() messages.
>
> Exactly!
>
> > 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.
> > 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
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Thu, Dec 21, 2023 at 02:00:39PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 21, 2023 at 1:52 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Dec 21, 2023 at 10:26:03AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
...
> > > Defining DEBUG makes sense to
> > > enable dev_dbg() messages.
> >
> > Exactly!
> >
> > > 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.
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?
> > > 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
On Thu, Dec 21, 2023 at 5:13 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Dec 21, 2023 at 02:00:39PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 21, 2023 at 1:52 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Thu, Dec 21, 2023 at 10:26:03AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <[email protected]> wrote:
>
> ...
>
> > > > Defining DEBUG makes sense to
> > > > enable dev_dbg() messages.
> > >
> > > Exactly!
> > >
> > > > 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.
>
> 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
> > > > 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
>
>
On Tue, Dec 19, 2023 at 9:11 PM Bartosz Golaszewski <[email protected]> wrote:
>
> 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
Hi,
On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> 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://http://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
On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <[email protected]> wrote:
>
> Hi,
>
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > 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
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
> 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://http://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
On 1/16/24 13:41, Bartosz Golaszewski wrote:
> On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <[email protected]> wrote:
>>
>> Hi,
>>
>> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
>>> 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
>
> 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
On Tue, Jan 16, 2024 at 11:34 PM Guenter Roeck <[email protected]> wrote:
>
> On 1/16/24 13:41, Bartosz Golaszewski wrote:
> > On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <[email protected]> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> >>> 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
> >
> > 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.
>
No worries, I'm just stating that before someone wants a revert. This
has been a bug all along in MMC code.
Bartosz