diff mbox series

pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration

Message ID 20240307112452.74220-1-claudiu.beznea.uj@bp.renesas.com
State Superseded
Headers show
Series pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration | expand

Commit Message

Claudiu March 7, 2024, 11:24 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Lockdep detects a possible deadlock as listed below. This is because it
detects the IA55 interrupt controller .irq_eoi() API is called from
interrupt context while configuration-specific API (e.g., .irq_enable())
could be called from process context on resume path (by calling
rzg2l_gpio_irq_restore()). To avoid this, protect the call of
rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore().
With this the same approach that is available in __setup_irq() is mimicked
to pinctrl IRQ resume function.

Below is the lockdep report:

 WARNING: inconsistent lock state
 6.8.0-rc5-next-20240219-arm64-renesas-00030-gb17a289abf1f #90 Not tainted
 --------------------------------
 inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
 str_rwdt_t_001./159 [HC0[0]:SC0[0]:HE1:SE1] takes:
 ffff00000b001d70 (&rzg2l_irqc_data->lock){?...}-{2:2}, at: rzg2l_irqc_irq_enable+0x60/0xa4
 {IN-HARDIRQ-W} state was registered at:
 lock_acquire+0x1e0/0x310
 _raw_spin_lock+0x44/0x58
 rzg2l_irqc_eoi+0x2c/0x130
 irq_chip_eoi_parent+0x18/0x20
 rzg2l_gpio_irqc_eoi+0xc/0x14
 handle_fasteoi_irq+0x134/0x230
 generic_handle_domain_irq+0x28/0x3c
 gic_handle_irq+0x4c/0xbc
 call_on_irq_stack+0x24/0x34
 do_interrupt_handler+0x78/0x7c
 el1_interrupt+0x30/0x5c
 el1h_64_irq_handler+0x14/0x1c
 el1h_64_irq+0x64/0x68
 _raw_spin_unlock_irqrestore+0x34/0x70
 __setup_irq+0x4d4/0x6b8
 request_threaded_irq+0xe8/0x1a0
 request_any_context_irq+0x60/0xb8
 devm_request_any_context_irq+0x74/0x104
 gpio_keys_probe+0x374/0xb08
 platform_probe+0x64/0xcc
 really_probe+0x140/0x2ac
 __driver_probe_device+0x74/0x124
 driver_probe_device+0x3c/0x15c
 __driver_attach+0xec/0x1c4
 bus_for_each_dev+0x70/0xcc
 driver_attach+0x20/0x28
 bus_add_driver+0xdc/0x1d0
 driver_register+0x5c/0x118
 __platform_driver_register+0x24/0x2c
 gpio_keys_init+0x18/0x20
 do_one_initcall+0x70/0x290
 kernel_init_freeable+0x294/0x504
 kernel_init+0x20/0x1cc
 ret_from_fork+0x10/0x20
 irq event stamp: 69071
 hardirqs last enabled at (69071): [<ffff800080e0dafc>] _raw_spin_unlock_irqrestore+0x6c/0x70
 hardirqs last disabled at (69070): [<ffff800080e0cfec>] _raw_spin_lock_irqsave+0x7c/0x80
 softirqs last enabled at (67654): [<ffff800080010614>] __do_softirq+0x494/0x4dc
 softirqs last disabled at (67645): [<ffff800080015238>] ____do_softirq+0xc/0x14

 other info that might help us debug this:
 Possible unsafe locking scenario:

 CPU0
 ----
 lock(&rzg2l_irqc_data->lock);
 <Interrupt>
 lock(&rzg2l_irqc_data->lock);

 *** DEADLOCK ***

 4 locks held by str_rwdt_t_001./159:
 #0: ffff00000b10f3f0 (sb_writers#4){.+.+}-{0:0}, at: vfs_write+0x1a4/0x35c
 #1: ffff00000e43ba88 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0xe8/0x1a8
 #2: ffff00000aa21dc8 (kn->active#40){.+.+}-{0:0}, at: kernfs_fop_write_iter+0xf0/0x1a8
 #3: ffff80008179d970 (system_transition_mutex){+.+.}-{3:3}, at: pm_suspend+0x9c/0x278

 stack backtrace:
 CPU: 0 PID: 159 Comm: str_rwdt_t_001. Not tainted 6.8.0-rc5-next-20240219-arm64-renesas-00030-gb17a289abf1f #90
 Hardware name: Renesas SMARC EVK version 2 based on r9a08g045s33 (DT)
 Call trace:
 dump_backtrace+0x94/0xe8
 show_stack+0x14/0x1c
 dump_stack_lvl+0x88/0xc4
 dump_stack+0x14/0x1c
 print_usage_bug.part.0+0x294/0x348
 mark_lock+0x6b0/0x948
 __lock_acquire+0x750/0x20b0
 lock_acquire+0x1e0/0x310
 _raw_spin_lock+0x44/0x58
 rzg2l_irqc_irq_enable+0x60/0xa4
 irq_chip_enable_parent+0x1c/0x34
 rzg2l_gpio_irq_enable+0xc4/0xd8
 rzg2l_pinctrl_resume_noirq+0x4cc/0x520
 pm_generic_resume_noirq+0x28/0x3c
 genpd_finish_resume+0xc0/0xdc
 genpd_resume_noirq+0x14/0x1c
 dpm_run_callback+0x34/0x90
 device_resume_noirq+0xa8/0x268
 dpm_noirq_resume_devices+0x13c/0x160
 dpm_resume_noirq+0xc/0x1c
 suspend_devices_and_enter+0x2c8/0x570
 pm_suspend+0x1ac/0x278
 state_store+0x88/0x124
 kobj_attr_store+0x14/0x24
 sysfs_kf_write+0x48/0x6c
 kernfs_fop_write_iter+0x118/0x1a8
 vfs_write+0x270/0x35c
 ksys_write+0x64/0xec
 __arm64_sys_write+0x18/0x20
 invoke_syscall+0x44/0x108
 el0_svc_common.constprop.0+0xb4/0xd4
 do_el0_svc+0x18/0x20
 el0_svc+0x3c/0xb8
 el0t_64_sync_handler+0xb8/0xbc
 el0t_64_sync+0x14c/0x150

Fixes: 254203f9a94c ("pinctrl: renesas: rzg2l: Add suspend/resume support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven March 14, 2024, 1:21 p.m. UTC | #1 
Hi Claudiu,

Thanks for your patch!

On Thu, Mar 7, 2024 at 12:25 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Lockdep detects a possible deadlock as listed below. This is because it
> detects the IA55 interrupt controller .irq_eoi() API is called from
> interrupt context while configuration-specific API (e.g., .irq_enable())
> could be called from process context on resume path (by calling
> rzg2l_gpio_irq_restore()). To avoid this, protect the call of
> rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore().
> With this the same approach that is available in __setup_irq() is mimicked
> to pinctrl IRQ resume function.

You mean __setup_irq() in kernel/irq/manage.c?
That one uses the raw spinlock methods?

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2063,8 +2063,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>                         continue;
>                 }
>
> -               if (!irqd_irq_disabled(data))
> +               if (!irqd_irq_disabled(data)) {
> +                       unsigned long flags;
> +
> +                       /*
> +                        * This has to be atomically executed to protect against a concurrent
> +                        * interrupt.
> +                        */
> +                       spin_lock_irqsave(&pctrl->lock, flags);
>                         rzg2l_gpio_irq_enable(data);
> +                       spin_unlock_irqrestore(&pctrl->lock, flags);
> +               }
>         }
>  }

Gr{oetje,eeting}s,

                        Geert
Claudiu March 14, 2024, 2:11 p.m. UTC | #2 
Hi, Geert,

On 14.03.2024 15:21, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch!
> 
> On Thu, Mar 7, 2024 at 12:25 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Lockdep detects a possible deadlock as listed below. This is because it
>> detects the IA55 interrupt controller .irq_eoi() API is called from
>> interrupt context while configuration-specific API (e.g., .irq_enable())
>> could be called from process context on resume path (by calling
>> rzg2l_gpio_irq_restore()). To avoid this, protect the call of
>> rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore().
>> With this the same approach that is available in __setup_irq() is mimicked
>> to pinctrl IRQ resume function.
> 
> You mean __setup_irq() in kernel/irq/manage.c?

Yes!

> That one uses the raw spinlock methods?

Yes! Would you prefer to have raw spinlock here, too?

Thank you,
Claudiu Beznea

> 
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -2063,8 +2063,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>>                         continue;
>>                 }
>>
>> -               if (!irqd_irq_disabled(data))
>> +               if (!irqd_irq_disabled(data)) {
>> +                       unsigned long flags;
>> +
>> +                       /*
>> +                        * This has to be atomically executed to protect against a concurrent
>> +                        * interrupt.
>> +                        */
>> +                       spin_lock_irqsave(&pctrl->lock, flags);
>>                         rzg2l_gpio_irq_enable(data);
>> +                       spin_unlock_irqrestore(&pctrl->lock, flags);
>> +               }
>>         }
>>  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Geert Uytterhoeven March 14, 2024, 2:31 p.m. UTC | #3 
Hi Claudiu,

On Thu, Mar 14, 2024 at 3:11 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
> On 14.03.2024 15:21, Geert Uytterhoeven wrote:
> > On Thu, Mar 7, 2024 at 12:25 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Lockdep detects a possible deadlock as listed below. This is because it
> >> detects the IA55 interrupt controller .irq_eoi() API is called from
> >> interrupt context while configuration-specific API (e.g., .irq_enable())
> >> could be called from process context on resume path (by calling
> >> rzg2l_gpio_irq_restore()). To avoid this, protect the call of
> >> rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore().
> >> With this the same approach that is available in __setup_irq() is mimicked
> >> to pinctrl IRQ resume function.
> >
> > You mean __setup_irq() in kernel/irq/manage.c?
>
> Yes!
>
> > That one uses the raw spinlock methods?
>
> Yes! Would you prefer to have raw spinlock here, too?

Most pin control driver needing protection in an irq_enable
method use raw spinlock, so I think it makes sense to follow that.

Gr{oetje,eeting}s,

                        Geert
Claudiu March 14, 2024, 2:35 p.m. UTC | #4 
On 14.03.2024 16:31, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Mar 14, 2024 at 3:11 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 14.03.2024 15:21, Geert Uytterhoeven wrote:
>>> On Thu, Mar 7, 2024 at 12:25 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Lockdep detects a possible deadlock as listed below. This is because it
>>>> detects the IA55 interrupt controller .irq_eoi() API is called from
>>>> interrupt context while configuration-specific API (e.g., .irq_enable())
>>>> could be called from process context on resume path (by calling
>>>> rzg2l_gpio_irq_restore()). To avoid this, protect the call of
>>>> rzg2l_gpio_irq_enable() with spin_lock_irqsave()/spin_unlock_irqrestore().
>>>> With this the same approach that is available in __setup_irq() is mimicked
>>>> to pinctrl IRQ resume function.
>>>
>>> You mean __setup_irq() in kernel/irq/manage.c?
>>
>> Yes!
>>
>>> That one uses the raw spinlock methods?
>>
>> Yes! Would you prefer to have raw spinlock here, too?
> 
> Most pin control driver needing protection in an irq_enable
> method use raw spinlock, so I think it makes sense to follow that.

Ok, I'll  update it, thanks!

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index eb5a8c654260..76124b860c3f 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -2063,8 +2063,17 @@  static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
 			continue;
 		}
 
-		if (!irqd_irq_disabled(data))
+		if (!irqd_irq_disabled(data)) {
+			unsigned long flags;
+
+			/*
+			 * This has to be atomically executed to protect against a concurrent
+			 * interrupt.
+			 */
+			spin_lock_irqsave(&pctrl->lock, flags);
 			rzg2l_gpio_irq_enable(data);
+			spin_unlock_irqrestore(&pctrl->lock, flags);
+		}
 	}
 }