Re: [RFC PATCH] gpiolib: remove extra_checks

[Guenter Roeck <linux@roeck-us.net>]

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