Re: [PATCH] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume

[Kevin Hilman <khilman@linaro.org>]

Nishanth Menon <nm@ti.com> writes:

> On 11/07/2013 05:44 PM, Kevin Hilman wrote:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> OMAP device hooks around suspend|resume_noirq ensures that hwmod
>>> devices are forced to idle using omap_device_idle/enable as part of
>>> the last stage of suspend activity.
>>>
>>> For a device such as i2c who uses autosuspend, it is possible to enter
>>> the suspend path with dev->power.runtime_status = RPM_ACTIVE.
>>>
>>> As part of the suspend flow, the generic runtime logic would increment
>>> it's dev->power.disable_depth to 1. This should prevent further
>>> pm_runtime_get_sync from succeeding once the runtime_status has been
>>> set to RPM_SUSPENDED.
>>>
>>> Now, as part of the suspend_noirq handler in omap_device, we force the
>>> following: if the device status is !suspended, we force the device
>>> to idle using omap_device_idle (clocks are cut etc..). This ensures
>>> that from a hardware perspective, the device is "suspended". However,
>>> runtime_status is left to be active.
>>>
>>> *if* an operation is attempted after this point to
>>> pm_runtime_get_sync, runtime framework depends on runtime_status to
>>> indicate accurately the device status, and since it sees it to be
>>> ACTIVE, it assumes the module is functional and returns a non-error
>>> value. As a result the user will see pm_runtime_get succeed, however a
>>> register access will crash due to the lack of clocks.
>>
>> Ouch.
>>
>> Dumb Q: who is requesting an i2c transaction after ->suspend_noirq().
>> The i2c driver itself should be able to detect that it's being accessed
>> after this point and return an error.
>
> i2c dropped it generic suspend handlers at
> commit 584b408d37af4e0b38ad5b60f236381bcdf396bc
> Author: Kevin Hilman <khilman@ti.com>

sheesh, who is that crazy TI guy.  They should probably fire him.

> Date:   Thu Aug 4 07:53:02 2011 -0700
>
>     Revert "i2c-omap: fix static suspend vs. runtime suspend"
>
>     Also, as of v3.1, the PM domain level code for OMAP handles device
>     power state transistions automatically for devices, so drivers no
>     longer need to specifically call the bus/pm_domain methods themselves.
>
>
> - So it is rightly in the mercy of runtime PM to adequately represent
> it's state. I disagree that i2c driver should in addition have to
> remember what it's generic suspend state is.

That's debatable I guess.  The ideal world is that runtime PM hides all
of this, but I'm not sure it's achievable in all cases.

>  - See the link to the cpufreq and the logs to see the call stack
> where it fails.
>
> Now, this is fine, since omap_i2c_xfer should ideally fail with a
> pm_runtime_get_sync returning -EACCESS when device is really suspended
> (remember, we are doing autosuspend - so, in the case I caught, there
> was regulator voltage set prior to entering suspend, but timeout was
> not yet hit for autosuspend of i2c to kick in yet).

Ah, I see.  Makes sense.

>>
>> That being said, I agree that omap_device should still be catching this
>> case in order to find/fix driver races like this.
>
>>
>>> To prevent this from happening, we should ensure that runtime_status
>>> exactly indicates the device status. As a result of this change
>>> any further calls to pm_runtime_get* would return -EACCES (since
>>> disable_depth is 1). On resume, we restore the clocks and runtime
>>> status exactly as we suspended with.
>>>
>>> Reported-by: J Keerthy <j-keerthy@ti.com>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>>> ---
>>> patch baseline: V3.12 tag (also applies on linux-next next-20131107 tag)
>>>
>>> Logs from 3.12 based vendor kernel:
>>> Before: http://pastebin.com/m5KxnB7a
>>> After: http://pastebin.com/8AfX4e7r
>>>
>>> The discussion on cpufreq side of the story which triggered this scenario:
>>> 	http://marc.info/?l=linux-pm&m=138263811321921&w=2
>>>
>>> Tested on TI vendor kernel (with dt boot):
>>> 	AM335x: evm, BBB, sk, BBW
>>> 	OMAP5uEVM, DRA7-evm
>>>
>>>   arch/arm/mach-omap2/omap_device.c |   16 ++++++++++++++--
>>>   arch/arm/mach-omap2/omap_device.h |    1 +
>>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>> index b69dd9a..87ecbb0 100644
>>> --- a/arch/arm/mach-omap2/omap_device.c
>>> +++ b/arch/arm/mach-omap2/omap_device.c
>>> @@ -621,6 +621,13 @@ static int _od_suspend_noirq(struct device *dev)
>>>
>>>   	if (!ret && !pm_runtime_status_suspended(dev)) {
>>>   		if (pm_generic_runtime_suspend(dev) == 0) {
>>> +			if (!pm_runtime_suspended(dev)) {
>>
>> Why the addition check for supended here?
>
> pm_runtime_status_suspended checks only the dev->power.runtime_status
> but that may not truely indicate device was in previous use - in the
> case of devices like i2c who depend on autosuspend.
>
> pm_runtime_suspended checks for dev->power.runtime_status ==
> RPM_SUSPENDED and disable_depth
>
>>
>> This version (as opposed to the _status_suspended() version above will
>> fail if runtime PM has been disabled from userspace (via
>> /sys/devices/.../power/control), and will thus prevent low power states
>> from being hit in suspend if runtime suspend has been disabled from
>> userspace.  That's a bug.
>
> yes, and so it should fail to achieve low power state. we explicitly
> stated disable suspend, yet we go behind the runtime PM's back and
> force disable the module clocks. now drivers should NOT know what the
> state of the omap_device meddling is and should obey the configuration
> and completely trust pm_runtime to accurately denote the module state.

No, that sysfs knob is for disabling runtime PM.  We still want the
device to hit low-power state in system suspend.  Solving that problem
is half the reason we have this omap_device noirq mess in the first
place.

You need to test this by disabling runtime PM from userspace and ensure
that the low power state is still hit during suspend.

>>
>>> +				/* NOTE: *might* indicate driver race */
>>
>> Yes, a driver race which should then be fixed in the driver.
>
> true if this is a non-autosuspend device, in auto suspend devices,
> this could be a regular phenomenon if timeout is pretty large.. but
> atleast that should allow debug.

Agreed.  I wasn't thinking about the autosuspend case.  Thanks for
clarifying.

>>
>>> +				dev_dbg(dev, "%s: Force suspending\n",
>>> +					__func__);
>>> +				pm_runtime_set_suspended(dev);
>>> +				od->flags |= OMAP_DEVICE_SUSPEND_FORCED;
>>
>> Not sure why you need an additonal flag.  Why not just always do this
>> and use the existin OMAP_DEVICE_SUSPENDED flag.
>
> restore of runtime data structure state is only needed for specific
> devices - not all..

The question is why do you a flag in addition to OMAP_DEVICE_SUSPEND.
Whenever that flag is set, omap_device has gone behind your back, and
the runtime PM status should be kept in sync.

Kevin