* [PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver
@ 2024-03-20 10:42 Claudiu
2024-03-20 10:42 ` [PATCH v3 1/2] pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration Claudiu
2024-03-20 10:42 ` [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume Claudiu
0 siblings, 2 replies; 8+ messages in thread
From: Claudiu @ 2024-03-20 10:42 UTC (permalink / raw)
To: geert+renesas, linus.walleij, tglx, biju.das.jz
Cc: linux-renesas-soc, linux-gpio, linux-kernel, claudiu.beznea,
Claudiu Beznea
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v3 1/2] pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration 2024-03-20 10:42 [PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver Claudiu @ 2024-03-20 10:42 ` Claudiu 2024-03-20 10:42 ` [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume Claudiu 1 sibling, 0 replies; 8+ messages in thread From: Claudiu @ 2024-03-20 10:42 UTC (permalink / raw) To: geert+renesas, linus.walleij, tglx, biju.das.jz Cc: linux-renesas-soc, linux-gpio, linux-kernel, claudiu.beznea, Claudiu Beznea 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") Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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(-) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-03-20 10:42 [PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver Claudiu 2024-03-20 10:42 ` [PATCH v3 1/2] pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration Claudiu @ 2024-03-20 10:42 ` Claudiu 2024-03-20 11:08 ` Biju Das 2024-04-18 14:07 ` Geert Uytterhoeven 1 sibling, 2 replies; 8+ messages in thread From: Claudiu @ 2024-03-20 10:42 UTC (permalink / raw) To: geert+renesas, linus.walleij, tglx, biju.das.jz Cc: linux-renesas-soc, linux-gpio, linux-kernel, claudiu.beznea, Claudiu Beznea From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> 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 <claudiu.beznea.uj@bp.renesas.com> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-03-20 10:42 ` [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume Claudiu @ 2024-03-20 11:08 ` Biju Das 2024-03-20 11:28 ` claudiu beznea 2024-04-18 14:07 ` Geert Uytterhoeven 1 sibling, 1 reply; 8+ messages in thread From: Biju Das @ 2024-03-20 11:08 UTC (permalink / raw) To: Claudiu.Beznea, geert+renesas, linus.walleij, tglx Cc: linux-renesas-soc, linux-gpio, linux-kernel, Claudiu.Beznea, Claudiu Beznea Hi Claudiu, > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > 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 <claudiu.beznea.uj@bp.renesas.com> > > 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 <claudiu.beznea.uj@bp.renesas.com> > --- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-03-20 11:08 ` Biju Das @ 2024-03-20 11:28 ` claudiu beznea 2024-03-20 11:39 ` Biju Das 0 siblings, 1 reply; 8+ messages in thread From: claudiu beznea @ 2024-03-20 11:28 UTC (permalink / raw) To: Biju Das, geert+renesas, linus.walleij, tglx Cc: linux-renesas-soc, linux-gpio, linux-kernel, Claudiu Beznea On 20.03.2024 13:08, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: Claudiu <claudiu.beznea@tuxon.dev> >> 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 <claudiu.beznea.uj@bp.renesas.com> >> >> 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 <claudiu.beznea.uj@bp.renesas.com> >> --- >> 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-03-20 11:28 ` claudiu beznea @ 2024-03-20 11:39 ` Biju Das 0 siblings, 0 replies; 8+ messages in thread From: Biju Das @ 2024-03-20 11:39 UTC (permalink / raw) To: Claudiu.Beznea, geert+renesas, linus.walleij, tglx Cc: linux-renesas-soc, linux-gpio, linux-kernel, Claudiu Beznea Hi Claudiu, > > On 20.03.2024 13:08, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: Claudiu <claudiu.beznea@tuxon.dev> > >> 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 <claudiu.beznea.uj@bp.renesas.com> > >> > >> 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 <claudiu.beznea.uj@bp.renesas.com> > >> --- > >> 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-03-20 10:42 ` [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume Claudiu 2024-03-20 11:08 ` Biju Das @ 2024-04-18 14:07 ` Geert Uytterhoeven 2024-04-19 5:48 ` claudiu beznea 1 sibling, 1 reply; 8+ messages in thread From: Geert Uytterhoeven @ 2024-04-18 14:07 UTC (permalink / raw) To: Claudiu Cc: linus.walleij, tglx, biju.das.jz, linux-renesas-soc, linux-gpio, linux-kernel, Claudiu Beznea Hi Claudiu, On Wed, Mar 20, 2024 at 11:43 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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 <claudiu.beznea.uj@bp.renesas.com> 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-m68k.org 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume 2024-04-18 14:07 ` Geert Uytterhoeven @ 2024-04-19 5:48 ` claudiu beznea 0 siblings, 0 replies; 8+ messages in thread From: claudiu beznea @ 2024-04-19 5:48 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linus.walleij, tglx, biju.das.jz, linux-renesas-soc, linux-gpio, linux-kernel, Claudiu Beznea On 18.04.2024 17:07, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Wed, Mar 20, 2024 at 11:43 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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 <claudiu.beznea.uj@bp.renesas.com> > > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-19 5:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-20 10:42 [PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver Claudiu 2024-03-20 10:42 ` [PATCH v3 1/2] pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration Claudiu 2024-03-20 10:42 ` [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume Claudiu 2024-03-20 11:08 ` Biju Das 2024-03-20 11:28 ` claudiu beznea 2024-03-20 11:39 ` Biju Das 2024-04-18 14:07 ` Geert Uytterhoeven 2024-04-19 5:48 ` claudiu beznea
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).