Hi, Geert,
On 9/11/25 12:53, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> 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
>
