Some IOs can be needed during suspend_noirq()/resume_noirq().
So move suspend()/resume() to noirq.
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
drivers/gpio/gpio-pca953x.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 00ffa168e405..6e495fc67a93 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1234,7 +1234,7 @@ static void pca953x_save_context(struct pca953x_chip *chip)
regcache_cache_only(chip->regmap, true);
}
-static int pca953x_suspend(struct device *dev)
+static int pca953x_suspend_noirq(struct device *dev)
{
struct pca953x_chip *chip = dev_get_drvdata(dev);
@@ -1248,7 +1248,7 @@ static int pca953x_suspend(struct device *dev)
return 0;
}
-static int pca953x_resume(struct device *dev)
+static int pca953x_resume_noirq(struct device *dev)
{
struct pca953x_chip *chip = dev_get_drvdata(dev);
int ret;
@@ -1268,7 +1268,8 @@ static int pca953x_resume(struct device *dev)
return ret;
}
-static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
+static DEFINE_NOIRQ_DEV_PM_OPS(pca953x_pm_ops,
+ pca953x_suspend_noirq, pca953x_resume_noirq);
/* convenience to stop overlong match-table lines */
#define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))
--
2.39.2Hi Thomas,
On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Thanks for your patch, which is now commit 86eb98127332748f ("gpio:
pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()")
in i2c-host/i2c/i2c-host.
This patch causes regressions on e.g. Salvator-XS.
s2idle:
Freezing user space processes
Freezing user space processes completed (elapsed 0.006 seconds)
OOM killer disabled.
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.003 seconds)
sd 0:0:0:0: [sda] Synchronizing SCSI cache
ata1.00: Entering standby power mode
+i2c-rcar e66d8000.i2c: error -16 : 10000005
+pca953x 4-0020: Failed to sync GPIO dir registers: -16
+pca953x 4-0020: Failed to restore register map: -16
+pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -16
+pca953x 4-0020: PM: failed to resume async noirq: error -16
s2ram:
Detected VIPT I-cache on CPU7
CPU7: Booted secondary processor 0x0000000103 [0x410fd034]
CPU7 is up
+i2c-rcar e66d8000.i2c: error -110 : 10000001
+pca953x 4-0020: Failed to sync GPIO dir registers: -110
+pca953x 4-0020: Failed to restore register map: -110
+pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -110
+pca953x 4-0020: PM: failed to resume async noirq: error -110
usb usb1: root hub lost power or was reset
...
PM: suspend exit
ata1: link resume succeeded after 1 retries
-ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
-sd 0:0:0:0: [sda] Starting disk
-ata1.00: configured for UDMA/133
-ata1.00: Entering active power mode
+ata1: SATA link down (SStatus 0 SControl 300)
+ata1: link resume succeeded after 1 retries
+ata1: SATA link down (SStatus 0 SControl 300)
+ata1: limiting SATA link speed to <unknown>
+ata1: link resume succeeded after 1 retries
+ata1: SATA link down (SStatus 0 SControl 3F0)
+ata1.00: disable device
+ata1.00: detaching (SCSI 0:0:0:0)
+sd 0:0:0:0: [sda] Synchronizing SCSI cache
+sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK
When trying to read from /dev/sda afterwards:
ata1: link resume succeeded after 1 retries
ata1: SATA link down (SStatus 0 SControl 3F0)
ata1.00: disable device
ata1.00: detaching (SCSI 0:0:0:0)
device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x80700 phys_seg 4 prio class 0
device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x0 phys_seg 1 prio class 0
Buffer I/O error on dev sda, logical block 0, async page read
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK
All issues above are fixed by reverting this commit.
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -1234,7 +1234,7 @@ static void pca953x_save_context(struct pca953x_chip *chip)
> regcache_cache_only(chip->regmap, true);
> }
>
> -static int pca953x_suspend(struct device *dev)
> +static int pca953x_suspend_noirq(struct device *dev)
> {
> struct pca953x_chip *chip = dev_get_drvdata(dev);
>
> @@ -1248,7 +1248,7 @@ static int pca953x_suspend(struct device *dev)
> return 0;
> }
>
> -static int pca953x_resume(struct device *dev)
> +static int pca953x_resume_noirq(struct device *dev)
> {
> struct pca953x_chip *chip = dev_get_drvdata(dev);
> int ret;
> @@ -1268,7 +1268,8 @@ static int pca953x_resume(struct device *dev)
> return ret;
> }
>
> -static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
> +static DEFINE_NOIRQ_DEV_PM_OPS(pca953x_pm_ops,
> + pca953x_suspend_noirq, pca953x_resume_noirq);
>
> /* convenience to stop overlong match-table lines */
> #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))
>
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
On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: ... > +i2c-rcar e66d8000.i2c: error -16 : 10000005 It probably means that I²C host controller is already in power off mode and can't serve anymore. > +pca953x 4-0020: Failed to sync GPIO dir registers: -16 > +pca953x 4-0020: Failed to restore register map: -16 > +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq > returns -16 > +pca953x 4-0020: PM: failed to resume async noirq: error -16 Yeah, with this it's kinda forcing _every_ I²C host controller PM to be moved also to noirq() or alike. ... > All issues above are fixed by reverting this commit. Let's revert as we close to the end of the cycle and this is not something that can be fixed ASAP in my opinion. ... Thanks for the report, It's a pity that it wasn't tested before from the mailing list... -- With Best Regards, Andy Shevchenko
On 4/23/24 12:34, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
>> <thomas.richard@bootlin.com> wrote:
>
> ...
>
>> +i2c-rcar e66d8000.i2c: error -16 : 10000005
>
> It probably means that I²C host controller is already in power off
> mode and can't serve anymore.
Hello,
Yes the i2c controller is already off.
In fact it's the same issue I had with the i2c-omap driver.
In suspend-noirq, the runtime pm is disabled, so you can't wakeup a
device. More details available in this thread [1].
So the trick is to wakeup the device during suspend (like I did for the
i2c-omap driver [2].
[1]
https://lore.kernel.org/all/f68c9a54-0fde-4709-9d2f-0d23a049341b@bootlin.com/
[2]
https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v5-2-4b8c46711ded@bootlin.com/
I think the patch below should fix the issue.
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1232,7 +1232,7 @@ static void rcar_i2c_remove(struct platform_device
*pdev)
pm_runtime_disable(dev);
}
-static int rcar_i2c_suspend(struct device *dev)
+static int rcar_i2c_suspend_noirq(struct device *dev)
{
struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
@@ -1240,7 +1240,7 @@ static int rcar_i2c_suspend(struct device *dev)
return 0;
}
-static int rcar_i2c_resume(struct device *dev)
+static int rcar_i2c_resume_noirq(struct device *dev)
{
struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
@@ -1248,8 +1248,23 @@ static int rcar_i2c_resume(struct device *dev)
return 0;
}
+static int rcar_i2c_suspend(struct device *dev)
+{
+ pm_runtime_get_sync(dev);
+
+ return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+ pm_runtime_put(dev);
+
+ return 0;
+}
+
static const struct dev_pm_ops rcar_i2c_pm_ops = {
- NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend_noirq,
rcar_i2c_resume_noirq)
+ SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
};
static struct platform_driver rcar_i2c_driver = {
>
>> +pca953x 4-0020: Failed to sync GPIO dir registers: -16
>> +pca953x 4-0020: Failed to restore register map: -16
>> +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
>> returns -16
>> +pca953x 4-0020: PM: failed to resume async noirq: error -16
>
> Yeah, with this it's kinda forcing _every_ I²C host controller PM to
> be moved also to noirq() or alike.
Yes indeed.
But this controller is already in noirq().
So the issue was already there.
We never saw it because we never did i2c accesses in noirq().
Best Regards,
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Thomas,
On Tue, Apr 23, 2024 at 12:53 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 4/23/24 12:34, Andy Shevchenko wrote:
> > On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> >> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
> >> <thomas.richard@bootlin.com> wrote:
> >
> > ...
> >
> >> +i2c-rcar e66d8000.i2c: error -16 : 10000005
> >
> > It probably means that I²C host controller is already in power off
> > mode and can't serve anymore.
>
> Yes the i2c controller is already off.
> In fact it's the same issue I had with the i2c-omap driver.
> In suspend-noirq, the runtime pm is disabled, so you can't wakeup a
> device. More details available in this thread [1].
> So the trick is to wakeup the device during suspend (like I did for the
> i2c-omap driver [2].
>
> [1]
> https://lore.kernel.org/all/f68c9a54-0fde-4709-9d2f-0d23a049341b@bootlin.com/
> [2]
> https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v5-2-4b8c46711ded@bootlin.com/
>
> I think the patch below should fix the issue.
Thanks, I gave that a try, but it doesn't make any difference.
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
© 2016 - 2026 Red Hat, Inc.