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

Commit Message

Jernej Škrabec Nov. 29, 2021, 4:55 p.m. UTC

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 <[e-mail]>
Signed-off-by: Michael Klein <[e-mail]>
Signed-off-by: Jernej Skrabec <[e-mail]>
---
Changes from v1:
- added Co-developed-by tag

 arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 3 ++-
 arch/arm/boot/dts/sun8i-h3-nanopi.dtsi               | 1 +
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts            | 3 ++-
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts           | 3 ++-
 arch/arm/boot/dts/sunxi-bananapi-m2-plus.dtsi        | 3 ++-
 arch/arm/boot/dts/sunxi-libretech-all-h3-cc.dtsi     | 1 +
 6 files changed, 10 insertions(+), 4 deletions(-)

Comments

Chen-Yu Tsai Nov. 29, 2021, 5:16 p.m. UTC | #1

On Tue, Nov 30, 2021 at 12:55 AM Jernej Skrabec
<[e-mail]> 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.

It might have been that after shutdown there was really no way to
"power on" the board with these GPIO power keys, so we didn't use
KEY_POWER for them?

> Adjust them, so they can function as intended by designer.
>
> Co-developed-by: Michael Klein <[e-mail]>
> Signed-off-by: Michael Klein <[e-mail]>
> Signed-off-by: Jernej Skrabec <[e-mail]>

Acked-by: Chen-Yu Tsai <[e-mail]>

Jernej Škrabec Nov. 29, 2021, 5:29 p.m. UTC | #2

Dne ponedeljek, 29. november 2021 ob 18:16:31 CET je Chen-Yu Tsai napisal(a):
> On Tue, Nov 30, 2021 at 12:55 AM Jernej Skrabec
> <[e-mail]> 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.
> 
> It might have been that after shutdown there was really no way to
> "power on" the board with these GPIO power keys, so we didn't use
> KEY_POWER for them?

KEY_POWER is actually processed by userspace, so it would still make sense to 
power down board if power key is pressed. Correct me if I'm wrong, but in 
combination with wfi it could work before SCP FW was available?

> 
> > Adjust them, so they can function as intended by designer.
> >
> > Co-developed-by: Michael Klein <[e-mail]>
> > Signed-off-by: Michael Klein <[e-mail]>
> > Signed-off-by: Jernej Skrabec <[e-mail]>
> 
> Acked-by: Chen-Yu Tsai <[e-mail]>
> 

Thanks!

Best regards,
Jernej

Chen-Yu Tsai Nov. 29, 2021, 5:35 p.m. UTC | #3

On Tue, Nov 30, 2021 at 1:29 AM Jernej Škrabec <[e-mail]> wrote:
>
> Dne ponedeljek, 29. november 2021 ob 18:16:31 CET je Chen-Yu Tsai napisal(a):
> > On Tue, Nov 30, 2021 at 12:55 AM Jernej Skrabec
> > <[e-mail]> 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.
> >
> > It might have been that after shutdown there was really no way to
> > "power on" the board with these GPIO power keys, so we didn't use
> > KEY_POWER for them?
>
> KEY_POWER is actually processed by userspace, so it would still make sense to
> power down board if power key is pressed. Correct me if I'm wrong, but in
> combination with wfi it could work before SCP FW was available?

I guess it could? But where would it do wfi, and what would execute
after wfi?

IIRC the kernel's halt sequence doesn't really clean up interrupts,
so wfi isn't going to get very far.

> >
> > > Adjust them, so they can function as intended by designer.
> > >
> > > Co-developed-by: Michael Klein <[e-mail]>
> > > Signed-off-by: Michael Klein <[e-mail]>
> > > Signed-off-by: Jernej Skrabec <[e-mail]>
> >
> > Acked-by: Chen-Yu Tsai <[e-mail]>
> >
>
> Thanks!
>
> Best regards,
> Jernej
>
>

Maxime Ripard Dec. 1, 2021, 8:21 a.m. UTC | #4

On Mon, 29 Nov 2021 17:55:10 +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.
> 
> 
> [...]

Applied to sunxi/linux.git (sunxi/dt-for-5.17).

Thanks!
Maxime

Guenter Roeck Feb. 15, 2022, 12:27 a.m. UTC | #5

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 <[e-mail]>
> Signed-off-by: Michael Klein <[e-mail]>
> Signed-off-by: Jernej Skrabec <[e-mail]>

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.

Guenter

Jernej Škrabec Feb. 15, 2022, 8:34 p.m. UTC | #6

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 <[e-mail]>
> > Signed-off-by: Michael Klein <[e-mail]>
> > Signed-off-by: Jernej Skrabec <[e-mail]>
> 
> 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?

Best regards,
Jernej

Guenter Roeck Feb. 15, 2022, 10:20 p.m. UTC | #7

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 <[e-mail]>
>>> Signed-off-by: Michael Klein <[e-mail]>
>>> Signed-off-by: Jernej Skrabec <[e-mail]>
>>
>> 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.

Guenter

Samuel Holland Feb. 16, 2022, 4:11 a.m. UTC | #8

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 <[e-mail]>
>>>> Signed-off-by: Michael Klein <[e-mail]>
>>>> Signed-off-by: Jernej Skrabec <[e-mail]>
>>>
>>> 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/[e-mail]/

Please see if it works for you.

Regards,
Samuel

Guenter Roeck Feb. 16, 2022, 4:26 a.m. UTC | #9

On Tue, Feb 15, 2022 at 10:11:36PM -0600, Samuel Holland wrote:
[ ... ]
> 
> 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/[e-mail]/
> 
> Please see if it works for you.
> 
Yes, it does. The warning is gone with your patch applied on top of linux-next.

Thanks,
Guenter

Patch

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
index 8e8634ff2f9d..d5c7b7984d85 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -52,8 +52,9 @@  gpio_keys {
 
 		sw4 {
 			label = "power";
-			linux,code = <BTN_0>;
+			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+			wakeup-source;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/sun8i-h3-nanopi.dtsi b/arch/arm/boot/dts/sun8i-h3-nanopi.dtsi
index c7c3e7d8b3c8..fc45d5aaa67f 100644
--- a/arch/arm/boot/dts/sun8i-h3-nanopi.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3-nanopi.dtsi
@@ -81,6 +81,7 @@  k1 {
 			label = "k1";
 			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+			wakeup-source;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
index 597c425d08ec..9daffd90c12f 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
@@ -99,8 +99,9 @@  sw2 {
 
 		sw4 {
 			label = "sw4";
-			linux,code = <BTN_0>;
+			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+			wakeup-source;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index 5aff8ecc66cb..90f75fa85e68 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -91,8 +91,9 @@  r_gpio_keys {
 
 		sw4 {
 			label = "sw4";
-			linux,code = <BTN_0>;
+			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+			wakeup-source;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/sunxi-bananapi-m2-plus.dtsi b/arch/arm/boot/dts/sunxi-bananapi-m2-plus.dtsi
index 7a6af54dd342..d03f5853ef7b 100644
--- a/arch/arm/boot/dts/sunxi-bananapi-m2-plus.dtsi
+++ b/arch/arm/boot/dts/sunxi-bananapi-m2-plus.dtsi
@@ -82,8 +82,9 @@  gpio_keys {
 
 		sw4 {
 			label = "power";
-			linux,code = <BTN_0>;
+			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+			wakeup-source;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/sunxi-libretech-all-h3-cc.dtsi b/arch/arm/boot/dts/sunxi-libretech-all-h3-cc.dtsi
index c44fd726945a..9e14fe5fdcde 100644
--- a/arch/arm/boot/dts/sunxi-libretech-all-h3-cc.dtsi
+++ b/arch/arm/boot/dts/sunxi-libretech-all-h3-cc.dtsi
@@ -49,6 +49,7 @@  power {
 			label = "power";
 			linux,code = <KEY_POWER>;
 			gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
+			wakeup-source;
 		};
 	};