[PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver

[PATCH v3 0/2] pinctrl: renesas: rzg2l: fixes for pinctrl driver

[Claudiu]

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

[PATCH v3 1/2] pinctrl: renesas: rzg2l: Execute atomically the interrupt configuration

[Claudiu]

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

[PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[Claudiu]

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

RE: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[Biju Das]

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

Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[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
> 

RE: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[Biju Das]

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
> >

Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[Geert Uytterhoeven]

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

Re: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[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
>