* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-12 23:08 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-12 23:08 UTC (permalink / raw)
To: Tony Lindgren, paul
Cc: rnayak, khilman, linux-omap, linux-arm-kernel, linux-kernel,
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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-12 23:08 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-12 23:08 UTC (permalink / raw)
To: Tony Lindgren, paul
Cc: Nishanth Menon, khilman, rnayak, linux-kernel, linux-omap,
linux-arm-kernel
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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-12 23:08 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-12 23:08 UTC (permalink / raw)
To: linux-arm-kernel
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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-12 23:08 ` Nishanth Menon
(?)
@ 2013-11-13 12:51 ` Felipe Balbi
-1 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 12:51 UTC (permalink / raw)
To: Nishanth Menon
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 12:51 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 12:51 UTC (permalink / raw)
To: Nishanth Menon
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 12:51 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 12:51 UTC (permalink / raw)
To: linux-arm-kernel
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131113/04bc436c/attachment.sig>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-13 12:51 ` Felipe Balbi
(?)
@ 2013-11-13 14:56 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 14:56 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 14:56 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 14:56 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 14:56 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 14:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-13 14:56 ` Nishanth Menon
(?)
@ 2013-11-13 15:20 ` Felipe Balbi
-1 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 15:20 UTC (permalink / raw)
To: Nishanth Menon
Cc: balbi, Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]
On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
> 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?
you mean:
WARN(pm_runtime_set_active(dev)); ?
sounds good
thanks
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:20 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 15:20 UTC (permalink / raw)
To: Nishanth Menon
Cc: paul, khilman, Tony Lindgren, rnayak, linux-kernel, balbi,
linux-omap, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1811 bytes --]
On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
> 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?
you mean:
WARN(pm_runtime_set_active(dev)); ?
sounds good
thanks
--
balbi
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:20 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-13 15:20 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
> 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?
you mean:
WARN(pm_runtime_set_active(dev)); ?
sounds good
thanks
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131113/01bd71b0/attachment.sig>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-13 15:20 ` Felipe Balbi
(?)
@ 2013-11-13 15:28 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 15:28 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
On 11/13/2013 09:20 AM, Felipe Balbi wrote:
> On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
>> 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?
>
> 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.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:28 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 15:28 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
On 11/13/2013 09:20 AM, Felipe Balbi wrote:
> On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
>> 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?
>
> 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.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:28 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 15:28 UTC (permalink / raw)
To: linux-arm-kernel
On 11/13/2013 09:20 AM, Felipe Balbi wrote:
> On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
>> 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?
>
> 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.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-13 14:56 ` Nishanth Menon
(?)
@ 2013-11-13 15:45 ` Kevin Hilman
-1 siblings, 0 replies; 44+ messages in thread
From: Kevin Hilman @ 2013-11-13 15:45 UTC (permalink / raw)
To: Nishanth Menon
Cc: balbi, Tony Lindgren, paul, rnayak, linux-omap, linux-arm-kernel,
linux-kernel
Nishanth Menon <nm@ti.com> writes:
> 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?
And also a note in the changelog (or comment at the WARN) about the
assumption that runtime PM is disabled at this point.
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:45 ` Kevin Hilman
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Hilman @ 2013-11-13 15:45 UTC (permalink / raw)
To: Nishanth Menon
Cc: balbi, Tony Lindgren, paul, rnayak, linux-omap, linux-arm-kernel,
linux-kernel
Nishanth Menon <nm@ti.com> writes:
> 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?
And also a note in the changelog (or comment at the WARN) about the
assumption that runtime PM is disabled at this point.
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 15:45 ` Kevin Hilman
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Hilman @ 2013-11-13 15:45 UTC (permalink / raw)
To: linux-arm-kernel
Nishanth Menon <nm@ti.com> writes:
> 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?
And also a note in the changelog (or comment at the WARN) about the
assumption that runtime PM is disabled at this point.
Kevin
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-13 15:45 ` Kevin Hilman
(?)
@ 2013-11-13 16:19 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 16:19 UTC (permalink / raw)
To: Kevin Hilman
Cc: balbi, Tony Lindgren, Paul Walmsley, Rajendra Nayak, linux-omap,
linux-arm-kernel, lkml
On Wed, Nov 13, 2013 at 9:45 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> 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?
>
> And also a note in the changelog (or comment at the WARN) about the
> assumption that runtime PM is disabled at this point.
Ofcourse. will do.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 16:19 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 16:19 UTC (permalink / raw)
To: Kevin Hilman
Cc: balbi, Tony Lindgren, Paul Walmsley, Rajendra Nayak, linux-omap,
linux-arm-kernel, lkml
On Wed, Nov 13, 2013 at 9:45 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> 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?
>
> And also a note in the changelog (or comment at the WARN) about the
> assumption that runtime PM is disabled at this point.
Ofcourse. will do.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-13 16:19 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-13 16:19 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 13, 2013 at 9:45 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> 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?
>
> And also a note in the changelog (or comment at the WARN) about the
> assumption that runtime PM is disabled at this point.
Ofcourse. will do.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-12 23:08 ` Nishanth Menon
(?)
@ 2013-11-14 17:05 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 17:05 UTC (permalink / raw)
To: Tony Lindgren, paul
Cc: rnayak, khilman, balbi, linux-omap, linux-arm-kernel,
linux-kernel, 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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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));
pm_generic_runtime_resume(dev);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 17:05 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 17:05 UTC (permalink / raw)
To: Tony Lindgren, paul
Cc: Nishanth Menon, khilman, rnayak, linux-kernel, balbi, linux-omap,
linux-arm-kernel
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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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));
pm_generic_runtime_resume(dev);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 17:05 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@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));
pm_generic_runtime_resume(dev);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-14 17:05 ` Nishanth Menon
(?)
@ 2013-11-14 18:55 ` Felipe Balbi
-1 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-14 18:55 UTC (permalink / raw)
To: Nishanth Menon
Cc: Tony Lindgren, paul, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4001 bytes --]
Hi,
On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
> 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-keerthy@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Acked-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Kevin Hilman <khilman@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));
if you want to print the device name, how about dev_WARN() ?
no strong feelings though:
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 18:55 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-14 18:55 UTC (permalink / raw)
To: Nishanth Menon
Cc: paul, khilman, Tony Lindgren, rnayak, linux-kernel, balbi,
linux-omap, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4001 bytes --]
Hi,
On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
> 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-keerthy@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Acked-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Kevin Hilman <khilman@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));
if you want to print the device name, how about dev_WARN() ?
no strong feelings though:
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 18:55 ` Felipe Balbi
0 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2013-11-14 18:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
> 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-keerthy@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Acked-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Kevin Hilman <khilman@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));
if you want to print the device name, how about dev_WARN() ?
no strong feelings though:
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131114/843ad991/attachment-0001.sig>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-14 18:55 ` Felipe Balbi
(?)
@ 2013-11-14 19:30 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 19:30 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
On 11/14/2013 12:55 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@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));
>
> if you want to print the device name, how about dev_WARN() ?
>
> no strong feelings though:
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 <balbi@ti.com>
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 19:30 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 19:30 UTC (permalink / raw)
To: balbi
Cc: Tony Lindgren, paul, rnayak, khilman, linux-omap,
linux-arm-kernel, linux-kernel
On 11/14/2013 12:55 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@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));
>
> if you want to print the device name, how about dev_WARN() ?
>
> no strong feelings though:
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 <balbi@ti.com>
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-14 19:30 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-14 19:30 UTC (permalink / raw)
To: linux-arm-kernel
On 11/14/2013 12:55 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote:
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@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));
>
> if you want to print the device name, how about dev_WARN() ?
>
> no strong feelings though:
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 <balbi@ti.com>
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-14 17:05 ` Nishanth Menon
@ 2013-11-15 8:07 ` Paul Walmsley
-1 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2013-11-15 8:07 UTC (permalink / raw)
To: Nishanth Menon
Cc: Tony Lindgren, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
On Thu, 14 Nov 2013, Nishanth Menon wrote:
> 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-keerthy@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Acked-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Kevin Hilman <khilman@linaro.org>
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 8:07 ` Paul Walmsley
0 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2013-11-15 8:07 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 14 Nov 2013, Nishanth Menon wrote:
> 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-keerthy@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Acked-by: Rajendra Nayak <rnayak@ti.com>
> Acked-by: Kevin Hilman <khilman@linaro.org>
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-15 8:07 ` Paul Walmsley
(?)
@ 2013-11-15 13:30 ` Nishanth Menon
-1 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-15 13:30 UTC (permalink / raw)
To: Paul Walmsley
Cc: Tony Lindgren, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> On Thu, 14 Nov 2013, Nishanth Menon wrote:
>
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@linaro.org>
>
> Looks reasonable to me. Looks like this should be considered for -stable
> - Nishanth, what do you think?
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 13:30 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-15 13:30 UTC (permalink / raw)
To: Paul Walmsley
Cc: Tony Lindgren, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> On Thu, 14 Nov 2013, Nishanth Menon wrote:
>
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@linaro.org>
>
> Looks reasonable to me. Looks like this should be considered for -stable
> - Nishanth, what do you think?
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 13:30 ` Nishanth Menon
0 siblings, 0 replies; 44+ messages in thread
From: Nishanth Menon @ 2013-11-15 13:30 UTC (permalink / raw)
To: linux-arm-kernel
On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> On Thu, 14 Nov 2013, Nishanth Menon wrote:
>
>> 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-keerthy@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> Acked-by: Kevin Hilman <khilman@linaro.org>
>
> Looks reasonable to me. Looks like this should be considered for -stable
> - Nishanth, what do you think?
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-15 13:30 ` Nishanth Menon
@ 2013-11-15 14:37 ` Tony Lindgren
-1 siblings, 0 replies; 44+ messages in thread
From: Tony Lindgren @ 2013-11-15 14:37 UTC (permalink / raw)
To: Nishanth Menon
Cc: Paul Walmsley, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
* Nishanth Menon <nm@ti.com> [131115 05:30]:
> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
> >
> >> 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-keerthy@ti.com>
> >> Signed-off-by: Nishanth Menon <nm@ti.com>
> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
> >> Acked-by: Kevin Hilman <khilman@linaro.org>
> >
> > Looks reasonable to me. Looks like this should be considered for -stable
> > - Nishanth, what do you think?
>
> 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?
I can take it unless you have other fixes pending right now.
Tony
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 14:37 ` Tony Lindgren
0 siblings, 0 replies; 44+ messages in thread
From: Tony Lindgren @ 2013-11-15 14:37 UTC (permalink / raw)
To: linux-arm-kernel
* Nishanth Menon <nm@ti.com> [131115 05:30]:
> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
> >
> >> 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-keerthy@ti.com>
> >> Signed-off-by: Nishanth Menon <nm@ti.com>
> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
> >> Acked-by: Kevin Hilman <khilman@linaro.org>
> >
> > Looks reasonable to me. Looks like this should be considered for -stable
> > - Nishanth, what do you think?
>
> 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?
I can take it unless you have other fixes pending right now.
Tony
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-15 14:37 ` Tony Lindgren
@ 2013-11-15 20:04 ` Paul Walmsley
-1 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2013-11-15 20:04 UTC (permalink / raw)
To: Tony Lindgren
Cc: Nishanth Menon, rnayak, khilman, balbi, linux-omap,
linux-arm-kernel, linux-kernel
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 <paul@pwsan.com>
- Paul
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 20:04 ` Paul Walmsley
0 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2013-11-15 20:04 UTC (permalink / raw)
To: linux-arm-kernel
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 <paul@pwsan.com>
- Paul
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-15 14:37 ` Tony Lindgren
@ 2013-11-15 22:03 ` Kevin Hilman
-1 siblings, 0 replies; 44+ messages in thread
From: Kevin Hilman @ 2013-11-15 22:03 UTC (permalink / raw)
To: Tony Lindgren
Cc: Nishanth Menon, Paul Walmsley, rnayak, balbi, linux-omap,
linux-arm-kernel, linux-kernel
Tony Lindgren <tony@atomide.com> writes:
> * Nishanth Menon <nm@ti.com> [131115 05:30]:
>> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
>> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
>> >
>> >> 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-keerthy@ti.com>
>> >> Signed-off-by: Nishanth Menon <nm@ti.com>
>> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> >> Acked-by: Kevin Hilman <khilman@linaro.org>
>> >
>> > Looks reasonable to me. Looks like this should be considered for -stable
>> > - Nishanth, what do you think?
>>
>> 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?
>
> I can take it unless you have other fixes pending right now.
This version looks good to me.
Reviewed-by: Kevin Hilman <khilman@linaro.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-15 22:03 ` Kevin Hilman
0 siblings, 0 replies; 44+ messages in thread
From: Kevin Hilman @ 2013-11-15 22:03 UTC (permalink / raw)
To: linux-arm-kernel
Tony Lindgren <tony@atomide.com> writes:
> * Nishanth Menon <nm@ti.com> [131115 05:30]:
>> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
>> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
>> >
>> >> 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-keerthy@ti.com>
>> >> Signed-off-by: Nishanth Menon <nm@ti.com>
>> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
>> >> Acked-by: Kevin Hilman <khilman@linaro.org>
>> >
>> > Looks reasonable to me. Looks like this should be considered for -stable
>> > - Nishanth, what do you think?
>>
>> 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?
>
> I can take it unless you have other fixes pending right now.
This version looks good to me.
Reviewed-by: Kevin Hilman <khilman@linaro.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
2013-11-15 22:03 ` Kevin Hilman
(?)
@ 2013-11-19 11:34 ` Ulf Hansson
-1 siblings, 0 replies; 44+ messages in thread
From: Ulf Hansson @ 2013-11-19 11:34 UTC (permalink / raw)
To: Kevin Hilman
Cc: Tony Lindgren, Nishanth Menon, Paul Walmsley, Rajendra Nayak,
Felipe Balbi, linux-omap, linux-arm-kernel, linux-kernel
On 15 November 2013 23:03, Kevin Hilman <khilman@linaro.org> wrote:
> Tony Lindgren <tony@atomide.com> writes:
>
>> * Nishanth Menon <nm@ti.com> [131115 05:30]:
>>> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
>>> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
>>> >
>>> >> 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-keerthy@ti.com>
>>> >> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
>>> >> Acked-by: Kevin Hilman <khilman@linaro.org>
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
>>> >
>>> > Looks reasonable to me. Looks like this should be considered for -stable
>>> > - Nishanth, what do you think?
>>>
>>> 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?
>>
>> I can take it unless you have other fixes pending right now.
>
> This version looks good to me.
>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-19 11:34 ` Ulf Hansson
0 siblings, 0 replies; 44+ messages in thread
From: Ulf Hansson @ 2013-11-19 11:34 UTC (permalink / raw)
To: Kevin Hilman
Cc: Tony Lindgren, Nishanth Menon, Paul Walmsley, Rajendra Nayak,
Felipe Balbi, linux-omap, linux-arm-kernel, linux-kernel
On 15 November 2013 23:03, Kevin Hilman <khilman@linaro.org> wrote:
> Tony Lindgren <tony@atomide.com> writes:
>
>> * Nishanth Menon <nm@ti.com> [131115 05:30]:
>>> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
>>> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
>>> >
>>> >> 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-keerthy@ti.com>
>>> >> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
>>> >> Acked-by: Kevin Hilman <khilman@linaro.org>
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
>>> >
>>> > Looks reasonable to me. Looks like this should be considered for -stable
>>> > - Nishanth, what do you think?
>>>
>>> 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?
>>
>> I can take it unless you have other fixes pending right now.
>
> This version looks good to me.
>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume
@ 2013-11-19 11:34 ` Ulf Hansson
0 siblings, 0 replies; 44+ messages in thread
From: Ulf Hansson @ 2013-11-19 11:34 UTC (permalink / raw)
To: linux-arm-kernel
On 15 November 2013 23:03, Kevin Hilman <khilman@linaro.org> wrote:
> Tony Lindgren <tony@atomide.com> writes:
>
>> * Nishanth Menon <nm@ti.com> [131115 05:30]:
>>> On 11/15/2013 02:07 AM, Paul Walmsley wrote:
>>> > On Thu, 14 Nov 2013, Nishanth Menon wrote:
>>> >
>>> >> 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-keerthy@ti.com>
>>> >> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> >> Acked-by: Rajendra Nayak <rnayak@ti.com>
>>> >> Acked-by: Kevin Hilman <khilman@linaro.org>
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
>>> >
>>> > Looks reasonable to me. Looks like this should be considered for -stable
>>> > - Nishanth, what do you think?
>>>
>>> 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?
>>
>> I can take it unless you have other fixes pending right now.
>
> This version looks good to me.
>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2013-11-19 11:34 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-12 23:08 [PATCH V2] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume Nishanth Menon
2013-11-12 23:08 ` Nishanth Menon
2013-11-12 23:08 ` Nishanth Menon
2013-11-13 12:51 ` Felipe Balbi
2013-11-13 12:51 ` Felipe Balbi
2013-11-13 12:51 ` Felipe Balbi
2013-11-13 14:56 ` Nishanth Menon
2013-11-13 14:56 ` Nishanth Menon
2013-11-13 14:56 ` Nishanth Menon
2013-11-13 15:20 ` Felipe Balbi
2013-11-13 15:20 ` Felipe Balbi
2013-11-13 15:20 ` Felipe Balbi
2013-11-13 15:28 ` Nishanth Menon
2013-11-13 15:28 ` Nishanth Menon
2013-11-13 15:28 ` Nishanth Menon
2013-11-13 15:45 ` Kevin Hilman
2013-11-13 15:45 ` Kevin Hilman
2013-11-13 15:45 ` Kevin Hilman
2013-11-13 16:19 ` Nishanth Menon
2013-11-13 16:19 ` Nishanth Menon
2013-11-13 16:19 ` Nishanth Menon
2013-11-14 17:05 ` [PATCH V3] " Nishanth Menon
2013-11-14 17:05 ` Nishanth Menon
2013-11-14 17:05 ` Nishanth Menon
2013-11-14 18:55 ` Felipe Balbi
2013-11-14 18:55 ` Felipe Balbi
2013-11-14 18:55 ` Felipe Balbi
2013-11-14 19:30 ` Nishanth Menon
2013-11-14 19:30 ` Nishanth Menon
2013-11-14 19:30 ` Nishanth Menon
2013-11-15 8:07 ` Paul Walmsley
2013-11-15 8:07 ` Paul Walmsley
2013-11-15 13:30 ` Nishanth Menon
2013-11-15 13:30 ` Nishanth Menon
2013-11-15 13:30 ` Nishanth Menon
2013-11-15 14:37 ` Tony Lindgren
2013-11-15 14:37 ` Tony Lindgren
2013-11-15 20:04 ` Paul Walmsley
2013-11-15 20:04 ` Paul Walmsley
2013-11-15 22:03 ` Kevin Hilman
2013-11-15 22:03 ` Kevin Hilman
2013-11-19 11:34 ` Ulf Hansson
2013-11-19 11:34 ` Ulf Hansson
2013-11-19 11:34 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.