[PATCH 0/9] PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI

[PATCH 0/9] PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI

[Ulf Hansson]

The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,
currently suffers from a regression, which easily can be triggerd during system
suspend/resume.

In particular the regression concerns those SoCs, which doesn't have the i2c
device being attached to the ACPI PM domain.

The regression, introduced in commit 8503ff166504 ("i2c: designware: Avoid
unnecessary resuming during system suspend"), takes advantage of the so called
direct_complete path during system sleep, which works well when using the ACPI
PM domain, however as stated, that's not the case for other SoCs.

In this series, patch 1, takes a very simplistic approach to solve the problem
and the change is limited to the i2c driver itself. The aim with this change is
to make it acceptable as a fix for v4.2+.

However, patch 1 is unfortunate also reverting back to the old behaviour,
avoiding the direct_complete path, by always runtime resuming the device in the
device_prepare() phase during system sleep. In many cases this is a waste,
especially when the device could have remained runtime suspended during the
entire system sleep sequence.

The rest of the changes in this series, addresses the above problem, avoiding
to runtime resume the device unless it's really needed. One could have tried
out other local hacks, limited to the i2c driver, but I believe the problem
could exist for other drivers/devices as well, thus I aim for a more generic
solution.

In principle, this series enables the proven runtime PM centric approach to
implement system sleep in the i2c driver. However, to do that the ACPI PM
domain have to collaborate and understand this behaviour. Therefore a number of
changes, patch 2 to patch 8, makes the needed changes to the ACPI PM domain.
In the last change, patch9, the i2c driver converts to the runtime PM centric
approach.

It shall be noted, the behaviour of the ACPI PM domain should remain intact,
still taking benefit of using the direct_complete path during system sleep,
except for those devices that explicitly request it not to.

This series has been tested on an ARM64 Hikey board, not having its i2c device
attach to the ACPI PM domain. This means that the ACPI changes has so far only
been compile tested. Any help in testing on relevant Intel SoCs is greatly
appreciated and so is of course review comments.

The series is based upon Rafael's pm tree and linux-next branch. It's also
available at the following git:

git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_i2c_rpm_path

Kind regards
Ulf Hansson


Ulf Hansson (9):
  i2c: designware: Fix system suspend
  PM / ACPI: Restore acpi_subsys_complete()
  PM / Sleep: Remove pm_complete_with_resume_check()
  PM / ACPI: Split code validating need for runtime resume in
    ->prepare()
  PM / ACPI: Split acpi_lpss_suspend_late|resume_early()
  PM / ACPI: Provide option to disable direct_complete for ACPI devices
  PM / ACPI: Enable the runtime PM centric approach for system sleep
  PM / ACPI: Avoid runtime resuming device in
    acpi_subsys_suspend|freeze()
  i2c: designware: Deploy the runtime PM centric approach for system
    sleep

 drivers/acpi/acpi_lpss.c                    |  81 ++++++++++----
 drivers/acpi/device_pm.c                    | 164 ++++++++++++++++++++++++----
 drivers/base/power/generic_ops.c            |  23 ----
 drivers/i2c/busses/i2c-designware-platdrv.c |  32 ++----
 include/acpi/acpi_bus.h                     |   1 +
 include/linux/acpi.h                        |   4 +
 include/linux/pm.h                          |   1 -
 7 files changed, 219 insertions(+), 87 deletions(-)

-- 
2.7.4

[PATCH 0/9] PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI

[Ulf Hansson]

The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,
currently suffers from a regression, which easily can be triggerd during system
suspend/resume.

In particular the regression concerns those SoCs, which doesn't have the i2c
device being attached to the ACPI PM domain.

The regression, introduced in commit 8503ff166504 ("i2c: designware: Avoid
unnecessary resuming during system suspend"), takes advantage of the so called
direct_complete path during system sleep, which works well when using the ACPI
PM domain, however as stated, that's not the case for other SoCs.

In this series, patch 1, takes a very simplistic approach to solve the problem
and the change is limited to the i2c driver itself. The aim with this change is
to make it acceptable as a fix for v4.2+.

However, patch 1 is unfortunate also reverting back to the old behaviour,
avoiding the direct_complete path, by always runtime resuming the device in the
device_prepare() phase during system sleep. In many cases this is a waste,
especially when the device could have remained runtime suspended during the
entire system sleep sequence.

The rest of the changes in this series, addresses the above problem, avoiding
to runtime resume the device unless it's really needed. One could have tried
out other local hacks, limited to the i2c driver, but I believe the problem
could exist for other drivers/devices as well, thus I aim for a more generic
solution.

In principle, this series enables the proven runtime PM centric approach to
implement system sleep in the i2c driver. However, to do that the ACPI PM
domain have to collaborate and understand this behaviour. Therefore a number of
changes, patch 2 to patch 8, makes the needed changes to the ACPI PM domain.
In the last change, patch9, the i2c driver converts to the runtime PM centric
approach.

It shall be noted, the behaviour of the ACPI PM domain should remain intact,
still taking benefit of using the direct_complete path during system sleep,
except for those devices that explicitly request it not to.

This series has been tested on an ARM64 Hikey board, not having its i2c device
attach to the ACPI PM domain. This means that the ACPI changes has so far only
been compile tested. Any help in testing on relevant Intel SoCs is greatly
appreciated and so is of course review comments.

The series is based upon Rafael's pm tree and linux-next branch. It's also
available at the following git:

git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_i2c_rpm_path

Kind regards
Ulf Hansson


Ulf Hansson (9):
  i2c: designware: Fix system suspend
  PM / ACPI: Restore acpi_subsys_complete()
  PM / Sleep: Remove pm_complete_with_resume_check()
  PM / ACPI: Split code validating need for runtime resume in
    ->prepare()
  PM / ACPI: Split acpi_lpss_suspend_late|resume_early()
  PM / ACPI: Provide option to disable direct_complete for ACPI devices
  PM / ACPI: Enable the runtime PM centric approach for system sleep
  PM / ACPI: Avoid runtime resuming device in
    acpi_subsys_suspend|freeze()
  i2c: designware: Deploy the runtime PM centric approach for system
    sleep

 drivers/acpi/acpi_lpss.c                    |  81 ++++++++++----
 drivers/acpi/device_pm.c                    | 164 ++++++++++++++++++++++++----
 drivers/base/power/generic_ops.c            |  23 ----
 drivers/i2c/busses/i2c-designware-platdrv.c |  32 ++----
 include/acpi/acpi_bus.h                     |   1 +
 include/linux/acpi.h                        |   4 +
 include/linux/pm.h                          |   1 -
 7 files changed, 219 insertions(+), 87 deletions(-)

-- 
2.7.4

[PATCH 1/9] 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()
callback.

Moreover, under certain circumstances the PM core may unset the
direct_complete flag for a parent device, in case its child device are
being system suspended before. In other words, the PM core doesn't skip
calling the system sleep callbacks, no matter if the device is runtime
suspended or not.

In cases of an i2c slave device, the above situation is triggered.
Unfortunate, this also breaks the assumption that the i2c device is always
runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
invoked, which then leads to a regression.

More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
clk_core_unprepare(), for an already disabled/unprepared clock, leading to
complaints about clocks calls being wrongly balanced.

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

To make a simple fix for this regression, let's runtime resume the i2c
device in the ->prepare() callback, assigned to dw_i2c_plat_prepare(). This
prevents the direct_complete path from being executed by the PM core and
guarantees the dw_i2c_plat_suspend() is called with the i2c device always
being runtime resumed.

Of course this change is suboptimal, because to always force a runtime
resume of the i2c device in ->prepare() is a waste, especially in those
cases when it could have remained runtime suspended during the entire
system sleep sequence. However, to accomplish that behaviour a bigger
change is needed, so defer that to future changes not applicable as fixes
or for stable.

Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
Cc: stable@vger.linux.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d1263b8..2b7fa75 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #ifdef CONFIG_PM_SLEEP
 static int dw_i2c_plat_prepare(struct device *dev)
 {
-	return pm_runtime_suspended(dev);
-}
-
-static void dw_i2c_plat_complete(struct device *dev)
-{
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
+	pm_runtime_resume(dev);
+	return 0;
 }
 #else
 #define dw_i2c_plat_prepare	NULL
-#define dw_i2c_plat_complete	NULL
 #endif
 
 #ifdef CONFIG_PM
@@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
 
 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)
 };
-- 
2.7.4

[PATCH 1/9] 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()
callback.

Moreover, under certain circumstances the PM core may unset the
direct_complete flag for a parent device, in case its child device are
being system suspended before. In other words, the PM core doesn't skip
calling the system sleep callbacks, no matter if the device is runtime
suspended or not.

In cases of an i2c slave device, the above situation is triggered.
Unfortunate, this also breaks the assumption that the i2c device is always
runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
invoked, which then leads to a regression.

More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
clk_core_unprepare(), for an already disabled/unprepared clock, leading to
complaints about clocks calls being wrongly balanced.

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

To make a simple fix for this regression, let's runtime resume the i2c
device in the ->prepare() callback, assigned to dw_i2c_plat_prepare(). This
prevents the direct_complete path from being executed by the PM core and
guarantees the dw_i2c_plat_suspend() is called with the i2c device always
being runtime resumed.

Of course this change is suboptimal, because to always force a runtime
resume of the i2c device in ->prepare() is a waste, especially in those
cases when it could have remained runtime suspended during the entire
system sleep sequence. However, to accomplish that behaviour a bigger
change is needed, so defer that to future changes not applicable as fixes
or for stable.

Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
Cc: stable at vger.linux.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d1263b8..2b7fa75 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #ifdef CONFIG_PM_SLEEP
 static int dw_i2c_plat_prepare(struct device *dev)
 {
-	return pm_runtime_suspended(dev);
-}
-
-static void dw_i2c_plat_complete(struct device *dev)
-{
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
+	pm_runtime_resume(dev);
+	return 0;
 }
 #else
 #define dw_i2c_plat_prepare	NULL
-#define dw_i2c_plat_complete	NULL
 #endif
 
 #ifdef CONFIG_PM
@@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
 
 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)
 };
-- 
2.7.4

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, 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()
> callback.
>
> Moreover, under certain circumstances the PM core may unset the
> direct_complete flag for a parent device, in case its child device are
> being system suspended before. In other words, the PM core doesn't skip
> calling the system sleep callbacks, no matter if the device is runtime
> suspended or not.
>
> In cases of an i2c slave device, the above situation is triggered.
> Unfortunate, this also breaks the assumption that the i2c device is always
> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> invoked, which then leads to a regression.
>
> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> complaints about clocks calls being wrongly balanced.
>
> In cases when the i2c device is attached to the ACPI PM domain, the problem
> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.

Which really is expected to happen, so direct_complete should only be
used along with the ACPI PM domain in this case.

Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
to do the right thing without dw_i2c_plat_prepare() and the return
value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
will only have effect without ACPI PM domain AFAICS.

It looks like commit 8503ff166504 is entirely misguided.

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, 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()
> callback.
>
> Moreover, under certain circumstances the PM core may unset the
> direct_complete flag for a parent device, in case its child device are
> being system suspended before. In other words, the PM core doesn't skip
> calling the system sleep callbacks, no matter if the device is runtime
> suspended or not.
>
> In cases of an i2c slave device, the above situation is triggered.
> Unfortunate, this also breaks the assumption that the i2c device is always
> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> invoked, which then leads to a regression.
>
> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> complaints about clocks calls being wrongly balanced.
>
> In cases when the i2c device is attached to the ACPI PM domain, the problem
> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.

Which really is expected to happen, so direct_complete should only be
used along with the ACPI PM domain in this case.

Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
to do the right thing without dw_i2c_plat_prepare() and the return
value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
will only have effect without ACPI PM domain AFAICS.

It looks like commit 8503ff166504 is entirely misguided.

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Mika Westerberg]

On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, 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()
> > callback.
> >
> > Moreover, under certain circumstances the PM core may unset the
> > direct_complete flag for a parent device, in case its child device are
> > being system suspended before. In other words, the PM core doesn't skip
> > calling the system sleep callbacks, no matter if the device is runtime
> > suspended or not.
> >
> > In cases of an i2c slave device, the above situation is triggered.
> > Unfortunate, this also breaks the assumption that the i2c device is always
> > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> > invoked, which then leads to a regression.
> >
> > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > complaints about clocks calls being wrongly balanced.
> >
> > In cases when the i2c device is attached to the ACPI PM domain, the problem
> > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> 
> Which really is expected to happen, so direct_complete should only be
> used along with the ACPI PM domain in this case.
> 
> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
> to do the right thing without dw_i2c_plat_prepare() and the return
> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
> will only have effect without ACPI PM domain AFAICS.
> 
> It looks like commit 8503ff166504 is entirely misguided.

Indeed it is. At the time I suggested that change I did not really
understand how the direct complete is supposed to be used :-/

Thanks Ulf for taking care of this!

I tested this series on Dell XPS 9350 which has touch panel connected to
I2C and suspend/resume still works fine and I can see the controller
going to D3 when the touch panel is idle.

I can perform more comprehensive testing next week.

[PATCH 1/9] i2c: designware: Fix system suspend

[Mika Westerberg]

On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, 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()
> > callback.
> >
> > Moreover, under certain circumstances the PM core may unset the
> > direct_complete flag for a parent device, in case its child device are
> > being system suspended before. In other words, the PM core doesn't skip
> > calling the system sleep callbacks, no matter if the device is runtime
> > suspended or not.
> >
> > In cases of an i2c slave device, the above situation is triggered.
> > Unfortunate, this also breaks the assumption that the i2c device is always
> > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> > invoked, which then leads to a regression.
> >
> > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > complaints about clocks calls being wrongly balanced.
> >
> > In cases when the i2c device is attached to the ACPI PM domain, the problem
> > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> 
> Which really is expected to happen, so direct_complete should only be
> used along with the ACPI PM domain in this case.
> 
> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
> to do the right thing without dw_i2c_plat_prepare() and the return
> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
> will only have effect without ACPI PM domain AFAICS.
> 
> It looks like commit 8503ff166504 is entirely misguided.

Indeed it is. At the time I suggested that change I did not really
understand how the direct complete is supposed to be used :-/

Thanks Ulf for taking care of this!

I tested this series on Dell XPS 9350 which has touch panel connected to
I2C and suspend/resume still works fine and I can see the controller
going to D3 when the touch panel is idle.

I can perform more comprehensive testing next week.

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/22/2017 01:49 PM, Mika Westerberg wrote:
> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Thanks Ulf for taking care of this!
>
Indeed!

> I tested this series on Dell XPS 9350 which has touch panel connected to
> I2C and suspend/resume still works fine and I can see the controller
> going to D3 when the touch panel is idle.
>
> I can perform more comprehensive testing next week.
>
Unfortunately I'm seeing interrupt storm during suspend/resume on 
platform using PM domain from drivers/acpi/acpi_lpss.c straight after 
this patch. Maybe some timing related as I see it only if I have debug 
messages on (i2c_designware_core.dyndbg=+p). But it occurs only after 
this patch.

Have to dig more deeply next week.

-- 
Jarkko

[PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/22/2017 01:49 PM, Mika Westerberg wrote:
> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Thanks Ulf for taking care of this!
>
Indeed!

> I tested this series on Dell XPS 9350 which has touch panel connected to
> I2C and suspend/resume still works fine and I can see the controller
> going to D3 when the touch panel is idle.
>
> I can perform more comprehensive testing next week.
>
Unfortunately I'm seeing interrupt storm during suspend/resume on 
platform using PM domain from drivers/acpi/acpi_lpss.c straight after 
this patch. Maybe some timing related as I see it only if I have debug 
messages on (i2c_designware_core.dyndbg=+p). But it occurs only after 
this patch.

Have to dig more deeply next week.

-- 
Jarkko

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>>> wrote:
>> Thanks Ulf for taking care of this!
>>
> Indeed!
>
>> I tested this series on Dell XPS 9350 which has touch panel connected to
>> I2C and suspend/resume still works fine and I can see the controller
>> going to D3 when the touch panel is idle.
>>
>> I can perform more comprehensive testing next week.
>>
> Unfortunately I'm seeing interrupt storm during suspend/resume on
> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
> this patch. Maybe some timing related as I see it only if I have debug
> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
> this patch.
>
Sorry the noise, this was bogus. That platform is doing this interrupt 
storm randomly and it occurs also without the patch.

-- 
Jarkko

[PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>>> wrote:
>> Thanks Ulf for taking care of this!
>>
> Indeed!
>
>> I tested this series on Dell XPS 9350 which has touch panel connected to
>> I2C and suspend/resume still works fine and I can see the controller
>> going to D3 when the touch panel is idle.
>>
>> I can perform more comprehensive testing next week.
>>
> Unfortunately I'm seeing interrupt storm during suspend/resume on
> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
> this patch. Maybe some timing related as I see it only if I have debug
> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
> this patch.
>
Sorry the noise, this was bogus. That platform is doing this interrupt 
storm randomly and it occurs also without the patch.

-- 
Jarkko

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
>>
>> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>>>
>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>
>>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>>>> wrote:
>>>
>>> Thanks Ulf for taking care of this!
>>>
>> Indeed!
>>
>>> I tested this series on Dell XPS 9350 which has touch panel connected to
>>> I2C and suspend/resume still works fine and I can see the controller
>>> going to D3 when the touch panel is idle.
>>>
>>> I can perform more comprehensive testing next week.
>>>
>> Unfortunately I'm seeing interrupt storm during suspend/resume on
>> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
>> this patch. Maybe some timing related as I see it only if I have debug
>> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
>> this patch.
>>
> Sorry the noise, this was bogus. That platform is doing this interrupt storm
> randomly and it occurs also without the patch.


Okay, then it seems like we should go with $subject patch, although
allow me to update the changelog and post a new version.

Kind regards
Uffe

[PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
>>
>> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>>>
>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>
>>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>>>> wrote:
>>>
>>> Thanks Ulf for taking care of this!
>>>
>> Indeed!
>>
>>> I tested this series on Dell XPS 9350 which has touch panel connected to
>>> I2C and suspend/resume still works fine and I can see the controller
>>> going to D3 when the touch panel is idle.
>>>
>>> I can perform more comprehensive testing next week.
>>>
>> Unfortunately I'm seeing interrupt storm during suspend/resume on
>> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
>> this patch. Maybe some timing related as I see it only if I have debug
>> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
>> this patch.
>>
> Sorry the noise, this was bogus. That platform is doing this interrupt storm
> randomly and it occurs also without the patch.


Okay, then it seems like we should go with $subject patch, although
allow me to update the changelog and post a new version.

Kind regards
Uffe

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Wednesday, June 28, 2017 04:01:39 PM Ulf Hansson wrote:
> On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> > On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
> >>
> >> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
> >>>
> >>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> >>>>
> >>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
> >>>> wrote:
> >>>
> >>> Thanks Ulf for taking care of this!
> >>>
> >> Indeed!
> >>
> >>> I tested this series on Dell XPS 9350 which has touch panel connected to
> >>> I2C and suspend/resume still works fine and I can see the controller
> >>> going to D3 when the touch panel is idle.
> >>>
> >>> I can perform more comprehensive testing next week.
> >>>
> >> Unfortunately I'm seeing interrupt storm during suspend/resume on
> >> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
> >> this patch. Maybe some timing related as I see it only if I have debug
> >> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
> >> this patch.
> >>
> > Sorry the noise, this was bogus. That platform is doing this interrupt storm
> > randomly and it occurs also without the patch.
> 
> 
> Okay, then it seems like we should go with $subject patch, although
> allow me to update the changelog and post a new version.

But as I said, it would be better to do the pm_runtime_resume() in ->resume
(unless that breaks something), for two reasons.

The first reason is that ->complete is synchronous and ->resume can be done
in parallel with other devices which potentially saves time.  The second reason
is that it wouldn't interfere with direct_complete on systems where that
actually works.

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Wednesday, June 28, 2017 04:01:39 PM Ulf Hansson wrote:
> On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> > On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
> >>
> >> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
> >>>
> >>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> >>>>
> >>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
> >>>> wrote:
> >>>
> >>> Thanks Ulf for taking care of this!
> >>>
> >> Indeed!
> >>
> >>> I tested this series on Dell XPS 9350 which has touch panel connected to
> >>> I2C and suspend/resume still works fine and I can see the controller
> >>> going to D3 when the touch panel is idle.
> >>>
> >>> I can perform more comprehensive testing next week.
> >>>
> >> Unfortunately I'm seeing interrupt storm during suspend/resume on
> >> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
> >> this patch. Maybe some timing related as I see it only if I have debug
> >> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
> >> this patch.
> >>
> > Sorry the noise, this was bogus. That platform is doing this interrupt storm
> > randomly and it occurs also without the patch.
> 
> 
> Okay, then it seems like we should go with $subject patch, although
> allow me to update the changelog and post a new version.

But as I said, it would be better to do the pm_runtime_resume() in ->resume
(unless that breaks something), for two reasons.

The first reason is that ->complete is synchronous and ->resume can be done
in parallel with other devices which potentially saves time.  The second reason
is that it wouldn't interfere with direct_complete on systems where that
actually works.

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 28 June 2017 at 16:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, June 28, 2017 04:01:39 PM Ulf Hansson wrote:
>> On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>> > On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
>> >>
>> >> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>> >>>
>> >>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> >>>>
>> >>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>> >>>> wrote:
>> >>>
>> >>> Thanks Ulf for taking care of this!
>> >>>
>> >> Indeed!
>> >>
>> >>> I tested this series on Dell XPS 9350 which has touch panel connected to
>> >>> I2C and suspend/resume still works fine and I can see the controller
>> >>> going to D3 when the touch panel is idle.
>> >>>
>> >>> I can perform more comprehensive testing next week.
>> >>>
>> >> Unfortunately I'm seeing interrupt storm during suspend/resume on
>> >> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
>> >> this patch. Maybe some timing related as I see it only if I have debug
>> >> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
>> >> this patch.
>> >>
>> > Sorry the noise, this was bogus. That platform is doing this interrupt storm
>> > randomly and it occurs also without the patch.
>>
>>
>> Okay, then it seems like we should go with $subject patch, although
>> allow me to update the changelog and post a new version.
>
> But as I said, it would be better to do the pm_runtime_resume() in ->resume
> (unless that breaks something), for two reasons.
>
> The first reason is that ->complete is synchronous and ->resume can be done
> in parallel with other devices which potentially saves time.  The second reason
> is that it wouldn't interfere with direct_complete on systems where that
> actually works.

Yes, that's good points - and I don't see any obvious reasons to why
it shouldn't work.

Kind regards
Uffe

[PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 28 June 2017 at 16:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, June 28, 2017 04:01:39 PM Ulf Hansson wrote:
>> On 27 June 2017 at 09:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>> > On 06/22/2017 02:16 PM, Jarkko Nikula wrote:
>> >>
>> >> On 06/22/2017 01:49 PM, Mika Westerberg wrote:
>> >>>
>> >>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> >>>>
>> >>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org>
>> >>>> wrote:
>> >>>
>> >>> Thanks Ulf for taking care of this!
>> >>>
>> >> Indeed!
>> >>
>> >>> I tested this series on Dell XPS 9350 which has touch panel connected to
>> >>> I2C and suspend/resume still works fine and I can see the controller
>> >>> going to D3 when the touch panel is idle.
>> >>>
>> >>> I can perform more comprehensive testing next week.
>> >>>
>> >> Unfortunately I'm seeing interrupt storm during suspend/resume on
>> >> platform using PM domain from drivers/acpi/acpi_lpss.c straight after
>> >> this patch. Maybe some timing related as I see it only if I have debug
>> >> messages on (i2c_designware_core.dyndbg=+p). But it occurs only after
>> >> this patch.
>> >>
>> > Sorry the noise, this was bogus. That platform is doing this interrupt storm
>> > randomly and it occurs also without the patch.
>>
>>
>> Okay, then it seems like we should go with $subject patch, although
>> allow me to update the changelog and post a new version.
>
> But as I said, it would be better to do the pm_runtime_resume() in ->resume
> (unless that breaks something), for two reasons.
>
> The first reason is that ->complete is synchronous and ->resume can be done
> in parallel with other devices which potentially saves time.  The second reason
> is that it wouldn't interfere with direct_complete on systems where that
> actually works.

Yes, that's good points - and I don't see any obvious reasons to why
it shouldn't work.

Kind regards
Uffe

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
> > > callback.
> > >
> > > Moreover, under certain circumstances the PM core may unset the
> > > direct_complete flag for a parent device, in case its child device are
> > > being system suspended before. In other words, the PM core doesn't skip
> > > calling the system sleep callbacks, no matter if the device is runtime
> > > suspended or not.
> > >
> > > In cases of an i2c slave device, the above situation is triggered.
> > > Unfortunate, this also breaks the assumption that the i2c device is always
> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> > > invoked, which then leads to a regression.
> > >
> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > > complaints about clocks calls being wrongly balanced.
> > >
> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> > 
> > Which really is expected to happen, so direct_complete should only be
> > used along with the ACPI PM domain in this case.
> > 
> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
> > to do the right thing without dw_i2c_plat_prepare() and the return
> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
> > will only have effect without ACPI PM domain AFAICS.
> > 
> > It looks like commit 8503ff166504 is entirely misguided.
> 
> Indeed it is. At the time I suggested that change I did not really
> understand how the direct complete is supposed to be used :-/

So can we go for a full revert, please, and then fix up things properly?

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
> > > callback.
> > >
> > > Moreover, under certain circumstances the PM core may unset the
> > > direct_complete flag for a parent device, in case its child device are
> > > being system suspended before. In other words, the PM core doesn't skip
> > > calling the system sleep callbacks, no matter if the device is runtime
> > > suspended or not.
> > >
> > > In cases of an i2c slave device, the above situation is triggered.
> > > Unfortunate, this also breaks the assumption that the i2c device is always
> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> > > invoked, which then leads to a regression.
> > >
> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > > complaints about clocks calls being wrongly balanced.
> > >
> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> > 
> > Which really is expected to happen, so direct_complete should only be
> > used along with the ACPI PM domain in this case.
> > 
> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
> > to do the right thing without dw_i2c_plat_prepare() and the return
> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
> > will only have effect without ACPI PM domain AFAICS.
> > 
> > It looks like commit 8503ff166504 is entirely misguided.
> 
> Indeed it is. At the time I suggested that change I did not really
> understand how the direct complete is supposed to be used :-/

So can we go for a full revert, please, and then fix up things properly?

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>> > > callback.
>> > >
>> > > Moreover, under certain circumstances the PM core may unset the
>> > > direct_complete flag for a parent device, in case its child device are
>> > > being system suspended before. In other words, the PM core doesn't skip
>> > > calling the system sleep callbacks, no matter if the device is runtime
>> > > suspended or not.
>> > >
>> > > In cases of an i2c slave device, the above situation is triggered.
>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>> > > invoked, which then leads to a regression.
>> > >
>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> > > complaints about clocks calls being wrongly balanced.
>> > >
>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>> >
>> > Which really is expected to happen, so direct_complete should only be
>> > used along with the ACPI PM domain in this case.
>> >
>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>> > to do the right thing without dw_i2c_plat_prepare() and the return
>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>> > will only have effect without ACPI PM domain AFAICS.
>> >
>> > It looks like commit 8503ff166504 is entirely misguided.
>>
>> Indeed it is. At the time I suggested that change I did not really
>> understand how the direct complete is supposed to be used :-/
>
> So can we go for a full revert, please, and then fix up things properly?

Unfortunate I think a revert is going to make it worse, for the non ACPI case.

Because in that case, unless I am reading the code wrong, I think when
the device is runtime suspended during system sleep, then the system
suspend callback will also be called (because the direct_complete
isn't run), again causing clock unbalance issues.

This makes it worse in that sense, that then you don't even need an
i2c slave to trigger the problem.

Kind regards
Uffe

[PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>> > > callback.
>> > >
>> > > Moreover, under certain circumstances the PM core may unset the
>> > > direct_complete flag for a parent device, in case its child device are
>> > > being system suspended before. In other words, the PM core doesn't skip
>> > > calling the system sleep callbacks, no matter if the device is runtime
>> > > suspended or not.
>> > >
>> > > In cases of an i2c slave device, the above situation is triggered.
>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>> > > invoked, which then leads to a regression.
>> > >
>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> > > complaints about clocks calls being wrongly balanced.
>> > >
>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>> >
>> > Which really is expected to happen, so direct_complete should only be
>> > used along with the ACPI PM domain in this case.
>> >
>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>> > to do the right thing without dw_i2c_plat_prepare() and the return
>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>> > will only have effect without ACPI PM domain AFAICS.
>> >
>> > It looks like commit 8503ff166504 is entirely misguided.
>>
>> Indeed it is. At the time I suggested that change I did not really
>> understand how the direct complete is supposed to be used :-/
>
> So can we go for a full revert, please, and then fix up things properly?

Unfortunate I think a revert is going to make it worse, for the non ACPI case.

Because in that case, unless I am reading the code wrong, I think when
the device is runtime suspended during system sleep, then the system
suspend callback will also be called (because the direct_complete
isn't run), again causing clock unbalance issues.

This makes it worse in that sense, that then you don't even need an
i2c slave to trigger the problem.

Kind regards
Uffe

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>> > > callback.
>>> > >
>>> > > Moreover, under certain circumstances the PM core may unset the
>>> > > direct_complete flag for a parent device, in case its child device are
>>> > > being system suspended before. In other words, the PM core doesn't skip
>>> > > calling the system sleep callbacks, no matter if the device is runtime
>>> > > suspended or not.
>>> > >
>>> > > In cases of an i2c slave device, the above situation is triggered.
>>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>> > > invoked, which then leads to a regression.
>>> > >
>>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>> > > complaints about clocks calls being wrongly balanced.
>>> > >
>>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>> >
>>> > Which really is expected to happen, so direct_complete should only be
>>> > used along with the ACPI PM domain in this case.
>>> >
>>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>> > to do the right thing without dw_i2c_plat_prepare() and the return
>>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>> > will only have effect without ACPI PM domain AFAICS.
>>> >
>>> > It looks like commit 8503ff166504 is entirely misguided.
>>>
>>> Indeed it is. At the time I suggested that change I did not really
>>> understand how the direct complete is supposed to be used :-/
>>
>> So can we go for a full revert, please, and then fix up things properly?
>
> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>
> Because in that case, unless I am reading the code wrong, I think when
> the device is runtime suspended during system sleep, then the system
> suspend callback will also be called (because the direct_complete
> isn't run), again causing clock unbalance issues.
>
> This makes it worse in that sense, that then you don't even need an
> i2c slave to trigger the problem.

In that case your changelog is a bit misleading.

It looks like the commit in question attempted to fix exactly this
issue, but it failed, so it should be replaced with something else
which is what your patch is effectively doing.

IMO you should describe the original problem, explain why that commit
is not sufficient to fix it and then describe the final fix.

Anyway, after reading the changelog it should be clear that things
were broken before the commit in question.

And BTW I'm not really sure how the rest of the series is related to this?

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>> > > callback.
>>> > >
>>> > > Moreover, under certain circumstances the PM core may unset the
>>> > > direct_complete flag for a parent device, in case its child device are
>>> > > being system suspended before. In other words, the PM core doesn't skip
>>> > > calling the system sleep callbacks, no matter if the device is runtime
>>> > > suspended or not.
>>> > >
>>> > > In cases of an i2c slave device, the above situation is triggered.
>>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>> > > invoked, which then leads to a regression.
>>> > >
>>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>> > > complaints about clocks calls being wrongly balanced.
>>> > >
>>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>> >
>>> > Which really is expected to happen, so direct_complete should only be
>>> > used along with the ACPI PM domain in this case.
>>> >
>>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>> > to do the right thing without dw_i2c_plat_prepare() and the return
>>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>> > will only have effect without ACPI PM domain AFAICS.
>>> >
>>> > It looks like commit 8503ff166504 is entirely misguided.
>>>
>>> Indeed it is. At the time I suggested that change I did not really
>>> understand how the direct complete is supposed to be used :-/
>>
>> So can we go for a full revert, please, and then fix up things properly?
>
> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>
> Because in that case, unless I am reading the code wrong, I think when
> the device is runtime suspended during system sleep, then the system
> suspend callback will also be called (because the direct_complete
> isn't run), again causing clock unbalance issues.
>
> This makes it worse in that sense, that then you don't even need an
> i2c slave to trigger the problem.

In that case your changelog is a bit misleading.

It looks like the commit in question attempted to fix exactly this
issue, but it failed, so it should be replaced with something else
which is what your patch is effectively doing.

IMO you should describe the original problem, explain why that commit
is not sufficient to fix it and then describe the final fix.

Anyway, after reading the changelog it should be clear that things
were broken before the commit in question.

And BTW I'm not really sure how the rest of the series is related to this?

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>> > > callback.
>>>> > >
>>>> > > Moreover, under certain circumstances the PM core may unset the
>>>> > > direct_complete flag for a parent device, in case its child device are
>>>> > > being system suspended before. In other words, the PM core doesn't skip
>>>> > > calling the system sleep callbacks, no matter if the device is runtime
>>>> > > suspended or not.
>>>> > >
>>>> > > In cases of an i2c slave device, the above situation is triggered.
>>>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>>>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>> > > invoked, which then leads to a regression.
>>>> > >
>>>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>> > > complaints about clocks calls being wrongly balanced.
>>>> > >
>>>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>> >
>>>> > Which really is expected to happen, so direct_complete should only be
>>>> > used along with the ACPI PM domain in this case.
>>>> >
>>>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>> > to do the right thing without dw_i2c_plat_prepare() and the return
>>>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>> > will only have effect without ACPI PM domain AFAICS.
>>>> >
>>>> > It looks like commit 8503ff166504 is entirely misguided.
>>>>
>>>> Indeed it is. At the time I suggested that change I did not really
>>>> understand how the direct complete is supposed to be used :-/
>>>
>>> So can we go for a full revert, please, and then fix up things properly?
>>
>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>
>> Because in that case, unless I am reading the code wrong, I think when
>> the device is runtime suspended during system sleep, then the system
>> suspend callback will also be called (because the direct_complete
>> isn't run), again causing clock unbalance issues.
>>
>> This makes it worse in that sense, that then you don't even need an
>> i2c slave to trigger the problem.
>
> In that case your changelog is a bit misleading.

Yeah, it is! I didn't realize that until I fully investigated the revert option.

>
> It looks like the commit in question attempted to fix exactly this
> issue, but it failed, so it should be replaced with something else
> which is what your patch is effectively doing.
>
> IMO you should describe the original problem, explain why that commit
> is not sufficient to fix it and then describe the final fix.
>
> Anyway, after reading the changelog it should be clear that things
> were broken before the commit in question.
>
> And BTW I'm not really sure how the rest of the series is related to this?

Going back in history, I realize the system sleep support in this
driver has been broken even before the commit $subject patch intends
to fix.
However it has been working fine for the ACPI case, because of how the
ACPI PM domain manages its devices during system sleep.

The commit in question, adds an improvement to the driver, because it
enables the direct_complete path. For ACPI, that was already working,
but not for the other cases. So to be able to support the similar
improvement as the direct_complete path offers, as that isn't working
for this driver, I tried out using the runtime PM centric path
instead. That is what the rest of the changes in this series takes
care of.

Now, as the system sleep support is broken I wanted to make a simple
fix for that first, via $subject patch. I guess what makes this a bit
confusing is that I shouldn't point to a certain commit, but rather
just add a stable tag and update the changelog accordingly.

Kind regards
Uffe

[PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>> > On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>> > > callback.
>>>> > >
>>>> > > Moreover, under certain circumstances the PM core may unset the
>>>> > > direct_complete flag for a parent device, in case its child device are
>>>> > > being system suspended before. In other words, the PM core doesn't skip
>>>> > > calling the system sleep callbacks, no matter if the device is runtime
>>>> > > suspended or not.
>>>> > >
>>>> > > In cases of an i2c slave device, the above situation is triggered.
>>>> > > Unfortunate, this also breaks the assumption that the i2c device is always
>>>> > > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>> > > invoked, which then leads to a regression.
>>>> > >
>>>> > > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>> > > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>> > > complaints about clocks calls being wrongly balanced.
>>>> > >
>>>> > > In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>> > > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>> > > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>> >
>>>> > Which really is expected to happen, so direct_complete should only be
>>>> > used along with the ACPI PM domain in this case.
>>>> >
>>>> > Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>> > to do the right thing without dw_i2c_plat_prepare() and the return
>>>> > value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>> > will only have effect without ACPI PM domain AFAICS.
>>>> >
>>>> > It looks like commit 8503ff166504 is entirely misguided.
>>>>
>>>> Indeed it is. At the time I suggested that change I did not really
>>>> understand how the direct complete is supposed to be used :-/
>>>
>>> So can we go for a full revert, please, and then fix up things properly?
>>
>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>
>> Because in that case, unless I am reading the code wrong, I think when
>> the device is runtime suspended during system sleep, then the system
>> suspend callback will also be called (because the direct_complete
>> isn't run), again causing clock unbalance issues.
>>
>> This makes it worse in that sense, that then you don't even need an
>> i2c slave to trigger the problem.
>
> In that case your changelog is a bit misleading.

Yeah, it is! I didn't realize that until I fully investigated the revert option.

>
> It looks like the commit in question attempted to fix exactly this
> issue, but it failed, so it should be replaced with something else
> which is what your patch is effectively doing.
>
> IMO you should describe the original problem, explain why that commit
> is not sufficient to fix it and then describe the final fix.
>
> Anyway, after reading the changelog it should be clear that things
> were broken before the commit in question.
>
> And BTW I'm not really sure how the rest of the series is related to this?

Going back in history, I realize the system sleep support in this
driver has been broken even before the commit $subject patch intends
to fix.
However it has been working fine for the ACPI case, because of how the
ACPI PM domain manages its devices during system sleep.

The commit in question, adds an improvement to the driver, because it
enables the direct_complete path. For ACPI, that was already working,
but not for the other cases. So to be able to support the similar
improvement as the direct_complete path offers, as that isn't working
for this driver, I tried out using the runtime PM centric path
instead. That is what the rest of the changes in this series takes
care of.

Now, as the system sleep support is broken I wanted to make a simple
fix for that first, via $subject patch. I guess what makes this a bit
confusing is that I shouldn't point to a certain commit, but rather
just add a stable tag and update the changelog accordingly.

Kind regards
Uffe

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Grygorii Strashko]

On 06/26/2017 11:49 AM, Ulf Hansson wrote:
> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>> callback.
>>>>>>>
>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>> suspended or not.
>>>>>>>
>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>> invoked, which then leads to a regression.
>>>>>>>
>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>
>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>
>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>> used along with the ACPI PM domain in this case.
>>>>>>
>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>
>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>
>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>> understand how the direct complete is supposed to be used :-/
>>>>
>>>> So can we go for a full revert, please, and then fix up things properly?
>>>
>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>
>>> Because in that case, unless I am reading the code wrong, I think when
>>> the device is runtime suspended during system sleep, then the system
>>> suspend callback will also be called (because the direct_complete
>>> isn't run), again causing clock unbalance issues.
>>>
>>> This makes it worse in that sense, that then you don't even need an
>>> i2c slave to trigger the problem.
>>
>> In that case your changelog is a bit misleading.
> 
> Yeah, it is! I didn't realize that until I fully investigated the revert option.
> 
>>
>> It looks like the commit in question attempted to fix exactly this
>> issue, but it failed, so it should be replaced with something else
>> which is what your patch is effectively doing.
>>
>> IMO you should describe the original problem, explain why that commit
>> is not sufficient to fix it and then describe the final fix.
>>
>> Anyway, after reading the changelog it should be clear that things
>> were broken before the commit in question.
>>
>> And BTW I'm not really sure how the rest of the series is related to this?
> 
> Going back in history, I realize the system sleep support in this
> driver has been broken even before the commit $subject patch intends
> to fix.
> However it has been working fine for the ACPI case, because of how the
> ACPI PM domain manages its devices during system sleep.
> 
> The commit in question, adds an improvement to the driver, because it
> enables the direct_complete path. For ACPI, that was already working,
> but not for the other cases. So to be able to support the similar
> improvement as the direct_complete path offers, as that isn't working
> for this driver, I tried out using the runtime PM centric path
> instead. That is what the rest of the changes in this series takes
> care of.
> 
> Now, as the system sleep support is broken I wanted to make a simple
> fix for that first, via $subject patch. I guess what makes this a bit
> confusing is that I shouldn't point to a certain commit, but rather
> just add a stable tag and update the changelog accordingly.
> 

Wouldn't it fix suspend for this driver if you will just replace 
dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
you've done in patch 9?

And, I think, direct_complete path should still work after this also.

-- 
regards,
-grygorii

[PATCH 1/9] i2c: designware: Fix system suspend

[Grygorii Strashko]

On 06/26/2017 11:49 AM, Ulf Hansson wrote:
> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>> callback.
>>>>>>>
>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>> suspended or not.
>>>>>>>
>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>> invoked, which then leads to a regression.
>>>>>>>
>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>
>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>
>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>> used along with the ACPI PM domain in this case.
>>>>>>
>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>
>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>
>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>> understand how the direct complete is supposed to be used :-/
>>>>
>>>> So can we go for a full revert, please, and then fix up things properly?
>>>
>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>
>>> Because in that case, unless I am reading the code wrong, I think when
>>> the device is runtime suspended during system sleep, then the system
>>> suspend callback will also be called (because the direct_complete
>>> isn't run), again causing clock unbalance issues.
>>>
>>> This makes it worse in that sense, that then you don't even need an
>>> i2c slave to trigger the problem.
>>
>> In that case your changelog is a bit misleading.
> 
> Yeah, it is! I didn't realize that until I fully investigated the revert option.
> 
>>
>> It looks like the commit in question attempted to fix exactly this
>> issue, but it failed, so it should be replaced with something else
>> which is what your patch is effectively doing.
>>
>> IMO you should describe the original problem, explain why that commit
>> is not sufficient to fix it and then describe the final fix.
>>
>> Anyway, after reading the changelog it should be clear that things
>> were broken before the commit in question.
>>
>> And BTW I'm not really sure how the rest of the series is related to this?
> 
> Going back in history, I realize the system sleep support in this
> driver has been broken even before the commit $subject patch intends
> to fix.
> However it has been working fine for the ACPI case, because of how the
> ACPI PM domain manages its devices during system sleep.
> 
> The commit in question, adds an improvement to the driver, because it
> enables the direct_complete path. For ACPI, that was already working,
> but not for the other cases. So to be able to support the similar
> improvement as the direct_complete path offers, as that isn't working
> for this driver, I tried out using the runtime PM centric path
> instead. That is what the rest of the changes in this series takes
> care of.
> 
> Now, as the system sleep support is broken I wanted to make a simple
> fix for that first, via $subject patch. I guess what makes this a bit
> confusing is that I shouldn't point to a certain commit, but rather
> just add a stable tag and update the changelog accordingly.
> 

Wouldn't it fix suspend for this driver if you will just replace 
dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
you've done in patch 9?

And, I think, direct_complete path should still work after this also.

-- 
regards,
-grygorii

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 06/26/2017 11:49 AM, Ulf Hansson wrote:

[cut]

>>>> This makes it worse in that sense, that then you don't even need an
>>>> i2c slave to trigger the problem.
>>>
>>> In that case your changelog is a bit misleading.
>>
>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>
>>>
>>> It looks like the commit in question attempted to fix exactly this
>>> issue, but it failed, so it should be replaced with something else
>>> which is what your patch is effectively doing.
>>>
>>> IMO you should describe the original problem, explain why that commit
>>> is not sufficient to fix it and then describe the final fix.
>>>
>>> Anyway, after reading the changelog it should be clear that things
>>> were broken before the commit in question.
>>>
>>> And BTW I'm not really sure how the rest of the series is related to this?
>>
>> Going back in history, I realize the system sleep support in this
>> driver has been broken even before the commit $subject patch intends
>> to fix.
>> However it has been working fine for the ACPI case, because of how the
>> ACPI PM domain manages its devices during system sleep.
>>
>> The commit in question, adds an improvement to the driver, because it
>> enables the direct_complete path. For ACPI, that was already working,
>> but not for the other cases. So to be able to support the similar
>> improvement as the direct_complete path offers, as that isn't working
>> for this driver, I tried out using the runtime PM centric path
>> instead. That is what the rest of the changes in this series takes
>> care of.
>>
>> Now, as the system sleep support is broken I wanted to make a simple
>> fix for that first, via $subject patch. I guess what makes this a bit
>> confusing is that I shouldn't point to a certain commit, but rather
>> just add a stable tag and update the changelog accordingly.

I agree, modulo the below.

>
> Wouldn't it fix suspend for this driver if you will just replace
> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
> you've done in patch 9?
>
> And, I think, direct_complete path should still work after this also.

That's a good point and I was about to mention that.

In any case, even if the pm_runtime_resume() added by the $subject
patch is necessary to start with, it could be added to the ->suspend
callback of the driver instead of the ->complete one, in which case
the ACPI path would not be affected by this change.

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 06/26/2017 11:49 AM, Ulf Hansson wrote:

[cut]

>>>> This makes it worse in that sense, that then you don't even need an
>>>> i2c slave to trigger the problem.
>>>
>>> In that case your changelog is a bit misleading.
>>
>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>
>>>
>>> It looks like the commit in question attempted to fix exactly this
>>> issue, but it failed, so it should be replaced with something else
>>> which is what your patch is effectively doing.
>>>
>>> IMO you should describe the original problem, explain why that commit
>>> is not sufficient to fix it and then describe the final fix.
>>>
>>> Anyway, after reading the changelog it should be clear that things
>>> were broken before the commit in question.
>>>
>>> And BTW I'm not really sure how the rest of the series is related to this?
>>
>> Going back in history, I realize the system sleep support in this
>> driver has been broken even before the commit $subject patch intends
>> to fix.
>> However it has been working fine for the ACPI case, because of how the
>> ACPI PM domain manages its devices during system sleep.
>>
>> The commit in question, adds an improvement to the driver, because it
>> enables the direct_complete path. For ACPI, that was already working,
>> but not for the other cases. So to be able to support the similar
>> improvement as the direct_complete path offers, as that isn't working
>> for this driver, I tried out using the runtime PM centric path
>> instead. That is what the rest of the changes in this series takes
>> care of.
>>
>> Now, as the system sleep support is broken I wanted to make a simple
>> fix for that first, via $subject patch. I guess what makes this a bit
>> confusing is that I shouldn't point to a certain commit, but rather
>> just add a stable tag and update the changelog accordingly.

I agree, modulo the below.

>
> Wouldn't it fix suspend for this driver if you will just replace
> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
> you've done in patch 9?
>
> And, I think, direct_complete path should still work after this also.

That's a good point and I was about to mention that.

In any case, even if the pm_runtime_resume() added by the $subject
patch is necessary to start with, it could be added to the ->suspend
callback of the driver instead of the ->complete one, in which case
the ACPI path would not be affected by this change.

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/27/2017 12:11 AM, Rafael J. Wysocki wrote:
> On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
>>
>> And, I think, direct_complete path should still work after this also.
>
> That's a good point and I was about to mention that.
>
Unfortunately it will cause regression to platforms using 
drivers/acpi/acpi_lpss.c power domain:

http://www.spinics.net/lists/linux-i2c/msg29079.html

-- 
Jarkko

[PATCH 1/9] i2c: designware: Fix system suspend

[Jarkko Nikula]

On 06/27/2017 12:11 AM, Rafael J. Wysocki wrote:
> On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
>>
>> And, I think, direct_complete path should still work after this also.
>
> That's a good point and I was about to mention that.
>
Unfortunately it will cause regression to platforms using 
drivers/acpi/acpi_lpss.c power domain:

http://www.spinics.net/lists/linux-i2c/msg29079.html

-- 
Jarkko

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 11:11 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>
> [cut]
>
>>>>> This makes it worse in that sense, that then you don't even need an
>>>>> i2c slave to trigger the problem.
>>>>
>>>> In that case your changelog is a bit misleading.
>>>
>>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>>
>>>>
>>>> It looks like the commit in question attempted to fix exactly this
>>>> issue, but it failed, so it should be replaced with something else
>>>> which is what your patch is effectively doing.
>>>>
>>>> IMO you should describe the original problem, explain why that commit
>>>> is not sufficient to fix it and then describe the final fix.
>>>>
>>>> Anyway, after reading the changelog it should be clear that things
>>>> were broken before the commit in question.
>>>>
>>>> And BTW I'm not really sure how the rest of the series is related to this?
>>>
>>> Going back in history, I realize the system sleep support in this
>>> driver has been broken even before the commit $subject patch intends
>>> to fix.
>>> However it has been working fine for the ACPI case, because of how the
>>> ACPI PM domain manages its devices during system sleep.
>>>
>>> The commit in question, adds an improvement to the driver, because it
>>> enables the direct_complete path. For ACPI, that was already working,
>>> but not for the other cases. So to be able to support the similar
>>> improvement as the direct_complete path offers, as that isn't working
>>> for this driver, I tried out using the runtime PM centric path
>>> instead. That is what the rest of the changes in this series takes
>>> care of.
>>>
>>> Now, as the system sleep support is broken I wanted to make a simple
>>> fix for that first, via $subject patch. I guess what makes this a bit
>>> confusing is that I shouldn't point to a certain commit, but rather
>>> just add a stable tag and update the changelog accordingly.
>
> I agree, modulo the below.
>
>>
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
>>
>> And, I think, direct_complete path should still work after this also.
>
> That's a good point and I was about to mention that.
>
> In any case, even if the pm_runtime_resume() added by the $subject
> patch is necessary to start with, it could be added to the ->suspend

I meant ->resume, sorry.

> callback of the driver instead of the ->complete one, in which case
> the ACPI path would not be affected by this change.

Thanks,
Rafael

[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 11:11 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>
> [cut]
>
>>>>> This makes it worse in that sense, that then you don't even need an
>>>>> i2c slave to trigger the problem.
>>>>
>>>> In that case your changelog is a bit misleading.
>>>
>>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>>
>>>>
>>>> It looks like the commit in question attempted to fix exactly this
>>>> issue, but it failed, so it should be replaced with something else
>>>> which is what your patch is effectively doing.
>>>>
>>>> IMO you should describe the original problem, explain why that commit
>>>> is not sufficient to fix it and then describe the final fix.
>>>>
>>>> Anyway, after reading the changelog it should be clear that things
>>>> were broken before the commit in question.
>>>>
>>>> And BTW I'm not really sure how the rest of the series is related to this?
>>>
>>> Going back in history, I realize the system sleep support in this
>>> driver has been broken even before the commit $subject patch intends
>>> to fix.
>>> However it has been working fine for the ACPI case, because of how the
>>> ACPI PM domain manages its devices during system sleep.
>>>
>>> The commit in question, adds an improvement to the driver, because it
>>> enables the direct_complete path. For ACPI, that was already working,
>>> but not for the other cases. So to be able to support the similar
>>> improvement as the direct_complete path offers, as that isn't working
>>> for this driver, I tried out using the runtime PM centric path
>>> instead. That is what the rest of the changes in this series takes
>>> care of.
>>>
>>> Now, as the system sleep support is broken I wanted to make a simple
>>> fix for that first, via $subject patch. I guess what makes this a bit
>>> confusing is that I shouldn't point to a certain commit, but rather
>>> just add a stable tag and update the changelog accordingly.
>
> I agree, modulo the below.
>
>>
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
>>
>> And, I think, direct_complete path should still work after this also.
>
> That's a good point and I was about to mention that.
>
> In any case, even if the pm_runtime_resume() added by the $subject
> patch is necessary to start with, it could be added to the ->suspend

I meant ->resume, sorry.

> callback of the driver instead of the ->complete one, in which case
> the ACPI path would not be affected by this change.

Thanks,
Rafael

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 26 June 2017 at 21:39, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>
> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>>> callback.
>>>>>>>>
>>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>>> suspended or not.
>>>>>>>>
>>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>>> invoked, which then leads to a regression.
>>>>>>>>
>>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>>
>>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>>
>>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>>> used along with the ACPI PM domain in this case.
>>>>>>>
>>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>>
>>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>>
>>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>>> understand how the direct complete is supposed to be used :-/
>>>>>
>>>>> So can we go for a full revert, please, and then fix up things properly?
>>>>
>>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>>
>>>> Because in that case, unless I am reading the code wrong, I think when
>>>> the device is runtime suspended during system sleep, then the system
>>>> suspend callback will also be called (because the direct_complete
>>>> isn't run), again causing clock unbalance issues.
>>>>
>>>> This makes it worse in that sense, that then you don't even need an
>>>> i2c slave to trigger the problem.
>>>
>>> In that case your changelog is a bit misleading.
>>
>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>
>>>
>>> It looks like the commit in question attempted to fix exactly this
>>> issue, but it failed, so it should be replaced with something else
>>> which is what your patch is effectively doing.
>>>
>>> IMO you should describe the original problem, explain why that commit
>>> is not sufficient to fix it and then describe the final fix.
>>>
>>> Anyway, after reading the changelog it should be clear that things
>>> were broken before the commit in question.
>>>
>>> And BTW I'm not really sure how the rest of the series is related to this?
>>
>> Going back in history, I realize the system sleep support in this
>> driver has been broken even before the commit $subject patch intends
>> to fix.
>> However it has been working fine for the ACPI case, because of how the
>> ACPI PM domain manages its devices during system sleep.
>>
>> The commit in question, adds an improvement to the driver, because it
>> enables the direct_complete path. For ACPI, that was already working,
>> but not for the other cases. So to be able to support the similar
>> improvement as the direct_complete path offers, as that isn't working
>> for this driver, I tried out using the runtime PM centric path
>> instead. That is what the rest of the changes in this series takes
>> care of.
>>
>> Now, as the system sleep support is broken I wanted to make a simple
>> fix for that first, via $subject patch. I guess what makes this a bit
>> confusing is that I shouldn't point to a certain commit, but rather
>> just add a stable tag and update the changelog accordingly.
>>
>
> Wouldn't it fix suspend for this driver if you will just replace
> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
> you've done in patch 9?

For the non-ACPI case - yes.

For the ACPI case - no. Here we need to adapt the ACPI PM domain
first, as it currently doesn't support the runtime PM centric path for
system sleep.

>
> And, I think, direct_complete path should still work after this also.

I don't understand why we want or need that?

To me it would only make the code in the ACPI PM domain more
complicated, comparing to the changes I have posted in rest of this
series.

Kind regards
Uffe

[PATCH 1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 26 June 2017 at 21:39, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>
> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>>> callback.
>>>>>>>>
>>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>>> suspended or not.
>>>>>>>>
>>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>>> invoked, which then leads to a regression.
>>>>>>>>
>>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>>
>>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>>
>>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>>> used along with the ACPI PM domain in this case.
>>>>>>>
>>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>>
>>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>>
>>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>>> understand how the direct complete is supposed to be used :-/
>>>>>
>>>>> So can we go for a full revert, please, and then fix up things properly?
>>>>
>>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>>
>>>> Because in that case, unless I am reading the code wrong, I think when
>>>> the device is runtime suspended during system sleep, then the system
>>>> suspend callback will also be called (because the direct_complete
>>>> isn't run), again causing clock unbalance issues.
>>>>
>>>> This makes it worse in that sense, that then you don't even need an
>>>> i2c slave to trigger the problem.
>>>
>>> In that case your changelog is a bit misleading.
>>
>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>
>>>
>>> It looks like the commit in question attempted to fix exactly this
>>> issue, but it failed, so it should be replaced with something else
>>> which is what your patch is effectively doing.
>>>
>>> IMO you should describe the original problem, explain why that commit
>>> is not sufficient to fix it and then describe the final fix.
>>>
>>> Anyway, after reading the changelog it should be clear that things
>>> were broken before the commit in question.
>>>
>>> And BTW I'm not really sure how the rest of the series is related to this?
>>
>> Going back in history, I realize the system sleep support in this
>> driver has been broken even before the commit $subject patch intends
>> to fix.
>> However it has been working fine for the ACPI case, because of how the
>> ACPI PM domain manages its devices during system sleep.
>>
>> The commit in question, adds an improvement to the driver, because it
>> enables the direct_complete path. For ACPI, that was already working,
>> but not for the other cases. So to be able to support the similar
>> improvement as the direct_complete path offers, as that isn't working
>> for this driver, I tried out using the runtime PM centric path
>> instead. That is what the rest of the changes in this series takes
>> care of.
>>
>> Now, as the system sleep support is broken I wanted to make a simple
>> fix for that first, via $subject patch. I guess what makes this a bit
>> confusing is that I shouldn't point to a certain commit, but rather
>> just add a stable tag and update the changelog accordingly.
>>
>
> Wouldn't it fix suspend for this driver if you will just replace
> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
> you've done in patch 9?

For the non-ACPI case - yes.

For the ACPI case - no. Here we need to adapt the ACPI PM domain
first, as it currently doesn't support the runtime PM centric path for
system sleep.

>
> And, I think, direct_complete path should still work after this also.

I don't understand why we want or need that?

To me it would only make the code in the ACPI PM domain more
complicated, comparing to the changes I have posted in rest of this
series.

Kind regards
Uffe

Re: [PATCH 1/9] i2c: designware: Fix system suspend

[Grygorii Strashko]

On 06/28/2017 09:31 AM, Ulf Hansson wrote:
> On 26 June 2017 at 21:39, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>>
>>
>> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>>> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>>>> callback.
>>>>>>>>>
>>>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>>>> suspended or not.
>>>>>>>>>
>>>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>>>> invoked, which then leads to a regression.
>>>>>>>>>
>>>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>>>
>>>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>>>
>>>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>>>> used along with the ACPI PM domain in this case.
>>>>>>>>
>>>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>>>
>>>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>>>
>>>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>>>> understand how the direct complete is supposed to be used :-/
>>>>>>
>>>>>> So can we go for a full revert, please, and then fix up things properly?
>>>>>
>>>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>>>
>>>>> Because in that case, unless I am reading the code wrong, I think when
>>>>> the device is runtime suspended during system sleep, then the system
>>>>> suspend callback will also be called (because the direct_complete
>>>>> isn't run), again causing clock unbalance issues.
>>>>>
>>>>> This makes it worse in that sense, that then you don't even need an
>>>>> i2c slave to trigger the problem.
>>>>
>>>> In that case your changelog is a bit misleading.
>>>
>>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>>
>>>>
>>>> It looks like the commit in question attempted to fix exactly this
>>>> issue, but it failed, so it should be replaced with something else
>>>> which is what your patch is effectively doing.
>>>>
>>>> IMO you should describe the original problem, explain why that commit
>>>> is not sufficient to fix it and then describe the final fix.
>>>>
>>>> Anyway, after reading the changelog it should be clear that things
>>>> were broken before the commit in question.
>>>>
>>>> And BTW I'm not really sure how the rest of the series is related to this?
>>>
>>> Going back in history, I realize the system sleep support in this
>>> driver has been broken even before the commit $subject patch intends
>>> to fix.
>>> However it has been working fine for the ACPI case, because of how the
>>> ACPI PM domain manages its devices during system sleep.
>>>
>>> The commit in question, adds an improvement to the driver, because it
>>> enables the direct_complete path. For ACPI, that was already working,
>>> but not for the other cases. So to be able to support the similar
>>> improvement as the direct_complete path offers, as that isn't working
>>> for this driver, I tried out using the runtime PM centric path
>>> instead. That is what the rest of the changes in this series takes
>>> care of.
>>>
>>> Now, as the system sleep support is broken I wanted to make a simple
>>> fix for that first, via $subject patch. I guess what makes this a bit
>>> confusing is that I shouldn't point to a certain commit, but rather
>>> just add a stable tag and update the changelog accordingly.
>>>
>>
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
> 
> For the non-ACPI case - yes.
> 
> For the ACPI case - no. Here we need to adapt the ACPI PM domain
> first, as it currently doesn't support the runtime PM centric path for
> system sleep.

Yeah. 

> 
>>
>> And, I think, direct_complete path should still work after this also.
> 
> I don't understand why we want or need that?
> 
> To me it would only make the code in the ACPI PM domain more
> complicated, comparing to the changes I have posted in rest of this
> series.
> 

I'm not sure about benefits with this particular driver - everything
depend on power on/off latencies, if they are big direct_complete make sense.

The main point of my comment was based on fact that dw_i2c driver uses
the same function for PM runtime and System suspend callbacks, but this is
definitely wrong (ACPI or not), because PM runtime and System suspend
are not synchronized.

-- 
regards,
-grygorii

[PATCH 1/9] i2c: designware: Fix system suspend

[Grygorii Strashko]

On 06/28/2017 09:31 AM, Ulf Hansson wrote:
> On 26 June 2017 at 21:39, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>>
>>
>> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>>> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, 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()
>>>>>>>>> callback.
>>>>>>>>>
>>>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>>>> suspended or not.
>>>>>>>>>
>>>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>>>> invoked, which then leads to a regression.
>>>>>>>>>
>>>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>>>
>>>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>>>
>>>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>>>> used along with the ACPI PM domain in this case.
>>>>>>>>
>>>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>>>
>>>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>>>
>>>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>>>> understand how the direct complete is supposed to be used :-/
>>>>>>
>>>>>> So can we go for a full revert, please, and then fix up things properly?
>>>>>
>>>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>>>
>>>>> Because in that case, unless I am reading the code wrong, I think when
>>>>> the device is runtime suspended during system sleep, then the system
>>>>> suspend callback will also be called (because the direct_complete
>>>>> isn't run), again causing clock unbalance issues.
>>>>>
>>>>> This makes it worse in that sense, that then you don't even need an
>>>>> i2c slave to trigger the problem.
>>>>
>>>> In that case your changelog is a bit misleading.
>>>
>>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>>
>>>>
>>>> It looks like the commit in question attempted to fix exactly this
>>>> issue, but it failed, so it should be replaced with something else
>>>> which is what your patch is effectively doing.
>>>>
>>>> IMO you should describe the original problem, explain why that commit
>>>> is not sufficient to fix it and then describe the final fix.
>>>>
>>>> Anyway, after reading the changelog it should be clear that things
>>>> were broken before the commit in question.
>>>>
>>>> And BTW I'm not really sure how the rest of the series is related to this?
>>>
>>> Going back in history, I realize the system sleep support in this
>>> driver has been broken even before the commit $subject patch intends
>>> to fix.
>>> However it has been working fine for the ACPI case, because of how the
>>> ACPI PM domain manages its devices during system sleep.
>>>
>>> The commit in question, adds an improvement to the driver, because it
>>> enables the direct_complete path. For ACPI, that was already working,
>>> but not for the other cases. So to be able to support the similar
>>> improvement as the direct_complete path offers, as that isn't working
>>> for this driver, I tried out using the runtime PM centric path
>>> instead. That is what the rest of the changes in this series takes
>>> care of.
>>>
>>> Now, as the system sleep support is broken I wanted to make a simple
>>> fix for that first, via $subject patch. I guess what makes this a bit
>>> confusing is that I shouldn't point to a certain commit, but rather
>>> just add a stable tag and update the changelog accordingly.
>>>
>>
>> Wouldn't it fix suspend for this driver if you will just replace
>> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
>> you've done in patch 9?
> 
> For the non-ACPI case - yes.
> 
> For the ACPI case - no. Here we need to adapt the ACPI PM domain
> first, as it currently doesn't support the runtime PM centric path for
> system sleep.

Yeah. 

> 
>>
>> And, I think, direct_complete path should still work after this also.
> 
> I don't understand why we want or need that?
> 
> To me it would only make the code in the ACPI PM domain more
> complicated, comparing to the changes I have posted in rest of this
> series.
> 

I'm not sure about benefits with this particular driver - everything
depend on power on/off latencies, if they are big direct_complete make sense.

The main point of my comment was based on fact that dw_i2c driver uses
the same function for PM runtime and System suspend callbacks, but this is
definitely wrong (ACPI or not), because PM runtime and System suspend
are not synchronized.

-- 
regards,
-grygorii

Re: [1/9] i2c: designware: Fix system suspend

[Wangtao (Kevin, Kirin)]

This patch can fix the issue we found on hikey960 that i2c doesn't skip system suspend when it is runtime suspended.

在 2017/6/22 3:21, 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()
> callback.
> 
> Moreover, under certain circumstances the PM core may unset the
> direct_complete flag for a parent device, in case its child device are
> being system suspended before. In other words, the PM core doesn't skip
> calling the system sleep callbacks, no matter if the device is runtime
> suspended or not.
> 
> In cases of an i2c slave device, the above situation is triggered.
> Unfortunate, this also breaks the assumption that the i2c device is always
> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> invoked, which then leads to a regression.
> 
> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> complaints about clocks calls being wrongly balanced.
> 
> In cases when the i2c device is attached to the ACPI PM domain, the problem
> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> 
> To make a simple fix for this regression, let's runtime resume the i2c
> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare(). This
> prevents the direct_complete path from being executed by the PM core and
> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
> being runtime resumed.
> 
> Of course this change is suboptimal, because to always force a runtime
> resume of the i2c device in ->prepare() is a waste, especially in those
> cases when it could have remained runtime suspended during the entire
> system sleep sequence. However, to accomplish that behaviour a bigger
> change is needed, so defer that to future changes not applicable as fixes
> or for stable.
> 
> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
> Cc: stable@vger.linux.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d1263b8..2b7fa75 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>   #ifdef CONFIG_PM_SLEEP
>   static int dw_i2c_plat_prepare(struct device *dev)
>   {
> -	return pm_runtime_suspended(dev);
> -}
> -
> -static void dw_i2c_plat_complete(struct device *dev)
> -{
> -	if (dev->power.direct_complete)
> -		pm_request_resume(dev);
> +	pm_runtime_resume(dev);
> +	return 0;
>   }
>   #else
>   #define dw_i2c_plat_prepare	NULL
> -#define dw_i2c_plat_complete	NULL
>   #endif
>   
>   #ifdef CONFIG_PM
> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>   
>   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)
>   };
> 

[1/9] i2c: designware: Fix system suspend

[Wangtao (Kevin, Kirin)]

This patch can fix the issue we found on hikey960 that i2c doesn't skip system suspend when it is runtime suspended.

? 2017/6/22 3:21, 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()
> callback.
> 
> Moreover, under certain circumstances the PM core may unset the
> direct_complete flag for a parent device, in case its child device are
> being system suspended before. In other words, the PM core doesn't skip
> calling the system sleep callbacks, no matter if the device is runtime
> suspended or not.
> 
> In cases of an i2c slave device, the above situation is triggered.
> Unfortunate, this also breaks the assumption that the i2c device is always
> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> invoked, which then leads to a regression.
> 
> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> complaints about clocks calls being wrongly balanced.
> 
> In cases when the i2c device is attached to the ACPI PM domain, the problem
> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
> 
> To make a simple fix for this regression, let's runtime resume the i2c
> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare(). This
> prevents the direct_complete path from being executed by the PM core and
> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
> being runtime resumed.
> 
> Of course this change is suboptimal, because to always force a runtime
> resume of the i2c device in ->prepare() is a waste, especially in those
> cases when it could have remained runtime suspended during the entire
> system sleep sequence. However, to accomplish that behaviour a bigger
> change is needed, so defer that to future changes not applicable as fixes
> or for stable.
> 
> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
> Cc: stable at vger.linux.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d1263b8..2b7fa75 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>   #ifdef CONFIG_PM_SLEEP
>   static int dw_i2c_plat_prepare(struct device *dev)
>   {
> -	return pm_runtime_suspended(dev);
> -}
> -
> -static void dw_i2c_plat_complete(struct device *dev)
> -{
> -	if (dev->power.direct_complete)
> -		pm_request_resume(dev);
> +	pm_runtime_resume(dev);
> +	return 0;
>   }
>   #else
>   #define dw_i2c_plat_prepare	NULL
> -#define dw_i2c_plat_complete	NULL
>   #endif
>   
>   #ifdef CONFIG_PM
> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>   
>   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)
>   };
> 

Re: [1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 8 September 2017 at 05:23, Wangtao (Kevin, Kirin)
<kevin.wangtao@hisilicon.com> wrote:
>
> This patch can fix the issue we found on hikey960 that i2c doesn't skip
> system suspend when it is runtime suspended.

We decided to go with a slightly different version. You may try out
the below and see if that also fixes your problem.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a23318feeff662c8d25d21623daebdd2e55ec221

Kind regards
Uffe

>
>
> 在 2017/6/22 3:21, 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()
>> callback.
>>
>> Moreover, under certain circumstances the PM core may unset the
>> direct_complete flag for a parent device, in case its child device are
>> being system suspended before. In other words, the PM core doesn't skip
>> calling the system sleep callbacks, no matter if the device is runtime
>> suspended or not.
>>
>> In cases of an i2c slave device, the above situation is triggered.
>> Unfortunate, this also breaks the assumption that the i2c device is always
>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>> invoked, which then leads to a regression.
>>
>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> complaints about clocks calls being wrongly balanced.
>>
>> In cases when the i2c device is attached to the ACPI PM domain, the
>> problem
>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>
>> To make a simple fix for this regression, let's runtime resume the i2c
>> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare().
>> This
>> prevents the direct_complete path from being executed by the PM core and
>> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
>> being runtime resumed.
>>
>> Of course this change is suboptimal, because to always force a runtime
>> resume of the i2c device in ->prepare() is a waste, especially in those
>> cases when it could have remained runtime suspended during the entire
>> system sleep sequence. However, to accomplish that behaviour a bigger
>> change is needed, so defer that to future changes not applicable as fixes
>> or for stable.
>>
>> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
>> Cc: stable@vger.linux.org
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>>   1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index d1263b8..2b7fa75 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>>   #ifdef CONFIG_PM_SLEEP
>>   static int dw_i2c_plat_prepare(struct device *dev)
>>   {
>> -       return pm_runtime_suspended(dev);
>> -}
>> -
>> -static void dw_i2c_plat_complete(struct device *dev)
>> -{
>> -       if (dev->power.direct_complete)
>> -               pm_request_resume(dev);
>> +       pm_runtime_resume(dev);
>> +       return 0;
>>   }
>>   #else
>>   #define dw_i2c_plat_prepare   NULL
>> -#define dw_i2c_plat_complete   NULL
>>   #endif
>>     #ifdef CONFIG_PM
>> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>>     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)
>>   };
>>
>

[1/9] i2c: designware: Fix system suspend

[Ulf Hansson]

On 8 September 2017 at 05:23, Wangtao (Kevin, Kirin)
<kevin.wangtao@hisilicon.com> wrote:
>
> This patch can fix the issue we found on hikey960 that i2c doesn't skip
> system suspend when it is runtime suspended.

We decided to go with a slightly different version. You may try out
the below and see if that also fixes your problem.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a23318feeff662c8d25d21623daebdd2e55ec221

Kind regards
Uffe

>
>
> ? 2017/6/22 3:21, 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()
>> callback.
>>
>> Moreover, under certain circumstances the PM core may unset the
>> direct_complete flag for a parent device, in case its child device are
>> being system suspended before. In other words, the PM core doesn't skip
>> calling the system sleep callbacks, no matter if the device is runtime
>> suspended or not.
>>
>> In cases of an i2c slave device, the above situation is triggered.
>> Unfortunate, this also breaks the assumption that the i2c device is always
>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>> invoked, which then leads to a regression.
>>
>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> complaints about clocks calls being wrongly balanced.
>>
>> In cases when the i2c device is attached to the ACPI PM domain, the
>> problem
>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>
>> To make a simple fix for this regression, let's runtime resume the i2c
>> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare().
>> This
>> prevents the direct_complete path from being executed by the PM core and
>> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
>> being runtime resumed.
>>
>> Of course this change is suboptimal, because to always force a runtime
>> resume of the i2c device in ->prepare() is a waste, especially in those
>> cases when it could have remained runtime suspended during the entire
>> system sleep sequence. However, to accomplish that behaviour a bigger
>> change is needed, so defer that to future changes not applicable as fixes
>> or for stable.
>>
>> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
>> Cc: stable at vger.linux.org
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>>   1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index d1263b8..2b7fa75 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>>   #ifdef CONFIG_PM_SLEEP
>>   static int dw_i2c_plat_prepare(struct device *dev)
>>   {
>> -       return pm_runtime_suspended(dev);
>> -}
>> -
>> -static void dw_i2c_plat_complete(struct device *dev)
>> -{
>> -       if (dev->power.direct_complete)
>> -               pm_request_resume(dev);
>> +       pm_runtime_resume(dev);
>> +       return 0;
>>   }
>>   #else
>>   #define dw_i2c_plat_prepare   NULL
>> -#define dw_i2c_plat_complete   NULL
>>   #endif
>>     #ifdef CONFIG_PM
>> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>>     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)
>>   };
>>
>

Re: [1/9] i2c: designware: Fix system suspend

[Wangtao (Kevin, Kirin)]

OK, this patch can also fix the problem.
On 2017/9/8 16:29, Ulf Hansson wrote:
> On 8 September 2017 at 05:23, Wangtao (Kevin, Kirin)
> <kevin.wangtao@hisilicon.com> wrote:
>>
>> This patch can fix the issue we found on hikey960 that i2c doesn't skip
>> system suspend when it is runtime suspended.
> 
> We decided to go with a slightly different version. You may try out
> the below and see if that also fixes your problem.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a23318feeff662c8d25d21623daebdd2e55ec221
> 
> Kind regards
> Uffe
> 
>>
>>
>> 在 2017/6/22 3:21, 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()
>>> callback.
>>>
>>> Moreover, under certain circumstances the PM core may unset the
>>> direct_complete flag for a parent device, in case its child device are
>>> being system suspended before. In other words, the PM core doesn't skip
>>> calling the system sleep callbacks, no matter if the device is runtime
>>> suspended or not.
>>>
>>> In cases of an i2c slave device, the above situation is triggered.
>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>> invoked, which then leads to a regression.
>>>
>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>> complaints about clocks calls being wrongly balanced.
>>>
>>> In cases when the i2c device is attached to the ACPI PM domain, the
>>> problem
>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>
>>> To make a simple fix for this regression, let's runtime resume the i2c
>>> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare().
>>> This
>>> prevents the direct_complete path from being executed by the PM core and
>>> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
>>> being runtime resumed.
>>>
>>> Of course this change is suboptimal, because to always force a runtime
>>> resume of the i2c device in ->prepare() is a waste, especially in those
>>> cases when it could have remained runtime suspended during the entire
>>> system sleep sequence. However, to accomplish that behaviour a bigger
>>> change is needed, so defer that to future changes not applicable as fixes
>>> or for stable.
>>>
>>> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
>>> Cc: stable@vger.linux.org
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>    drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>>>    1 file changed, 2 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d1263b8..2b7fa75 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>>>    #ifdef CONFIG_PM_SLEEP
>>>    static int dw_i2c_plat_prepare(struct device *dev)
>>>    {
>>> -       return pm_runtime_suspended(dev);
>>> -}
>>> -
>>> -static void dw_i2c_plat_complete(struct device *dev)
>>> -{
>>> -       if (dev->power.direct_complete)
>>> -               pm_request_resume(dev);
>>> +       pm_runtime_resume(dev);
>>> +       return 0;
>>>    }
>>>    #else
>>>    #define dw_i2c_plat_prepare   NULL
>>> -#define dw_i2c_plat_complete   NULL
>>>    #endif
>>>      #ifdef CONFIG_PM
>>> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>>>      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)
>>>    };
>>>
>>
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1/9] i2c: designware: Fix system suspend

[Wangtao (Kevin, Kirin)]

OK, this patch can also fix the problem.
On 2017/9/8 16:29, Ulf Hansson wrote:
> On 8 September 2017 at 05:23, Wangtao (Kevin, Kirin)
> <kevin.wangtao@hisilicon.com> wrote:
>>
>> This patch can fix the issue we found on hikey960 that i2c doesn't skip
>> system suspend when it is runtime suspended.
> 
> We decided to go with a slightly different version. You may try out
> the below and see if that also fixes your problem.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a23318feeff662c8d25d21623daebdd2e55ec221
> 
> Kind regards
> Uffe
> 
>>
>>
>> ? 2017/6/22 3:21, 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()
>>> callback.
>>>
>>> Moreover, under certain circumstances the PM core may unset the
>>> direct_complete flag for a parent device, in case its child device are
>>> being system suspended before. In other words, the PM core doesn't skip
>>> calling the system sleep callbacks, no matter if the device is runtime
>>> suspended or not.
>>>
>>> In cases of an i2c slave device, the above situation is triggered.
>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>> invoked, which then leads to a regression.
>>>
>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>> complaints about clocks calls being wrongly balanced.
>>>
>>> In cases when the i2c device is attached to the ACPI PM domain, the
>>> problem
>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>
>>> To make a simple fix for this regression, let's runtime resume the i2c
>>> device in the ->prepare() callback, assigned to dw_i2c_plat_prepare().
>>> This
>>> prevents the direct_complete path from being executed by the PM core and
>>> guarantees the dw_i2c_plat_suspend() is called with the i2c device always
>>> being runtime resumed.
>>>
>>> Of course this change is suboptimal, because to always force a runtime
>>> resume of the i2c device in ->prepare() is a waste, especially in those
>>> cases when it could have remained runtime suspended during the entire
>>> system sleep sequence. However, to accomplish that behaviour a bigger
>>> change is needed, so defer that to future changes not applicable as fixes
>>> or for stable.
>>>
>>> Fixes: 8503ff166504 ("i2c: designware: Avoid unnecessary resuming...")
>>> Cc: stable at vger.linux.org
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>    drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++---------
>>>    1 file changed, 2 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d1263b8..2b7fa75 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -375,17 +375,11 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>>>    #ifdef CONFIG_PM_SLEEP
>>>    static int dw_i2c_plat_prepare(struct device *dev)
>>>    {
>>> -       return pm_runtime_suspended(dev);
>>> -}
>>> -
>>> -static void dw_i2c_plat_complete(struct device *dev)
>>> -{
>>> -       if (dev->power.direct_complete)
>>> -               pm_request_resume(dev);
>>> +       pm_runtime_resume(dev);
>>> +       return 0;
>>>    }
>>>    #else
>>>    #define dw_i2c_plat_prepare   NULL
>>> -#define dw_i2c_plat_complete   NULL
>>>    #endif
>>>      #ifdef CONFIG_PM
>>> @@ -413,7 +407,6 @@ static int dw_i2c_plat_resume(struct device *dev)
>>>      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)
>>>    };
>>>
>>
> 
> .
> 

[PATCH 2/9] PM / ACPI: Restore acpi_subsys_complete()

[Ulf Hansson]

The commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have
been reset by firmware"), made PCI's and ACPI's ->complete() callbacks to
be assigned to a new API called pm_complete_with_resume_check(), which was
introduced in the same change.

Later it turned out that using pm_complete_with_resume_check() isn't good
enough for PCI, as it needs additional PCI specific checks, before deciding
whether runtime resuming the device is needed when running the ->complete()
callback.

This leaves ACPI being the only user of pm_complete_with_resume_check().
Therefore let's restore ACPI's acpi_subsys_complete(), which was dropped in
commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have been
reset by firmware").

This enables us to remove the pm_complete_with_resume_check() API in a
following change, but it also enables ACPI to add more ACPI specific checks
in acpi_subsys_complete() if it turns out that is needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c |  2 +-
 drivers/acpi/device_pm.c | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 10347e3..d7b6778 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -879,7 +879,7 @@ static struct dev_pm_domain acpi_lpss_pm_domain = {
 #ifdef CONFIG_PM
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_lpss_suspend_late,
 		.resume_early = acpi_lpss_resume_early,
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ca02102..d38acbc 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -964,6 +964,23 @@ int acpi_subsys_prepare(struct device *dev)
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
+ * acpi_subsys_complete - Finalize device's resume during system resume.
+ * @dev: Device to handle.
+ */
+void acpi_subsys_complete(struct device *dev)
+{
+	pm_generic_complete(dev);
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete && pm_resume_via_firmware())
+		pm_request_resume(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_complete);
+
+/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -1031,7 +1048,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
 		.runtime_resume = acpi_subsys_runtime_resume,
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,
-- 
2.7.4

[PATCH 2/9] PM / ACPI: Restore acpi_subsys_complete()

[Ulf Hansson]

The commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have
been reset by firmware"), made PCI's and ACPI's ->complete() callbacks to
be assigned to a new API called pm_complete_with_resume_check(), which was
introduced in the same change.

Later it turned out that using pm_complete_with_resume_check() isn't good
enough for PCI, as it needs additional PCI specific checks, before deciding
whether runtime resuming the device is needed when running the ->complete()
callback.

This leaves ACPI being the only user of pm_complete_with_resume_check().
Therefore let's restore ACPI's acpi_subsys_complete(), which was dropped in
commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have been
reset by firmware").

This enables us to remove the pm_complete_with_resume_check() API in a
following change, but it also enables ACPI to add more ACPI specific checks
in acpi_subsys_complete() if it turns out that is needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c |  2 +-
 drivers/acpi/device_pm.c | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 10347e3..d7b6778 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -879,7 +879,7 @@ static struct dev_pm_domain acpi_lpss_pm_domain = {
 #ifdef CONFIG_PM
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_lpss_suspend_late,
 		.resume_early = acpi_lpss_resume_early,
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ca02102..d38acbc 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -964,6 +964,23 @@ int acpi_subsys_prepare(struct device *dev)
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
+ * acpi_subsys_complete - Finalize device's resume during system resume.
+ * @dev: Device to handle.
+ */
+void acpi_subsys_complete(struct device *dev)
+{
+	pm_generic_complete(dev);
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete && pm_resume_via_firmware())
+		pm_request_resume(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_complete);
+
+/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -1031,7 +1048,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
 		.runtime_resume = acpi_subsys_runtime_resume,
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,
-- 
2.7.4

[PATCH 3/9] PM / Sleep: Remove pm_complete_with_resume_check()

[Ulf Hansson]

According to recent changes for ACPI, the are longer any users of
pm_complete_with_resume_check(), thus let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c | 23 -----------------------
 include/linux/pm.h               |  1 -
 2 files changed, 24 deletions(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 07c3c4a..b2ed606 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -9,7 +9,6 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
-#include <linux/suspend.h>
 
 #ifdef CONFIG_PM
 /**
@@ -298,26 +297,4 @@ void pm_generic_complete(struct device *dev)
 	if (drv && drv->pm && drv->pm->complete)
 		drv->pm->complete(dev);
 }
-
-/**
- * pm_complete_with_resume_check - Complete a device power transition.
- * @dev: Device to handle.
- *
- * Complete a device power transition during a system-wide power transition and
- * optionally schedule a runtime resume of the device if the system resume in
- * progress has been initated by the platform firmware and the device had its
- * power.direct_complete flag set.
- */
-void pm_complete_with_resume_check(struct device *dev)
-{
-	pm_generic_complete(dev);
-	/*
-	 * If the device had been runtime-suspended before the system went into
-	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
-	 */
-	if (dev->power.direct_complete && pm_resume_via_firmware())
-		pm_request_resume(dev);
-}
-EXPORT_SYMBOL_GPL(pm_complete_with_resume_check);
 #endif /* CONFIG_PM_SLEEP */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a0894bc..de28313 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -733,7 +733,6 @@ extern int pm_generic_poweroff_noirq(struct device *dev);
 extern int pm_generic_poweroff_late(struct device *dev);
 extern int pm_generic_poweroff(struct device *dev);
 extern void pm_generic_complete(struct device *dev);
-extern void pm_complete_with_resume_check(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 
-- 
2.7.4

[PATCH 3/9] PM / Sleep: Remove pm_complete_with_resume_check()

[Ulf Hansson]

According to recent changes for ACPI, the are longer any users of
pm_complete_with_resume_check(), thus let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c | 23 -----------------------
 include/linux/pm.h               |  1 -
 2 files changed, 24 deletions(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 07c3c4a..b2ed606 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -9,7 +9,6 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
-#include <linux/suspend.h>
 
 #ifdef CONFIG_PM
 /**
@@ -298,26 +297,4 @@ void pm_generic_complete(struct device *dev)
 	if (drv && drv->pm && drv->pm->complete)
 		drv->pm->complete(dev);
 }
-
-/**
- * pm_complete_with_resume_check - Complete a device power transition.
- * @dev: Device to handle.
- *
- * Complete a device power transition during a system-wide power transition and
- * optionally schedule a runtime resume of the device if the system resume in
- * progress has been initated by the platform firmware and the device had its
- * power.direct_complete flag set.
- */
-void pm_complete_with_resume_check(struct device *dev)
-{
-	pm_generic_complete(dev);
-	/*
-	 * If the device had been runtime-suspended before the system went into
-	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
-	 */
-	if (dev->power.direct_complete && pm_resume_via_firmware())
-		pm_request_resume(dev);
-}
-EXPORT_SYMBOL_GPL(pm_complete_with_resume_check);
 #endif /* CONFIG_PM_SLEEP */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a0894bc..de28313 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -733,7 +733,6 @@ extern int pm_generic_poweroff_noirq(struct device *dev);
 extern int pm_generic_poweroff_late(struct device *dev);
 extern int pm_generic_poweroff(struct device *dev);
 extern void pm_generic_complete(struct device *dev);
-extern void pm_complete_with_resume_check(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 
-- 
2.7.4

[PATCH 4/9] PM / ACPI: Split code validating need for runtime resume in ->prepare()

[Ulf Hansson]

Move the code dealing with validation of whether runtime resuming the
device is needed during system suspend.

In this way it becomes more clear for what circumstances ACPI is prevented
from trying the direct_complete path.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index d38acbc..ee51e75 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -933,6 +933,27 @@ int acpi_dev_resume_early(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 
+static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
+{
+	u32 sys_target = acpi_target_system_state();
+	int ret, state;
+
+	if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		return true;
+
+	if (sys_target == ACPI_STATE_S0)
+		return false;
+
+	if (adev->power.flags.dsw_present)
+		return true;
+
+	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+	if (ret)
+		return true;
+
+	return !(state == adev->power.state);
+}
+
 /**
  * acpi_subsys_prepare - Prepare device for system transition to a sleep state.
  * @dev: Device to prepare.
@@ -940,26 +961,16 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 int acpi_subsys_prepare(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
-	u32 sys_target;
-	int ret, state;
+	int ret;
 
 	ret = pm_generic_prepare(dev);
 	if (ret < 0)
 		return ret;
 
-	if (!adev || !pm_runtime_suspended(dev)
-	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
-		return 0;
-
-	sys_target = acpi_target_system_state();
-	if (sys_target == ACPI_STATE_S0)
-		return 1;
-
-	if (adev->power.flags.dsw_present)
+	if (!adev || !pm_runtime_suspended(dev))
 		return 0;
 
-	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
-	return !ret && state == adev->power.state;
+	return !acpi_dev_needs_resume(dev, adev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
-- 
2.7.4

[PATCH 4/9] PM / ACPI: Split code validating need for runtime resume in ->prepare()

[Ulf Hansson]

Move the code dealing with validation of whether runtime resuming the
device is needed during system suspend.

In this way it becomes more clear for what circumstances ACPI is prevented
from trying the direct_complete path.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index d38acbc..ee51e75 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -933,6 +933,27 @@ int acpi_dev_resume_early(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 
+static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
+{
+	u32 sys_target = acpi_target_system_state();
+	int ret, state;
+
+	if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		return true;
+
+	if (sys_target == ACPI_STATE_S0)
+		return false;
+
+	if (adev->power.flags.dsw_present)
+		return true;
+
+	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+	if (ret)
+		return true;
+
+	return !(state == adev->power.state);
+}
+
 /**
  * acpi_subsys_prepare - Prepare device for system transition to a sleep state.
  * @dev: Device to prepare.
@@ -940,26 +961,16 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 int acpi_subsys_prepare(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
-	u32 sys_target;
-	int ret, state;
+	int ret;
 
 	ret = pm_generic_prepare(dev);
 	if (ret < 0)
 		return ret;
 
-	if (!adev || !pm_runtime_suspended(dev)
-	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
-		return 0;
-
-	sys_target = acpi_target_system_state();
-	if (sys_target == ACPI_STATE_S0)
-		return 1;
-
-	if (adev->power.flags.dsw_present)
+	if (!adev || !pm_runtime_suspended(dev))
 		return 0;
 
-	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
-	return !ret && state == adev->power.state;
+	return !acpi_dev_needs_resume(dev, adev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
-- 
2.7.4

Re: [PATCH 4/9] PM / ACPI: Split code validating need for runtime resume in ->prepare()

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Move the code dealing with validation of whether runtime resuming the
> device is needed during system suspend.
>
> In this way it becomes more clear for what circumstances ACPI is prevented
> from trying the direct_complete path.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index d38acbc..ee51e75 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -933,6 +933,27 @@ int acpi_dev_resume_early(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
>
> +static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
> +{
> +       u32 sys_target = acpi_target_system_state();
> +       int ret, state;
> +
> +       if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
> +               return true;
> +
> +       if (sys_target == ACPI_STATE_S0)
> +               return false;
> +
> +       if (adev->power.flags.dsw_present)
> +               return true;
> +
> +       ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
> +       if (ret)
> +               return true;
> +
> +       return !(state == adev->power.state);

Nit: I would write this as

  return state != adev->power.state;

Thanks,
Rafael

[PATCH 4/9] PM / ACPI: Split code validating need for runtime resume in ->prepare()

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Move the code dealing with validation of whether runtime resuming the
> device is needed during system suspend.
>
> In this way it becomes more clear for what circumstances ACPI is prevented
> from trying the direct_complete path.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index d38acbc..ee51e75 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -933,6 +933,27 @@ int acpi_dev_resume_early(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
>
> +static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
> +{
> +       u32 sys_target = acpi_target_system_state();
> +       int ret, state;
> +
> +       if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
> +               return true;
> +
> +       if (sys_target == ACPI_STATE_S0)
> +               return false;
> +
> +       if (adev->power.flags.dsw_present)
> +               return true;
> +
> +       ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
> +       if (ret)
> +               return true;
> +
> +       return !(state == adev->power.state);

Nit: I would write this as

  return state != adev->power.state;

Thanks,
Rafael

[PATCH 5/9] PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

[Ulf Hansson]

Move the code which is special to ACPI LPSS into separate functions. This
may clarify the code a bit, but the main purpose of this change, is instead
to prepare for additional changes on top. Ideally the following changes
should then become easier to review.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index d7b6778..12bc5c7 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -702,22 +702,28 @@ static void acpi_lpss_dismiss(struct device *dev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int acpi_lpss_suspend_late(struct device *dev)
+static int lpss_suspend_late(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
+		acpi_lpss_save_ctx(dev, pdata);
+
+	return acpi_dev_suspend_late(dev);
+}
+
+static int acpi_lpss_suspend_late(struct device *dev)
+{
 	int ret;
 
 	ret = pm_generic_suspend_late(dev);
 	if (ret)
 		return ret;
 
-	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
-		acpi_lpss_save_ctx(dev, pdata);
-
-	return acpi_dev_suspend_late(dev);
+	return lpss_suspend_late(dev);
 }
 
-static int acpi_lpss_resume_early(struct device *dev)
+static int lpss_resume_early(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -731,6 +737,17 @@ static int acpi_lpss_resume_early(struct device *dev)
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
 		acpi_lpss_restore_ctx(dev, pdata);
 
+	return 0;
+}
+
+static int acpi_lpss_resume_early(struct device *dev)
+{
+	int ret;
+
+	ret = lpss_resume_early(dev);
+	if (ret)
+		return ret;
+
 	return pm_generic_resume_early(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.7.4

[PATCH 5/9] PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

[Ulf Hansson]

Move the code which is special to ACPI LPSS into separate functions. This
may clarify the code a bit, but the main purpose of this change, is instead
to prepare for additional changes on top. Ideally the following changes
should then become easier to review.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index d7b6778..12bc5c7 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -702,22 +702,28 @@ static void acpi_lpss_dismiss(struct device *dev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int acpi_lpss_suspend_late(struct device *dev)
+static int lpss_suspend_late(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
+		acpi_lpss_save_ctx(dev, pdata);
+
+	return acpi_dev_suspend_late(dev);
+}
+
+static int acpi_lpss_suspend_late(struct device *dev)
+{
 	int ret;
 
 	ret = pm_generic_suspend_late(dev);
 	if (ret)
 		return ret;
 
-	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
-		acpi_lpss_save_ctx(dev, pdata);
-
-	return acpi_dev_suspend_late(dev);
+	return lpss_suspend_late(dev);
 }
 
-static int acpi_lpss_resume_early(struct device *dev)
+static int lpss_resume_early(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -731,6 +737,17 @@ static int acpi_lpss_resume_early(struct device *dev)
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
 		acpi_lpss_restore_ctx(dev, pdata);
 
+	return 0;
+}
+
+static int acpi_lpss_resume_early(struct device *dev)
+{
+	int ret;
+
+	ret = lpss_resume_early(dev);
+	if (ret)
+		return ret;
+
 	return pm_generic_resume_early(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.7.4

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

In some cases a driver for an ACPI device needs to be able to prevent the
ACPI PM domain from using the direct_complete path during system sleep.

One typical case is when the driver for the device needs its device to stay
runtime enabled, during the __device_suspend phase. This isn't the case
when the direct_complete path is being executed by the PM core, as it then
disables runtime PM for the device in __device_suspend(). Any following
attempts to runtime resume the device after that point, just fails.

A workaround to this problem is to let the driver runtime resume its device
from its ->prepare() callback, as that would prevent the direct_complete
path from being executed. However, that may often be a waste, especially if
it turned out that no one really needed the device.

For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
drivers to inform the ACPI PM domain to change its default behaviour during
system sleep, and thus control whether it may use the direct_complete path
or not.

Typically a driver should call acpi_dev_disable_direct_comlete() during
->probe() and acpi_dev_enable_direct_complete() in ->remove().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
 include/acpi/acpi_bus.h  |  1 +
 include/linux/acpi.h     |  4 ++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ee51e75..2393a1a 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
 
 #ifdef CONFIG_PM_SLEEP
 /**
+ * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
+ * @dev: Device to disable the path for.
+ *
+ * Per default the ACPI PM domain tries to use the direct_complete path for its
+ * devices during system sleep. This function allows a user, typically a driver
+ * during probe, to disable the direct_complete path from being used by ACPI.
+ */
+void acpi_dev_disable_direct_complete(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev)
+		adev->no_direct_complete = true;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
+
+/**
+ * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
+ * @dev: Device to enable the path for.
+ *
+ * Enable the direct_complete path to be used during system suspend for the ACPI
+ * PM domain, which is the default option. Typically a driver that disabled the
+ * path during ->probe(), must call this function during ->remove() to re-enable
+ * the direct_complete path to be used by ACPI.
+ */
+void acpi_dev_enable_direct_complete(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev)
+		adev->no_direct_complete = false;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
+
+/**
  * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
  * @dev: Device to put into a low-power state.
  *
@@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	if (!adev || !pm_runtime_suspended(dev))
+	if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
 		return 0;
 
 	return !acpi_dev_needs_resume(dev, adev);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 63a90a6..2293d24 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -380,6 +380,7 @@ struct acpi_device {
 	struct list_head physical_node_list;
 	struct mutex physical_node_lock;
 	void (*remove)(struct acpi_device *);
+	bool no_direct_complete;
 };
 
 /* Non-device subnode */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 137e4a3..a41cca5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -859,6 +859,8 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
+void acpi_dev_disable_direct_complete(struct device *dev);
+void acpi_dev_enable_direct_complete(struct device *dev);
 int acpi_dev_suspend_late(struct device *dev);
 int acpi_dev_resume_early(struct device *dev);
 int acpi_subsys_prepare(struct device *dev);
@@ -868,6 +870,8 @@ int acpi_subsys_resume_early(struct device *dev);
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
 #else
+static inline void acpi_dev_disable_direct_complete(struct device *dev) {}
+static inline void acpi_dev_enable_direct_complete(struct device *dev) {}
 static inline int acpi_dev_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
-- 
2.7.4

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

In some cases a driver for an ACPI device needs to be able to prevent the
ACPI PM domain from using the direct_complete path during system sleep.

One typical case is when the driver for the device needs its device to stay
runtime enabled, during the __device_suspend phase. This isn't the case
when the direct_complete path is being executed by the PM core, as it then
disables runtime PM for the device in __device_suspend(). Any following
attempts to runtime resume the device after that point, just fails.

A workaround to this problem is to let the driver runtime resume its device
from its ->prepare() callback, as that would prevent the direct_complete
path from being executed. However, that may often be a waste, especially if
it turned out that no one really needed the device.

For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
drivers to inform the ACPI PM domain to change its default behaviour during
system sleep, and thus control whether it may use the direct_complete path
or not.

Typically a driver should call acpi_dev_disable_direct_comlete() during
->probe() and acpi_dev_enable_direct_complete() in ->remove().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
 include/acpi/acpi_bus.h  |  1 +
 include/linux/acpi.h     |  4 ++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ee51e75..2393a1a 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
 
 #ifdef CONFIG_PM_SLEEP
 /**
+ * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
+ * @dev: Device to disable the path for.
+ *
+ * Per default the ACPI PM domain tries to use the direct_complete path for its
+ * devices during system sleep. This function allows a user, typically a driver
+ * during probe, to disable the direct_complete path from being used by ACPI.
+ */
+void acpi_dev_disable_direct_complete(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev)
+		adev->no_direct_complete = true;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
+
+/**
+ * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
+ * @dev: Device to enable the path for.
+ *
+ * Enable the direct_complete path to be used during system suspend for the ACPI
+ * PM domain, which is the default option. Typically a driver that disabled the
+ * path during ->probe(), must call this function during ->remove() to re-enable
+ * the direct_complete path to be used by ACPI.
+ */
+void acpi_dev_enable_direct_complete(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev)
+		adev->no_direct_complete = false;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
+
+/**
  * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
  * @dev: Device to put into a low-power state.
  *
@@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	if (!adev || !pm_runtime_suspended(dev))
+	if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
 		return 0;
 
 	return !acpi_dev_needs_resume(dev, adev);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 63a90a6..2293d24 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -380,6 +380,7 @@ struct acpi_device {
 	struct list_head physical_node_list;
 	struct mutex physical_node_lock;
 	void (*remove)(struct acpi_device *);
+	bool no_direct_complete;
 };
 
 /* Non-device subnode */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 137e4a3..a41cca5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -859,6 +859,8 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
+void acpi_dev_disable_direct_complete(struct device *dev);
+void acpi_dev_enable_direct_complete(struct device *dev);
 int acpi_dev_suspend_late(struct device *dev);
 int acpi_dev_resume_early(struct device *dev);
 int acpi_subsys_prepare(struct device *dev);
@@ -868,6 +870,8 @@ int acpi_subsys_resume_early(struct device *dev);
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
 #else
+static inline void acpi_dev_disable_direct_complete(struct device *dev) {}
+static inline void acpi_dev_enable_direct_complete(struct device *dev) {}
 static inline int acpi_dev_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
-- 
2.7.4

Re: [PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> In some cases a driver for an ACPI device needs to be able to prevent the
> ACPI PM domain from using the direct_complete path during system sleep.
>
> One typical case is when the driver for the device needs its device to stay
> runtime enabled, during the __device_suspend phase. This isn't the case
> when the direct_complete path is being executed by the PM core, as it then
> disables runtime PM for the device in __device_suspend(). Any following
> attempts to runtime resume the device after that point, just fails.
>
> A workaround to this problem is to let the driver runtime resume its device
> from its ->prepare() callback, as that would prevent the direct_complete
> path from being executed. However, that may often be a waste, especially if
> it turned out that no one really needed the device.
>
> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
> drivers to inform the ACPI PM domain to change its default behaviour during
> system sleep, and thus control whether it may use the direct_complete path
> or not.
>
> Typically a driver should call acpi_dev_disable_direct_comlete() during
> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>  include/acpi/acpi_bus.h  |  1 +
>  include/linux/acpi.h     |  4 ++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index ee51e75..2393a1a 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>
>  #ifdef CONFIG_PM_SLEEP
>  /**
> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
> + * @dev: Device to disable the path for.
> + *
> + * Per default the ACPI PM domain tries to use the direct_complete path for its
> + * devices during system sleep. This function allows a user, typically a driver
> + * during probe, to disable the direct_complete path from being used by ACPI.
> + */
> +void acpi_dev_disable_direct_complete(struct device *dev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +       if (adev)
> +               adev->no_direct_complete = true;

We have an analogous flag in PCI now and it is called needs_resume, so
it would be good to be consistent with that.

> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
> +
> +/**
> + * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
> + * @dev: Device to enable the path for.
> + *
> + * Enable the direct_complete path to be used during system suspend for the ACPI
> + * PM domain, which is the default option. Typically a driver that disabled the
> + * path during ->probe(), must call this function during ->remove() to re-enable
> + * the direct_complete path to be used by ACPI.
> + */
> +void acpi_dev_enable_direct_complete(struct device *dev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +       if (adev)
> +               adev->no_direct_complete = false;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
> +
> +/**
>   * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
>   * @dev: Device to put into a low-power state.
>   *
> @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
>         if (ret < 0)
>                 return ret;
>
> -       if (!adev || !pm_runtime_suspended(dev))
> +       if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
>                 return 0;
>
>         return !acpi_dev_needs_resume(dev, adev);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 63a90a6..2293d24 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -380,6 +380,7 @@ struct acpi_device {
>         struct list_head physical_node_list;
>         struct mutex physical_node_lock;
>         void (*remove)(struct acpi_device *);
> +       bool no_direct_complete;

Also what about adding this to struct acpi_device_power instead?

Thanks,
Rafael

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> In some cases a driver for an ACPI device needs to be able to prevent the
> ACPI PM domain from using the direct_complete path during system sleep.
>
> One typical case is when the driver for the device needs its device to stay
> runtime enabled, during the __device_suspend phase. This isn't the case
> when the direct_complete path is being executed by the PM core, as it then
> disables runtime PM for the device in __device_suspend(). Any following
> attempts to runtime resume the device after that point, just fails.
>
> A workaround to this problem is to let the driver runtime resume its device
> from its ->prepare() callback, as that would prevent the direct_complete
> path from being executed. However, that may often be a waste, especially if
> it turned out that no one really needed the device.
>
> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
> drivers to inform the ACPI PM domain to change its default behaviour during
> system sleep, and thus control whether it may use the direct_complete path
> or not.
>
> Typically a driver should call acpi_dev_disable_direct_comlete() during
> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>  include/acpi/acpi_bus.h  |  1 +
>  include/linux/acpi.h     |  4 ++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index ee51e75..2393a1a 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>
>  #ifdef CONFIG_PM_SLEEP
>  /**
> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
> + * @dev: Device to disable the path for.
> + *
> + * Per default the ACPI PM domain tries to use the direct_complete path for its
> + * devices during system sleep. This function allows a user, typically a driver
> + * during probe, to disable the direct_complete path from being used by ACPI.
> + */
> +void acpi_dev_disable_direct_complete(struct device *dev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +       if (adev)
> +               adev->no_direct_complete = true;

We have an analogous flag in PCI now and it is called needs_resume, so
it would be good to be consistent with that.

> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
> +
> +/**
> + * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
> + * @dev: Device to enable the path for.
> + *
> + * Enable the direct_complete path to be used during system suspend for the ACPI
> + * PM domain, which is the default option. Typically a driver that disabled the
> + * path during ->probe(), must call this function during ->remove() to re-enable
> + * the direct_complete path to be used by ACPI.
> + */
> +void acpi_dev_enable_direct_complete(struct device *dev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +       if (adev)
> +               adev->no_direct_complete = false;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
> +
> +/**
>   * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
>   * @dev: Device to put into a low-power state.
>   *
> @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
>         if (ret < 0)
>                 return ret;
>
> -       if (!adev || !pm_runtime_suspended(dev))
> +       if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
>                 return 0;
>
>         return !acpi_dev_needs_resume(dev, adev);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 63a90a6..2293d24 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -380,6 +380,7 @@ struct acpi_device {
>         struct list_head physical_node_list;
>         struct mutex physical_node_lock;
>         void (*remove)(struct acpi_device *);
> +       bool no_direct_complete;

Also what about adding this to struct acpi_device_power instead?

Thanks,
Rafael

Re: [PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> In some cases a driver for an ACPI device needs to be able to prevent the
>> ACPI PM domain from using the direct_complete path during system sleep.
>>
>> One typical case is when the driver for the device needs its device to stay
>> runtime enabled, during the __device_suspend phase. This isn't the case
>> when the direct_complete path is being executed by the PM core, as it then
>> disables runtime PM for the device in __device_suspend(). Any following
>> attempts to runtime resume the device after that point, just fails.
>>
>> A workaround to this problem is to let the driver runtime resume its device
>> from its ->prepare() callback, as that would prevent the direct_complete
>> path from being executed. However, that may often be a waste, especially if
>> it turned out that no one really needed the device.
>>
>> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>> drivers to inform the ACPI PM domain to change its default behaviour during
>> system sleep, and thus control whether it may use the direct_complete path
>> or not.
>>
>> Typically a driver should call acpi_dev_disable_direct_comlete() during
>> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  include/acpi/acpi_bus.h  |  1 +
>>  include/linux/acpi.h     |  4 ++++
>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index ee51e75..2393a1a 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>>
>>  #ifdef CONFIG_PM_SLEEP
>>  /**
>> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>> + * @dev: Device to disable the path for.
>> + *
>> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>> + * devices during system sleep. This function allows a user, typically a driver
>> + * during probe, to disable the direct_complete path from being used by ACPI.
>> + */
>> +void acpi_dev_disable_direct_complete(struct device *dev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +       if (adev)
>> +               adev->no_direct_complete = true;
>
> We have an analogous flag in PCI now and it is called needs_resume, so
> it would be good to be consistent with that.

I was trying to come up with a nice name, however I couldn't find
anything better than no_direct_complete.

However as the next patch somewhat, extends the flag to also be use
for the runtime PM centric path, I should perhaps choose something
more related to that?

I think make "needs_resume" is going to e bit confusing, especially
while extending the usage for the flag it in the next patch, no?

>
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
>> +
>> +/**
>> + * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
>> + * @dev: Device to enable the path for.
>> + *
>> + * Enable the direct_complete path to be used during system suspend for the ACPI
>> + * PM domain, which is the default option. Typically a driver that disabled the
>> + * path during ->probe(), must call this function during ->remove() to re-enable
>> + * the direct_complete path to be used by ACPI.
>> + */
>> +void acpi_dev_enable_direct_complete(struct device *dev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +       if (adev)
>> +               adev->no_direct_complete = false;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
>> +
>> +/**
>>   * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
>>   * @dev: Device to put into a low-power state.
>>   *
>> @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       if (!adev || !pm_runtime_suspended(dev))
>> +       if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
>>                 return 0;
>>
>>         return !acpi_dev_needs_resume(dev, adev);
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 63a90a6..2293d24 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -380,6 +380,7 @@ struct acpi_device {
>>         struct list_head physical_node_list;
>>         struct mutex physical_node_lock;
>>         void (*remove)(struct acpi_device *);
>> +       bool no_direct_complete;
>
> Also what about adding this to struct acpi_device_power instead?

Yes, thanks for the suggestion!

>
> Thanks,
> Rafael

Kind regards
Uffe

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> In some cases a driver for an ACPI device needs to be able to prevent the
>> ACPI PM domain from using the direct_complete path during system sleep.
>>
>> One typical case is when the driver for the device needs its device to stay
>> runtime enabled, during the __device_suspend phase. This isn't the case
>> when the direct_complete path is being executed by the PM core, as it then
>> disables runtime PM for the device in __device_suspend(). Any following
>> attempts to runtime resume the device after that point, just fails.
>>
>> A workaround to this problem is to let the driver runtime resume its device
>> from its ->prepare() callback, as that would prevent the direct_complete
>> path from being executed. However, that may often be a waste, especially if
>> it turned out that no one really needed the device.
>>
>> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>> drivers to inform the ACPI PM domain to change its default behaviour during
>> system sleep, and thus control whether it may use the direct_complete path
>> or not.
>>
>> Typically a driver should call acpi_dev_disable_direct_comlete() during
>> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  include/acpi/acpi_bus.h  |  1 +
>>  include/linux/acpi.h     |  4 ++++
>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index ee51e75..2393a1a 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>>
>>  #ifdef CONFIG_PM_SLEEP
>>  /**
>> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>> + * @dev: Device to disable the path for.
>> + *
>> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>> + * devices during system sleep. This function allows a user, typically a driver
>> + * during probe, to disable the direct_complete path from being used by ACPI.
>> + */
>> +void acpi_dev_disable_direct_complete(struct device *dev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +       if (adev)
>> +               adev->no_direct_complete = true;
>
> We have an analogous flag in PCI now and it is called needs_resume, so
> it would be good to be consistent with that.

I was trying to come up with a nice name, however I couldn't find
anything better than no_direct_complete.

However as the next patch somewhat, extends the flag to also be use
for the runtime PM centric path, I should perhaps choose something
more related to that?

I think make "needs_resume" is going to e bit confusing, especially
while extending the usage for the flag it in the next patch, no?

>
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete);
>> +
>> +/**
>> + * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI.
>> + * @dev: Device to enable the path for.
>> + *
>> + * Enable the direct_complete path to be used during system suspend for the ACPI
>> + * PM domain, which is the default option. Typically a driver that disabled the
>> + * path during ->probe(), must call this function during ->remove() to re-enable
>> + * the direct_complete path to be used by ACPI.
>> + */
>> +void acpi_dev_enable_direct_complete(struct device *dev)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +       if (adev)
>> +               adev->no_direct_complete = false;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete);
>> +
>> +/**
>>   * acpi_dev_suspend_late - Put device into a low-power state using ACPI.
>>   * @dev: Device to put into a low-power state.
>>   *
>> @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       if (!adev || !pm_runtime_suspended(dev))
>> +       if (!adev || adev->no_direct_complete || !pm_runtime_suspended(dev))
>>                 return 0;
>>
>>         return !acpi_dev_needs_resume(dev, adev);
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 63a90a6..2293d24 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -380,6 +380,7 @@ struct acpi_device {
>>         struct list_head physical_node_list;
>>         struct mutex physical_node_lock;
>>         void (*remove)(struct acpi_device *);
>> +       bool no_direct_complete;
>
> Also what about adding this to struct acpi_device_power instead?

Yes, thanks for the suggestion!

>
> Thanks,
> Rafael

Kind regards
Uffe

Re: [PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> In some cases a driver for an ACPI device needs to be able to prevent the
> >> ACPI PM domain from using the direct_complete path during system sleep.
> >>
> >> One typical case is when the driver for the device needs its device to stay
> >> runtime enabled, during the __device_suspend phase. This isn't the case
> >> when the direct_complete path is being executed by the PM core, as it then
> >> disables runtime PM for the device in __device_suspend(). Any following
> >> attempts to runtime resume the device after that point, just fails.
> >>
> >> A workaround to this problem is to let the driver runtime resume its device
> >> from its ->prepare() callback, as that would prevent the direct_complete
> >> path from being executed. However, that may often be a waste, especially if
> >> it turned out that no one really needed the device.
> >>
> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
> >> drivers to inform the ACPI PM domain to change its default behaviour during
> >> system sleep, and thus control whether it may use the direct_complete path
> >> or not.
> >>
> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
> >>
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> ---
> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>  include/acpi/acpi_bus.h  |  1 +
> >>  include/linux/acpi.h     |  4 ++++
> >>  3 files changed, 41 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> >> index ee51e75..2393a1a 100644
> >> --- a/drivers/acpi/device_pm.c
> >> +++ b/drivers/acpi/device_pm.c
> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
> >>
> >>  #ifdef CONFIG_PM_SLEEP
> >>  /**
> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
> >> + * @dev: Device to disable the path for.
> >> + *
> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
> >> + * devices during system sleep. This function allows a user, typically a driver
> >> + * during probe, to disable the direct_complete path from being used by ACPI.
> >> + */
> >> +void acpi_dev_disable_direct_complete(struct device *dev)
> >> +{
> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> >> +
> >> +       if (adev)
> >> +               adev->no_direct_complete = true;
> >
> > We have an analogous flag in PCI now and it is called needs_resume, so
> > it would be good to be consistent with that.
> 
> I was trying to come up with a nice name, however I couldn't find
> anything better than no_direct_complete.
> 
> However as the next patch somewhat, extends the flag to also be use
> for the runtime PM centric path, I should perhaps choose something
> more related to that?
> 
> I think make "needs_resume" is going to e bit confusing, especially
> while extending the usage for the flag it in the next patch, no?

OK, fair enough, but I still think the name isn't really nice. :-)

In fact, I'm not sure if the new flag is necessary at all.

If the driver is expected to use pm_runtime_force_suspend|resume() along with
it every time, why not to make the ACPI PM domain code check if the driver's
callback pointers point to pm_runtime_force_suspend|resume() and work
accordingly?

Thanks,
Rafael

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> In some cases a driver for an ACPI device needs to be able to prevent the
> >> ACPI PM domain from using the direct_complete path during system sleep.
> >>
> >> One typical case is when the driver for the device needs its device to stay
> >> runtime enabled, during the __device_suspend phase. This isn't the case
> >> when the direct_complete path is being executed by the PM core, as it then
> >> disables runtime PM for the device in __device_suspend(). Any following
> >> attempts to runtime resume the device after that point, just fails.
> >>
> >> A workaround to this problem is to let the driver runtime resume its device
> >> from its ->prepare() callback, as that would prevent the direct_complete
> >> path from being executed. However, that may often be a waste, especially if
> >> it turned out that no one really needed the device.
> >>
> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
> >> drivers to inform the ACPI PM domain to change its default behaviour during
> >> system sleep, and thus control whether it may use the direct_complete path
> >> or not.
> >>
> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
> >>
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> ---
> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>  include/acpi/acpi_bus.h  |  1 +
> >>  include/linux/acpi.h     |  4 ++++
> >>  3 files changed, 41 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> >> index ee51e75..2393a1a 100644
> >> --- a/drivers/acpi/device_pm.c
> >> +++ b/drivers/acpi/device_pm.c
> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
> >>
> >>  #ifdef CONFIG_PM_SLEEP
> >>  /**
> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
> >> + * @dev: Device to disable the path for.
> >> + *
> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
> >> + * devices during system sleep. This function allows a user, typically a driver
> >> + * during probe, to disable the direct_complete path from being used by ACPI.
> >> + */
> >> +void acpi_dev_disable_direct_complete(struct device *dev)
> >> +{
> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> >> +
> >> +       if (adev)
> >> +               adev->no_direct_complete = true;
> >
> > We have an analogous flag in PCI now and it is called needs_resume, so
> > it would be good to be consistent with that.
> 
> I was trying to come up with a nice name, however I couldn't find
> anything better than no_direct_complete.
> 
> However as the next patch somewhat, extends the flag to also be use
> for the runtime PM centric path, I should perhaps choose something
> more related to that?
> 
> I think make "needs_resume" is going to e bit confusing, especially
> while extending the usage for the flag it in the next patch, no?

OK, fair enough, but I still think the name isn't really nice. :-)

In fact, I'm not sure if the new flag is necessary at all.

If the driver is expected to use pm_runtime_force_suspend|resume() along with
it every time, why not to make the ACPI PM domain code check if the driver's
callback pointers point to pm_runtime_force_suspend|resume() and work
accordingly?

Thanks,
Rafael

Re: [PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >> In some cases a driver for an ACPI device needs to be able to prevent the
>> >> ACPI PM domain from using the direct_complete path during system sleep.
>> >>
>> >> One typical case is when the driver for the device needs its device to stay
>> >> runtime enabled, during the __device_suspend phase. This isn't the case
>> >> when the direct_complete path is being executed by the PM core, as it then
>> >> disables runtime PM for the device in __device_suspend(). Any following
>> >> attempts to runtime resume the device after that point, just fails.
>> >>
>> >> A workaround to this problem is to let the driver runtime resume its device
>> >> from its ->prepare() callback, as that would prevent the direct_complete
>> >> path from being executed. However, that may often be a waste, especially if
>> >> it turned out that no one really needed the device.
>> >>
>> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>> >> drivers to inform the ACPI PM domain to change its default behaviour during
>> >> system sleep, and thus control whether it may use the direct_complete path
>> >> or not.
>> >>
>> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
>> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>> >>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> ---
>> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>> >>  include/acpi/acpi_bus.h  |  1 +
>> >>  include/linux/acpi.h     |  4 ++++
>> >>  3 files changed, 41 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> >> index ee51e75..2393a1a 100644
>> >> --- a/drivers/acpi/device_pm.c
>> >> +++ b/drivers/acpi/device_pm.c
>> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>> >>
>> >>  #ifdef CONFIG_PM_SLEEP
>> >>  /**
>> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>> >> + * @dev: Device to disable the path for.
>> >> + *
>> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>> >> + * devices during system sleep. This function allows a user, typically a driver
>> >> + * during probe, to disable the direct_complete path from being used by ACPI.
>> >> + */
>> >> +void acpi_dev_disable_direct_complete(struct device *dev)
>> >> +{
>> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> >> +
>> >> +       if (adev)
>> >> +               adev->no_direct_complete = true;
>> >
>> > We have an analogous flag in PCI now and it is called needs_resume, so
>> > it would be good to be consistent with that.
>>
>> I was trying to come up with a nice name, however I couldn't find
>> anything better than no_direct_complete.
>>
>> However as the next patch somewhat, extends the flag to also be use
>> for the runtime PM centric path, I should perhaps choose something
>> more related to that?
>>
>> I think make "needs_resume" is going to e bit confusing, especially
>> while extending the usage for the flag it in the next patch, no?
>
> OK, fair enough, but I still think the name isn't really nice. :-)

How about use_rpm_centric_sleep or use_rpm_ops_for_sleep?

:-)

>
> In fact, I'm not sure if the new flag is necessary at all.
>
> If the driver is expected to use pm_runtime_force_suspend|resume() along with
> it every time, why not to make the ACPI PM domain code check if the driver's
> callback pointers point to pm_runtime_force_suspend|resume() and work
> accordingly?

I get the idea, however it would limit the driver from doing other
specific things from its sleep callbacks.

It may for example deal with wakeups during system suspend, and when
that is done, it can call pm_runtime_force_suspend() from that
callback.

Potentially that could be done from the runtime PM callback itself,
(via checking pm_runtime_enabled)...

Kind regards
Uffe

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >> In some cases a driver for an ACPI device needs to be able to prevent the
>> >> ACPI PM domain from using the direct_complete path during system sleep.
>> >>
>> >> One typical case is when the driver for the device needs its device to stay
>> >> runtime enabled, during the __device_suspend phase. This isn't the case
>> >> when the direct_complete path is being executed by the PM core, as it then
>> >> disables runtime PM for the device in __device_suspend(). Any following
>> >> attempts to runtime resume the device after that point, just fails.
>> >>
>> >> A workaround to this problem is to let the driver runtime resume its device
>> >> from its ->prepare() callback, as that would prevent the direct_complete
>> >> path from being executed. However, that may often be a waste, especially if
>> >> it turned out that no one really needed the device.
>> >>
>> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>> >> drivers to inform the ACPI PM domain to change its default behaviour during
>> >> system sleep, and thus control whether it may use the direct_complete path
>> >> or not.
>> >>
>> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
>> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>> >>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> ---
>> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>> >>  include/acpi/acpi_bus.h  |  1 +
>> >>  include/linux/acpi.h     |  4 ++++
>> >>  3 files changed, 41 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> >> index ee51e75..2393a1a 100644
>> >> --- a/drivers/acpi/device_pm.c
>> >> +++ b/drivers/acpi/device_pm.c
>> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>> >>
>> >>  #ifdef CONFIG_PM_SLEEP
>> >>  /**
>> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>> >> + * @dev: Device to disable the path for.
>> >> + *
>> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>> >> + * devices during system sleep. This function allows a user, typically a driver
>> >> + * during probe, to disable the direct_complete path from being used by ACPI.
>> >> + */
>> >> +void acpi_dev_disable_direct_complete(struct device *dev)
>> >> +{
>> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> >> +
>> >> +       if (adev)
>> >> +               adev->no_direct_complete = true;
>> >
>> > We have an analogous flag in PCI now and it is called needs_resume, so
>> > it would be good to be consistent with that.
>>
>> I was trying to come up with a nice name, however I couldn't find
>> anything better than no_direct_complete.
>>
>> However as the next patch somewhat, extends the flag to also be use
>> for the runtime PM centric path, I should perhaps choose something
>> more related to that?
>>
>> I think make "needs_resume" is going to e bit confusing, especially
>> while extending the usage for the flag it in the next patch, no?
>
> OK, fair enough, but I still think the name isn't really nice. :-)

How about use_rpm_centric_sleep or use_rpm_ops_for_sleep?

:-)

>
> In fact, I'm not sure if the new flag is necessary at all.
>
> If the driver is expected to use pm_runtime_force_suspend|resume() along with
> it every time, why not to make the ACPI PM domain code check if the driver's
> callback pointers point to pm_runtime_force_suspend|resume() and work
> accordingly?

I get the idea, however it would limit the driver from doing other
specific things from its sleep callbacks.

It may for example deal with wakeups during system suspend, and when
that is done, it can call pm_runtime_force_suspend() from that
callback.

Potentially that could be done from the runtime PM callback itself,
(via checking pm_runtime_enabled)...

Kind regards
Uffe

Re: [PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:29 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
>>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> >> In some cases a driver for an ACPI device needs to be able to prevent the
>>> >> ACPI PM domain from using the direct_complete path during system sleep.
>>> >>
>>> >> One typical case is when the driver for the device needs its device to stay
>>> >> runtime enabled, during the __device_suspend phase. This isn't the case
>>> >> when the direct_complete path is being executed by the PM core, as it then
>>> >> disables runtime PM for the device in __device_suspend(). Any following
>>> >> attempts to runtime resume the device after that point, just fails.
>>> >>
>>> >> A workaround to this problem is to let the driver runtime resume its device
>>> >> from its ->prepare() callback, as that would prevent the direct_complete
>>> >> path from being executed. However, that may often be a waste, especially if
>>> >> it turned out that no one really needed the device.
>>> >>
>>> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>>> >> drivers to inform the ACPI PM domain to change its default behaviour during
>>> >> system sleep, and thus control whether it may use the direct_complete path
>>> >> or not.
>>> >>
>>> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
>>> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>>> >>
>>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> >> ---
>>> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>>> >>  include/acpi/acpi_bus.h  |  1 +
>>> >>  include/linux/acpi.h     |  4 ++++
>>> >>  3 files changed, 41 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>>> >> index ee51e75..2393a1a 100644
>>> >> --- a/drivers/acpi/device_pm.c
>>> >> +++ b/drivers/acpi/device_pm.c
>>> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>>> >>
>>> >>  #ifdef CONFIG_PM_SLEEP
>>> >>  /**
>>> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>>> >> + * @dev: Device to disable the path for.
>>> >> + *
>>> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>>> >> + * devices during system sleep. This function allows a user, typically a driver
>>> >> + * during probe, to disable the direct_complete path from being used by ACPI.
>>> >> + */
>>> >> +void acpi_dev_disable_direct_complete(struct device *dev)
>>> >> +{
>>> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>>> >> +
>>> >> +       if (adev)
>>> >> +               adev->no_direct_complete = true;
>>> >
>>> > We have an analogous flag in PCI now and it is called needs_resume, so
>>> > it would be good to be consistent with that.
>>>
>>> I was trying to come up with a nice name, however I couldn't find
>>> anything better than no_direct_complete.
>>>
>>> However as the next patch somewhat, extends the flag to also be use
>>> for the runtime PM centric path, I should perhaps choose something
>>> more related to that?
>>>
>>> I think make "needs_resume" is going to e bit confusing, especially
>>> while extending the usage for the flag it in the next patch, no?
>>
>> OK, fair enough, but I still think the name isn't really nice. :-)
>
> How about use_rpm_centric_sleep or use_rpm_ops_for_sleep?
>
> :-)

no_direct_complete is better than that IMO.

>>
>> In fact, I'm not sure if the new flag is necessary at all.
>>
>> If the driver is expected to use pm_runtime_force_suspend|resume() along with
>> it every time, why not to make the ACPI PM domain code check if the driver's
>> callback pointers point to pm_runtime_force_suspend|resume() and work
>> accordingly?
>
> I get the idea, however it would limit the driver from doing other
> specific things from its sleep callbacks.
>
> It may for example deal with wakeups during system suspend, and when
> that is done, it can call pm_runtime_force_suspend() from that
> callback.

I see.

> Potentially that could be done from the runtime PM callback itself,
> (via checking pm_runtime_enabled)...

Anyway, it would be good to have a way to verify that the driver is
doing the right thing or we'll have bugs in that area and people
wondering what's up.

Thanks,
Rafael

[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:29 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
>>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> >> In some cases a driver for an ACPI device needs to be able to prevent the
>>> >> ACPI PM domain from using the direct_complete path during system sleep.
>>> >>
>>> >> One typical case is when the driver for the device needs its device to stay
>>> >> runtime enabled, during the __device_suspend phase. This isn't the case
>>> >> when the direct_complete path is being executed by the PM core, as it then
>>> >> disables runtime PM for the device in __device_suspend(). Any following
>>> >> attempts to runtime resume the device after that point, just fails.
>>> >>
>>> >> A workaround to this problem is to let the driver runtime resume its device
>>> >> from its ->prepare() callback, as that would prevent the direct_complete
>>> >> path from being executed. However, that may often be a waste, especially if
>>> >> it turned out that no one really needed the device.
>>> >>
>>> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
>>> >> drivers to inform the ACPI PM domain to change its default behaviour during
>>> >> system sleep, and thus control whether it may use the direct_complete path
>>> >> or not.
>>> >>
>>> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
>>> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>>> >>
>>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> >> ---
>>> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>>> >>  include/acpi/acpi_bus.h  |  1 +
>>> >>  include/linux/acpi.h     |  4 ++++
>>> >>  3 files changed, 41 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>>> >> index ee51e75..2393a1a 100644
>>> >> --- a/drivers/acpi/device_pm.c
>>> >> +++ b/drivers/acpi/device_pm.c
>>> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>>> >>
>>> >>  #ifdef CONFIG_PM_SLEEP
>>> >>  /**
>>> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>>> >> + * @dev: Device to disable the path for.
>>> >> + *
>>> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>>> >> + * devices during system sleep. This function allows a user, typically a driver
>>> >> + * during probe, to disable the direct_complete path from being used by ACPI.
>>> >> + */
>>> >> +void acpi_dev_disable_direct_complete(struct device *dev)
>>> >> +{
>>> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>>> >> +
>>> >> +       if (adev)
>>> >> +               adev->no_direct_complete = true;
>>> >
>>> > We have an analogous flag in PCI now and it is called needs_resume, so
>>> > it would be good to be consistent with that.
>>>
>>> I was trying to come up with a nice name, however I couldn't find
>>> anything better than no_direct_complete.
>>>
>>> However as the next patch somewhat, extends the flag to also be use
>>> for the runtime PM centric path, I should perhaps choose something
>>> more related to that?
>>>
>>> I think make "needs_resume" is going to e bit confusing, especially
>>> while extending the usage for the flag it in the next patch, no?
>>
>> OK, fair enough, but I still think the name isn't really nice. :-)
>
> How about use_rpm_centric_sleep or use_rpm_ops_for_sleep?
>
> :-)

no_direct_complete is better than that IMO.

>>
>> In fact, I'm not sure if the new flag is necessary at all.
>>
>> If the driver is expected to use pm_runtime_force_suspend|resume() along with
>> it every time, why not to make the ACPI PM domain code check if the driver's
>> callback pointers point to pm_runtime_force_suspend|resume() and work
>> accordingly?
>
> I get the idea, however it would limit the driver from doing other
> specific things from its sleep callbacks.
>
> It may for example deal with wakeups during system suspend, and when
> that is done, it can call pm_runtime_force_suspend() from that
> callback.

I see.

> Potentially that could be done from the runtime PM callback itself,
> (via checking pm_runtime_enabled)...

Anyway, it would be good to have a way to verify that the driver is
doing the right thing or we'll have bugs in that area and people
wondering what's up.

Thanks,
Rafael

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

This change extends the interpretation of the ACPI's no_direct_complete
flag to be used to enable the so called runtime PM centric approach, for
devices being attached to the ACPI PM domain.

The principle behind the runtime PM centric approach is to re-use the
runtime PM callbacks to implement system sleep for drivers/subsystems.
Moreover, using the runtime PM centric approach gives an optimized
behaviour around avoiding to wake up a device from its low power state
during system sleep, unless really needed.

To deploy the runtime PM centric approach for a subsystem/driver, the
following adaptations needs to be made.

First, the runtime PM callbacks may be called when runtime PM has been
disabled for the device. This serves as an indication for the callbacks to
understand they are running in the system sleep sequence, instead of in the
regular runtime PM path. In some cases, a callback needs to take different
actions depending in what path it is being executed in, as is the case for
the ACPI PM domain.

In particular for the ACPI PM domain's ->runtime_suspend|resume()
callbacks, when those finds runtime PM being disabled for the device, it
instead executes the same operations as normally being run when
->suspend_late() and ->resume_early() callbacks are invoked during system
sleep.

Second, at the PM domain level, it is expected that the driver for the
device makes use of pm_runtime_force_suspend|resume(), to re-use the
runtime PM callbacks to put the device into low power state and to wake it
up when needed during system sleep.

For the ACPI PM domain's ->suspend_late() and ->resume_early() callbacks,
it means bypassing the operations putting the device into low power state
and the operations that wakes it up. Instead it shall invoke only the lower
level ->suspend_late() and ->resume_early() callbacks for the driver, if
present.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c | 58 +++++++++++++++++++++++++++++++++---------------
 drivers/acpi/device_pm.c | 56 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 12bc5c7..adc84b3 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -714,10 +714,11 @@ static int lpss_suspend_late(struct device *dev)
 
 static int acpi_lpss_suspend_late(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	int ret;
 
 	ret = pm_generic_suspend_late(dev);
-	if (ret)
+	if (ret || adev->no_direct_complete)
 		return ret;
 
 	return lpss_suspend_late(dev);
@@ -742,13 +743,23 @@ static int lpss_resume_early(struct device *dev)
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
 
-	ret = lpss_resume_early(dev);
-	if (ret)
-		return ret;
+	if (!adev->no_direct_complete)
+		ret = lpss_resume_early(dev);
 
-	return pm_generic_resume_early(dev);
+	return ret ? ret : pm_generic_resume_early(dev);
+}
+#else
+static inline int lpss_suspend_late(struct device *dev)
+{
+	return 0;
+}
+
+static inline int lpss_resume_early(struct device *dev)
+{
+	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
 
@@ -846,6 +857,9 @@ static int acpi_lpss_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (!pm_runtime_enabled(dev))
+		return lpss_suspend_late(dev);
+
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
 		acpi_lpss_save_ctx(dev, pdata);
 
@@ -867,21 +881,29 @@ static int acpi_lpss_runtime_resume(struct device *dev)
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
 
-	/*
-	 * This call is kept first to be in symmetry with
-	 * acpi_lpss_runtime_suspend() one.
-	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
-		lpss_iosf_exit_d3_state();
+	if (pm_runtime_enabled(dev)) {
+		/*
+		 * This call is kept first to be in symmetry with
+		 * acpi_lpss_runtime_suspend() one.
+		 */
+		if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
+		    iosf_mbi_available())
+			lpss_iosf_exit_d3_state();
 
-	ret = acpi_dev_runtime_resume(dev);
-	if (ret)
-		return ret;
+		ret = acpi_dev_runtime_resume(dev);
+		if (ret)
+			return ret;
 
-	acpi_lpss_d3_to_d0_delay(pdata);
+		acpi_lpss_d3_to_d0_delay(pdata);
 
-	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
-		acpi_lpss_restore_ctx(dev, pdata);
+		if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
+			acpi_lpss_restore_ctx(dev, pdata);
+
+	} else {
+		ret = lpss_resume_early(dev);
+		if (ret)
+			return ret;
+	}
 
 	return pm_generic_runtime_resume(dev);
 }
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 2393a1a..e0324ab 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -859,7 +859,14 @@ EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
 int acpi_subsys_runtime_suspend(struct device *dev)
 {
 	int ret = pm_generic_runtime_suspend(dev);
-	return ret ? ret : acpi_dev_runtime_suspend(dev);
+
+	if (ret)
+		return ret;
+
+	if (!pm_runtime_enabled(dev))
+		return acpi_dev_suspend_late(dev);
+
+	return acpi_dev_runtime_suspend(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
 
@@ -872,7 +879,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
  */
 int acpi_subsys_runtime_resume(struct device *dev)
 {
-	int ret = acpi_dev_runtime_resume(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
+
+	if (!adev)
+		return 0;
+
+	if (!pm_runtime_enabled(dev))
+		ret = acpi_dev_resume_early(dev);
+	else
+		ret = acpi_dev_runtime_resume(dev);
+
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
@@ -1015,13 +1032,21 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
  */
 void acpi_subsys_complete(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return;
+
 	pm_generic_complete(dev);
 	/*
 	 * If the device had been runtime-suspended before the system went into
 	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
+	 * now, resume it in case the firmware powered it up. Also resume it in
+	 * case no_direct_complete is set for the device, to be sure the device
+	 * are managed correctly when firmware has powered it up.
 	 */
-	if (dev->power.direct_complete && pm_resume_via_firmware())
+	if ((dev->power.direct_complete || adev->no_direct_complete) &&
+	    pm_resume_via_firmware())
 		pm_request_resume(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_complete);
@@ -1049,8 +1074,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
  */
 int acpi_subsys_suspend_late(struct device *dev)
 {
-	int ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_dev_suspend_late(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret;
+
+	if (!adev)
+		return 0;
+
+	ret = pm_generic_suspend_late(dev);
+	if (ret || adev->no_direct_complete)
+		return ret;
+
+	return acpi_dev_suspend_late(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
 
@@ -1064,7 +1098,15 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
  */
 int acpi_subsys_resume_early(struct device *dev)
 {
-	int ret = acpi_dev_resume_early(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
+
+	if (!adev)
+		return 0;
+
+	if (!adev->no_direct_complete)
+		ret = acpi_dev_resume_early(dev);
+
 	return ret ? ret : pm_generic_resume_early(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
-- 
2.7.4

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

This change extends the interpretation of the ACPI's no_direct_complete
flag to be used to enable the so called runtime PM centric approach, for
devices being attached to the ACPI PM domain.

The principle behind the runtime PM centric approach is to re-use the
runtime PM callbacks to implement system sleep for drivers/subsystems.
Moreover, using the runtime PM centric approach gives an optimized
behaviour around avoiding to wake up a device from its low power state
during system sleep, unless really needed.

To deploy the runtime PM centric approach for a subsystem/driver, the
following adaptations needs to be made.

First, the runtime PM callbacks may be called when runtime PM has been
disabled for the device. This serves as an indication for the callbacks to
understand they are running in the system sleep sequence, instead of in the
regular runtime PM path. In some cases, a callback needs to take different
actions depending in what path it is being executed in, as is the case for
the ACPI PM domain.

In particular for the ACPI PM domain's ->runtime_suspend|resume()
callbacks, when those finds runtime PM being disabled for the device, it
instead executes the same operations as normally being run when
->suspend_late() and ->resume_early() callbacks are invoked during system
sleep.

Second, at the PM domain level, it is expected that the driver for the
device makes use of pm_runtime_force_suspend|resume(), to re-use the
runtime PM callbacks to put the device into low power state and to wake it
up when needed during system sleep.

For the ACPI PM domain's ->suspend_late() and ->resume_early() callbacks,
it means bypassing the operations putting the device into low power state
and the operations that wakes it up. Instead it shall invoke only the lower
level ->suspend_late() and ->resume_early() callbacks for the driver, if
present.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c | 58 +++++++++++++++++++++++++++++++++---------------
 drivers/acpi/device_pm.c | 56 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 12bc5c7..adc84b3 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -714,10 +714,11 @@ static int lpss_suspend_late(struct device *dev)
 
 static int acpi_lpss_suspend_late(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	int ret;
 
 	ret = pm_generic_suspend_late(dev);
-	if (ret)
+	if (ret || adev->no_direct_complete)
 		return ret;
 
 	return lpss_suspend_late(dev);
@@ -742,13 +743,23 @@ static int lpss_resume_early(struct device *dev)
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
 
-	ret = lpss_resume_early(dev);
-	if (ret)
-		return ret;
+	if (!adev->no_direct_complete)
+		ret = lpss_resume_early(dev);
 
-	return pm_generic_resume_early(dev);
+	return ret ? ret : pm_generic_resume_early(dev);
+}
+#else
+static inline int lpss_suspend_late(struct device *dev)
+{
+	return 0;
+}
+
+static inline int lpss_resume_early(struct device *dev)
+{
+	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
 
@@ -846,6 +857,9 @@ static int acpi_lpss_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (!pm_runtime_enabled(dev))
+		return lpss_suspend_late(dev);
+
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
 		acpi_lpss_save_ctx(dev, pdata);
 
@@ -867,21 +881,29 @@ static int acpi_lpss_runtime_resume(struct device *dev)
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
 
-	/*
-	 * This call is kept first to be in symmetry with
-	 * acpi_lpss_runtime_suspend() one.
-	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
-		lpss_iosf_exit_d3_state();
+	if (pm_runtime_enabled(dev)) {
+		/*
+		 * This call is kept first to be in symmetry with
+		 * acpi_lpss_runtime_suspend() one.
+		 */
+		if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
+		    iosf_mbi_available())
+			lpss_iosf_exit_d3_state();
 
-	ret = acpi_dev_runtime_resume(dev);
-	if (ret)
-		return ret;
+		ret = acpi_dev_runtime_resume(dev);
+		if (ret)
+			return ret;
 
-	acpi_lpss_d3_to_d0_delay(pdata);
+		acpi_lpss_d3_to_d0_delay(pdata);
 
-	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
-		acpi_lpss_restore_ctx(dev, pdata);
+		if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
+			acpi_lpss_restore_ctx(dev, pdata);
+
+	} else {
+		ret = lpss_resume_early(dev);
+		if (ret)
+			return ret;
+	}
 
 	return pm_generic_runtime_resume(dev);
 }
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 2393a1a..e0324ab 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -859,7 +859,14 @@ EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
 int acpi_subsys_runtime_suspend(struct device *dev)
 {
 	int ret = pm_generic_runtime_suspend(dev);
-	return ret ? ret : acpi_dev_runtime_suspend(dev);
+
+	if (ret)
+		return ret;
+
+	if (!pm_runtime_enabled(dev))
+		return acpi_dev_suspend_late(dev);
+
+	return acpi_dev_runtime_suspend(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
 
@@ -872,7 +879,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
  */
 int acpi_subsys_runtime_resume(struct device *dev)
 {
-	int ret = acpi_dev_runtime_resume(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
+
+	if (!adev)
+		return 0;
+
+	if (!pm_runtime_enabled(dev))
+		ret = acpi_dev_resume_early(dev);
+	else
+		ret = acpi_dev_runtime_resume(dev);
+
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
@@ -1015,13 +1032,21 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
  */
 void acpi_subsys_complete(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return;
+
 	pm_generic_complete(dev);
 	/*
 	 * If the device had been runtime-suspended before the system went into
 	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
+	 * now, resume it in case the firmware powered it up. Also resume it in
+	 * case no_direct_complete is set for the device, to be sure the device
+	 * are managed correctly when firmware has powered it up.
 	 */
-	if (dev->power.direct_complete && pm_resume_via_firmware())
+	if ((dev->power.direct_complete || adev->no_direct_complete) &&
+	    pm_resume_via_firmware())
 		pm_request_resume(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_complete);
@@ -1049,8 +1074,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
  */
 int acpi_subsys_suspend_late(struct device *dev)
 {
-	int ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_dev_suspend_late(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret;
+
+	if (!adev)
+		return 0;
+
+	ret = pm_generic_suspend_late(dev);
+	if (ret || adev->no_direct_complete)
+		return ret;
+
+	return acpi_dev_suspend_late(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
 
@@ -1064,7 +1098,15 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
  */
 int acpi_subsys_resume_early(struct device *dev)
 {
-	int ret = acpi_dev_resume_early(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int ret = 0;
+
+	if (!adev)
+		return 0;
+
+	if (!adev->no_direct_complete)
+		ret = acpi_dev_resume_early(dev);
+
 	return ret ? ret : pm_generic_resume_early(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
-- 
2.7.4

Re: [PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> This change extends the interpretation of the ACPI's no_direct_complete
> flag to be used to enable the so called runtime PM centric approach, for
> devices being attached to the ACPI PM domain.
>
> The principle behind the runtime PM centric approach is to re-use the
> runtime PM callbacks to implement system sleep for drivers/subsystems.
> Moreover, using the runtime PM centric approach gives an optimized
> behaviour around avoiding to wake up a device from its low power state
> during system sleep, unless really needed.
>
> To deploy the runtime PM centric approach for a subsystem/driver, the
> following adaptations needs to be made.
>
> First, the runtime PM callbacks may be called when runtime PM has been
> disabled for the device. This serves as an indication for the callbacks to
> understand they are running in the system sleep sequence, instead of in the
> regular runtime PM path. In some cases, a callback needs to take different
> actions depending in what path it is being executed in, as is the case for
> the ACPI PM domain.
>
> In particular for the ACPI PM domain's ->runtime_suspend|resume()
> callbacks, when those finds runtime PM being disabled for the device, it
> instead executes the same operations as normally being run when
> ->suspend_late() and ->resume_early() callbacks are invoked during system
> sleep.
>
> Second, at the PM domain level, it is expected that the driver for the
> device makes use of pm_runtime_force_suspend|resume(), to re-use the
> runtime PM callbacks to put the device into low power state and to wake it
> up when needed during system sleep.

What if it doesn't do that?

Do all drivers of devices that may fall into the ACPI PM domain use
pm_runtime_force_suspend|resume()?

Thanks,
Rafael

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> This change extends the interpretation of the ACPI's no_direct_complete
> flag to be used to enable the so called runtime PM centric approach, for
> devices being attached to the ACPI PM domain.
>
> The principle behind the runtime PM centric approach is to re-use the
> runtime PM callbacks to implement system sleep for drivers/subsystems.
> Moreover, using the runtime PM centric approach gives an optimized
> behaviour around avoiding to wake up a device from its low power state
> during system sleep, unless really needed.
>
> To deploy the runtime PM centric approach for a subsystem/driver, the
> following adaptations needs to be made.
>
> First, the runtime PM callbacks may be called when runtime PM has been
> disabled for the device. This serves as an indication for the callbacks to
> understand they are running in the system sleep sequence, instead of in the
> regular runtime PM path. In some cases, a callback needs to take different
> actions depending in what path it is being executed in, as is the case for
> the ACPI PM domain.
>
> In particular for the ACPI PM domain's ->runtime_suspend|resume()
> callbacks, when those finds runtime PM being disabled for the device, it
> instead executes the same operations as normally being run when
> ->suspend_late() and ->resume_early() callbacks are invoked during system
> sleep.
>
> Second, at the PM domain level, it is expected that the driver for the
> device makes use of pm_runtime_force_suspend|resume(), to re-use the
> runtime PM callbacks to put the device into low power state and to wake it
> up when needed during system sleep.

What if it doesn't do that?

Do all drivers of devices that may fall into the ACPI PM domain use
pm_runtime_force_suspend|resume()?

Thanks,
Rafael

Re: [PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> This change extends the interpretation of the ACPI's no_direct_complete
>> flag to be used to enable the so called runtime PM centric approach, for
>> devices being attached to the ACPI PM domain.
>>
>> The principle behind the runtime PM centric approach is to re-use the
>> runtime PM callbacks to implement system sleep for drivers/subsystems.
>> Moreover, using the runtime PM centric approach gives an optimized
>> behaviour around avoiding to wake up a device from its low power state
>> during system sleep, unless really needed.
>>
>> To deploy the runtime PM centric approach for a subsystem/driver, the
>> following adaptations needs to be made.
>>
>> First, the runtime PM callbacks may be called when runtime PM has been
>> disabled for the device. This serves as an indication for the callbacks to
>> understand they are running in the system sleep sequence, instead of in the
>> regular runtime PM path. In some cases, a callback needs to take different
>> actions depending in what path it is being executed in, as is the case for
>> the ACPI PM domain.
>>
>> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>> callbacks, when those finds runtime PM being disabled for the device, it
>> instead executes the same operations as normally being run when
>> ->suspend_late() and ->resume_early() callbacks are invoked during system
>> sleep.
>>
>> Second, at the PM domain level, it is expected that the driver for the
>> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>> runtime PM callbacks to put the device into low power state and to wake it
>> up when needed during system sleep.
>
> What if it doesn't do that?
>
> Do all drivers of devices that may fall into the ACPI PM domain use
> pm_runtime_force_suspend|resume()?

No, no - the runtime PM centric path is optional by all ACPI
devices/drivers. The default is still for the ACPI PM domain to try
the direct_complete path.

However if an ACPI device/driver (i2c designware in this case) likes
to do that, they need to inform the ACPI PM domain about it. Then they
call acpi_dev_disable_direct_complete() and makes use of
pm_runtime_force_suspend|resume() to deal with system sleep.

Does that makes sense?

Kind regards
Uffe

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> This change extends the interpretation of the ACPI's no_direct_complete
>> flag to be used to enable the so called runtime PM centric approach, for
>> devices being attached to the ACPI PM domain.
>>
>> The principle behind the runtime PM centric approach is to re-use the
>> runtime PM callbacks to implement system sleep for drivers/subsystems.
>> Moreover, using the runtime PM centric approach gives an optimized
>> behaviour around avoiding to wake up a device from its low power state
>> during system sleep, unless really needed.
>>
>> To deploy the runtime PM centric approach for a subsystem/driver, the
>> following adaptations needs to be made.
>>
>> First, the runtime PM callbacks may be called when runtime PM has been
>> disabled for the device. This serves as an indication for the callbacks to
>> understand they are running in the system sleep sequence, instead of in the
>> regular runtime PM path. In some cases, a callback needs to take different
>> actions depending in what path it is being executed in, as is the case for
>> the ACPI PM domain.
>>
>> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>> callbacks, when those finds runtime PM being disabled for the device, it
>> instead executes the same operations as normally being run when
>> ->suspend_late() and ->resume_early() callbacks are invoked during system
>> sleep.
>>
>> Second, at the PM domain level, it is expected that the driver for the
>> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>> runtime PM callbacks to put the device into low power state and to wake it
>> up when needed during system sleep.
>
> What if it doesn't do that?
>
> Do all drivers of devices that may fall into the ACPI PM domain use
> pm_runtime_force_suspend|resume()?

No, no - the runtime PM centric path is optional by all ACPI
devices/drivers. The default is still for the ACPI PM domain to try
the direct_complete path.

However if an ACPI device/driver (i2c designware in this case) likes
to do that, they need to inform the ACPI PM domain about it. Then they
call acpi_dev_disable_direct_complete() and makes use of
pm_runtime_force_suspend|resume() to deal with system sleep.

Does that makes sense?

Kind regards
Uffe

Re: [PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> This change extends the interpretation of the ACPI's no_direct_complete
> >> flag to be used to enable the so called runtime PM centric approach, for
> >> devices being attached to the ACPI PM domain.
> >>
> >> The principle behind the runtime PM centric approach is to re-use the
> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
> >> Moreover, using the runtime PM centric approach gives an optimized
> >> behaviour around avoiding to wake up a device from its low power state
> >> during system sleep, unless really needed.
> >>
> >> To deploy the runtime PM centric approach for a subsystem/driver, the
> >> following adaptations needs to be made.
> >>
> >> First, the runtime PM callbacks may be called when runtime PM has been
> >> disabled for the device. This serves as an indication for the callbacks to
> >> understand they are running in the system sleep sequence, instead of in the
> >> regular runtime PM path. In some cases, a callback needs to take different
> >> actions depending in what path it is being executed in, as is the case for
> >> the ACPI PM domain.
> >>
> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
> >> callbacks, when those finds runtime PM being disabled for the device, it
> >> instead executes the same operations as normally being run when
> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
> >> sleep.
> >>
> >> Second, at the PM domain level, it is expected that the driver for the
> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
> >> runtime PM callbacks to put the device into low power state and to wake it
> >> up when needed during system sleep.
> >
> > What if it doesn't do that?
> >
> > Do all drivers of devices that may fall into the ACPI PM domain use
> > pm_runtime_force_suspend|resume()?
> 
> No, no - the runtime PM centric path is optional by all ACPI
> devices/drivers. The default is still for the ACPI PM domain to try
> the direct_complete path.
> 
> However if an ACPI device/driver (i2c designware in this case) likes
> to do that, they need to inform the ACPI PM domain about it. Then they
> call acpi_dev_disable_direct_complete() and makes use of
> pm_runtime_force_suspend|resume() to deal with system sleep.
> 
> Does that makes sense?

Overall, yes, it does, but then it should be made clear that when you use
"no_direct_complete" (all what you are going to call that eventually), you
also must use pm_runtime_force_suspend|resume() as your sleep callbacks.

Otherwise things may not work correctly if my understanding is correct.

Thanks,
Rafael

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> This change extends the interpretation of the ACPI's no_direct_complete
> >> flag to be used to enable the so called runtime PM centric approach, for
> >> devices being attached to the ACPI PM domain.
> >>
> >> The principle behind the runtime PM centric approach is to re-use the
> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
> >> Moreover, using the runtime PM centric approach gives an optimized
> >> behaviour around avoiding to wake up a device from its low power state
> >> during system sleep, unless really needed.
> >>
> >> To deploy the runtime PM centric approach for a subsystem/driver, the
> >> following adaptations needs to be made.
> >>
> >> First, the runtime PM callbacks may be called when runtime PM has been
> >> disabled for the device. This serves as an indication for the callbacks to
> >> understand they are running in the system sleep sequence, instead of in the
> >> regular runtime PM path. In some cases, a callback needs to take different
> >> actions depending in what path it is being executed in, as is the case for
> >> the ACPI PM domain.
> >>
> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
> >> callbacks, when those finds runtime PM being disabled for the device, it
> >> instead executes the same operations as normally being run when
> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
> >> sleep.
> >>
> >> Second, at the PM domain level, it is expected that the driver for the
> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
> >> runtime PM callbacks to put the device into low power state and to wake it
> >> up when needed during system sleep.
> >
> > What if it doesn't do that?
> >
> > Do all drivers of devices that may fall into the ACPI PM domain use
> > pm_runtime_force_suspend|resume()?
> 
> No, no - the runtime PM centric path is optional by all ACPI
> devices/drivers. The default is still for the ACPI PM domain to try
> the direct_complete path.
> 
> However if an ACPI device/driver (i2c designware in this case) likes
> to do that, they need to inform the ACPI PM domain about it. Then they
> call acpi_dev_disable_direct_complete() and makes use of
> pm_runtime_force_suspend|resume() to deal with system sleep.
> 
> Does that makes sense?

Overall, yes, it does, but then it should be made clear that when you use
"no_direct_complete" (all what you are going to call that eventually), you
also must use pm_runtime_force_suspend|resume() as your sleep callbacks.

Otherwise things may not work correctly if my understanding is correct.

Thanks,
Rafael

Re: [PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

On 22 June 2017 at 16:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
>> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >> This change extends the interpretation of the ACPI's no_direct_complete
>> >> flag to be used to enable the so called runtime PM centric approach, for
>> >> devices being attached to the ACPI PM domain.
>> >>
>> >> The principle behind the runtime PM centric approach is to re-use the
>> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
>> >> Moreover, using the runtime PM centric approach gives an optimized
>> >> behaviour around avoiding to wake up a device from its low power state
>> >> during system sleep, unless really needed.
>> >>
>> >> To deploy the runtime PM centric approach for a subsystem/driver, the
>> >> following adaptations needs to be made.
>> >>
>> >> First, the runtime PM callbacks may be called when runtime PM has been
>> >> disabled for the device. This serves as an indication for the callbacks to
>> >> understand they are running in the system sleep sequence, instead of in the
>> >> regular runtime PM path. In some cases, a callback needs to take different
>> >> actions depending in what path it is being executed in, as is the case for
>> >> the ACPI PM domain.
>> >>
>> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>> >> callbacks, when those finds runtime PM being disabled for the device, it
>> >> instead executes the same operations as normally being run when
>> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
>> >> sleep.
>> >>
>> >> Second, at the PM domain level, it is expected that the driver for the
>> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>> >> runtime PM callbacks to put the device into low power state and to wake it
>> >> up when needed during system sleep.
>> >
>> > What if it doesn't do that?
>> >
>> > Do all drivers of devices that may fall into the ACPI PM domain use
>> > pm_runtime_force_suspend|resume()?
>>
>> No, no - the runtime PM centric path is optional by all ACPI
>> devices/drivers. The default is still for the ACPI PM domain to try
>> the direct_complete path.
>>
>> However if an ACPI device/driver (i2c designware in this case) likes
>> to do that, they need to inform the ACPI PM domain about it. Then they
>> call acpi_dev_disable_direct_complete() and makes use of
>> pm_runtime_force_suspend|resume() to deal with system sleep.
>>
>> Does that makes sense?
>
> Overall, yes, it does, but then it should be made clear that when you use
> "no_direct_complete" (all what you are going to call that eventually), you
> also must use pm_runtime_force_suspend|resume() as your sleep callbacks.

Okay, so I will clarify that in the header of
acpi_dev_disable_direct_complete() and I guess I should also update
some documentation around ACPI.

>
> Otherwise things may not work correctly if my understanding is correct.

Correct.

Kind regards
Uffe

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson]

On 22 June 2017 at 16:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
>> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >> This change extends the interpretation of the ACPI's no_direct_complete
>> >> flag to be used to enable the so called runtime PM centric approach, for
>> >> devices being attached to the ACPI PM domain.
>> >>
>> >> The principle behind the runtime PM centric approach is to re-use the
>> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
>> >> Moreover, using the runtime PM centric approach gives an optimized
>> >> behaviour around avoiding to wake up a device from its low power state
>> >> during system sleep, unless really needed.
>> >>
>> >> To deploy the runtime PM centric approach for a subsystem/driver, the
>> >> following adaptations needs to be made.
>> >>
>> >> First, the runtime PM callbacks may be called when runtime PM has been
>> >> disabled for the device. This serves as an indication for the callbacks to
>> >> understand they are running in the system sleep sequence, instead of in the
>> >> regular runtime PM path. In some cases, a callback needs to take different
>> >> actions depending in what path it is being executed in, as is the case for
>> >> the ACPI PM domain.
>> >>
>> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>> >> callbacks, when those finds runtime PM being disabled for the device, it
>> >> instead executes the same operations as normally being run when
>> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
>> >> sleep.
>> >>
>> >> Second, at the PM domain level, it is expected that the driver for the
>> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>> >> runtime PM callbacks to put the device into low power state and to wake it
>> >> up when needed during system sleep.
>> >
>> > What if it doesn't do that?
>> >
>> > Do all drivers of devices that may fall into the ACPI PM domain use
>> > pm_runtime_force_suspend|resume()?
>>
>> No, no - the runtime PM centric path is optional by all ACPI
>> devices/drivers. The default is still for the ACPI PM domain to try
>> the direct_complete path.
>>
>> However if an ACPI device/driver (i2c designware in this case) likes
>> to do that, they need to inform the ACPI PM domain about it. Then they
>> call acpi_dev_disable_direct_complete() and makes use of
>> pm_runtime_force_suspend|resume() to deal with system sleep.
>>
>> Does that makes sense?
>
> Overall, yes, it does, but then it should be made clear that when you use
> "no_direct_complete" (all what you are going to call that eventually), you
> also must use pm_runtime_force_suspend|resume() as your sleep callbacks.

Okay, so I will clarify that in the header of
acpi_dev_disable_direct_complete() and I guess I should also update
some documentation around ACPI.

>
> Otherwise things may not work correctly if my understanding is correct.

Correct.

Kind regards
Uffe

Re: [PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
>>> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> >> This change extends the interpretation of the ACPI's no_direct_complete
>>> >> flag to be used to enable the so called runtime PM centric approach, for
>>> >> devices being attached to the ACPI PM domain.
>>> >>
>>> >> The principle behind the runtime PM centric approach is to re-use the
>>> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
>>> >> Moreover, using the runtime PM centric approach gives an optimized
>>> >> behaviour around avoiding to wake up a device from its low power state
>>> >> during system sleep, unless really needed.
>>> >>
>>> >> To deploy the runtime PM centric approach for a subsystem/driver, the
>>> >> following adaptations needs to be made.
>>> >>
>>> >> First, the runtime PM callbacks may be called when runtime PM has been
>>> >> disabled for the device. This serves as an indication for the callbacks to
>>> >> understand they are running in the system sleep sequence, instead of in the
>>> >> regular runtime PM path. In some cases, a callback needs to take different
>>> >> actions depending in what path it is being executed in, as is the case for
>>> >> the ACPI PM domain.
>>> >>
>>> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>>> >> callbacks, when those finds runtime PM being disabled for the device, it
>>> >> instead executes the same operations as normally being run when
>>> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
>>> >> sleep.
>>> >>
>>> >> Second, at the PM domain level, it is expected that the driver for the
>>> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>>> >> runtime PM callbacks to put the device into low power state and to wake it
>>> >> up when needed during system sleep.
>>> >
>>> > What if it doesn't do that?
>>> >
>>> > Do all drivers of devices that may fall into the ACPI PM domain use
>>> > pm_runtime_force_suspend|resume()?
>>>
>>> No, no - the runtime PM centric path is optional by all ACPI
>>> devices/drivers. The default is still for the ACPI PM domain to try
>>> the direct_complete path.
>>>
>>> However if an ACPI device/driver (i2c designware in this case) likes
>>> to do that, they need to inform the ACPI PM domain about it. Then they
>>> call acpi_dev_disable_direct_complete() and makes use of
>>> pm_runtime_force_suspend|resume() to deal with system sleep.
>>>
>>> Does that makes sense?
>>
>> Overall, yes, it does, but then it should be made clear that when you use
>> "no_direct_complete" (all what you are going to call that eventually), you
>> also must use pm_runtime_force_suspend|resume() as your sleep callbacks.
>
> Okay, so I will clarify that in the header of
> acpi_dev_disable_direct_complete() and I guess I should also update
> some documentation around ACPI.

I think it also would be good to make
acpi_dev_disable_direct_complete() check the conditions and throw a
stack dump if they are not met.

>>
>> Otherwise things may not work correctly if my understanding is correct.
>
> Correct.

OK

In that case the no_direct_complete flag should be set for the entire
time the driver is bound to the device.  Therefore
(a) It should go into struct acpi_device_power_flags.
(b) It should be set at the probe time and cleared at the remove time.

Thanks,
Rafael

[PATCH 7/9] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 June 2017 at 16:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, June 22, 2017 11:42:11 AM Ulf Hansson wrote:
>>> On 21 June 2017 at 23:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> >> This change extends the interpretation of the ACPI's no_direct_complete
>>> >> flag to be used to enable the so called runtime PM centric approach, for
>>> >> devices being attached to the ACPI PM domain.
>>> >>
>>> >> The principle behind the runtime PM centric approach is to re-use the
>>> >> runtime PM callbacks to implement system sleep for drivers/subsystems.
>>> >> Moreover, using the runtime PM centric approach gives an optimized
>>> >> behaviour around avoiding to wake up a device from its low power state
>>> >> during system sleep, unless really needed.
>>> >>
>>> >> To deploy the runtime PM centric approach for a subsystem/driver, the
>>> >> following adaptations needs to be made.
>>> >>
>>> >> First, the runtime PM callbacks may be called when runtime PM has been
>>> >> disabled for the device. This serves as an indication for the callbacks to
>>> >> understand they are running in the system sleep sequence, instead of in the
>>> >> regular runtime PM path. In some cases, a callback needs to take different
>>> >> actions depending in what path it is being executed in, as is the case for
>>> >> the ACPI PM domain.
>>> >>
>>> >> In particular for the ACPI PM domain's ->runtime_suspend|resume()
>>> >> callbacks, when those finds runtime PM being disabled for the device, it
>>> >> instead executes the same operations as normally being run when
>>> >> ->suspend_late() and ->resume_early() callbacks are invoked during system
>>> >> sleep.
>>> >>
>>> >> Second, at the PM domain level, it is expected that the driver for the
>>> >> device makes use of pm_runtime_force_suspend|resume(), to re-use the
>>> >> runtime PM callbacks to put the device into low power state and to wake it
>>> >> up when needed during system sleep.
>>> >
>>> > What if it doesn't do that?
>>> >
>>> > Do all drivers of devices that may fall into the ACPI PM domain use
>>> > pm_runtime_force_suspend|resume()?
>>>
>>> No, no - the runtime PM centric path is optional by all ACPI
>>> devices/drivers. The default is still for the ACPI PM domain to try
>>> the direct_complete path.
>>>
>>> However if an ACPI device/driver (i2c designware in this case) likes
>>> to do that, they need to inform the ACPI PM domain about it. Then they
>>> call acpi_dev_disable_direct_complete() and makes use of
>>> pm_runtime_force_suspend|resume() to deal with system sleep.
>>>
>>> Does that makes sense?
>>
>> Overall, yes, it does, but then it should be made clear that when you use
>> "no_direct_complete" (all what you are going to call that eventually), you
>> also must use pm_runtime_force_suspend|resume() as your sleep callbacks.
>
> Okay, so I will clarify that in the header of
> acpi_dev_disable_direct_complete() and I guess I should also update
> some documentation around ACPI.

I think it also would be good to make
acpi_dev_disable_direct_complete() check the conditions and throw a
stack dump if they are not met.

>>
>> Otherwise things may not work correctly if my understanding is correct.
>
> Correct.

OK

In that case the no_direct_complete flag should be set for the entire
time the driver is bound to the device.  Therefore
(a) It should go into struct acpi_device_power_flags.
(b) It should be set at the probe time and cleared at the remove time.

Thanks,
Rafael

[PATCH 8/9] PM / ACPI: Avoid runtime resuming device in acpi_subsys_suspend|freeze()

[Ulf Hansson]

In the case when the no_direct_complete flag is set for the ACPI device, we
can under some conditions during system sleep, avoid runtime resuming the
device in acpi_subsys_suspend|freeze(). This will of course improve the
time it takes for the system to suspend, but also avoid wasting power.

More precisely, from acpi_subsys_suspend|freeze(), let's call
acpi_dev_needs_resume() when the no_direct_complete flag is set, as it
tells us when runtime resume of the device is needed, thus let's avoid it
if possible.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index e0324ab..f3061f9 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1056,10 +1056,19 @@ EXPORT_SYMBOL_GPL(acpi_subsys_complete);
  * @dev: Device to handle.
  *
  * Follow PCI and resume devices suspended at run time before running their
- * system suspend callbacks.
+ * system suspend callbacks. However, try to avoid it when no_direct_complete
+ * is set.
  */
 int acpi_subsys_suspend(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return 0;
+
+	if (adev->no_direct_complete && !acpi_dev_needs_resume(dev, adev))
+		return pm_generic_suspend(dev);
+
 	pm_runtime_resume(dev);
 	return pm_generic_suspend(dev);
 }
@@ -1117,12 +1126,22 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
  */
 int acpi_subsys_freeze(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return 0;
+
 	/*
 	 * This used to be done in acpi_subsys_prepare() for all devices and
 	 * some drivers may depend on it, so do it here.  Ideally, however,
 	 * runtime-suspended devices should not be touched during freeze/thaw
-	 * transitions.
+	 * transitions. At least when no_direct_complete is set, let's try to
+	 * avoid it.
 	 */
+
+	if (adev->no_direct_complete && !acpi_dev_needs_resume(dev, adev))
+		return pm_generic_freeze(dev);
+
 	pm_runtime_resume(dev);
 	return pm_generic_freeze(dev);
 }
-- 
2.7.4

[PATCH 8/9] PM / ACPI: Avoid runtime resuming device in acpi_subsys_suspend|freeze()

[Ulf Hansson]

In the case when the no_direct_complete flag is set for the ACPI device, we
can under some conditions during system sleep, avoid runtime resuming the
device in acpi_subsys_suspend|freeze(). This will of course improve the
time it takes for the system to suspend, but also avoid wasting power.

More precisely, from acpi_subsys_suspend|freeze(), let's call
acpi_dev_needs_resume() when the no_direct_complete flag is set, as it
tells us when runtime resume of the device is needed, thus let's avoid it
if possible.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index e0324ab..f3061f9 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1056,10 +1056,19 @@ EXPORT_SYMBOL_GPL(acpi_subsys_complete);
  * @dev: Device to handle.
  *
  * Follow PCI and resume devices suspended at run time before running their
- * system suspend callbacks.
+ * system suspend callbacks. However, try to avoid it when no_direct_complete
+ * is set.
  */
 int acpi_subsys_suspend(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return 0;
+
+	if (adev->no_direct_complete && !acpi_dev_needs_resume(dev, adev))
+		return pm_generic_suspend(dev);
+
 	pm_runtime_resume(dev);
 	return pm_generic_suspend(dev);
 }
@@ -1117,12 +1126,22 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
  */
 int acpi_subsys_freeze(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		return 0;
+
 	/*
 	 * This used to be done in acpi_subsys_prepare() for all devices and
 	 * some drivers may depend on it, so do it here.  Ideally, however,
 	 * runtime-suspended devices should not be touched during freeze/thaw
-	 * transitions.
+	 * transitions. At least when no_direct_complete is set, let's try to
+	 * avoid it.
 	 */
+
+	if (adev->no_direct_complete && !acpi_dev_needs_resume(dev, adev))
+		return pm_generic_freeze(dev);
+
 	pm_runtime_resume(dev);
 	return pm_generic_freeze(dev);
 }
-- 
2.7.4

[PATCH 9/9] i2c: designware: Deploy the runtime PM centric approach for system sleep

[Ulf Hansson]

Currently we runtime resume the device in the ->prepare() callback to make
sure we manage system sleep properly.

The earlier attempt to avoid that, as it's surely a waste in many
situations, was based upon using the direct_complete path during system
sleep. However, that failed because we could end up having the ->suspend()
callback being invoked when the device was runtime suspended, causing calls
for clock enable/disable and clock prepare/unprepare to become unbalanced.

Deploying the runtime PM centric approach, via using the
pm_runtime_force_suspend|resume() helpers as the system sleep callbacks,
improves the behavior in the following regards:

*)
It avoids runtime resuming the device in the ->prepare() callback.

**)
The device remains runtime PM enabled during the device_suspend() phase,
which thus makes it possible for users to send i2c transfers this entire
phase.

***)
It doesn't unnecessary runtime resume the device during system suspend, but
allows the device to remain runtime suspended if that is possible.

****)
It avoids to bring the device back to full power during system resume,
unless really needed. Instead this task may get postponed to be managed by
runtime PM.

In case when the i2c device is attached to the ACPI PM domain, we need to
inform ACPI to adapt its behavior for the runtime PM centric path during
system sleep, which is done by calling acpi_dev_disable_direct_complete().

Worth to notice, comparing the earlier attempt of using the
direct_complete() approach is that **) couldn't be supported and ****)
relied on the device to be runtime suspended in the device_suspend() phase.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2b7fa75..b6e05ba 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -331,6 +331,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (r)
 		goto exit_probe;
 
+	acpi_dev_disable_direct_complete(&pdev->dev);
+
 	return r;
 
 exit_probe:
@@ -346,6 +348,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 
+	acpi_dev_enable_direct_complete(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
 	i2c_del_adapter(&dev->adapter);
@@ -372,18 +375,8 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_prepare(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return 0;
-}
-#else
-#define dw_i2c_plat_prepare	NULL
-#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);
@@ -394,7 +387,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_plat_resume(struct device *dev)
+static int dw_i2c_plat_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
@@ -406,9 +399,11 @@ static int dw_i2c_plat_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	.prepare = dw_i2c_plat_prepare,
-	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_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
+			   dw_i2c_plat_runtime_resume,
+			   NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
2.7.4

[PATCH 9/9] i2c: designware: Deploy the runtime PM centric approach for system sleep

[Ulf Hansson]

Currently we runtime resume the device in the ->prepare() callback to make
sure we manage system sleep properly.

The earlier attempt to avoid that, as it's surely a waste in many
situations, was based upon using the direct_complete path during system
sleep. However, that failed because we could end up having the ->suspend()
callback being invoked when the device was runtime suspended, causing calls
for clock enable/disable and clock prepare/unprepare to become unbalanced.

Deploying the runtime PM centric approach, via using the
pm_runtime_force_suspend|resume() helpers as the system sleep callbacks,
improves the behavior in the following regards:

*)
It avoids runtime resuming the device in the ->prepare() callback.

**)
The device remains runtime PM enabled during the device_suspend() phase,
which thus makes it possible for users to send i2c transfers this entire
phase.

***)
It doesn't unnecessary runtime resume the device during system suspend, but
allows the device to remain runtime suspended if that is possible.

****)
It avoids to bring the device back to full power during system resume,
unless really needed. Instead this task may get postponed to be managed by
runtime PM.

In case when the i2c device is attached to the ACPI PM domain, we need to
inform ACPI to adapt its behavior for the runtime PM centric path during
system sleep, which is done by calling acpi_dev_disable_direct_complete().

Worth to notice, comparing the earlier attempt of using the
direct_complete() approach is that **) couldn't be supported and ****)
relied on the device to be runtime suspended in the device_suspend() phase.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2b7fa75..b6e05ba 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -331,6 +331,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (r)
 		goto exit_probe;
 
+	acpi_dev_disable_direct_complete(&pdev->dev);
+
 	return r;
 
 exit_probe:
@@ -346,6 +348,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 
+	acpi_dev_enable_direct_complete(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
 	i2c_del_adapter(&dev->adapter);
@@ -372,18 +375,8 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_prepare(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return 0;
-}
-#else
-#define dw_i2c_plat_prepare	NULL
-#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);
@@ -394,7 +387,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_plat_resume(struct device *dev)
+static int dw_i2c_plat_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
@@ -406,9 +399,11 @@ static int dw_i2c_plat_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	.prepare = dw_i2c_plat_prepare,
-	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_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
+			   dw_i2c_plat_runtime_resume,
+			   NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
2.7.4

Re: [PATCH 0/9] PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI

[John Stultz]

On Wed, Jun 21, 2017 at 12:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,
> currently suffers from a regression, which easily can be triggerd during system
> suspend/resume.
>
> In particular the regression concerns those SoCs, which doesn't have the i2c
> device being attached to the ACPI PM domain.
>
> The regression, introduced in commit 8503ff166504 ("i2c: designware: Avoid
> unnecessary resuming during system suspend"), takes advantage of the so called
> direct_complete path during system sleep, which works well when using the ACPI
> PM domain, however as stated, that's not the case for other SoCs.
>
> In this series, patch 1, takes a very simplistic approach to solve the problem
> and the change is limited to the i2c driver itself. The aim with this change is
> to make it acceptable as a fix for v4.2+.
>
> However, patch 1 is unfortunate also reverting back to the old behaviour,
> avoiding the direct_complete path, by always runtime resuming the device in the
> device_prepare() phase during system sleep. In many cases this is a waste,
> especially when the device could have remained runtime suspended during the
> entire system sleep sequence.
>
> The rest of the changes in this series, addresses the above problem, avoiding
> to runtime resume the device unless it's really needed. One could have tried
> out other local hacks, limited to the i2c driver, but I believe the problem
> could exist for other drivers/devices as well, thus I aim for a more generic
> solution.
>
> In principle, this series enables the proven runtime PM centric approach to
> implement system sleep in the i2c driver. However, to do that the ACPI PM
> domain have to collaborate and understand this behaviour. Therefore a number of
> changes, patch 2 to patch 8, makes the needed changes to the ACPI PM domain.
> In the last change, patch9, the i2c driver converts to the runtime PM centric
> approach.
>
> It shall be noted, the behaviour of the ACPI PM domain should remain intact,
> still taking benefit of using the direct_complete path during system sleep,
> except for those devices that explicitly request it not to.
>
> This series has been tested on an ARM64 Hikey board, not having its i2c device
> attach to the ACPI PM domain. This means that the ACPI changes has so far only
> been compile tested. Any help in testing on relevant Intel SoCs is greatly
> appreciated and so is of course review comments.
>
> The series is based upon Rafael's pm tree and linux-next branch. It's also
> available at the following git:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_i2c_rpm_path

With this patchset I'm not seeing any i2c errors on resume w/ the
HiKey (which I do see if I remove these patches).

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

Unfortunately... that said, in testing I did just find a regression
elsewhere with suspend/resume on HiKey, where we'll not go all the way
to suspend if wifi is enabled. So If anyone else is having trouble
testing these patches on HiKey, turn off wifi for now.

thanks
-john

[PATCH 0/9] PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI

[John Stultz]

On Wed, Jun 21, 2017 at 12:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,
> currently suffers from a regression, which easily can be triggerd during system
> suspend/resume.
>
> In particular the regression concerns those SoCs, which doesn't have the i2c
> device being attached to the ACPI PM domain.
>
> The regression, introduced in commit 8503ff166504 ("i2c: designware: Avoid
> unnecessary resuming during system suspend"), takes advantage of the so called
> direct_complete path during system sleep, which works well when using the ACPI
> PM domain, however as stated, that's not the case for other SoCs.
>
> In this series, patch 1, takes a very simplistic approach to solve the problem
> and the change is limited to the i2c driver itself. The aim with this change is
> to make it acceptable as a fix for v4.2+.
>
> However, patch 1 is unfortunate also reverting back to the old behaviour,
> avoiding the direct_complete path, by always runtime resuming the device in the
> device_prepare() phase during system sleep. In many cases this is a waste,
> especially when the device could have remained runtime suspended during the
> entire system sleep sequence.
>
> The rest of the changes in this series, addresses the above problem, avoiding
> to runtime resume the device unless it's really needed. One could have tried
> out other local hacks, limited to the i2c driver, but I believe the problem
> could exist for other drivers/devices as well, thus I aim for a more generic
> solution.
>
> In principle, this series enables the proven runtime PM centric approach to
> implement system sleep in the i2c driver. However, to do that the ACPI PM
> domain have to collaborate and understand this behaviour. Therefore a number of
> changes, patch 2 to patch 8, makes the needed changes to the ACPI PM domain.
> In the last change, patch9, the i2c driver converts to the runtime PM centric
> approach.
>
> It shall be noted, the behaviour of the ACPI PM domain should remain intact,
> still taking benefit of using the direct_complete path during system sleep,
> except for those devices that explicitly request it not to.
>
> This series has been tested on an ARM64 Hikey board, not having its i2c device
> attach to the ACPI PM domain. This means that the ACPI changes has so far only
> been compile tested. Any help in testing on relevant Intel SoCs is greatly
> appreciated and so is of course review comments.
>
> The series is based upon Rafael's pm tree and linux-next branch. It's also
> available at the following git:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_i2c_rpm_path

With this patchset I'm not seeing any i2c errors on resume w/ the
HiKey (which I do see if I remove these patches).

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

Unfortunately... that said, in testing I did just find a regression
elsewhere with suspend/resume on HiKey, where we'll not go all the way
to suspend if wifi is enabled. So If anyone else is having trouble
testing these patches on HiKey, turn off wifi for now.

thanks
-john