| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdVVWTAvEMgv2NVg0-2f4Cs4oXp4yBM1tQYUUbMKH6EiGQ@mail.gmail.com>
Date: Thu, 11 Sep 2025 11:53:43 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: linus.walleij@...aro.org, biju.das.jz@...renesas.com,
linux-renesas-soc@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, stable@...r.kernel.org
Subject: Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
Hi Claudiu,
On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...r.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...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@...ux-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
Powered by blists - more mailing lists