On 26 June 2017 at 21:39, Grygorii Strashko [off-list ref] wrote:

On 06/26/2017 11:49 AM, Ulf Hansson wrote:

quoted

On 23 June 2017 at 00:01, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson [off-list ref] wrote:

quoted

On 22 June 2017 at 16:41, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:

quoted

On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:

quoted

On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson [off-list ref] wrote:

quoted

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.

quoted

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