On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
