From: Claudiu Beznea <[email protected]>
Hi,
Series adds 2 fixes for pinctrl driver.
Thank you,
Claudiu Beznea
Changes in v3:
- collected tag on patch 1/2
- added patch 2/2 in this series
Claudiu Beznea (2):
pinctrl: renesas: rzg2l: Execute atomically the interrupt
configuration
pinctrl: renesas: rzg2l: Configure the interrupt type on resume
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
--
2.39.2
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")
Reviewed-by: Geert Uytterhoeven <[email protected]>
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..93916553bcc7 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.
+ */
+ raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
rzg2l_gpio_irq_enable(data);
+ raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
+ }
}
}
--
2.39.2
From: Claudiu Beznea <[email protected]>
Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
source at the same time") removed the setup of TINT from
rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the setup
of TINT has been moved in rzg2l_tint_set_edge() though
rzg2l_disable_tint_and_set_tint_source(). With this, the interrupts are
not properly re-configured after a suspend-to-RAM cycle. To address
this issue and avoid spurious interrupts while resumming set the
interrupt type before enabling it.
Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 93916553bcc7..4fee3b0e6c5e 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
struct irq_data *data;
+ unsigned long flags;
unsigned int virq;
+ int ret;
if (!pctrl->hwirq[i])
continue;
@@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
continue;
}
- if (!irqd_irq_disabled(data)) {
- unsigned long flags;
-
- /*
- * This has to be atomically executed to protect against a concurrent
- * interrupt.
- */
- raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
+ /*
+ * This has to be atomically executed to protect against a concurrent
+ * interrupt.
+ */
+ raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
+ ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
+ if (ret)
+ dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
+ else if (!irqd_irq_disabled(data))
rzg2l_gpio_irq_enable(data);
- raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
- }
+ raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
}
}
--
2.39.2
Hi Claudiu,
> -----Original Message-----
> From: Claudiu <[email protected]>
> Sent: Wednesday, March 20, 2024 10:43 AM
> Subject: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume
>
> From: Claudiu Beznea <[email protected]>
>
> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time") removed
> the setup of TINT from rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the setup of
> TINT has been moved in rzg2l_tint_set_edge() though rzg2l_disable_tint_and_set_tint_source(). With
> this, the interrupts are not properly re-configured after a suspend-to-RAM cycle. To address this issue
> and avoid spurious interrupts while resumming set the interrupt type before enabling it.
Just a question,
Previously you don't save/restore irq_type() register during suspend/resume()??
After STR, any way we will lose that information.
Cheers,
Biju
>
> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
> Signed-off-by: Claudiu Beznea <[email protected]>
> ---
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 93916553bcc7..4fee3b0e6c5e 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>
> for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> struct irq_data *data;
> + unsigned long flags;
> unsigned int virq;
> + int ret;
>
> if (!pctrl->hwirq[i])
> continue;
> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
> continue;
> }
>
> - if (!irqd_irq_disabled(data)) {
> - unsigned long flags;
> -
> - /*
> - * This has to be atomically executed to protect against a concurrent
> - * interrupt.
> - */
> - raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> + /*
> + * This has to be atomically executed to protect against a concurrent
> + * interrupt.
> + */
> + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> + ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
> + if (ret)
> + dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
> + else if (!irqd_irq_disabled(data))
> rzg2l_gpio_irq_enable(data);
> - raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> - }
> + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> }
> }
>
> --
> 2.39.2
On 20.03.2024 13:08, Biju Das wrote:
> Hi Claudiu,
>
>> -----Original Message-----
>> From: Claudiu <[email protected]>
>> Sent: Wednesday, March 20, 2024 10:43 AM
>> Subject: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume
>>
>> From: Claudiu Beznea <[email protected]>
>>
>> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time") removed
>> the setup of TINT from rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the setup of
>> TINT has been moved in rzg2l_tint_set_edge() though rzg2l_disable_tint_and_set_tint_source(). With
>> this, the interrupts are not properly re-configured after a suspend-to-RAM cycle. To address this issue
>> and avoid spurious interrupts while resumming set the interrupt type before enabling it.
>
> Just a question,
>
> Previously you don't save/restore irq_type() register during suspend/resume()??
> After STR, any way we will lose that information.
Part of IA55 registers are saved/restored in IA55 suspend/resume functions.
The rest of configuration (enable and TINT) was done though pinctrl driver
because IA55 is resumed before pinctrl driver and if we enable the
interrupt at that point the pin may be in unwanted state and IA55 may
report invalid interrupts.
As TINT was removed from enable we need to handle it now.
>
> Cheers,
> Biju
>
>
>
>>
>> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
>> Signed-off-by: Claudiu Beznea <[email protected]>
>> ---
>> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> index 93916553bcc7..4fee3b0e6c5e 100644
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>>
>> for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
>> struct irq_data *data;
>> + unsigned long flags;
>> unsigned int virq;
>> + int ret;
>>
>> if (!pctrl->hwirq[i])
>> continue;
>> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>> continue;
>> }
>>
>> - if (!irqd_irq_disabled(data)) {
>> - unsigned long flags;
>> -
>> - /*
>> - * This has to be atomically executed to protect against a concurrent
>> - * interrupt.
>> - */
>> - raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
>> + /*
>> + * This has to be atomically executed to protect against a concurrent
>> + * interrupt.
>> + */
>> + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
>> + ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>> + if (ret)
>> + dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
>> + else if (!irqd_irq_disabled(data))
>> rzg2l_gpio_irq_enable(data);
>> - raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
>> - }
>> + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
>> }
>> }
>>
>> --
>> 2.39.2
>
Hi Claudiu,
>
> On 20.03.2024 13:08, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <[email protected]>
> >> Sent: Wednesday, March 20, 2024 10:43 AM
> >> Subject: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the
> >> interrupt type on resume
> >>
> >> From: Claudiu Beznea <[email protected]>
> >>
> >> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
> >> source at the same time") removed the setup of TINT from
> >> rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the
> >> setup of TINT has been moved in rzg2l_tint_set_edge() though
> >> rzg2l_disable_tint_and_set_tint_source(). With this, the interrupts are not properly re-configured
> after a suspend-to-RAM cycle. To address this issue and avoid spurious interrupts while resumming set
> the interrupt type before enabling it.
> >
> > Just a question,
> >
> > Previously you don't save/restore irq_type() register during suspend/resume()??
> > After STR, any way we will lose that information.
>
> Part of IA55 registers are saved/restored in IA55 suspend/resume functions.
>
> The rest of configuration (enable and TINT) was done though pinctrl driver because IA55 is resumed
> before pinctrl driver and if we enable the interrupt at that point the pin may be in unwanted state and
> IA55 may report invalid interrupts.
>
> As TINT was removed from enable we need to handle it now.
OK got it, So previously you are not storing/restoring TINT source along with interrupt type()in IA55.
But depend upon the pinctrl() which just call enable() without irq_set_type().
Now the issue is fixed by calling set_type() + enable().
Cheers,
Biju
> >
> >
> >>
> >> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
> >> source at the same time")
> >> Signed-off-by: Claudiu Beznea <[email protected]>
> >> ---
> >> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
> >> 1 file changed, 12 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> index 93916553bcc7..4fee3b0e6c5e 100644
> >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct
> >> rzg2l_pinctrl *pctrl)
> >>
> >> for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> >> struct irq_data *data;
> >> + unsigned long flags;
> >> unsigned int virq;
> >> + int ret;
> >>
> >> if (!pctrl->hwirq[i])
> >> continue;
> >> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
> >> continue;
> >> }
> >>
> >> - if (!irqd_irq_disabled(data)) {
> >> - unsigned long flags;
> >> -
> >> - /*
> >> - * This has to be atomically executed to protect against a concurrent
> >> - * interrupt.
> >> - */
> >> - raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> >> + /*
> >> + * This has to be atomically executed to protect against a concurrent
> >> + * interrupt.
> >> + */
> >> + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> >> + ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
> >> + if (ret)
> >> + dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
> >> + else if (!irqd_irq_disabled(data))
> >> rzg2l_gpio_irq_enable(data);
> >> - raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> >> - }
> >> + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> >> }
> >> }
> >>
> >> --
> >> 2.39.2
> >
Hi Claudiu,
On Wed, Mar 20, 2024 at 11:43 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
> source at the same time") removed the setup of TINT from
> rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the setup
> of TINT has been moved in rzg2l_tint_set_edge() though
> rzg2l_disable_tint_and_set_tint_source(). With this, the interrupts are
> not properly re-configured after a suspend-to-RAM cycle. To address
> this issue and avoid spurious interrupts while resumming set the
> interrupt type before enabling it.
>
> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>
> for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> struct irq_data *data;
> + unsigned long flags;
> unsigned int virq;
> + int ret;
>
> if (!pctrl->hwirq[i])
> continue;
> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
> continue;
> }
>
> - if (!irqd_irq_disabled(data)) {
> - unsigned long flags;
> -
> - /*
> - * This has to be atomically executed to protect against a concurrent
> - * interrupt.
> - */
> - raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> + /*
> + * This has to be atomically executed to protect against a concurrent
> + * interrupt.
> + */
> + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> + ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
> + if (ret)
> + dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
> + else if (!irqd_irq_disabled(data))
> rzg2l_gpio_irq_enable(data);
> - raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> - }
> + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> }
> }
LGTM, but I'd rather move the dev_crit() outside (i.e. after) the
critical section.
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 18.04.2024 17:07, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Wed, Mar 20, 2024 at 11:43 AM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
>> source at the same time") removed the setup of TINT from
>> rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the setup
>> of TINT has been moved in rzg2l_tint_set_edge() though
>> rzg2l_disable_tint_and_set_tint_source(). With this, the interrupts are
>> not properly re-configured after a suspend-to-RAM cycle. To address
>> this issue and avoid spurious interrupts while resumming set the
>> interrupt type before enabling it.
>>
>> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
> Thanks for your patch!
>
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>>
>> for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
>> struct irq_data *data;
>> + unsigned long flags;
>> unsigned int virq;
>> + int ret;
>>
>> if (!pctrl->hwirq[i])
>> continue;
>> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>> continue;
>> }
>>
>> - if (!irqd_irq_disabled(data)) {
>> - unsigned long flags;
>> -
>> - /*
>> - * This has to be atomically executed to protect against a concurrent
>> - * interrupt.
>> - */
>> - raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
>> + /*
>> + * This has to be atomically executed to protect against a concurrent
>> + * interrupt.
>> + */
>> + raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
>> + ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>> + if (ret)
>> + dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
>> + else if (!irqd_irq_disabled(data))
>> rzg2l_gpio_irq_enable(data);
>> - raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
>> - }
>> + raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
>> }
>> }
>
> LGTM, but I'd rather move the dev_crit() outside (i.e. after) the
> critical section.
I was in balance about having it as proposed. I'll send an update to move
it outside.
Thank you,
Claudiu Beznea
>
> Gr{oetje,eeting}s,
>
> Geert
>