i2c: tegra: Remove suspend-resume

PATCH Applied Dmitry Osipenko 5 participants 16 messages May 13, 2018 – Oct 17, 2018

Subsystems (4)

Tegra architecture support I2C subsystem host drivers Multifunction devices (MFD) Tegra I2C driver

lore

Share & tools

lore

Dmitry Osipenko

author reviewer

2018-05-13 21:13 UTC #0 lore raw

PATCH 1/1 i2c: tegra: Remove suspend-resume

Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.

Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.

Signed-off-by Dmitry Osipenko <[email protected]>

Cc <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

drivers/i2c/busses/i2c-tegra.c +0 ~~-33~~

index 60292d243e24..5fccd1f1bca8 100644
@@ -173,7 +173,6 @@ struct tegra_i2c_hw_feature {
  * @msg_buf_remaining: size of unsent data in the message buffer
  * @msg_read: identifies read transfers
  * @bus_clk_rate: current i2c bus clock rate
- * @is_suspended: prevents i2c controller accesses after suspend is called
  */
 struct tegra_i2c_dev {
 	struct device *dev;

@@ -194,7 +193,6 @@ struct tegra_i2c_dev {
 	int msg_read;
 	u32 bus_clk_rate;
 	u16 clk_divisor_non_hs_mode;
-	bool is_suspended;
 	bool is_multimaster_mode;
 	spinlock_t xfer_lock;
 };

@@ -734,9 +732,6 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	int i;
 	int ret = 0;
 
-	if (i2c_dev->is_suspended)
-		return -EBUSY;
-
 	ret = pm_runtime_get_sync(i2c_dev->dev);
 	if (ret < 0) {
 		dev_err(i2c_dev->dev, "runtime resume failed %d\n", ret);

@@ -1051,37 +1046,9 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int tegra_i2c_suspend(struct device *dev)
-{
-	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-
-	i2c_lock_adapter(&i2c_dev->adapter);
-	i2c_dev->is_suspended = true;
-	i2c_unlock_adapter(&i2c_dev->adapter);
-
-	return 0;
-}
-
-static int tegra_i2c_resume(struct device *dev)
-{
-	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-	int ret;
-
-	i2c_lock_adapter(&i2c_dev->adapter);
-
-	ret = tegra_i2c_init(i2c_dev);
-	if (!ret)
-		i2c_dev->is_suspended = false;
-
-	i2c_unlock_adapter(&i2c_dev->adapter);
-
-	return ret;
-}
-
 static const struct dev_pm_ops tegra_i2c_pm = {
 	SET_RUNTIME_PM_OPS(tegra_i2c_runtime_suspend, tegra_i2c_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)
 };
 #define TEGRA_I2C_PM	(&tegra_i2c_pm)
 #else

Signature · 1 line

2.17.0

Thierry Reding

2018-05-14 11:59 UTC #1 lore raw

Quoted 18 lines · Dmitry Osipenko

Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.

Quoted 5 lines

Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.

Thierry

Wolfram Sang

2018-05-14 12:18 UTC #2 lore raw

Quoted 25 lines · Thierry Reding

Quoted 18 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

Quoted 6 lines

Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.

Thanks for this patch!

This is closely related to a discussion we started recently:

"I2C PM overhaul needed?" https://lkml.org/lkml/2018/5/4/329

And part of the outcome is that the I2C core should print a WARN if an I2C client tries to use I2C at suspend_noirq state or later. This is to remove logic like in this patch from all I2C host drivers and to make it more explicit that those I2C client drivers need their PM fixed.

Dmitry Osipenko

author reviewer

2018-05-14 13:03 UTC #3 lore raw

Quoted 35 lines · Wolfram Sang

Quoted 25 lines · Thierry Reding
Quoted 18 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)
Quoted 6 lines
Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.
Thanks for this patch!

Quoted 9 lines

This is closely related to a discussion we started recently:
"I2C PM overhaul needed?" https://lkml.org/lkml/2018/5/4/329
And part of the outcome is that the I2C core should print a WARN if an I2C client tries to use I2C at suspend_noirq state or later. This is to remove logic like in this patch from all I2C host drivers and to make it more explicit that those I2C client drivers need their PM fixed.

Thank you very much for pointing at the discussion. Indeed it should be nicer if I2C core handled the buggy client drivers and the rest of related suspend/resume issues instead of having each I2C BUS driver to do it on its own.

Laxman Dewangan

maintainer

2018-05-14 12:21 UTC #4 lore raw

In reply to Thierry Reding

Quoted 26 lines · Thierry Reding

* PGP Signed by an unknown key
Quoted 18 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
  drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
  1 file changed, 33 deletions(-)

Quoted 7 lines

Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.

Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream.
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend,
tegra_i2c_resume)

In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer.

Thierry Reding

2018-05-14 12:47 UTC #5 lore raw

Quoted 37 lines · Laxman Dewangan

Quoted 26 lines · Thierry Reding
* PGP Signed by an unknown key
Quoted 18 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
  drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
  1 file changed, 33 deletions(-)
Quoted 7 lines
Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.

Quoted 4 lines

Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream.
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)

That seems to me like it shouldn't be necessary. If all clients are properly suspended, then when the I2C controller gets suspended there should be no pending transfers. That's provided that the driver model properly orders suspend of clients vs. their controller. I think it didn't use to do that (especially when deferred probing was involved) but I remember that getting fixed sometime in the last couple of years.

Quoted 2 lines

In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer.

Do we really need to bit-bang the GPIOs? Couldn't the I2C controller operate in a polling mode where only the interrupt was disabled but we still polled the status register to know when a transfer was finished?

Thierry

Dmitry Osipenko

author reviewer

2018-05-14 12:51 UTC #6 lore raw

In reply to Laxman Dewangan

Quoted 37 lines · Laxman Dewangan

Quoted 26 lines · Thierry Reding
* PGP Signed by an unknown key
Quoted 18 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 33 ---------------------------------
  1 file changed, 33 deletions(-)
Quoted 7 lines
Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful.

Quoted 7 lines

Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream. SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)
In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer.

In the current upstream kernel suspend/resume can't be simply moved to the 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and that doesn't work with the IRQ's being disabled. But things do not work even with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote above the I2C transfer fails (due to timeout) and a "fix" for that failure was to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for a complete suspend/resume removal for now as it is causes problem.

Wolfram Sang

2018-05-29 18:06 UTC #7 lore raw

Quoted 8 lines

Quoted 8 lines
Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream. SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)
In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer.

Quoted 7 lines

In the current upstream kernel suspend/resume can't be simply moved to the 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and that doesn't work with the IRQ's being disabled. But things do not work even with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote above the I2C transfer fails (due to timeout) and a "fix" for that failure was to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for a complete suspend/resume removal for now as it is causes problem.

Laxman, are you convinced or do you have still objections?

Laxman Dewangan

maintainer Acked-by

2018-05-30 10:59 UTC #8 lore raw

Quoted 7 lines · Wolfram Sang

Quoted 7 lines
Quoted 7 lines
Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream.
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)
In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer.

Quoted 8 lines

Quoted 7 lines
In the current upstream kernel suspend/resume can't be simply moved to the 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and that doesn't work with the IRQ's being disabled. But things do not work even with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote above the I2C transfer fails (due to timeout) and a "fix" for that failure was to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for a complete suspend/resume removal for now as it is causes problem.
Laxman, are you convinced or do you have still objections?

Fine with me. Please add my Ack

Acked-by Laxman Dewangan <[email protected]>

Wolfram Sang

Applied

2018-05-30 20:18 UTC #9 lore raw

In reply to Dmitry Osipenko

Quoted 18 lines · Dmitry Osipenko

Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support.

Quoted 2 lines

Signed-off-by Dmitry Osipenko <[email protected]>
Cc <[email protected]>

Applied to for-next, thanks!

Wolfram Sang

2018-05-30 20:25 UTC #10 lore raw

Quoted 2 lines

Applied to for-next, thanks!

I removed the stable tag, though. I am not 100% sure if there are not any side-effects for other users. If you still think it should go to stable, please mention this patch to stable@ after it was applied to linus tree. Maybe with a word or two about regression safety.

Dmitry Osipenko

author reviewer

2018-05-30 22:17 UTC #11 lore raw

Quoted 8 lines · Wolfram Sang

Quoted 2 lines
Applied to for-next, thanks!
I removed the stable tag, though. I am not 100% sure if there are not any side-effects for other users. If you still think it should go to stable, please mention this patch to stable@ after it was applied to linus tree. Maybe with a word or two about regression safety.

Backporting to stable isn't really necessary, so no objections from me. Thank you.

Jon Hunter

maintainer

2018-10-17 13:59 UTC #12 lore raw

In reply to Dmitry Osipenko

Quoted 9 lines · Dmitry Osipenko

Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.

I have been looking at the above issue with the tps6586x because I am seeing delays on resume caused by this driver on the stable branches. I understand that this patch was dropped for stable, but looking at the specific issue with the tps6586x I am curious why the tps6586x driver was not fixed because it appears to me that the issue largely resides with that driver and any other device that uses the tps6586x is susceptible to it. I was able to fix the tps6586x driver by doing the following and I am interested in your thoughts ...

Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend

The tps6586x device is registered as an irqchip and the tps6586x-rtc interrupt is one of it's interrupt sources. When using the tps6586x-rtc as a wake-up device from suspend, the following is seen:

PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. Disabling non-boot CPUs ...
Entering suspend state LP1
Enabling non-boot CPUs ...
CPU1 is up
tps6586x 3-0034: failed to read interrupt status
tps6586x 3-0034: failed to read interrupt status

The reason why the tps6586x interrupt status cannot be read is because the tps6586x interrupt is not masked during suspend and when the tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is seen before the i2c controller has been resumed in order to read the tps6586x interrupt status.

The tps6586x-rtc driver sets it's interrupt as a wake-up source during suspend, which gets propagated to the parent tps6586x interrupt. However, the tps6586x-rtc driver cannot disable it's interrupt during suspend otherwise we would never be woken up and so the tps6586x must disable it's interrupt instead.

Fix this by disabling the tps6586x interrupt on entering suspend and re-enabling it on resuming from suspend.

---
 drivers/mfd/tps6586x.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/mfd/tps6586x.c +0 -0

index 5628a6b5b19b..165ac8a3d22c 100644
@@ -594,6 +594,27 @@ static int tps6586x_i2c_remove(struct i2c_client

*client)
return 0;
}

+static int __maybe_unused tps6586x_i2c_suspend(struct device *dev) +{
+ struct tps6586x *tps6586x = dev_get_drvdata(dev);
+
+ disable_irq(tps6586x->client->irq);
+
+ return 0;
+}
+
+static int __maybe_unused tps6586x_i2c_resume(struct device *dev) +{
+ struct tps6586x *tps6586x = dev_get_drvdata(dev);
+
+ enable_irq(tps6586x->client->irq);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(tps6586x_pm_ops, tps6586x_i2c_suspend,
+ tps6586x_i2c_resume);
+
static const struct i2c_device_id tps6586x_id_table[] = {
{ "tps6586x", 0 },
{ },
@@ -604,6 +625,7 @@ static int tps6586x_i2c_remove(struct i2c_client *client)
.driver = {
.name = "tps6586x",
.of_match_table = of_match_ptr(tps6586x_of_match),
+ .pm = &tps6586x_pm_ops,
},
.probe = tps6586x_i2c_probe,
.remove = tps6586x_i2c_remove,

Signature · 4 lines

1.9.1

-- 
nvpublic

Dmitry Osipenko

author reviewer

2018-10-17 14:30 UTC #13 lore raw

Quoted 44 lines · Jon Hunter

Quoted 9 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
I have been looking at the above issue with the tps6586x because I am seeing delays on resume caused by this driver on the stable branches. I understand that this patch was dropped for stable, but looking at the specific issue with the tps6586x I am curious why the tps6586x driver was not fixed because it appears to me that the issue largely resides with that driver and any other device that uses the tps6586x is susceptible to it. I was able to fix the tps6586x driver by doing the following and I am interested in your thoughts ...
Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend
The tps6586x device is registered as an irqchip and the tps6586x-rtc interrupt is one of it's interrupt sources. When using the tps6586x-rtc as a wake-up device from suspend, the following is seen:
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. Disabling non-boot CPUs ...
Entering suspend state LP1
Enabling non-boot CPUs ...
CPU1 is up
tps6586x 3-0034: failed to read interrupt status
tps6586x 3-0034: failed to read interrupt status
The reason why the tps6586x interrupt status cannot be read is because the tps6586x interrupt is not masked during suspend and when the tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is seen before the i2c controller has been resumed in order to read the tps6586x interrupt status.

Quoted 8 lines

The tps6586x-rtc driver sets it's interrupt as a wake-up source during suspend, which gets propagated to the parent tps6586x interrupt. However, the tps6586x-rtc driver cannot disable it's interrupt during suspend otherwise we would never be woken up and so the tps6586x must disable it's interrupt instead.
Fix this by disabling the tps6586x interrupt on entering suspend and re-enabling it on resuming from suspend.

Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels.

I just found this [0], seems your patch need to address the same review comment.

[0] https://lkml.org/lkml/2011/3/29/18

Jon Hunter

maintainer

2018-10-17 19:41 UTC #14 lore raw

Quoted 51 lines · Dmitry Osipenko

Quoted 50 lines · Jon Hunter
Quoted 9 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
I have been looking at the above issue with the tps6586x because I am seeing delays on resume caused by this driver on the stable branches. I understand that this patch was dropped for stable, but looking at the specific issue with the tps6586x I am curious why the tps6586x driver was not fixed because it appears to me that the issue largely resides with that driver and any other device that uses the tps6586x is susceptible to it. I was able to fix the tps6586x driver by doing the following and I am interested in your thoughts ...
Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend
The tps6586x device is registered as an irqchip and the tps6586x-rtc interrupt is one of it's interrupt sources. When using the tps6586x-rtc as a wake-up device from suspend, the following is seen:
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. Disabling non-boot CPUs ...
Entering suspend state LP1
Enabling non-boot CPUs ...
CPU1 is up
tps6586x 3-0034: failed to read interrupt status
tps6586x 3-0034: failed to read interrupt status
The reason why the tps6586x interrupt status cannot be read is because the tps6586x interrupt is not masked during suspend and when the tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is seen before the i2c controller has been resumed in order to read the tps6586x interrupt status.
The tps6586x-rtc driver sets it's interrupt as a wake-up source during suspend, which gets propagated to the parent tps6586x interrupt. However, the tps6586x-rtc driver cannot disable it's interrupt during suspend otherwise we would never be woken up and so the tps6586x must disable it's interrupt instead.

Quoted 4 lines

Quoted 2 lines
Fix this by disabling the tps6586x interrupt on entering suspend and re-enabling it on resuming from suspend.
Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels.

Indeed, I have been setting up some automated testing of various stable branches (mainly 4.x LTS releases) and I am seeing this problem there. Furthermore, I am using this to validate the change as well.

Quoted 3 lines

I just found this [0], seems your patch need to address the same review comment.
[0] https://lkml.org/lkml/2011/3/29/18

Thanks will do.

I know we don't support low power modes (ie. LP0), however, I do wonder if we should have some sort of i2c suspend/resume handler for Tegra? Eventually we will need this.

Cheers
Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author reviewer

2018-10-17 20:49 UTC #15 lore raw

Quoted 64 lines · Jon Hunter

Quoted 51 lines · Dmitry Osipenko
Quoted 50 lines · Jon Hunter
Quoted 9 lines · Dmitry Osipenko
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process.
I have been looking at the above issue with the tps6586x because I am seeing delays on resume caused by this driver on the stable branches. I understand that this patch was dropped for stable, but looking at the specific issue with the tps6586x I am curious why the tps6586x driver was not fixed because it appears to me that the issue largely resides with that driver and any other device that uses the tps6586x is susceptible to it. I was able to fix the tps6586x driver by doing the following and I am interested in your thoughts ...
Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend
The tps6586x device is registered as an irqchip and the tps6586x-rtc interrupt is one of it's interrupt sources. When using the tps6586x-rtc as a wake-up device from suspend, the following is seen:
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. Disabling non-boot CPUs ...
Entering suspend state LP1
Enabling non-boot CPUs ...
CPU1 is up
tps6586x 3-0034: failed to read interrupt status
tps6586x 3-0034: failed to read interrupt status
The reason why the tps6586x interrupt status cannot be read is because the tps6586x interrupt is not masked during suspend and when the tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is seen before the i2c controller has been resumed in order to read the tps6586x interrupt status.
The tps6586x-rtc driver sets it's interrupt as a wake-up source during suspend, which gets propagated to the parent tps6586x interrupt. However, the tps6586x-rtc driver cannot disable it's interrupt during suspend otherwise we would never be woken up and so the tps6586x must disable it's interrupt instead.
Quoted 4 lines
Quoted 2 lines
Fix this by disabling the tps6586x interrupt on entering suspend and re-enabling it on resuming from suspend.
Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels.
Indeed, I have been setting up some automated testing of various stable branches (mainly 4.x LTS releases) and I am seeing this problem there. Furthermore, I am using this to validate the change as well.
Quoted 2 lines
I just found this [0], seems your patch need to address the same review comment.

Quoted 7 lines

Quoted 1 line
[0] https://lkml.org/lkml/2011/3/29/18
Thanks will do.
I know we don't support low power modes (ie. LP0), however, I do wonder if we should have some sort of i2c suspend/resume handler for Tegra? Eventually we will need this.

I'd suggest to support LP0 in the core first and only then to start making necessary suspend/resume changes in the drivers, otherwise it may end up being wasted time and effort for you and other maintainers.