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

[Nishanth Menon]

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.

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-ke...@ti.com>
Signed-off-by: Nishanth Menon <n...@ti.com>
Acked-by: Rajendra Nayak <rna...@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)) {
+ /* NOTE: *might* indicate driver race */
+ dev_dbg(dev, "%s: Force suspending\n",
+ __func__);
+ pm_runtime_set_suspended(dev);
+ od->flags |= OMAP_DEVICE_SUSPEND_FORCED;
+ }
omap_device_idle(pdev);
od->flags |= OMAP_DEVICE_SUSPENDED;
}
@@ -634,10 +641,15 @@ static int _od_resume_noirq(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct omap_device *od = to_omap_device(pdev);

- if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
- !pm_runtime_status_suspended(dev)) {
+ if (od->flags & OMAP_DEVICE_SUSPENDED) {
od->flags &= ~OMAP_DEVICE_SUSPENDED;
omap_device_enable(pdev);
+
+ if (od->flags & OMAP_DEVICE_SUSPEND_FORCED) {
+ pm_runtime_set_active(dev);
+ od->flags &= ~OMAP_DEVICE_SUSPEND_FORCED;
+ }
+
pm_generic_runtime_resume(dev);
}

diff --git a/arch/arm/mach-omap2/omap_device.h b/arch/arm/mach-omap2/omap_device.h
index 17ca1ae..45885b0 100644
--- a/arch/arm/mach-omap2/omap_device.h
+++ b/arch/arm/mach-omap2/omap_device.h
@@ -38,6 +38,7 @@ extern struct dev_pm_domain omap_device_pm_domain;

/* omap_device.flags values */
#define OMAP_DEVICE_SUSPENDED BIT(0)
+#define OMAP_DEVICE_SUSPEND_FORCED BIT(1)

/**
* struct omap_device - omap_device wrapper for platform_devices
--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Kevin Hilman]

Nishanth Menon <n...@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.

That being said, I agree that omap_device should still be catching this
case in order to find/fix driver races like this.

Why the addition check for supended here?

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.

> + /* NOTE: *might* indicate driver race */

Yes, a driver race which should then be fixed in the driver.

> + 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.

Kevin

[Nishanth Menon]

i2c dropped it generic suspend handlers at
commit 584b408d37af4e0b38ad5b60f236381bcdf396bc
Author: Kevin Hilman <khi...@ti.com>
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.
- 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).

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.

>
>> + /* 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.

>
>> + 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..

--
Regards,
Nishanth Menon

[Kevin Hilman]

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.

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

[Nishanth Menon]

On 16:38-20131107, Kevin Hilman wrote:
[...]

> 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.
>

Agreed. some drivers like edma need to save and restore context around
suspend.
[...]

> 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.
>

Done and it still does work, makes sense since it just ensures that
runtime PM's dev->power.runtime_status is set to RPM_SUSPENDED instead
of RPM_ACTIVE for devices that depend on autosuspend.

Logs (based on vendor kernel which has relevant out of tree patches to
enable suspend resume - still in the works):
AM335x-BBB: http://pastie.org/8472182
OMAP5-uEVM: http://pastie.org/8472183

> >>
> >>> + /* 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.

Yes, you are right, originally, I had intended this to indicate devices
that needed to be runtime_status updated, but then, now I realize that
it is true for all devices that have OMAP_DEVICE_SUSPEND set. It can be
applied without an additional flag. Do see if the updated patch is more
sensible:
-- >8 --
From 96b5a7b89fef4ba55bca48bae83e5536d697c6c4 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <n...@ti.com>
Date: Thu, 24 Oct 2013 09:12:42 -0500
Subject: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status
around suspend/resume

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.

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-ke...@ti.com>
Signed-off-by: Nishanth Menon <n...@ti.com>
Acked-by: Rajendra Nayak <rna...@ti.com>
---

arch/arm/mach-omap2/omap_device.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index b69dd9a..f97b34b 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)

if (!ret && !pm_runtime_status_suspended(dev)) {
if (pm_generic_runtime_suspend(dev) == 0) {

+ pm_runtime_set_suspended(dev);

omap_device_idle(pdev);
od->flags |= OMAP_DEVICE_SUSPENDED;
}

@@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)

struct platform_device *pdev = to_platform_device(dev);
struct omap_device *od = to_omap_device(pdev);

- if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
- !pm_runtime_status_suspended(dev)) {
+ if (od->flags & OMAP_DEVICE_SUSPENDED) {
od->flags &= ~OMAP_DEVICE_SUSPENDED;
omap_device_enable(pdev);

+ pm_runtime_set_active(dev);
pm_generic_runtime_resume(dev);
}

--
1.7.9.5

--
Regards,
Nishanth Menon

[Kevin Hilman]

Yes, the updated version looks much more sensible. Please repost in its
own thread so it gets a better chance at broader review, and feel free
to add

Acked-by: Kevin Hilman <khi...@linaro.org>

Kevin

[Nishanth Menon]

Acked-by: Kevin Hilman <khi...@linaro.org>
---
Changes in V2 (resend):
- dropped the unnecessary flag for runtime status restore
- picked up kevin's ack from https://patchwork.kernel.org/patch/3168371/

v1: https://patchwork.kernel.org/patch/3154501/

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

[Nishanth Menon]

On 11/12/2013 03:26 PM, Kevin Hilman wrote:
[..]

> Yes, the updated version looks much more sensible. Please repost in its
> own thread so it gets a better chance at broader review, and feel free
> to add
>
> Acked-by: Kevin Hilman <khi...@linaro.org>

Thanks. this is done[1]

[1] https://patchwork.kernel.org/patch/3176501/


--
Regards,
Nishanth Menon

[Felipe Balbi]

Hi,

On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index b69dd9a..f97b34b 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
>
> if (!ret && !pm_runtime_status_suspended(dev)) {
> if (pm_generic_runtime_suspend(dev) == 0) {
> + pm_runtime_set_suspended(dev);

don't you have to disable pm_runtime around status changes ? Or is
pm_runtime already disabled by the time we get here ?

> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
> struct platform_device *pdev = to_platform_device(dev);
> struct omap_device *od = to_omap_device(pdev);
>
> - if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
> - !pm_runtime_status_suspended(dev)) {
> + if (od->flags & OMAP_DEVICE_SUSPENDED) {
> od->flags &= ~OMAP_DEVICE_SUSPENDED;
> omap_device_enable(pdev);
> + pm_runtime_set_active(dev);

ditto, also pm_runtime_set_active() may fail.

--
balbi

[Nishanth Menon]

On 11/13/2013 06:51 AM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index b69dd9a..f97b34b 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
>>
>> if (!ret && !pm_runtime_status_suspended(dev)) {
>> if (pm_generic_runtime_suspend(dev) == 0) {
>> + pm_runtime_set_suspended(dev);
>
> don't you have to disable pm_runtime around status changes ? Or is
> pm_runtime already disabled by the time we get here ?

pm_runtime is already disabled by the time no_irq suspend is invoked.

>
>> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
>> struct platform_device *pdev = to_platform_device(dev);
>> struct omap_device *od = to_omap_device(pdev);
>>
>> - if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>> - !pm_runtime_status_suspended(dev)) {
>> + if (od->flags & OMAP_DEVICE_SUSPENDED) {
>> od->flags &= ~OMAP_DEVICE_SUSPENDED;
>> omap_device_enable(pdev);
>> + pm_runtime_set_active(dev);
>
> ditto, also pm_runtime_set_active() may fail.
>

again, pm_runtime is not yet active here yet - we just restore the pm
runtime state with which we went down with -> and that is not expected
to fail either - So, how about just adding a WARN if our expectation
of balanced operation was somehow broken in the future with changes to
runtime framework?

--
Regards,
Nishanth Menon

[Nishanth Menon]

On 11/13/2013 09:20 AM, Felipe Balbi wrote:

> you mean:
>
> WARN(pm_runtime_set_active(dev)); ?

yes

>
> sounds good

Thanks. Will post a v3 tomorrow morning to give a chance for
discussions on further comments if any.

[Felipe Balbi]

On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:

you mean:

WARN(pm_runtime_set_active(dev)); ?

sounds good

thanks

--
balbi

[Kevin Hilman]

And also a note in the changelog (or comment at the WARN) about the
assumption that runtime PM is disabled at this point.

Kevin

[Nishanth Menon]

Ofcourse. will do.

--
Regards,
Nishanth Menon

[Nishanth Menon]

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.

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. These operations are not expected
to fail as we update the states after the core runtime framework has
suspended itself and restore before the core runtime framework has
resumed.

Reported-by: J Keerthy <j-ke...@ti.com>
Signed-off-by: Nishanth Menon <n...@ti.com>
Acked-by: Rajendra Nayak <rna...@ti.com>
Acked-by: Kevin Hilman <khi...@linaro.org>
---

Changes in V3:
- Added WARN in case of unexpected failure of runtime pm status restore
v2: https://patchwork.kernel.org/patch/3176501/

v1: https://patchwork.kernel.org/patch/3154501/

patch baseline: V3.12 tag (also applies on linux-next next-20131107 tag)

arch/arm/mach-omap2/omap_device.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index b69dd9a..53f0735 100644

--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)

if (!ret && !pm_runtime_status_suspended(dev)) {
if (pm_generic_runtime_suspend(dev) == 0) {
+ pm_runtime_set_suspended(dev);
omap_device_idle(pdev);
od->flags |= OMAP_DEVICE_SUSPENDED;
}

@@ -634,10 +635,18 @@ static int _od_resume_noirq(struct device *dev)

struct platform_device *pdev = to_platform_device(dev);
struct omap_device *od = to_omap_device(pdev);

- if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
- !pm_runtime_status_suspended(dev)) {
+ if (od->flags & OMAP_DEVICE_SUSPENDED) {
od->flags &= ~OMAP_DEVICE_SUSPENDED;
omap_device_enable(pdev);

+ /*
+ * XXX: we run before core runtime pm has resumed itself. At
+ * this point in time, we just restore the runtime pm state and
+ * considering symmetric operations in resume, we donot expect
+ * to fail. If we failed, something changed in core runtime_pm
+ * framework OR some device driver messed things up, hence, WARN
+ */
+ WARN(pm_runtime_set_active(dev),
+ "Could not set %s runtime state active\n", dev_name(dev));

[Felipe Balbi]

Hi,

if you want to print the device name, how about dev_WARN() ?

no strong feelings though:

Reviewed-by: Felipe Balbi <ba...@ti.com>

--
balbi

[Nishanth Menon]

I would like to stick with WARN as dev_WARN would probably need an
condition check.. unless someone has a strong opinion, I dont see it
adding value here.

>
> Reviewed-by: Felipe Balbi <ba...@ti.com>
>


--

Regards,
Nishanth Menon

[Paul Walmsley]

Looks reasonable to me. Looks like this should be considered for -stable
- Nishanth, what do you think?

Tony or Kevin, do you want to take this one, or want me to?


- Paul

[Nishanth Menon]

Every product kernel since 3.4 needed to be hacked (we have hacked in
different ways so far) to work around this (since we never spend time
digging deeper :( ), So, I do agree with your view that a -stable tag
will be most beneficial.

>
> Tony or Kevin, do you want to take this one, or want me to?
>
>
> - Paul
>


--

Regards,
Nishanth Menon

[Tony Lindgren]

* Nishanth Menon <n...@ti.com> [131115 05:30]:

I can take it unless you have other fixes pending right now.

Tony

[Paul Walmsley]

On Fri, 15 Nov 2013, Tony Lindgren wrote:

> I can take it unless you have other fixes pending right now.

Ran a quick test with the patch applied on v3.12, results here:

http://www.pwsan.com/omap/testlogs/test_nm_omap_device_fix_v3.12-rc/20131115123132/

No other fixes queued here right now, so:

Acked-by: Paul Walmsley <pa...@pwsan.com>


- Paul

[Kevin Hilman]

This version looks good to me.

Reviewed-by: Kevin Hilman <khi...@linaro.org>

[Ulf Hansson]

Hi Kevin,

I am wondering if OMAP would benefit from letting the PM core allow
runtime suspends during system suspend, which is not the case as of
now.

My impression is that it could simplify OMAP drivers and the power
domain, but it would be interesting to hear your opinion in this. It
somewhat touches this patch. I guess.

The reason for bringing this up here, is that I wanted to highlight
that we are at the moment discussing the following RFC on the linux-pm
mailing list:

"[RFC PATCH] PM / Runtime: Allow to inactivate devices during system suspend".

Kind regards
Ulf Hansson