diff mbox series

[v2] gpio: pca953x: fix IRQ storm on system wake up

Message ID 20250509141828.57851-1-francesco@dolcini.it
State Superseded
Headers show
Series [v2] gpio: pca953x: fix IRQ storm on system wake up | expand

Commit Message

Francesco Dolcini May 9, 2025, 2:18 p.m. UTC 
From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

If an input changes state during wake-up and is used as an interrupt
source, the IRQ handler reads the volatile input register to clear the
interrupt mask and deassert the IRQ line. However, the IRQ handler is
triggered before access to the register is granted, causing the read
operation to fail.

As a result, the IRQ handler enters a loop, repeatedly printing the
"failed reading register" message, until `pca953x_resume` is eventually
called, which restores the driver context and enables access to
registers.

Fix by disabling the IRQ line before entering suspend mode, and
re-enabling it after the driver context is restored in `pca953x_resume`.

An irq can be disabled with disable_irq() and still wake the system as
long as the irq has wake enabled, so the wake-up functionality is
preserved.

Fixes: b76574300504 ("gpio: pca953x: Restore registers after suspend/resume cycle")
Cc: stable@vger.kernel.org
Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v1 -> v2
 - Instead of calling PM ops with disabled interrupts, just disable the
   irq while going in suspend and re-enable it after restoring the
   context in resume function.
---
 drivers/gpio/gpio-pca953x.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Geert Uytterhoeven May 12, 2025, 9:17 a.m. UTC | #1 
Hi Francesco,

On Fri, 9 May 2025 at 16:18, Francesco Dolcini <francesco@dolcini.it> wrote:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
>
> If an input changes state during wake-up and is used as an interrupt
> source, the IRQ handler reads the volatile input register to clear the
> interrupt mask and deassert the IRQ line. However, the IRQ handler is
> triggered before access to the register is granted, causing the read
> operation to fail.
>
> As a result, the IRQ handler enters a loop, repeatedly printing the
> "failed reading register" message, until `pca953x_resume` is eventually
> called, which restores the driver context and enables access to
> registers.
>
> Fix by disabling the IRQ line before entering suspend mode, and
> re-enabling it after the driver context is restored in `pca953x_resume`.
>
> An irq can be disabled with disable_irq() and still wake the system as
> long as the irq has wake enabled, so the wake-up functionality is
> preserved.
>
> Fixes: b76574300504 ("gpio: pca953x: Restore registers after suspend/resume cycle")
> Cc: stable@vger.kernel.org
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v1 -> v2
>  - Instead of calling PM ops with disabled interrupts, just disable the
>    irq while going in suspend and re-enable it after restoring the
>    context in resume function.

Thanks for the update!

> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -1226,6 +1226,8 @@ static int pca953x_restore_context(struct pca953x_chip *chip)
>
>         guard(mutex)(&chip->i2c_lock);
>
> +       if (chip->client->irq > 0)
> +               enable_irq(chip->client->irq);
>         regcache_cache_only(chip->regmap, false);
>         regcache_mark_dirty(chip->regmap);
>         ret = pca953x_regcache_sync(chip);
> @@ -1238,6 +1240,10 @@ static int pca953x_restore_context(struct pca953x_chip *chip)
>  static void pca953x_save_context(struct pca953x_chip *chip)
>  {
>         guard(mutex)(&chip->i2c_lock);
> +
> +       /* Disable IRQ to prevent early triggering while regmap "cache only" is on */
> +       if (chip->client->irq > 0)
> +               disable_irq(chip->client->irq);
>         regcache_cache_only(chip->regmap, true);
>  }

While this does not cause the regression seen on Salvator-XS with
the earlier approach[1], I expect this will break using a GPIO as a
wake-up source?

[1] https://lore.kernel.org/linux-gpio/CAMuHMdVnKX23yi7ir1LVxfXAMeeWMFzM+cdgSSTNjpn1OnC2xw@mail.gmail.com

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
Andy Shevchenko May 12, 2025, 9:23 a.m. UTC | #2 
On Mon, May 12, 2025 at 11:17:48AM +0200, Geert Uytterhoeven wrote:
> On Fri, 9 May 2025 at 16:18, Francesco Dolcini <francesco@dolcini.it> wrote:
> >
> > If an input changes state during wake-up and is used as an interrupt
> > source, the IRQ handler reads the volatile input register to clear the
> > interrupt mask and deassert the IRQ line. However, the IRQ handler is
> > triggered before access to the register is granted, causing the read
> > operation to fail.
> >
> > As a result, the IRQ handler enters a loop, repeatedly printing the
> > "failed reading register" message, until `pca953x_resume` is eventually
> > called, which restores the driver context and enables access to
> > registers.
> >
> > Fix by disabling the IRQ line before entering suspend mode, and
> > re-enabling it after the driver context is restored in `pca953x_resume`.
> >
> > An irq can be disabled with disable_irq() and still wake the system as
> > long as the irq has wake enabled, so the wake-up functionality is
> > preserved.

...

> While this does not cause the regression seen on Salvator-XS with
> the earlier approach[1], I expect this will break using a GPIO as a
> wake-up source?

Good point! Have this code been checked for that kind of scenarios?

> [1] https://lore.kernel.org/linux-gpio/CAMuHMdVnKX23yi7ir1LVxfXAMeeWMFzM+cdgSSTNjpn1OnC2xw@mail.gmail.com
Emanuele Ghidoli May 12, 2025, 9:38 a.m. UTC | #3 
On 12/05/2025 11:23, Andy Shevchenko wrote:
> On Mon, May 12, 2025 at 11:17:48AM +0200, Geert Uytterhoeven wrote:
>> On Fri, 9 May 2025 at 16:18, Francesco Dolcini <francesco@dolcini.it> wrote:
>>> An irq can be disabled with disable_irq() and still wake the system as
>>> long as the irq has wake enabled, so the wake-up functionality is
>>> preserved.
> 
> ...
> 
>> While this does not cause the regression seen on Salvator-XS with
>> the earlier approach[1], I expect this will break using a GPIO as a
>> wake-up source?
> 
> Good point! Have this code been checked for that kind of scenarios?
> 
>> [1] https://lore.kernel.org/linux-gpio/CAMuHMdVnKX23yi7ir1LVxfXAMeeWMFzM+cdgSSTNjpn1OnC2xw@mail.gmail.com
> 
Yes, I tested this specific scenario with its GPIOs as wake-up sources, and it
worked as expected. I already included the note in the commit message.

Kind regards,
Emanuele
Geert Uytterhoeven May 12, 2025, 9:42 a.m. UTC | #4 
Hi Emanuele,

On Mon, 12 May 2025 at 11:38, Emanuele Ghidoli
<ghidoliemanuele@gmail.com> wrote:
> On 12/05/2025 11:23, Andy Shevchenko wrote:
> > On Mon, May 12, 2025 at 11:17:48AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, 9 May 2025 at 16:18, Francesco Dolcini <francesco@dolcini.it> wrote:
> >>> An irq can be disabled with disable_irq() and still wake the system as
> >>> long as the irq has wake enabled, so the wake-up functionality is
> >>> preserved.
> >
> > ...
> >
> >> While this does not cause the regression seen on Salvator-XS with
> >> the earlier approach[1], I expect this will break using a GPIO as a
> >> wake-up source?
> >
> > Good point! Have this code been checked for that kind of scenarios?
> >
> >> [1] https://lore.kernel.org/linux-gpio/CAMuHMdVnKX23yi7ir1LVxfXAMeeWMFzM+cdgSSTNjpn1OnC2xw@mail.gmail.com
> >
> Yes, I tested this specific scenario with its GPIOs as wake-up sources, and it
> worked as expected. I already included the note in the commit message.

Sorry, I missed that.

Then I have no objections from my side.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ab2c0fd428fb..b852e4997629 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1226,6 +1226,8 @@  static int pca953x_restore_context(struct pca953x_chip *chip)
 
 	guard(mutex)(&chip->i2c_lock);
 
+	if (chip->client->irq > 0)
+		enable_irq(chip->client->irq);
 	regcache_cache_only(chip->regmap, false);
 	regcache_mark_dirty(chip->regmap);
 	ret = pca953x_regcache_sync(chip);
@@ -1238,6 +1240,10 @@  static int pca953x_restore_context(struct pca953x_chip *chip)
 static void pca953x_save_context(struct pca953x_chip *chip)
 {
 	guard(mutex)(&chip->i2c_lock);
+
+	/* Disable IRQ to prevent early triggering while regmap "cache only" is on */
+	if (chip->client->irq > 0)
+		disable_irq(chip->client->irq);
 	regcache_cache_only(chip->regmap, true);
 }