Re: [PATCH] pinctrl: sunxi: Use unique lockdep classes for IRQs

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On Tue, Feb 15, 2022 at 10:00:36PM -0600, Samuel Holland wrote:
> This driver, like several others, uses a chained IRQ for each GPIO bank,
> and forwards .irq_set_wake to the GPIO bank's upstream IRQ. As a result,
> a call to irq_set_irq_wake() needs to lock both the upstream and
> downstream irq_desc's. Lockdep considers this to be a possible deadlock
> when the irq_desc's share lockdep classes, which they do by default:
> 
>  ============================================
>  WARNING: possible recursive locking detected
>  5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
>  --------------------------------------------
>  init/307 is trying to acquire lock:
>  c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  but task is already holding lock:
>  c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  other info that might help us debug this:
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>    lock(&irq_desc_lock_class);
>    lock(&irq_desc_lock_class);
> 
>   *** DEADLOCK ***
> 
>   May be due to missing lock nesting notation
> 
>  4 locks held by init/307:
>   #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
>   #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
>   #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
>   #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  stack backtrace:
>  CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
>  Hardware name: Allwinner sun8i Family
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x90
>   dump_stack_lvl from __lock_acquire+0x1680/0x31a0
>   __lock_acquire from lock_acquire+0x148/0x3dc
>   lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
>   _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
>   __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
>   irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
>     [tail call from sunxi_pinctrl_irq_set_wake]
>   irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
>   gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
>   gpio_keys_shutdown from device_shutdown+0x180/0x224
>   device_shutdown from __do_sys_reboot+0x134/0x23c
>   __do_sys_reboot from ret_fast_syscall+0x0/0x1c
> 
> However, this can never deadlock because the upstream and downstream
> IRQs are never the same (nor do they even involve the same irqchip).
> 
> Silence this erroneous lockdep splat by applying what appears to be the
> usual fix of moving the GPIO IRQs to separate lockdep classes.
> 
> Fixes: a59c99d9eaf9 ("pinctrl: sunxi: Forward calls to irq_set_irq_wake")
> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>

Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>

Guenter

> ---
> 
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 80d6750c74a6..9e6ed1175db3 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -36,6 +36,13 @@
>  #include "../core.h"
>  #include "pinctrl-sunxi.h"
>  
> +/*
> + * These lock classes tell lockdep that GPIO IRQs are in a different
> + * category than their parents, so it won't report false recursion.
> + */
> +static struct lock_class_key sunxi_pinctrl_irq_lock_class;
> +static struct lock_class_key sunxi_pinctrl_irq_request_class;
> +
>  static struct irq_chip sunxi_pinctrl_edge_irq_chip;
>  static struct irq_chip sunxi_pinctrl_level_irq_chip;
>  
> @@ -1555,6 +1562,8 @@ int sunxi_pinctrl_init_with_variant(struct platform_device *pdev,
>  	for (i = 0; i < (pctl->desc->irq_banks * IRQ_PER_BANK); i++) {
>  		int irqno = irq_create_mapping(pctl->domain, i);
>  
> +		irq_set_lockdep_class(irqno, &sunxi_pinctrl_irq_lock_class,
> +				      &sunxi_pinctrl_irq_request_class);
>  		irq_set_chip_and_handler(irqno, &sunxi_pinctrl_edge_irq_chip,
>  					 handle_edge_irq);
>  		irq_set_chip_data(irqno, pctl);
> -- 
> 2.33.1
>