Re: [RFC PATCH] gpiolib: remove extra_checks

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Jan 16, 2024 at 11:34 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 1/16/24 13:41, Bartosz Golaszewski wrote:
> > On Tue, Jan 16, 2024 at 7:23 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Dec 19, 2023 at 09:11:02PM +0100, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>
> >> 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