Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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
>