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?
Thanks,
Rafael
