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