[PATCH] i2c: designware: Fix system suspend

[PATCH] i2c: designware: Fix system suspend

[Ulf Hansson]

The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
during system suspend"), may suggest to the PM core to try out the so
called direct_complete path for system sleep. In this path, the PM core
treats a runtime suspended device as it's already in a proper low power
state for system sleep, which makes it skip calling the system sleep
callbacks for the device, except for the ->prepare() and the ->complete()
callbacks.

However, the PM core may unset the direct_complete flag for a parent
device, in case its child device are being system suspended before. In this
scenario, the PM core invokes the system sleep callbacks, no matter if the
device is runtime suspended or not.

Particularly in cases of an existing i2c slave device, the above path is
triggered, which breaks the assumption that the i2c device is always
runtime resumed whenever the dw_i2c_plat_suspend() is being called.

More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
clk_core_unprepare(), for an already disabled/unprepared clock, leading to
a splat in the log about clocks calls being wrongly balanced and breaking
system sleep.

To still allow the direct_complete path in cases when it's possible, but
also to keep the fix simple, let's runtime resume the i2c device in the
->suspend() callback, before continuing to put the device into low power
state.

Note, in cases when the i2c device is attached to the ACPI PM domain, this
problem doesn't occur, because ACPI's ->suspend() callback, assigned to
acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.

It should also be noted that this change does not fix commit 8503ff166504
("i2c: designware: Avoid unnecessary resuming during system suspend").
Because for the non-ACPI case, the system sleep support was already broken
prior that point.

Cc: <stable@vger.kernel.org> # v4.4+
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

I decided to post this as a separate change, instead of as earlier folding it
in the series that extends the ACPI PM domain to cope with the runtime PM
centric path for system sleep.

This change applies on a fresh v4.4+. If someone wants it's applied for earlier
version, please send a backport yourself.

It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.

Changes in v2:
	- Updated changelog.
	- Runtime resume the device in ->suspend() instead of in ->prepare().

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2ea6d0d..bb3b8c8 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -426,7 +426,7 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
-static int dw_i2c_plat_suspend(struct device *dev)
+static int dw_i2c_plat_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
@@ -448,11 +448,21 @@ static int dw_i2c_plat_resume(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int dw_i2c_plat_suspend(struct device *dev)
+{
+	pm_runtime_resume(dev);
+	return dw_i2c_plat_runtime_suspend(dev);
+}
+#endif
+
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
 	.complete = dw_i2c_plat_complete,
 	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
-	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
+			   dw_i2c_plat_resume,
+			   NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
2.7.4

[PATCH] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Wednesday, August 9, 2017 3:28:22 PM CEST Ulf Hansson wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
> 
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
> 
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
> 
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
> 
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
> 
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
> 
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
> 
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Looks OK to me.

> ---
> 
> I decided to post this as a separate change, instead of as earlier folding it
> in the series that extends the ACPI PM domain to cope with the runtime PM
> centric path for system sleep.
> 
> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
> version, please send a backport yourself.
> 
> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
> 
> Changes in v2:
> 	- Updated changelog.
> 	- Runtime resume the device in ->suspend() instead of in ->prepare().
> 
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 2ea6d0d..bb3b8c8 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -426,7 +426,7 @@ static void dw_i2c_plat_complete(struct device *dev)
>  #endif
>  
>  #ifdef CONFIG_PM
> -static int dw_i2c_plat_suspend(struct device *dev)
> +static int dw_i2c_plat_runtime_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> @@ -448,11 +448,21 @@ static int dw_i2c_plat_resume(struct device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int dw_i2c_plat_suspend(struct device *dev)
> +{
> +	pm_runtime_resume(dev);
> +	return dw_i2c_plat_runtime_suspend(dev);
> +}
> +#endif
> +
>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>  	.prepare = dw_i2c_plat_prepare,
>  	.complete = dw_i2c_plat_complete,
>  	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> -	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
> +	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
> +			   dw_i2c_plat_resume,
> +			   NULL)
>  };
>  
>  #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
> 

[PATCH] i2c: designware: Fix system suspend

[John Stultz]

On Wed, Aug 9, 2017 at 6:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
>
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
>
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
>
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
>
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
>
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> I decided to post this as a separate change, instead of as earlier folding it
> in the series that extends the ACPI PM domain to cope with the runtime PM
> centric path for system sleep.
>
> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
> version, please send a backport yourself.
>
> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
>
> Changes in v2:
>         - Updated changelog.
>         - Runtime resume the device in ->suspend() instead of in ->prepare().


This avoids the suspend/resume warning I've seen w/o the patch on HiKey.

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john

[PATCH] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 08/09/2017 11:55 PM, John Stultz wrote:
> On Wed, Aug 9, 2017 at 6:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
>> during system suspend"), may suggest to the PM core to try out the so
>> called direct_complete path for system sleep. In this path, the PM core
>> treats a runtime suspended device as it's already in a proper low power
>> state for system sleep, which makes it skip calling the system sleep
>> callbacks for the device, except for the ->prepare() and the ->complete()
>> callbacks.
>>
>> However, the PM core may unset the direct_complete flag for a parent
>> device, in case its child device are being system suspended before. In this
>> scenario, the PM core invokes the system sleep callbacks, no matter if the
>> device is runtime suspended or not.
>>
>> Particularly in cases of an existing i2c slave device, the above path is
>> triggered, which breaks the assumption that the i2c device is always
>> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>>
>> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> a splat in the log about clocks calls being wrongly balanced and breaking
>> system sleep.
>>
>> To still allow the direct_complete path in cases when it's possible, but
>> also to keep the fix simple, let's runtime resume the i2c device in the
>> ->suspend() callback, before continuing to put the device into low power
>> state.
>>
>> Note, in cases when the i2c device is attached to the ACPI PM domain, this
>> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
>> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>>
>> It should also be noted that this change does not fix commit 8503ff166504
>> ("i2c: designware: Avoid unnecessary resuming during system suspend").
>> Because for the non-ACPI case, the system sleep support was already broken
>> prior that point.
>>
>> Cc: <stable@vger.kernel.org> # v4.4+
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> I decided to post this as a separate change, instead of as earlier folding it
>> in the series that extends the ACPI PM domain to cope with the runtime PM
>> centric path for system sleep.
>>
>> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
>> version, please send a backport yourself.
>>
>> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
>>
>> Changes in v2:
>>          - Updated changelog.
>>          - Runtime resume the device in ->suspend() instead of in ->prepare().
> 
> 
> This avoids the suspend/resume warning I've seen w/o the patch on HiKey.
> 
> Tested-by: John Stultz <john.stultz@linaro.org>
> 
Cool, no issues from acpi_lpss.c nor mfd/intel-lpss.c platforms.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH] i2c: designware: Fix system suspend

[Mika Westerberg]

On Thu, Aug 10, 2017 at 01:25:33PM +0300, Jarkko Nikula wrote:
> Cool, no issues from acpi_lpss.c nor mfd/intel-lpss.c platforms.
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Looks good to me as well,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH] i2c: designware: Fix system suspend

[Wolfram Sang]

On Wed, Aug 09, 2017 at 10:43:22PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, August 9, 2017 3:28:22 PM CEST Ulf Hansson wrote:
> > The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> > during system suspend"), may suggest to the PM core to try out the so
> > called direct_complete path for system sleep. In this path, the PM core
> > treats a runtime suspended device as it's already in a proper low power
> > state for system sleep, which makes it skip calling the system sleep
> > callbacks for the device, except for the ->prepare() and the ->complete()
> > callbacks.
> > 
> > However, the PM core may unset the direct_complete flag for a parent
> > device, in case its child device are being system suspended before. In this
> > scenario, the PM core invokes the system sleep callbacks, no matter if the
> > device is runtime suspended or not.
> > 
> > Particularly in cases of an existing i2c slave device, the above path is
> > triggered, which breaks the assumption that the i2c device is always
> > runtime resumed whenever the dw_i2c_plat_suspend() is being called.
> > 
> > More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > a splat in the log about clocks calls being wrongly balanced and breaking
> > system sleep.
> > 
> > To still allow the direct_complete path in cases when it's possible, but
> > also to keep the fix simple, let's runtime resume the i2c device in the
> > ->suspend() callback, before continuing to put the device into low power
> > state.
> > 
> > Note, in cases when the i2c device is attached to the ACPI PM domain, this
> > problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> > acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
> > 
> > It should also be noted that this change does not fix commit 8503ff166504
> > ("i2c: designware: Avoid unnecessary resuming during system suspend").
> > Because for the non-ACPI case, the system sleep support was already broken
> > prior that point.
> > 
> > Cc: <stable@vger.kernel.org> # v4.4+
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Looks OK to me.

I read this as Acked-by. If so, using the real "Acked-by" tag would be
helpful because patchwork collects them automatically.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170814/62953eb5/attachment.sig>

[PATCH] i2c: designware: Fix system suspend

[Wolfram Sang]

On Wed, Aug 09, 2017 at 03:28:22PM +0200, Ulf Hansson wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
> 
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
> 
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
> 
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
> 
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
> 
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
> 
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
> 
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Applied to for-current, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170814/74a86566/attachment.sig>