From: Claudiu Beznea <[email protected]>
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 <[email protected]>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
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);
+ }
}
}
--
2.39.2
Hi Claudiu,
Thanks for your patch!
On Thu, Mar 7, 2024 at 12:25 PM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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 <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> 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
>
Hi Claudiu,
On Thu, Mar 14, 2024 at 3:11 PM claudiu beznea <[email protected]> wrote:
> On 14.03.2024 15:21, Geert Uytterhoeven wrote:
> > On Thu, Mar 7, 2024 at 12:25 PM Claudiu <[email protected]> wrote:
> >> From: Claudiu Beznea <[email protected]>
> >>
> >> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 14.03.2024 16:31, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Thu, Mar 14, 2024 at 3:11 PM claudiu beznea <[email protected]> wrote:
>> On 14.03.2024 15:21, Geert Uytterhoeven wrote:
>>> On Thu, Mar 7, 2024 at 12:25 PM Claudiu <[email protected]> wrote:
>>>> From: Claudiu Beznea <[email protected]>
>>>>
>>>> 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
>