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

[Ulf Hansson]

On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael at kernel.org> wrote:
> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson at linaro.org> wrote:
>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw at 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 at 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