diff mbox series

[RFC] gpiolib: remove extra_checks

Message ID 20231219201102.41639-1-brgl@bgdev.pl
State New
Headers show
Series [RFC] gpiolib: remove extra_checks | expand

Commit Message

Bartosz Golaszewski Dec. 19, 2023, 8:11 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

Comments

Linus Walleij Dec. 20, 2023, 11:35 a.m. UTC | #1 
On Tue, Dec 19, 2023 at 9:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 <bartosz.golaszewski@linaro.org>

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 <linus.walleij@linaro.org>

Yours,
Linus Walleij
Andy Shevchenko Dec. 20, 2023, 2:03 p.m. UTC | #2 
On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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.
Bartosz Golaszewski Dec. 20, 2023, 2:08 p.m. UTC | #3 
On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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
>
>
Linus Walleij Dec. 20, 2023, 3:28 p.m. UTC | #4 
On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 Dec. 21, 2023, 9:26 a.m. UTC | #5 
On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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
Andy Shevchenko Dec. 21, 2023, 12:52 p.m. UTC | #6 
On Thu, Dec 21, 2023 at 10:26:03AM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 4:28 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > 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.
Bartosz Golaszewski Dec. 21, 2023, 1 p.m. UTC | #7 
On Thu, Dec 21, 2023 at 1:52 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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 <linus.walleij@linaro.org> wrote:
> > > On Wed, Dec 20, 2023 at 3:03 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > 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
>
>
Andy Shevchenko Dec. 21, 2023, 4:13 p.m. UTC | #8 
On Thu, Dec 21, 2023 at 02:00:39PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 21, 2023 at 1:52 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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 <linus.walleij@linaro.org> 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) {
		...
	}
Bartosz Golaszewski Dec. 21, 2023, 6:50 p.m. UTC | #9 
On Thu, Dec 21, 2023 at 5:13 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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
> > <andriy.shevchenko@linux.intel.com> 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 <linus.walleij@linaro.org> 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
>
>
Bartosz Golaszewski Dec. 27, 2023, 2:59 p.m. UTC | #10 
On Tue, Dec 19, 2023 at 9:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 <bartosz.golaszewski@linaro.org>
> ---

Patch applied.

Bart
Guenter Roeck Jan. 16, 2024, 6:23 p.m. UTC | #11 
Hi,

On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>

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 Jan. 16, 2024, 9:41 p.m. UTC | #12 
On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Hi,
>
> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 <bartosz.golaszewski@linaro.org>
>
> 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://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 Jan. 16, 2024, 10:34 p.m. UTC | #13 
On 1/16/24 13:41, Bartosz Golaszewski wrote:
> On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Hi,
>>
>> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> 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 <bartosz.golaszewski@linaro.org>
>>
>> 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
Bartosz Golaszewski Jan. 17, 2024, 8:51 a.m. UTC | #14 
On Tue, Jan 16, 2024 at 11:34 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/16/24 13:41, Bartosz Golaszewski wrote:
> > On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>> 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 <bartosz.golaszewski@linaro.org>
> >>
> >> 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
diff mbox series

Patch

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,