On Thu, Apr 20, 2017 at 9:25 AM, Jarkko Nikula
<jarkko.nikula@xxxxxxxxxxxxxxx> wrote:
> Hi
>
>
> On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote:
>>
>> On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula
>> <jarkko.nikula@xxxxxxxxxxxxxxx> wrote:
>>>
>>> There is possibility to enter dw_i2c_plat_suspend() callback twice
>>> during system suspend under certain cases which is causing here warnings
>>> from clk_core_disable() and clk_core_unprepare() as well as accessing the
>>> registers that can be power gated.
>>>
>>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
>>> system suspend") implemented a prepare callback that checks for runtime
>>> suspended device which allow PM core to set direct_complete flag and
>>> skip system suspend and resume callbacks.
>>>
>>> However it can still happen if nothing resumes the device prior system
>>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
>>> unsets the direct_complete flag of the parent in __device_suspend() thus
>>> causing PM code to not skip the system suspend/resume callbacks.
>>>
>>> Avoid this by checking runtime status in suspend and resume callbacks
>>> and return directly if device is runtime suspended. This affects only
>>> system suspend/resume since during runtime suspend/resume runtime status
>>> is suspending (not suspended) or resuming.
>>>
>>> Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
>>> ---
>>> I'm able to trigger system suspend callback while device is runtime
>>> suspended by removing the pm_runtime_resume() call from
>>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
>>> slave (ACPI enumerated but doesn't bind due an error in probe function).
>>> In that case __device_suspend() for that unbound device has NULL suspend
>>> callback, and thus doesn't cause any runtime resume chain but still
>>> unsets
>>> the parent's direct_complete flag.
>>> John Stult <john.stultz@xxxxxxxxxx> has reported he can trigger this on
>>> HiKey board too.
>>>
>>> I'm not sure is this the right thing to do. It feels something the PM
>>> core
>>> should do but I'm not sure that either. One alternative could be to
>>> resume
>>> runtime suspended parent in in __device_suspend() right after where
>>> parent's direct_complete flag is unset.
>>
>>
>> In that case the core expects that the ->prepare callback for the
>> slave will also return 1 (or a positive number in general).
>>
>> If that doesn't happen, then from the core's perspective it is not
>> safe to allow the master's system PM callbacks to be skipped and
>> that's why direct_complete is unset for it.
>>
> So it's then right thing to check runtime PM status in driver as patch does
> below?
If you know for a fact that none of the device's children and none of
the children thereof and so on and nothing that may depend on the
device via a device_link, either directly or indirectly, will ever
need to be resumed during system suspend, then yes, it is.
Otherwise, no, it isn't.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
