Re: [PATCH v2] ARM: dts: sun8i: Adjust power key nodes

[Samuel Holland <samuel@sholland.org>]

On 2/15/22 4:20 PM, Guenter Roeck wrote:
> On 2/15/22 12:34, Jernej Škrabec wrote:
>> Hi!
>>
>> Dne torek, 15. februar 2022 ob 01:27:32 CET je Guenter Roeck napisal(a):
>>> Hi,
>>>
>>> On Mon, Nov 29, 2021 at 05:55:10PM +0100, Jernej Skrabec wrote:
>>>> Several H3 and one H2+ board have power key nodes, which are slightly
>>>> off. Some are missing wakeup-source property and some have BTN_0 code
>>>> assigned instead of KEY_POWER.
>>>>
>>>> Adjust them, so they can function as intended by designer.
>>>>
>>>> Co-developed-by: Michael Klein <michael@fossekall.de>
>>>> Signed-off-by: Michael Klein <michael@fossekall.de>
>>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>
>>> This patch results in the following traceback when rebooting an
>>> orangepi-pc qemu emulation.
>>>
>>> [   30.899594]
>>> [   30.899685] ============================================
>>> [   30.899757] WARNING: possible recursive locking detected
>>> [   30.899938] 5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
>>> [   30.900055] --------------------------------------------
>>> [   30.900124] init/307 is trying to acquire lock:
>>> [   30.900246] c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at:
>> __irq_get_desc_lock+0x58/0xa0
>>> [   30.900900]
>>> [   30.900900] but task is already holding lock:
>>> [   30.900974] c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at:
>> __irq_get_desc_lock+0x58/0xa0
>>> [   30.901101]
>>> [   30.901101] other info that might help us debug this:
>>> [   30.901188]  Possible unsafe locking scenario:
>>> [   30.901188]
>>> [   30.901262]        CPU0
>>> [   30.901301]        ----
>>> [   30.901339]   lock(&irq_desc_lock_class);
>>> [   30.901411]   lock(&irq_desc_lock_class);
>>> [   30.901480]
>>> [   30.901480]  *** DEADLOCK ***
>>> [   30.901480]
>>> [   30.901554]  May be due to missing lock nesting notation
>>> [   30.901554]
>>> [   30.901657] 4 locks held by init/307:
>>> [   30.901724]  #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at:
>> __do_sys_reboot+0x90/0x23c
>>> [   30.901889]  #1: c20f7760 (&dev->mutex){....}-{3:3}, at:
>> device_shutdown+0xf4/0x224
>>> [   30.902016]  #2: c2e804d8 (&dev->mutex){....}-{3:3}, at:
>> device_shutdown+0x104/0x224
>>> [   30.902138]  #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at:
>> __irq_get_desc_lock+0x58/0xa0
>>> [   30.902281]
>>> [   30.902281] stack backtrace:
>>> [   30.902462] CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-
>> gc849047c2473 #1
>>> [   30.902572] Hardware name: Allwinner sun8i Family
>>> [   30.902781]  unwind_backtrace from show_stack+0x10/0x14
>>> [   30.902895]  show_stack from dump_stack_lvl+0x68/0x90
>>> [   30.902970]  dump_stack_lvl from __lock_acquire+0x1680/0x31a0
>>> [   30.903047]  __lock_acquire from lock_acquire+0x148/0x3dc
>>> [   30.903118]  lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
>>> [   30.903197]  _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
>>> [   30.903282]  __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
>>> [   30.903366]  irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
>>> [   30.903442]  irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
>>> [   30.903523]  gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
>>> [   30.903603]  gpio_keys_shutdown from device_shutdown+0x180/0x224
>>> [   30.903685]  device_shutdown from __do_sys_reboot+0x134/0x23c
>>> [   30.903764]  __do_sys_reboot from ret_fast_syscall+0x0/0x1c
>>> [   30.903894] Exception stack(0xc584ffa8 to 0xc584fff0)
>>> [   30.904013] ffa0:                   01234567 000c623f fee1dead 28121969
>> 01234567 00000000
>>> [   30.904117] ffc0: 01234567 000c623f 00000001 00000058 000d85c0 00000000
>> 00000000 00000000
>>> [   30.904213] ffe0: 000d8298 be84ddf4 000918bc b6eb0edc
>>> [   30.905189] reboot: Restarting system
>>>
>>> The warning is no longer seen after reverting this patch.
>>>
>>> The problem exists but is not seen in v5.17-rc4 because a bug in commit
>>> 8df89a7cbc63 ("pinctrl-sunxi: don't call pinctrl_gpio_direction()")
>>> hides it. That problem is fixed with commit 3c5412cdec9f ("pinctrl-sunxi:
>>> sunxi_pinctrl_gpio_direction_in/output: use correct offset") in linux-next,
>>> and the traceback is seen there.
>>
>> Hm... These DT changes were tested with many users on older kernels for some
>> time now and new properties conform to bindings. Should we revert pinctrl
>> changes?
>>
> 
> I don't think those changes were tested with orangepi-pc on real hardware.
> Maybe I didn't explain it clearly enough: Commit 8df89a7cbc63 does _not_
> introduce the problem. It hides the problem. Reverting commit 8df89a7cbc63
> won't help but result in exactly the same backtrace (I tried).
> 
> Some more details: This commit introduces "wakeup-source;" to various
> orangepi-pc nodes. This triggers in a call to sunxi_pinctrl_irq_set_wake(),
> which did not happen before and which may result in the traceback.

I don't think there is any real issue here. The two irq_desc's being locked are
always different, so there is no deadlock. This recursive irq_set_irq_wake seems
to be a reasonably common pattern in GPIO drivers, and several of them contain
code to silence lockdep. I've sent a patch adding a copy of that to the sunxi
driver:

https://lore.kernel.org/lkml/20220216040037.22730-1-samuel@sholland.org/

Please see if it works for you.

Regards,
Samuel