drivers/pinctrl/renesas/pinctrl-rzg2l.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
spurious IRQs.
The resume code used rzg2l_gpio_irq_enable() (called from
rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
interrupts.
After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
resume.
Fix this by adding rzg2l_gpio_irq_endisable() back into
rzg2l_gpio_irq_enable(), and by using its unlocked variant in
rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()
should be safe with respect to spurious IRQs, as in the probe case IRQs are
enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
were detected on suspend/resume tests (executed on RZ/G3S).
Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index b182b3b8a542..6ae1ee3ffc81 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
}
static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
- unsigned int hwirq, bool enable)
+ unsigned int hwirq, bool enable, bool lock)
{
const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
u64 *pin_data = pin_desc->drv_data;
@@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
addr += 4;
}
- spin_lock_irqsave(&pctrl->lock, flags);
+ if (lock)
+ spin_lock_irqsave(&pctrl->lock, flags);
+
if (enable)
writel(readl(addr) | BIT(bit * 8), addr);
else
writel(readl(addr) & ~BIT(bit * 8), addr);
- spin_unlock_irqrestore(&pctrl->lock, flags);
+
+ if (lock)
+ spin_unlock_irqrestore(&pctrl->lock, flags);
}
static void rzg2l_gpio_irq_disable(struct irq_data *d)
@@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
gpiochip_disable_irq(gc, hwirq);
}
-static void rzg2l_gpio_irq_enable(struct irq_data *d)
+static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
unsigned int hwirq = irqd_to_hwirq(d);
gpiochip_enable_irq(gc, hwirq);
+ rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
irq_chip_enable_parent(d);
}
+static void rzg2l_gpio_irq_enable(struct irq_data *d)
+{
+ rzg2l_gpio_irq_enable_helper(d, true);
+}
+
static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
{
return irq_chip_set_type_parent(d, type);
@@ -2570,7 +2581,7 @@ static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
goto err;
}
- rzg2l_gpio_irq_endisable(pctrl, child, true);
+ rzg2l_gpio_irq_endisable(pctrl, child, true, true);
pctrl->hwirq[irq] = child;
irq += pctrl->data->hwcfg->tint_start_index;
@@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
spin_lock_irqsave(&pctrl->lock, flags);
ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
if (!ret && !irqd_irq_disabled(data))
- rzg2l_gpio_irq_enable(data);
+ rzg2l_gpio_irq_enable_helper(data, false);
spin_unlock_irqrestore(&pctrl->lock, flags);
if (ret)
@@ -2640,7 +2651,7 @@ static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int v
for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
if (pctrl->hwirq[i] == hwirq) {
- rzg2l_gpio_irq_endisable(pctrl, hwirq, false);
+ rzg2l_gpio_irq_endisable(pctrl, hwirq, false, true);
rzg2l_gpio_free(gc, hwirq);
spin_lock_irqsave(&pctrl->bitmap_lock, flags);
bitmap_release_region(pctrl->tint_slot, i, get_order(1));
--
2.43.0Hi Claudiu,
On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
> spurious IRQs.
>
> The resume code used rzg2l_gpio_irq_enable() (called from
> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
> drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
> eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
> interrupts.
>
> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
> in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
> resume.
>
> Fix this by adding rzg2l_gpio_irq_endisable() back into
> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
> rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()
enabled
> should be safe with respect to spurious IRQs, as in the probe case IRQs are
> enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
> were detected on suspend/resume tests (executed on RZ/G3S).
>
> Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Thanks for your patch!
I have to admit I don't fully understand what is going on...
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
> }
>
> static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> - unsigned int hwirq, bool enable)
> + unsigned int hwirq, bool enable, bool lock)
> {
> const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
> u64 *pin_data = pin_desc->drv_data;
> @@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> addr += 4;
> }
>
> - spin_lock_irqsave(&pctrl->lock, flags);
> + if (lock)
> + spin_lock_irqsave(&pctrl->lock, flags);
> +
> if (enable)
> writel(readl(addr) | BIT(bit * 8), addr);
> else
> writel(readl(addr) & ~BIT(bit * 8), addr);
> - spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + if (lock)
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> }
I am not so fond of these "if (lock) ..."-constructs, especially as
the function now takes two bool parameters, which is error-prone.
What about renaming rzg2l_gpio_irq_endisable() to
__rzg2l_gpio_irq_endisable(), and moving the locking to a wrapper
rzg2l_gpio_irq_endisable()?
static void __rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
unsigned int hwirq, bool enable)
{
/* old functionality without locking */
...
}
static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
unsigned int hwirq, bool enable)
{
unsigned long flags;
spin_lock_irqsave(&pctrl->lock, flags);
__rzg2l_gpio_irq_endisable(pctrl, hwirq, enable);
spin_unlock_irqrestore(&pctrl->lock, flags);
}
Then no existing callers of rzg2l_gpio_irq_endisable() need to be
changed.
> @@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
> gpiochip_disable_irq(gc, hwirq);
> }
>
> -static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
Here we can't do without the "lock" parameter, unless duplicating the
full body, so this is fine. I'd rename it to __rzg2l_gpio_irq_enable(),
though.
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> unsigned int hwirq = irqd_to_hwirq(d);
>
> gpiochip_enable_irq(gc, hwirq);
> + rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
if (lock)
rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
else
__rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
> irq_chip_enable_parent(d);
> }
>
> +static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +{
> + rzg2l_gpio_irq_enable_helper(d, true);
__rzg2l_gpio_irq_enable(d, true);
> +}
> +
> static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> {
> return irq_chip_set_type_parent(d, type);
> @@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
> spin_lock_irqsave(&pctrl->lock, flags);
> ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
> if (!ret && !irqd_irq_disabled(data))
> - rzg2l_gpio_irq_enable(data);
> + rzg2l_gpio_irq_enable_helper(data, false);
__rzg2l_gpio_irq_enable(data, false);
Before, the lock was taken again, while it was already held.
Didn't this cause a deadlock?
> spin_unlock_irqrestore(&pctrl->lock, flags);
>
> if (ret)
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
Hi, Geert,
On 9/11/25 12:53, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
>> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
>> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
>> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
>> spurious IRQs.
>>
>> The resume code used rzg2l_gpio_irq_enable() (called from
>> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
>> drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
>> eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
>> interrupts.
>>
>> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
>> in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
>> resume.
>>
>> Fix this by adding rzg2l_gpio_irq_endisable() back into
>> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
>> rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()
>
> enabled
>
>> should be safe with respect to spurious IRQs, as in the probe case IRQs are
>> enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
>> were detected on suspend/resume tests (executed on RZ/G3S).
>>
>> Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Thanks for your patch!
>
> I have to admit I don't fully understand what is going on...
Sorry about that. Basically, ISEL is not properly configured as a result of
removing rzg2l_gpio_irq_endisable() from rzg2l_gpio_irq_enable() which was
previously called on interrupt reconfiguration path.
>
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
>> }
>>
>> static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>> - unsigned int hwirq, bool enable)
>> + unsigned int hwirq, bool enable, bool lock)
>> {
>> const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
>> u64 *pin_data = pin_desc->drv_data;
>> @@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>> addr += 4;
>> }
>>
>> - spin_lock_irqsave(&pctrl->lock, flags);
>> + if (lock)
>> + spin_lock_irqsave(&pctrl->lock, flags);
>> +
>> if (enable)
>> writel(readl(addr) | BIT(bit * 8), addr);
>> else
>> writel(readl(addr) & ~BIT(bit * 8), addr);
>> - spin_unlock_irqrestore(&pctrl->lock, flags);
>> +
>> + if (lock)
>> + spin_unlock_irqrestore(&pctrl->lock, flags);
>> }
>
> I am not so fond of these "if (lock) ..."-constructs, especially as
> the function now takes two bool parameters, which is error-prone.
>
> What about renaming rzg2l_gpio_irq_endisable() to
> __rzg2l_gpio_irq_endisable(), and moving the locking to a wrapper
> rzg2l_gpio_irq_endisable()?
That was my other option but, if I remember correctly, it generated
duplicated code, thus I ended up with this.
>
> static void __rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> unsigned int hwirq, bool enable)
> {
> /* old functionality without locking */
> ...
> }
>
> static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> unsigned int hwirq, bool enable)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&pctrl->lock, flags);
> __rzg2l_gpio_irq_endisable(pctrl, hwirq, enable);
> spin_unlock_irqrestore(&pctrl->lock, flags);
> }
>
> Then no existing callers of rzg2l_gpio_irq_endisable() need to be
> changed.
>
>> @@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
>> gpiochip_disable_irq(gc, hwirq);
>> }
>>
>> -static void rzg2l_gpio_irq_enable(struct irq_data *d)
>> +static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
>
> Here we can't do without the "lock" parameter, unless duplicating the
> full body, so this is fine. I'd rename it to __rzg2l_gpio_irq_enable(),
> though.
>
>> {
>> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
>> unsigned int hwirq = irqd_to_hwirq(d);
>>
>> gpiochip_enable_irq(gc, hwirq);
>> + rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
>
> if (lock)
> rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
> else
> __rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
>
>> irq_chip_enable_parent(d);
>> }
>>
>> +static void rzg2l_gpio_irq_enable(struct irq_data *d)
>> +{
>> + rzg2l_gpio_irq_enable_helper(d, true);
>
> __rzg2l_gpio_irq_enable(d, true);
>
>> +}
>> +
>> static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> {
>> return irq_chip_set_type_parent(d, type);
>> @@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>> spin_lock_irqsave(&pctrl->lock, flags);
>> ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>> if (!ret && !irqd_irq_disabled(data))
>> - rzg2l_gpio_irq_enable(data);
>> + rzg2l_gpio_irq_enable_helper(data, false);
>
> __rzg2l_gpio_irq_enable(data, false);
>
> Before, the lock was taken again, while it was already held.
> Didn't this cause a deadlock?
The only locking issue I've seen around this code was fixed by commit
a39741d38c04 ("pinctrl: renesas: rzg2l: Use
spin_{lock,unlock}_irq{save,restore}"
I'll use the approach proposed by you in the next version.
Thank you for your review,
Claudiu
>
>> spin_unlock_irqrestore(&pctrl->lock, flags);
>>
>> if (ret)
>
> Gr{oetje,eeting}s,
>
> Geert
>
© 2016 - 2026 Red Hat, Inc.