Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

Resending with the corrected mailing list

Hello Kevin,

I want to draw your attention to an issue the following patch introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc


arch/arm/mach-omap2/pm_bus.c  patch | blob | history 
diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
index 9719a9f..5e453dc 100644 (file)

--- a/arch/arm/mach-omap2/pm_bus.c
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -68,3 +68,51 @@ int platform_pm_runtime_idle(struct device *dev)
 };
 #endif /* CONFIG_PM_RUNTIME */
 
+#ifdef CONFIG_SUSPEND
+int platform_pm_suspend_noirq(struct device *dev)
+{
+       struct device_driver *drv = dev->driver;
+       int ret = 0;
+
+       if (!drv)
+               return 0;
+
+       if (drv->pm) {
+               if (drv->pm->suspend_noirq)
+                       ret = drv->pm->suspend_noirq(dev);
+       }
+
+       /*
+        * The DPM core has done a 'get' to prevent runtime PM
+        * transitions during system PM.  This put is to balance
+        * out that get so that this device can now be runtime
+        * suspended.
+        */
+       pm_runtime_put_sync(dev);
+
+       return ret;
+}
+
+int platform_pm_resume_noirq(struct device *dev)
+{
+       struct device_driver *drv = dev->driver;
+       int ret = 0;
+
+       /* 
+        * This 'get' is to balance the 'put' in the above suspend_noirq
+        * method so that the runtime PM usage counting is in the same
+        * state it was when suspend was called.
+        */
+       pm_runtime_get_sync(dev);
+
+       if (!drv)
+               return 0;
+
+       if (drv->pm) {
+               if (drv->pm->resume_noirq)
+                       ret = drv->pm->resume_noirq(dev);
+       }
+
+       return ret;
+}
+#endif /* CONFIG_SUSPEND */

I believe, it is not correct to call the pm_runtime APIs from interrupt locked context since the underlying functions __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call schedule().

Further, from a s/w layering standpoint, platform-bus is a deeper layer than the Runtime layer, which the runtime layer calls into. So it may not be correct to have this layer calling into pm_runtime(). It should directly call omap_device APIs.

We are facing a similar issue with a few drivers USB, UART & GPIO where, we need to enable/disable clocks & restore/save context in the CPU-Idle path with interrupts locked. 

As we are unable to use Runtime APIs, we are using omap_device APIs directly with the following modification in the .activate_funcion/ deactivate_funcion (example UART driver)

static int uart_idle_hwmod(struct omap_device *od)
{
	if (!irqs_disabled())
		omap_hwmod_idle(od->hwmods[0]);
	else
		_omap_hwmod_idle(od->hwmods[0]);

	return 0;
}

static int uart_enable_hwmod(struct omap_device *od)
{
	if (!irqs_disabled())
		omap_hwmod_enable(od->hwmods[0]);
	else
		_omap_hwmod_enable(od->hwmods[0]);

	return 0;
}

Can you feedback on my observations and comment on the approach taken for these drivers? (We will be posting out the refreshed patches for GPIO, WDT, Timers shortly).

Thanks
Partha

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

"Basak, Partha" <p-basak2@ti.com> writes:

> Resending with the corrected mailing list
>
> Hello Kevin,
>
> I want to draw your attention to an issue the following patch
> introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc

[...]

> I believe, it is not correct to call the pm_runtime APIs from
> interrupt locked context since the underlying functions
> __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
> schedule().
>
> Further, from a s/w layering standpoint, platform-bus is a deeper
> layer than the Runtime layer, which the runtime layer calls into. So
> it may not be correct to have this layer calling into pm_runtime(). It
> should directly call omap_device APIs.

The original version of this patch did directly call the omap_device
APIs.  However, Paul didn't like that approach and there were
use-counting problems to handle as well (e.g. if a driver was not (yet)
in use, it would already be disabled, and a suspend would trigger an
omap_device_disable() which would fail since device was already
disabled.)

Paul and I agreed that this current approach would solve the
use-counting problems, but you're correct in that it introduces
new problems as these functions can _possibly_ sleep/schedule.

That being said, have you actually seen problems here?   I would like
to see how they are triggered?

By the time the drivers _noirq hooks are called, the PM core has
shutdown all the drivers, prevented any runtime PM transitions and
disabled interrupts, so no pending runtime transitions will be queued
so the sleeping patch of the __pm_runtime_* should never be entered.

> We are facing a similar issue with a few drivers USB, UART & GPIO
> where, we need to enable/disable clocks & restore/save context in the
> CPU-Idle path with interrupts locked.

These are unrelated issues.  The above is for the static suspend/resume
case.  These are for runtime PM.

> As we are unable to use Runtime APIs, we are using omap_device APIs
> directly with the following modification in the .activate_funcion/
> deactivate_funcion (example UART driver)

[...]

The UART driver is a special case as it is managed from deep inside the
idle path.

> Can you feedback on my observations and comment on the approach taken
> for these drivers?

I cannot comment until I understand why these drivers need to
enable/disable with interrupts off.

In general, any use of the interrupts-off versions of the omap_device
APIs need to be thorougly justified.

Even the UART one will go away when the omap-serial driver is converted
to runtime PM.

Kevin

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, August 03, 2010 11:06 PM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> > Resending with the corrected mailing list
> >
> > Hello Kevin,
> >
> > I want to draw your attention to an issue the following patch
> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
> 
> [...]
> 
> > I believe, it is not correct to call the pm_runtime APIs from
> > interrupt locked context since the underlying functions
> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
> > schedule().
> >
> > Further, from a s/w layering standpoint, platform-bus is a deeper
> > layer than the Runtime layer, which the runtime layer calls into. So
> > it may not be correct to have this layer calling into pm_runtime(). It
> > should directly call omap_device APIs.
> 
> The original version of this patch did directly call the omap_device
> APIs.  However, Paul didn't like that approach and there were
> use-counting problems to handle as well (e.g. if a driver was not (yet)
> in use, it would already be disabled, and a suspend would trigger an
> omap_device_disable() which would fail since device was already
> disabled.)
> 
> Paul and I agreed that this current approach would solve the
> use-counting problems, but you're correct in that it introduces
> new problems as these functions can _possibly_ sleep/schedule.
> 
> That being said, have you actually seen problems here?   I would like
> to see how they are triggered?

Scenario 1:
Consider the case where a driver has already called pm_runtime_put_sync as part of aggressive clock cutting scheme. Subsequently, when system suspend is called, pm_runtime_put_sync is called again. Due to atomic_dec_and_test check on usage_count, the second call goes through w/o error. 

At System resume, pm_runtime_get_sync is called twice, once by resume, once by the driver itself. Because of the extra reference count, the aggressive clock control by the driver is broken, as call to put_sync by a driver reduces to usage_count to 1. As a result clock transition in Idle-path is broken.

Scenario2:

Tested GPIO for Suspend with these changes & obtained dump of Circular Locking dependencies:

# echo mem > /sys/power/state
[  809.981658] PM: Syncing filesystems ... done.
[  810.037963] Freezing user space processes ... (elapsed 0.02 seconds) done.
[  810.067901] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
[  810.105224] Suspending console(s) (use no_console_suspend to debug)
[  810.166748] PM: suspend of devices complete after 46.142 msecs
[  810.170562]
[  810.170593] =======================================================
[  810.170593] [ INFO: possible circular locking dependency detected ]
[  810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
[  810.170654] -------------------------------------------------------
[  810.170654] sh/670 is trying to acquire lock:
[  810.170684]  (omap_hwmod_mutex){+.+.+.}, at: [<c004fe84>] omap_hwmod_idle+0x1c/0x38
[  810.170745]
[  810.170745] but task is already holding lock:
[  810.170776]  (dpm_list_mtx){+.+...}, at: [<c023baf8>] dpm_suspend_noirq+0x28/0x188
[  810.170806]
[  810.170837] which lock already depends on the new lock.
[  810.170837]
[  810.170837]
[  810.170837] the existing dependency chain (in reverse order) is:
[  810.170867]
[  810.170867] -> #1 (dpm_list_mtx){+.+...}:
[  810.170898]        [<c009bc3c>] lock_acquire+0x60/0x74
[  810.170959]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
[  810.170989]        [<c023bcc0>] device_pm_add+0x14/0xcc
[  810.171020]        [<c0235304>] device_add+0x3b8/0x564
[  810.171051]        [<c0238834>] platform_device_add+0x104/0x160
[  810.171112]        [<c005f2a8>] omap_device_build_ss+0x14c/0x1c8
[  810.171142]        [<c005f36c>] omap_device_build+0x48/0x50
[  810.171173]        [<c004d34c>] omap2_init_gpio+0xf0/0x15c
[  810.171203]        [<c004f254>] omap_hwmod_for_each_by_class+0x60/0xa4
[  810.171264]        [<c0040340>] do_one_initcall+0x58/0x1b4
[  810.171295]        [<c0008574>] kernel_init+0x98/0x150
[  810.171325]        [<c0041968>] kernel_thread_exit+0x0/0x8
[  810.171356]
[  810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
[  810.171386]        [<c009b4e4>] __lock_acquire+0x108c/0x1784
[  810.171447]        [<c009bc3c>] lock_acquire+0x60/0x74
[  810.171478]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
[  810.171508]        [<c004fe84>] omap_hwmod_idle+0x1c/0x38
[  810.171539]        [<c005eb9c>] omap_device_idle_hwmods+0x20/0x3c
[  810.171600]        [<c005ec88>] _omap_device_deactivate+0x58/0x14c
[  810.171630]        [<c005ef50>] omap_device_idle+0x4c/0x6c
[  810.171661]        [<c0053e7c>] platform_pm_runtime_suspend+0x4c/0x74
[  810.171691]        [<c023c9f8>] __pm_runtime_suspend+0x204/0x34c
[  810.171722]        [<c023cbe0>] pm_runtime_suspend+0x20/0x34
[  810.171752]        [<c0053dbc>] platform_pm_runtime_idle+0x8/0x10
[  810.171783]        [<c023c344>] __pm_runtime_idle+0x15c/0x198
[  810.171813]        [<c023c3f8>] pm_runtime_idle+0x1c/0x30
[  810.171844]        [<c0053dac>] platform_pm_suspend_noirq+0x48/0x50
[  810.171875]        [<c023ad4c>] pm_noirq_op+0xa0/0x184
[  810.171905]        [<c023bb7c>] dpm_suspend_noirq+0xac/0x188
[  810.171936]        [<c00a5d00>] suspend_devices_and_enter+0x94/0x1d8
[  810.171966]        [<c00a5f00>] enter_state+0xbc/0x120
[  810.171997]        [<c00a5654>] state_store+0xa4/0xb8
[  810.172027]        [<c01ea9e0>] kobj_attr_store+0x18/0x1c
[  810.172088]        [<c0129acc>] sysfs_write_file+0x10c/0x144
[  810.172119]        [<c00df83c>] vfs_write+0xb0/0x148
[  810.172149]        [<c00df984>] sys_write+0x3c/0x68
[  810.172180]        [<c0040920>] ret_fast_syscall+0x0/0x3c
[  810.172210]
[  810.172210] other info that might help us debug this:
[  810.172210]
[  810.172241] 4 locks held by sh/670:
[  810.172241]  #0:  (&buffer->mutex){+.+.+.}, at: [<c01299e8>] sysfs_write_file+0x28/0x144
[  810.172302]  #1:  (s_active#5){.+.+.+}, at: [<c0129aa8>] sysfs_write_file+0xe8/0x144
[  810.172363]  #2:  (pm_mutex){+.+.+.}, at: [<c00a5e64>] enter_state+0x20/0x120
[  810.172393]  #3:  (dpm_list_mtx){+.+...}, at: [<c023baf8>] dpm_suspend_noirq+0x28/0x188
[  810.172454]
[  810.172454] stack backtrace:
[  810.172515] [<c00460d4>] (unwind_backtrace+0x0/0xe4) from [<c00996f4>] (print_circular_bug+0xc4/0xdc)
[  810.172576] [<c00996f4>] (print_circular_bug+0xc4/0xdc) from [<c009b4e4>] (__lock_acquire+0x108c/0x1784)
[  810.172637] [<c009b4e4>] (__lock_acquire+0x108c/0x1784) from [<c009bc3c>] (lock_acquire+0x60/0x74)
[  810.172698] [<c009bc3c>] (lock_acquire+0x60/0x74) from [<c0437a9c>] (mutex_lock_nested+0x58/0x2e4)
[  810.172760] [<c0437a9c>] (mutex_lock_nested+0x58/0x2e4) from [<c004fe84>] (omap_hwmod_idle+0x1c/0x38)
[  810.172821] [<c004fe84>] (omap_hwmod_idle+0x1c/0x38) from [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c)
[  810.172882] [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c) from [<c005ec88>] (_omap_device_deactivate+0x58/0x14c)
[  810.172943] [<c005ec88>] (_omap_device_deactivate+0x58/0x14c) from [<c005ef50>] (omap_device_idle+0x4c/0x6c)
[  810.173004] [<c005ef50>] (omap_device_idle+0x4c/0x6c) from [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74)
[  810.173065] [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74) from [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c)
[  810.173095] [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c) from [<c023cbe0>] (pm_runtime_suspend+0x20/0x34)
[  810.173156] [<c023cbe0>] (pm_runtime_suspend+0x20/0x34) from [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10)
[  810.173217] [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10) from [<c023c344>] (__pm_runtime_idle+0x15c/0x198)
[  810.173248] [<c023c344>] (__pm_runtime_idle+0x15c/0x198) from [<c023c3f8>] (pm_runtime_idle+0x1c/0x30)
[  810.173309] [<c023c3f8>] (pm_runtime_idle+0x1c/0x30) from [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50)
[  810.173339] [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50) from [<c023ad4c>] (pm_noirq_op+0xa0/0x184)
> 
> By the time the drivers _noirq hooks are called, the PM core has
> shutdown all the drivers, prevented any runtime PM transitions and
> disabled interrupts, so no pending runtime transitions will be queued
> so the sleeping patch of the __pm_runtime_* should never be entered.
> 
> > We are facing a similar issue with a few drivers USB, UART & GPIO
> > where, we need to enable/disable clocks & restore/save context in the
> > CPU-Idle path with interrupts locked.
>


> These are unrelated issues.  The above is for the static suspend/resume
> case.  These are for runtime PM.
> 
> > As we are unable to use Runtime APIs, we are using omap_device APIs
> > directly with the following modification in the .activate_funcion/
> > deactivate_funcion (example UART driver)
> 
> [...]
> 
> The UART driver is a special case as it is managed from deep inside the
> idle path.
> 
> > Can you feedback on my observations and comment on the approach taken
> > for these drivers?
> 
> I cannot comment until I understand why these drivers need to
> enable/disable with interrupts off.
> 
> In general, any use of the interrupts-off versions of the omap_device
> APIs need to be thorougly justified.
> 
> Even the UART one will go away when the omap-serial driver is converted
> to runtime PM.
>

For GPIO, it is a must to relinquish clocks in Idle-path as it is possible to have a GPIO bank requested and still allow PER domain to go to OFF.

For USB, this is required because of a h/w issue.

My point is, just by exposing a _omap_hwmod_idle()(mutex-free version) will not let us call pm_runtime APIs in Idle path as we are not supposed to enable interrupts there. We have faced problems with USB big-time in Idle path while using pm_runtime functions. So, we are resorting to calling omap_device_idle() in CPU_Idle path, and runtime functions in thread-context. 

Is this acceptable, given the limitations?
 
> Kevin
> 

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

"Basak, Partha" <p-basak2@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Tuesday, August 03, 2010 11:06 PM
>> To: Basak, Partha
>> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
>> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
>> Subject: Re: Issues with calling pm_runtime functions in
>> platform_pm_suspend_noirq/IRQ disabled context.
>> 
>> "Basak, Partha" <p-basak2@ti.com> writes:
>> 
>> > Resending with the corrected mailing list
>> >
>> > Hello Kevin,
>> >
>> > I want to draw your attention to an issue the following patch
>> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
>> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
>> 
>> [...]
>> 
>> > I believe, it is not correct to call the pm_runtime APIs from
>> > interrupt locked context since the underlying functions
>> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
>> > schedule().
>> >
>> > Further, from a s/w layering standpoint, platform-bus is a deeper
>> > layer than the Runtime layer, which the runtime layer calls into. So
>> > it may not be correct to have this layer calling into pm_runtime(). It
>> > should directly call omap_device APIs.
>> 
>> The original version of this patch did directly call the omap_device
>> APIs.  However, Paul didn't like that approach and there were
>> use-counting problems to handle as well (e.g. if a driver was not (yet)
>> in use, it would already be disabled, and a suspend would trigger an
>> omap_device_disable() which would fail since device was already
>> disabled.)
>> 
>> Paul and I agreed that this current approach would solve the
>> use-counting problems, but you're correct in that it introduces
>> new problems as these functions can _possibly_ sleep/schedule.
>> 
>> That being said, have you actually seen problems here?   I would like
>> to see how they are triggered?
>
> Scenario 1:
>
> Consider the case where a driver has already called
> pm_runtime_put_sync as part of aggressive clock cutting
> scheme. Subsequently, when system suspend is called,
> pm_runtime_put_sync is called again. 

> Due to atomic_dec_and_test check
> on usage_count, the second call goes through w/o error.

I don't think you're fully understanding the point of the put/get in the
suspend/resume path.

The reason for the 'put' in the suspend path is because the PM core has
done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that
runtime PM transitions are prevented during suspend/resume.

On OMAP, we want to allow those transitions to happen so we can use the
same runtime PM calls in the idle and suspend paths.

> At System resume, pm_runtime_get_sync is called twice, once by resume,
> once by the driver itself. 

Yes, but there is a 'put_sync' in between those done by the PM core
during resume (c.f. dpm_complete() in drivers/base/power/main.c]

> Because of the extra reference count, the
> aggressive clock control by the driver is broken, as call to put_sync
> by a driver reduces to usage_count to 1. As a result clock transition
> in Idle-path is broken.


> Scenario2:
>
> Tested GPIO for Suspend with these changes & obtained dump of Circular Locking dependencies:

I don't think these to problems are related, AFAICT, they are separate
issues.  I'll respond to this scenario in a different reply.

>> 
>> [...]
>> 
>> The UART driver is a special case as it is managed from deep inside the
>> idle path.
>> 
>> > Can you feedback on my observations and comment on the approach taken
>> > for these drivers?
>> 
>> I cannot comment until I understand why these drivers need to
>> enable/disable with interrupts off.
>> 
>> In general, any use of the interrupts-off versions of the omap_device
>> APIs need to be thorougly justified.
>> 
>> Even the UART one will go away when the omap-serial driver is converted
>> to runtime PM.
>>
>
> For GPIO, it is a must to relinquish clocks in Idle-path as it is
> possible to have a GPIO bank requested and still allow PER domain to
> go to OFF.

These the kind of things that I would expect to be discussed/described
in the changelogs to patches posted to the list.

> For USB, this is required because of a h/w issue.

Again, patches with descriptive changelogs describing the "h/w issue"
please.

> My point is, just by exposing a _omap_hwmod_idle()(mutex-free version) will not let us call pm_runtime APIs in Idle path as we are not supposed to enable interrupts there. We have faced problems with USB big-time in Idle path while using pm_runtime functions. So, we are resorting to calling omap_device_idle() in CPU_Idle path, and runtime functions in thread-context. 
>
> Is this acceptable, given the limitations?

No, this is not (yet) acceptable.

The limitations you've come up against are not fully understood yet.
Rather trying to hack around the limitations, we need to fully
understand the root causes, which has not yet been done.

It's possible (and likely) that there are different ways to handle these
problems, and entirely possible that there are locking issues we have to
resolve in the omap_device/hwmod layers as well.

Kevin

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> "Basak, Partha" <p-basak2@ti.com> writes:
>
>>> -----Original Message-----
>>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>> Sent: Tuesday, August 03, 2010 11:06 PM
>>> To: Basak, Partha
>>> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
>>> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
>>> Subject: Re: Issues with calling pm_runtime functions in
>>> platform_pm_suspend_noirq/IRQ disabled context.
>>> 
>>> "Basak, Partha" <p-basak2@ti.com> writes:
>>> 
>>> > Resending with the corrected mailing list
>>> >
>>> > Hello Kevin,
>>> >
>>> > I want to draw your attention to an issue the following patch
>>> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
>>> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
>>> 
>>> [...]
>>> 
>>> > I believe, it is not correct to call the pm_runtime APIs from
>>> > interrupt locked context since the underlying functions
>>> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
>>> > schedule().
>>> >
>>> > Further, from a s/w layering standpoint, platform-bus is a deeper
>>> > layer than the Runtime layer, which the runtime layer calls into. So
>>> > it may not be correct to have this layer calling into pm_runtime(). It
>>> > should directly call omap_device APIs.
>>> 
>>> The original version of this patch did directly call the omap_device
>>> APIs.  However, Paul didn't like that approach and there were
>>> use-counting problems to handle as well (e.g. if a driver was not (yet)
>>> in use, it would already be disabled, and a suspend would trigger an
>>> omap_device_disable() which would fail since device was already
>>> disabled.)
>>> 
>>> Paul and I agreed that this current approach would solve the
>>> use-counting problems, but you're correct in that it introduces
>>> new problems as these functions can _possibly_ sleep/schedule.
>>> 
>>> That being said, have you actually seen problems here?   I would like
>>> to see how they are triggered?
>>
>> Scenario 1:
>>
>> Consider the case where a driver has already called
>> pm_runtime_put_sync as part of aggressive clock cutting
>> scheme. Subsequently, when system suspend is called,
>> pm_runtime_put_sync is called again. 
>
>> Due to atomic_dec_and_test check
>> on usage_count, the second call goes through w/o error.
>
> I don't think you're fully understanding the point of the put/get in the
> suspend/resume path.
>
> The reason for the 'put' in the suspend path is because the PM core has
> done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that
> runtime PM transitions are prevented during suspend/resume.
>
> On OMAP, we want to allow those transitions to happen so we can use the
> same runtime PM calls in the idle and suspend paths.
>
>> At System resume, pm_runtime_get_sync is called twice, once by resume,
>> once by the driver itself. 
>
> Yes, but there is a 'put_sync' in between those done by the PM core
> during resume (c.f. dpm_complete() in drivers/base/power/main.c]
>
>> Because of the extra reference count, the
>> aggressive clock control by the driver is broken, as call to put_sync
>> by a driver reduces to usage_count to 1. As a result clock transition
>> in Idle-path is broken.

A closer look here, and there is indeed a bug in the _resume_noirq()
method.  The get here needs to be a _noresume version to match what is
done in the PM core.

This doesn't change the use count, but it does change whether the device
is actually awoken by this 'get' or not.  This get should never actually
awake the device.  It is only there to compensate for what the PM core
does.

Can you try this patch?  Will post a version to the list with
changelog shortly.

Kevin

diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
index bab0186..01bbe65 100644
--- a/arch/arm/mach-omap2/pm_bus.c
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -90,7 +90,7 @@ int platform_pm_resume_noirq(struct device *dev)
 	 * method so that the runtime PM usage counting is in the same
 	 * state it was when suspend was called.
 	 */
-	pm_runtime_get_sync(dev);
+	pm_runtime_get_noresume(dev);
 
 	if (!drv)
 		return 0;

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
>> "Basak, Partha" <p-basak2@ti.com> writes:
>>
>>>> -----Original Message-----
>>>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>>> Sent: Tuesday, August 03, 2010 11:06 PM
>>>> To: Basak, Partha
>>>> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
>>>> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
>>>> Subject: Re: Issues with calling pm_runtime functions in
>>>> platform_pm_suspend_noirq/IRQ disabled context.
>>>> 
>>>> "Basak, Partha" <p-basak2@ti.com> writes:
>>>> 
>>>> > Resending with the corrected mailing list
>>>> >
>>>> > Hello Kevin,
>>>> >
>>>> > I want to draw your attention to an issue the following patch
>>>> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
>>>> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
>>>> 
>>>> [...]
>>>> 
>>>> > I believe, it is not correct to call the pm_runtime APIs from
>>>> > interrupt locked context since the underlying functions
>>>> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
>>>> > schedule().
>>>> >
>>>> > Further, from a s/w layering standpoint, platform-bus is a deeper
>>>> > layer than the Runtime layer, which the runtime layer calls into. So
>>>> > it may not be correct to have this layer calling into pm_runtime(). It
>>>> > should directly call omap_device APIs.
>>>> 
>>>> The original version of this patch did directly call the omap_device
>>>> APIs.  However, Paul didn't like that approach and there were
>>>> use-counting problems to handle as well (e.g. if a driver was not (yet)
>>>> in use, it would already be disabled, and a suspend would trigger an
>>>> omap_device_disable() which would fail since device was already
>>>> disabled.)
>>>> 
>>>> Paul and I agreed that this current approach would solve the
>>>> use-counting problems, but you're correct in that it introduces
>>>> new problems as these functions can _possibly_ sleep/schedule.
>>>> 
>>>> That being said, have you actually seen problems here?   I would like
>>>> to see how they are triggered?
>>>
>>> Scenario 1:
>>>
>>> Consider the case where a driver has already called
>>> pm_runtime_put_sync as part of aggressive clock cutting
>>> scheme. Subsequently, when system suspend is called,
>>> pm_runtime_put_sync is called again. 
>>
>>> Due to atomic_dec_and_test check
>>> on usage_count, the second call goes through w/o error.
>>
>> I don't think you're fully understanding the point of the put/get in the
>> suspend/resume path.
>>
>> The reason for the 'put' in the suspend path is because the PM core has
>> done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that
>> runtime PM transitions are prevented during suspend/resume.
>>
>> On OMAP, we want to allow those transitions to happen so we can use the
>> same runtime PM calls in the idle and suspend paths.
>>
>>> At System resume, pm_runtime_get_sync is called twice, once by resume,
>>> once by the driver itself. 
>>
>> Yes, but there is a 'put_sync' in between those done by the PM core
>> during resume (c.f. dpm_complete() in drivers/base/power/main.c]
>>
>>> Because of the extra reference count, the
>>> aggressive clock control by the driver is broken, as call to put_sync
>>> by a driver reduces to usage_count to 1. As a result clock transition
>>> in Idle-path is broken.
>
> A closer look here, and there is indeed a bug in the _resume_noirq()
> method.  The get here needs to be a _noresume version to match what is
> done in the PM core.
>
> This doesn't change the use count, but it does change whether the device
> is actually awoken by this 'get' or not.  This get should never actually
> awake the device.  It is only there to compensate for what the PM core
> does.
>
> Can you try this patch?  Will post a version to the list with
> changelog shortly.

After a little more thinking, I'm not sure yet if this is a real problem
or not.

One other question.  At least for GPIO testing, does it work if you
simply remove the _noirq hooks from pm_bus.c?  If so, please feel to
post a version with the dependency that the patch adding suspend/resume
hooks in pm_bus.c needs to be reverted first.

Kevin

> Kevin
>
> diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
> index bab0186..01bbe65 100644
> --- a/arch/arm/mach-omap2/pm_bus.c
> +++ b/arch/arm/mach-omap2/pm_bus.c
> @@ -90,7 +90,7 @@ int platform_pm_resume_noirq(struct device *dev)
>  	 * method so that the runtime PM usage counting is in the same
>  	 * state it was when suspend was called.
>  	 */
> -	pm_runtime_get_sync(dev);
> +	pm_runtime_get_noresume(dev);
>  
>  	if (!drv)
>  		return 0;

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

"Basak, Partha" <p-basak2@ti.com> writes:

>
> Tested GPIO for Suspend with these changes & obtained dump of Circular Locking dependencies:
>

It would *really* help to have these GPIO conversion patches posted
against a public tree/branch one could see exactly what is going on and
be able to reproduce this problem for easier analysis.  I have some
hunches as to what is going wrong here, but cannot be sure.  I'd much
rather be able to see and try the code.

Fortunately, lockdep is verbose enough to try and understand the
symptoms here.  Lockdep seems to have detected that these two locks have
been acquired in different order, resulting in *potential* deadlock.

Indeed, during init (#0 below) you have the hwmod_mutex acquired
(hwmod_for_each_by_class()) then the dpm_list_mtx acquired
(device_pm_add()).  Later, during suspend the dpm_list_mtx is aquired
first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
(omap_hwmod_idle()).

Taking the same locks in different order at different times is the
circular dependency and a recipe for potential deadlock.

In our case, there is no real potential for deadlock here as the locks
are only taken the hwmod->dpm order once during init, all other times
(when the omap_device/hwmod are ready) it will happen in the dpm->hwmod
order.

The simple fix here is probably to remove the locking in the
omap_hwmod_for_each* functions.  Could you try that?

I'm a bit confused why I don't see the same problem with the UART layer
which currently also handles things in the idle path as well.

Kevin

> # echo mem > /sys/power/state
> [  809.981658] PM: Syncing filesystems ... done.
> [  810.037963] Freezing user space processes ... (elapsed 0.02 seconds) done.
> [  810.067901] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
> [  810.105224] Suspending console(s) (use no_console_suspend to debug)
> [  810.166748] PM: suspend of devices complete after 46.142 msecs
> [  810.170562]
> [  810.170593] =======================================================
> [  810.170593] [ INFO: possible circular locking dependency detected ]
> [  810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
> [  810.170654] -------------------------------------------------------
> [  810.170654] sh/670 is trying to acquire lock:
> [  810.170684]  (omap_hwmod_mutex){+.+.+.}, at: [<c004fe84>] omap_hwmod_idle+0x1c/0x38
> [  810.170745]
> [  810.170745] but task is already holding lock:
> [  810.170776]  (dpm_list_mtx){+.+...}, at: [<c023baf8>] dpm_suspend_noirq+0x28/0x188
> [  810.170806]
> [  810.170837] which lock already depends on the new lock.
> [  810.170837]
> [  810.170837]
> [  810.170837] the existing dependency chain (in reverse order) is:
> [  810.170867]
> [  810.170867] -> #1 (dpm_list_mtx){+.+...}:
> [  810.170898]        [<c009bc3c>] lock_acquire+0x60/0x74
> [  810.170959]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
> [  810.170989]        [<c023bcc0>] device_pm_add+0x14/0xcc
> [  810.171020]        [<c0235304>] device_add+0x3b8/0x564
> [  810.171051]        [<c0238834>] platform_device_add+0x104/0x160
> [  810.171112]        [<c005f2a8>] omap_device_build_ss+0x14c/0x1c8
> [  810.171142]        [<c005f36c>] omap_device_build+0x48/0x50
> [  810.171173]        [<c004d34c>] omap2_init_gpio+0xf0/0x15c
> [  810.171203]        [<c004f254>] omap_hwmod_for_each_by_class+0x60/0xa4
> [  810.171264]        [<c0040340>] do_one_initcall+0x58/0x1b4
> [  810.171295]        [<c0008574>] kernel_init+0x98/0x150
> [  810.171325]        [<c0041968>] kernel_thread_exit+0x0/0x8
> [  810.171356]
> [  810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
> [  810.171386]        [<c009b4e4>] __lock_acquire+0x108c/0x1784
> [  810.171447]        [<c009bc3c>] lock_acquire+0x60/0x74
> [  810.171478]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
> [  810.171508]        [<c004fe84>] omap_hwmod_idle+0x1c/0x38
> [  810.171539]        [<c005eb9c>] omap_device_idle_hwmods+0x20/0x3c
> [  810.171600]        [<c005ec88>] _omap_device_deactivate+0x58/0x14c
> [  810.171630]        [<c005ef50>] omap_device_idle+0x4c/0x6c
> [  810.171661]        [<c0053e7c>] platform_pm_runtime_suspend+0x4c/0x74
> [  810.171691]        [<c023c9f8>] __pm_runtime_suspend+0x204/0x34c
> [  810.171722]        [<c023cbe0>] pm_runtime_suspend+0x20/0x34
> [  810.171752]        [<c0053dbc>] platform_pm_runtime_idle+0x8/0x10
> [  810.171783]        [<c023c344>] __pm_runtime_idle+0x15c/0x198
> [  810.171813]        [<c023c3f8>] pm_runtime_idle+0x1c/0x30
> [  810.171844]        [<c0053dac>] platform_pm_suspend_noirq+0x48/0x50
> [  810.171875]        [<c023ad4c>] pm_noirq_op+0xa0/0x184
> [  810.171905]        [<c023bb7c>] dpm_suspend_noirq+0xac/0x188
> [  810.171936]        [<c00a5d00>] suspend_devices_and_enter+0x94/0x1d8
> [  810.171966]        [<c00a5f00>] enter_state+0xbc/0x120
> [  810.171997]        [<c00a5654>] state_store+0xa4/0xb8
> [  810.172027]        [<c01ea9e0>] kobj_attr_store+0x18/0x1c
> [  810.172088]        [<c0129acc>] sysfs_write_file+0x10c/0x144
> [  810.172119]        [<c00df83c>] vfs_write+0xb0/0x148
> [  810.172149]        [<c00df984>] sys_write+0x3c/0x68
> [  810.172180]        [<c0040920>] ret_fast_syscall+0x0/0x3c
> [  810.172210]
> [  810.172210] other info that might help us debug this:
> [  810.172210]
> [  810.172241] 4 locks held by sh/670:
> [  810.172241]  #0:  (&buffer->mutex){+.+.+.}, at: [<c01299e8>] sysfs_write_file+0x28/0x144
> [  810.172302]  #1:  (s_active#5){.+.+.+}, at: [<c0129aa8>] sysfs_write_file+0xe8/0x144
> [  810.172363]  #2:  (pm_mutex){+.+.+.}, at: [<c00a5e64>] enter_state+0x20/0x120
> [  810.172393]  #3:  (dpm_list_mtx){+.+...}, at: [<c023baf8>] dpm_suspend_noirq+0x28/0x188
> [  810.172454]
> [  810.172454] stack backtrace:
> [  810.172515] [<c00460d4>] (unwind_backtrace+0x0/0xe4) from [<c00996f4>] (print_circular_bug+0xc4/0xdc)
> [  810.172576] [<c00996f4>] (print_circular_bug+0xc4/0xdc) from [<c009b4e4>] (__lock_acquire+0x108c/0x1784)
> [  810.172637] [<c009b4e4>] (__lock_acquire+0x108c/0x1784) from [<c009bc3c>] (lock_acquire+0x60/0x74)
> [  810.172698] [<c009bc3c>] (lock_acquire+0x60/0x74) from [<c0437a9c>] (mutex_lock_nested+0x58/0x2e4)
> [  810.172760] [<c0437a9c>] (mutex_lock_nested+0x58/0x2e4) from [<c004fe84>] (omap_hwmod_idle+0x1c/0x38)
> [  810.172821] [<c004fe84>] (omap_hwmod_idle+0x1c/0x38) from [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c)
> [  810.172882] [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c) from [<c005ec88>] (_omap_device_deactivate+0x58/0x14c)
> [  810.172943] [<c005ec88>] (_omap_device_deactivate+0x58/0x14c) from [<c005ef50>] (omap_device_idle+0x4c/0x6c)
> [  810.173004] [<c005ef50>] (omap_device_idle+0x4c/0x6c) from [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74)
> [  810.173065] [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74) from [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c)
> [  810.173095] [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c) from [<c023cbe0>] (pm_runtime_suspend+0x20/0x34)
> [  810.173156] [<c023cbe0>] (pm_runtime_suspend+0x20/0x34) from [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10)
> [  810.173217] [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10) from [<c023c344>] (__pm_runtime_idle+0x15c/0x198)
> [  810.173248] [<c023c344>] (__pm_runtime_idle+0x15c/0x198) from [<c023c3f8>] (pm_runtime_idle+0x1c/0x30)
> [  810.173309] [<c023c3f8>] (pm_runtime_idle+0x1c/0x30) from [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50)
> [  810.173339] [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50) from [<c023ad4c>] (pm_noirq_op+0xa0/0x184)
>> 
>> By the time the drivers _noirq hooks are called, the PM core has
>> shutdown all the drivers, prevented any runtime PM transitions and
>> disabled interrupts, so no pending runtime transitions will be queued
>> so the sleeping patch of the __pm_runtime_* should never be entered.
>> 
>> > We are facing a similar issue with a few drivers USB, UART & GPIO
>> > where, we need to enable/disable clocks & restore/save context in the
>> > CPU-Idle path with interrupts locked.
>>
>
>
>> These are unrelated issues.  The above is for the static suspend/resume
>> case.  These are for runtime PM.
>> 
>> > As we are unable to use Runtime APIs, we are using omap_device APIs
>> > directly with the following modification in the .activate_funcion/
>> > deactivate_funcion (example UART driver)
>> 
>> [...]
>> 
>> The UART driver is a special case as it is managed from deep inside the
>> idle path.
>> 
>> > Can you feedback on my observations and comment on the approach taken
>> > for these drivers?
>> 
>> I cannot comment until I understand why these drivers need to
>> enable/disable with interrupts off.
>> 
>> In general, any use of the interrupts-off versions of the omap_device
>> APIs need to be thorougly justified.
>> 
>> Even the UART one will go away when the omap-serial driver is converted
>> to runtime PM.
>>
>
> For GPIO, it is a must to relinquish clocks in Idle-path as it is possible to have a GPIO bank requested and still allow PER domain to go to OFF.
>
> For USB, this is required because of a h/w issue.
>
> My point is, just by exposing a _omap_hwmod_idle()(mutex-free version) will not let us call pm_runtime APIs in Idle path as we are not supposed to enable interrupts there. We have faced problems with USB big-time in Idle path while using pm_runtime functions. So, we are resorting to calling omap_device_idle() in CPU_Idle path, and runtime functions in thread-context. 
>
> Is this acceptable, given the limitations?
>  
>> Kevin
>> 

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Thursday, August 05, 2010 5:05 AM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> >
> > Tested GPIO for Suspend with these changes & obtained dump of Circular
> Locking dependencies:
> >
> 
> It would *really* help to have these GPIO conversion patches posted
> against a public tree/branch one could see exactly what is going on and
> be able to reproduce this problem for easier analysis.  I have some
> hunches as to what is going wrong here, but cannot be sure.  I'd much
> rather be able to see and try the code.
> 
> Fortunately, lockdep is verbose enough to try and understand the
> symptoms here.  Lockdep seems to have detected that these two locks have
> been acquired in different order, resulting in *potential* deadlock.
> 
> Indeed, during init (#0 below) you have the hwmod_mutex acquired
> (hwmod_for_each_by_class()) then the dpm_list_mtx acquired
> (device_pm_add()).  Later, during suspend the dpm_list_mtx is aquired
> first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
> (omap_hwmod_idle()).
> 
> Taking the same locks in different order at different times is the
> circular dependency and a recipe for potential deadlock.
> 
> In our case, there is no real potential for deadlock here as the locks
> are only taken the hwmod->dpm order once during init, all other times
> (when the omap_device/hwmod are ready) it will happen in the dpm->hwmod
> order.
> 
> The simple fix here is probably to remove the locking in the
> omap_hwmod_for_each* functions.  Could you try that?
> 
We tried this, it works. But the GPIO patch series sent out(posted today, Aug 06 2010) are tested based on reverting the pm_bus.c suspend_no_irq patch. 

> I'm a bit confused why I don't see the same problem with the UART layer
> which currently also handles things in the idle path as well.
> 
> Kevin
> 
> > # echo mem > /sys/power/state
> > [  809.981658] PM: Syncing filesystems ... done.
> > [  810.037963] Freezing user space processes ... (elapsed 0.02 seconds)
> done.
> > [  810.067901] Freezing remaining freezable tasks ... (elapsed 0.02
> seconds) done.
> > [  810.105224] Suspending console(s) (use no_console_suspend to debug)
> > [  810.166748] PM: suspend of devices complete after 46.142 msecs
> > [  810.170562]
> > [  810.170593] =======================================================
> > [  810.170593] [ INFO: possible circular locking dependency detected ]
> > [  810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
> > [  810.170654] -------------------------------------------------------
> > [  810.170654] sh/670 is trying to acquire lock:
> > [  810.170684]  (omap_hwmod_mutex){+.+.+.}, at: [<c004fe84>]
> omap_hwmod_idle+0x1c/0x38
> > [  810.170745]
> > [  810.170745] but task is already holding lock:
> > [  810.170776]  (dpm_list_mtx){+.+...}, at: [<c023baf8>]
> dpm_suspend_noirq+0x28/0x188
> > [  810.170806]
> > [  810.170837] which lock already depends on the new lock.
> > [  810.170837]
> > [  810.170837]
> > [  810.170837] the existing dependency chain (in reverse order) is:
> > [  810.170867]
> > [  810.170867] -> #1 (dpm_list_mtx){+.+...}:
> > [  810.170898]        [<c009bc3c>] lock_acquire+0x60/0x74
> > [  810.170959]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
> > [  810.170989]        [<c023bcc0>] device_pm_add+0x14/0xcc
> > [  810.171020]        [<c0235304>] device_add+0x3b8/0x564
> > [  810.171051]        [<c0238834>] platform_device_add+0x104/0x160
> > [  810.171112]        [<c005f2a8>] omap_device_build_ss+0x14c/0x1c8
> > [  810.171142]        [<c005f36c>] omap_device_build+0x48/0x50
> > [  810.171173]        [<c004d34c>] omap2_init_gpio+0xf0/0x15c
> > [  810.171203]        [<c004f254>]
> omap_hwmod_for_each_by_class+0x60/0xa4
> > [  810.171264]        [<c0040340>] do_one_initcall+0x58/0x1b4
> > [  810.171295]        [<c0008574>] kernel_init+0x98/0x150
> > [  810.171325]        [<c0041968>] kernel_thread_exit+0x0/0x8
> > [  810.171356]
> > [  810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
> > [  810.171386]        [<c009b4e4>] __lock_acquire+0x108c/0x1784
> > [  810.171447]        [<c009bc3c>] lock_acquire+0x60/0x74
> > [  810.171478]        [<c0437a9c>] mutex_lock_nested+0x58/0x2e4
> > [  810.171508]        [<c004fe84>] omap_hwmod_idle+0x1c/0x38
> > [  810.171539]        [<c005eb9c>] omap_device_idle_hwmods+0x20/0x3c
> > [  810.171600]        [<c005ec88>] _omap_device_deactivate+0x58/0x14c
> > [  810.171630]        [<c005ef50>] omap_device_idle+0x4c/0x6c
> > [  810.171661]        [<c0053e7c>] platform_pm_runtime_suspend+0x4c/0x74
> > [  810.171691]        [<c023c9f8>] __pm_runtime_suspend+0x204/0x34c
> > [  810.171722]        [<c023cbe0>] pm_runtime_suspend+0x20/0x34
> > [  810.171752]        [<c0053dbc>] platform_pm_runtime_idle+0x8/0x10
> > [  810.171783]        [<c023c344>] __pm_runtime_idle+0x15c/0x198
> > [  810.171813]        [<c023c3f8>] pm_runtime_idle+0x1c/0x30
> > [  810.171844]        [<c0053dac>] platform_pm_suspend_noirq+0x48/0x50
> > [  810.171875]        [<c023ad4c>] pm_noirq_op+0xa0/0x184
> > [  810.171905]        [<c023bb7c>] dpm_suspend_noirq+0xac/0x188
> > [  810.171936]        [<c00a5d00>] suspend_devices_and_enter+0x94/0x1d8
> > [  810.171966]        [<c00a5f00>] enter_state+0xbc/0x120
> > [  810.171997]        [<c00a5654>] state_store+0xa4/0xb8
> > [  810.172027]        [<c01ea9e0>] kobj_attr_store+0x18/0x1c
> > [  810.172088]        [<c0129acc>] sysfs_write_file+0x10c/0x144
> > [  810.172119]        [<c00df83c>] vfs_write+0xb0/0x148
> > [  810.172149]        [<c00df984>] sys_write+0x3c/0x68
> > [  810.172180]        [<c0040920>] ret_fast_syscall+0x0/0x3c
> > [  810.172210]
> > [  810.172210] other info that might help us debug this:
> > [  810.172210]
> > [  810.172241] 4 locks held by sh/670:
> > [  810.172241]  #0:  (&buffer->mutex){+.+.+.}, at: [<c01299e8>]
> sysfs_write_file+0x28/0x144
> > [  810.172302]  #1:  (s_active#5){.+.+.+}, at: [<c0129aa8>]
> sysfs_write_file+0xe8/0x144
> > [  810.172363]  #2:  (pm_mutex){+.+.+.}, at: [<c00a5e64>]
> enter_state+0x20/0x120
> > [  810.172393]  #3:  (dpm_list_mtx){+.+...}, at: [<c023baf8>]
> dpm_suspend_noirq+0x28/0x188
> > [  810.172454]
> > [  810.172454] stack backtrace:
> > [  810.172515] [<c00460d4>] (unwind_backtrace+0x0/0xe4) from
> [<c00996f4>] (print_circular_bug+0xc4/0xdc)
> > [  810.172576] [<c00996f4>] (print_circular_bug+0xc4/0xdc) from
> [<c009b4e4>] (__lock_acquire+0x108c/0x1784)
> > [  810.172637] [<c009b4e4>] (__lock_acquire+0x108c/0x1784) from
> [<c009bc3c>] (lock_acquire+0x60/0x74)
> > [  810.172698] [<c009bc3c>] (lock_acquire+0x60/0x74) from [<c0437a9c>]
> (mutex_lock_nested+0x58/0x2e4)
> > [  810.172760] [<c0437a9c>] (mutex_lock_nested+0x58/0x2e4) from
> [<c004fe84>] (omap_hwmod_idle+0x1c/0x38)
> > [  810.172821] [<c004fe84>] (omap_hwmod_idle+0x1c/0x38) from
> [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c)
> > [  810.172882] [<c005eb9c>] (omap_device_idle_hwmods+0x20/0x3c) from
> [<c005ec88>] (_omap_device_deactivate+0x58/0x14c)
> > [  810.172943] [<c005ec88>] (_omap_device_deactivate+0x58/0x14c) from
> [<c005ef50>] (omap_device_idle+0x4c/0x6c)
> > [  810.173004] [<c005ef50>] (omap_device_idle+0x4c/0x6c) from
> [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74)
> > [  810.173065] [<c0053e7c>] (platform_pm_runtime_suspend+0x4c/0x74) from
> [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c)
> > [  810.173095] [<c023c9f8>] (__pm_runtime_suspend+0x204/0x34c) from
> [<c023cbe0>] (pm_runtime_suspend+0x20/0x34)
> > [  810.173156] [<c023cbe0>] (pm_runtime_suspend+0x20/0x34) from
> [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10)
> > [  810.173217] [<c0053dbc>] (platform_pm_runtime_idle+0x8/0x10) from
> [<c023c344>] (__pm_runtime_idle+0x15c/0x198)
> > [  810.173248] [<c023c344>] (__pm_runtime_idle+0x15c/0x198) from
> [<c023c3f8>] (pm_runtime_idle+0x1c/0x30)
> > [  810.173309] [<c023c3f8>] (pm_runtime_idle+0x1c/0x30) from
> [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50)
> > [  810.173339] [<c0053dac>] (platform_pm_suspend_noirq+0x48/0x50) from
> [<c023ad4c>] (pm_noirq_op+0xa0/0x184)
> >>
> >> By the time the drivers _noirq hooks are called, the PM core has
> >> shutdown all the drivers, prevented any runtime PM transitions and
> >> disabled interrupts, so no pending runtime transitions will be queued
> >> so the sleeping patch of the __pm_runtime_* should never be entered.
> >>
> >> > We are facing a similar issue with a few drivers USB, UART & GPIO
> >> > where, we need to enable/disable clocks & restore/save context in the
> >> > CPU-Idle path with interrupts locked.
> >>
> >
> >
> >> These are unrelated issues.  The above is for the static suspend/resume
> >> case.  These are for runtime PM.
> >>
> >> > As we are unable to use Runtime APIs, we are using omap_device APIs
> >> > directly with the following modification in the .activate_funcion/
> >> > deactivate_funcion (example UART driver)
> >>
> >> [...]
> >>
> >> The UART driver is a special case as it is managed from deep inside the
> >> idle path.
> >>
> >> > Can you feedback on my observations and comment on the approach taken
> >> > for these drivers?
> >>
> >> I cannot comment until I understand why these drivers need to
> >> enable/disable with interrupts off.
> >>
> >> In general, any use of the interrupts-off versions of the omap_device
> >> APIs need to be thorougly justified.
> >>
> >> Even the UART one will go away when the omap-serial driver is converted
> >> to runtime PM.
> >>
> >
> > For GPIO, it is a must to relinquish clocks in Idle-path as it is
> possible to have a GPIO bank requested and still allow PER domain to go to
> OFF.
> >
> > For USB, this is required because of a h/w issue.
> >
> > My point is, just by exposing a _omap_hwmod_idle()(mutex-free version)
> will not let us call pm_runtime APIs in Idle path as we are not supposed
> to enable interrupts there. We have faced problems with USB big-time in
> Idle path while using pm_runtime functions. So, we are resorting to
> calling omap_device_idle() in CPU_Idle path, and runtime functions in
> thread-context.
> >
> > Is this acceptable, given the limitations?
> >
> >> Kevin
> >>

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Thursday, August 05, 2010 4:51 AM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> Kevin Hilman <khilman@deeprootsystems.com> writes:
> 
> > Kevin Hilman <khilman@deeprootsystems.com> writes:
> >
> >> "Basak, Partha" <p-basak2@ti.com> writes:
> >>
> >>>> -----Original Message-----
> >>>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >>>> Sent: Tuesday, August 03, 2010 11:06 PM
> >>>> To: Basak, Partha
> >>>> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema;
> Raja,
> >>>> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> >>>> Subject: Re: Issues with calling pm_runtime functions in
> >>>> platform_pm_suspend_noirq/IRQ disabled context.
> >>>>
> >>>> "Basak, Partha" <p-basak2@ti.com> writes:
> >>>>
> >>>> > Resending with the corrected mailing list
> >>>> >
> >>>> > Hello Kevin,
> >>>> >
> >>>> > I want to draw your attention to an issue the following patch
> >>>> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
> >>>> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
> >>>>
> >>>> [...]
> >>>>
> >>>> > I believe, it is not correct to call the pm_runtime APIs from
> >>>> > interrupt locked context since the underlying functions
> >>>> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and
> call
> >>>> > schedule().
> >>>> >
> >>>> > Further, from a s/w layering standpoint, platform-bus is a deeper
> >>>> > layer than the Runtime layer, which the runtime layer calls into.
> So
> >>>> > it may not be correct to have this layer calling into pm_runtime().
> It
> >>>> > should directly call omap_device APIs.
> >>>>
> >>>> The original version of this patch did directly call the omap_device
> >>>> APIs.  However, Paul didn't like that approach and there were
> >>>> use-counting problems to handle as well (e.g. if a driver was not
> (yet)
> >>>> in use, it would already be disabled, and a suspend would trigger an
> >>>> omap_device_disable() which would fail since device was already
> >>>> disabled.)
> >>>>
> >>>> Paul and I agreed that this current approach would solve the
> >>>> use-counting problems, but you're correct in that it introduces
> >>>> new problems as these functions can _possibly_ sleep/schedule.
> >>>>
> >>>> That being said, have you actually seen problems here?   I would like
> >>>> to see how they are triggered?
> >>>
> >>> Scenario 1:
> >>>
> >>> Consider the case where a driver has already called
> >>> pm_runtime_put_sync as part of aggressive clock cutting
> >>> scheme. Subsequently, when system suspend is called,
> >>> pm_runtime_put_sync is called again.
> >>
> >>> Due to atomic_dec_and_test check
> >>> on usage_count, the second call goes through w/o error.
> >>
> >> I don't think you're fully understanding the point of the put/get in
> the
> >> suspend/resume path.
> >>
> >> The reason for the 'put' in the suspend path is because the PM core has
> >> done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that
> >> runtime PM transitions are prevented during suspend/resume.
> >>
> >> On OMAP, we want to allow those transitions to happen so we can use the
> >> same runtime PM calls in the idle and suspend paths.
> >>
> >>> At System resume, pm_runtime_get_sync is called twice, once by resume,
> >>> once by the driver itself.
> >>
> >> Yes, but there is a 'put_sync' in between those done by the PM core
> >> during resume (c.f. dpm_complete() in drivers/base/power/main.c]
> >>
> >>> Because of the extra reference count, the
> >>> aggressive clock control by the driver is broken, as call to put_sync
> >>> by a driver reduces to usage_count to 1. As a result clock transition
> >>> in Idle-path is broken.
> >
> > A closer look here, and there is indeed a bug in the _resume_noirq()
> > method.  The get here needs to be a _noresume version to match what is
> > done in the PM core.
> >
> > This doesn't change the use count, but it does change whether the device
> > is actually awoken by this 'get' or not.  This get should never actually
> > awake the device.  It is only there to compensate for what the PM core
> > does.
> >
> > Can you try this patch?  Will post a version to the list with
> > changelog shortly.
> 
> After a little more thinking, I'm not sure yet if this is a real problem
> or not.
> 
> One other question.  At least for GPIO testing, does it work if you
> simply remove the _noirq hooks from pm_bus.c?  If so, please feel to
> post a version with the dependency that the patch adding suspend/resume
> hooks in pm_bus.c needs to be reverted first.
> 
We have reverted this patch and tested before submitting the modified GPIO series.

> Kevin
> 
> > Kevin
> >
> > diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
> > index bab0186..01bbe65 100644
> > --- a/arch/arm/mach-omap2/pm_bus.c
> > +++ b/arch/arm/mach-omap2/pm_bus.c
> > @@ -90,7 +90,7 @@ int platform_pm_resume_noirq(struct device *dev)
> >  	 * method so that the runtime PM usage counting is in the same
> >  	 * state it was when suspend was called.
> >  	 */
> > -	pm_runtime_get_sync(dev);
> > +	pm_runtime_get_noresume(dev);
> >
> >  	if (!drv)
> >  		return 0;

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Thursday, August 05, 2010 3:17 AM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> Sent: Tuesday, August 03, 2010 11:06 PM
> >> To: Basak, Partha
> >> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> >> Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
> >> Subject: Re: Issues with calling pm_runtime functions in
> >> platform_pm_suspend_noirq/IRQ disabled context.
> >>
> >> "Basak, Partha" <p-basak2@ti.com> writes:
> >>
> >> > Resending with the corrected mailing list
> >> >
> >> > Hello Kevin,
> >> >
> >> > I want to draw your attention to an issue the following patch
> >> > introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
> >> adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
> >>
> >> [...]
> >>
> >> > I believe, it is not correct to call the pm_runtime APIs from
> >> > interrupt locked context since the underlying functions
> >> > __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
> >> > schedule().
> >> >
> >> > Further, from a s/w layering standpoint, platform-bus is a deeper
> >> > layer than the Runtime layer, which the runtime layer calls into. So
> >> > it may not be correct to have this layer calling into pm_runtime().
> It
> >> > should directly call omap_device APIs.
> >>
> >> The original version of this patch did directly call the omap_device
> >> APIs.  However, Paul didn't like that approach and there were
> >> use-counting problems to handle as well (e.g. if a driver was not (yet)
> >> in use, it would already be disabled, and a suspend would trigger an
> >> omap_device_disable() which would fail since device was already
> >> disabled.)
> >>
> >> Paul and I agreed that this current approach would solve the
> >> use-counting problems, but you're correct in that it introduces
> >> new problems as these functions can _possibly_ sleep/schedule.
> >>
> >> That being said, have you actually seen problems here?   I would like
> >> to see how they are triggered?
> >
> > Scenario 1:
> >
> > Consider the case where a driver has already called
> > pm_runtime_put_sync as part of aggressive clock cutting
> > scheme. Subsequently, when system suspend is called,
> > pm_runtime_put_sync is called again.
> 
> > Due to atomic_dec_and_test check
> > on usage_count, the second call goes through w/o error.
> 
> I don't think you're fully understanding the point of the put/get in the
> suspend/resume path.
> 
> The reason for the 'put' in the suspend path is because the PM core has
> done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that
> runtime PM transitions are prevented during suspend/resume.
> 
> On OMAP, we want to allow those transitions to happen so we can use the
> same runtime PM calls in the idle and suspend paths.
> 
> > At System resume, pm_runtime_get_sync is called twice, once by resume,
> > once by the driver itself.
> 
> Yes, but there is a 'put_sync' in between those done by the PM core
> during resume (c.f. dpm_complete() in drivers/base/power/main.c]
> 
> > Because of the extra reference count, the
> > aggressive clock control by the driver is broken, as call to put_sync
> > by a driver reduces to usage_count to 1. As a result clock transition
> > in Idle-path is broken.
> 
I understand the rationale better, but having these changes is making the next Idle call after a suspend-resume cycle to fail. I will continue debugging on this and come back with more details.

> 
> > Scenario2:
> >
> > Tested GPIO for Suspend with these changes & obtained dump of Circular
> Locking dependencies:
> 
> I don't think these to problems are related, AFAICT, they are separate
> issues.  I'll respond to this scenario in a different reply.
> 
> >>
> >> [...]
> >>
> >> The UART driver is a special case as it is managed from deep inside the
> >> idle path.
> >>
> >> > Can you feedback on my observations and comment on the approach taken
> >> > for these drivers?
> >>
> >> I cannot comment until I understand why these drivers need to
> >> enable/disable with interrupts off.
> >>
> >> In general, any use of the interrupts-off versions of the omap_device
> >> APIs need to be thorougly justified.
> >>
> >> Even the UART one will go away when the omap-serial driver is converted
> >> to runtime PM.
> >>
> >
> > For GPIO, it is a must to relinquish clocks in Idle-path as it is
> > possible to have a GPIO bank requested and still allow PER domain to
> > go to OFF.
> 
> These the kind of things that I would expect to be discussed/described
> in the changelogs to patches posted to the list.
> 
> > For USB, this is required because of a h/w issue.
> 
> Again, patches with descriptive changelogs describing the "h/w issue"
> please.
> 
> > My point is, just by exposing a _omap_hwmod_idle()(mutex-free version)
> will not let us call pm_runtime APIs in Idle path as we are not supposed
> to enable interrupts there. We have faced problems with USB big-time in
> Idle path while using pm_runtime functions. So, we are resorting to
> calling omap_device_idle() in CPU_Idle path, and runtime functions in
> thread-context.
> >
> > Is this acceptable, given the limitations?
> 
> No, this is not (yet) acceptable.
> 
> The limitations you've come up against are not fully understood yet.
> Rather trying to hack around the limitations, we need to fully
> understand the root causes, which has not yet been done.
> 
> It's possible (and likely) that there are different ways to handle these
> problems, and entirely possible that there are locking issues we have to
> resolve in the omap_device/hwmod layers as well.
> 

GPIO:
The rationale for doing omap_device_idle/enable + context save/restore in the Idle path has been provided in the GPIO patch. We can continue the discussion on this topic on the individual submission lists of the drivers.

UART:
We will try to remove it during UART cleanup activity that will happen .

USB: Calling pm_runtime_get/put_sync in Idle path for USB is leading to IO-pad interrupts, therefore we are resorting to using omap_deviceIdle/Enable APIs. The MUSB patch will be posted shortly.
> Kevin

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Monday, August 09, 2010 9:01 PM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> >> Finally, we have omap_sram_idle():
> >>
> >> > In particular, for USB, while executing SRAM_Idle, is we use
> pm_runtime
> >> > functions, we see spurious IO-Pad interrupts.
> >>
> >> Your message doesn't specify what PM runtime functions you are
> executing.
> >> But if those functions are calling mutex_lock(), then they obviously
> must
> >> not be called from omap_sram_idle() in interrupt context.
> >>
> >> It's not clear from your message why you need to call PM runtime
> functions
> >> from the idle loop.  I'd suggest that you post the problematic code in
> >> question to the list.
> >>
> >
> > USB issue:
> >
> > Please refer to the USB patch attached (will be sent to the list as well
> in a few minutes)
> > As I mentioned previously, few drivers like GPIO, USB & UART have clock-
> disable/enable + context save/restore in Idle path(omap_sram_idle()).
> >
> > In particular, I am referring to calling of the functions
> pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.
> >
> > Now, the following call sequence from omap_sram_idle()will enable IRQs:
> > pm_runtime_put_sync-->
> > 	__pm_runtime_put-->
> > 		pm_runtime_idle-->
> > 			spin_unlock_irq(),
> > 			__pm_runtime_idle(),-->
> > 			 spin_unlock_irq,
> > 				spin_unlock_irq
> >
> > The functions pm_runtime_idle() & __ pm_runtime_idle() do not use
> > spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of
> > the interrupts in omap_sram_idle unconditionally, which for USB in
> > particular is leading to IO-pad interrupt being triggered which does
> > not come otherwise.
> 
> You seem to have correctly identified the problem.  Can you try the
> patch below (untested) which attempts to address the root cause you've
> identified?
> 
> Kevin
> 
> > To work around this problem, instead of using Runtime APIs, we are using
> omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle
> >
> > Simply put, I am not talking about grabbing a mutex when interrupts are
> disabled, rather of a scenario where interrupts are getting enabled as a
> side-effect of calling these functions in   omap_sram_idle().
> 
> From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@deeprootsystems.com>
> Date: Mon, 9 Aug 2010 08:12:39 -0700
> Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled
> context
> 
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts may already be disabled when
> runtime PM for these devices is initiated (using the _sync versions of
> the runtime PM API.)
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
> 
> pm_runtime_put_sync()
>     __pm_runtime_put()
>         pm_runtime_idle()
>             spin_lock_irq()
>             __pm_runtime_idle()
>                 spin_unlock_irq()   <--- interrupts unconditionally
> enabled
>                 dev->bus->pm->runtime_idle()
>                 spin_lock_irq()
>              spin_unlock_irq()
> 
> To fix, use the save/restore versions of the spinlock API.
> 
> Reported-by: Partha Basak <p-basak2@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  drivers/base/power/runtime.c |   68 ++++++++++++++++++++++---------------
> ----
>  include/linux/pm.h           |    1 +
>  2 files changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b0ec0e9..b02ef35 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -80,24 +80,24 @@ static int __pm_runtime_idle(struct device *dev)
>  	dev->power.idle_notification = true;
> 
>  	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		dev->bus->pm->runtime_idle(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	} else if (dev->type && dev->type->pm && dev->type->pm-
> >runtime_idle) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		dev->type->pm->runtime_idle(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	} else if (dev->class && dev->class->pm
>  	    && dev->class->pm->runtime_idle) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		dev->class->pm->runtime_idle(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	}
> 
>  	dev->power.idle_notification = false;
> @@ -115,9 +115,9 @@ int pm_runtime_idle(struct device *dev)
>  {
>  	int retval;
> 
> -	spin_lock_irq(&dev->power.lock);
> +	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	retval = __pm_runtime_idle(dev);
> -	spin_unlock_irq(&dev->power.lock);
> +	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
> 
>  	return retval;
>  }
> @@ -187,11 +187,13 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
>  			if (dev->power.runtime_status != RPM_SUSPENDING)
>  				break;
> 
> -			spin_unlock_irq(&dev->power.lock);
> +			spin_unlock_irqrestore(&dev->power.lock,
> +					       dev->power.lock_flags);
> 
>  			schedule();
> 
> -			spin_lock_irq(&dev->power.lock);
> +			spin_lock_irqsave(&dev->power.lock,
> +					  dev->power.lock_flags);
>  		}
>  		finish_wait(&dev->power.wait_queue, &wait);
>  		goto repeat;
> @@ -201,27 +203,27 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
>  	dev->power.deferred_resume = false;
> 
>  	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->bus->pm->runtime_suspend(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else if (dev->type && dev->type->pm
>  	    && dev->type->pm->runtime_suspend) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->type->pm->runtime_suspend(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else if (dev->class && dev->class->pm
>  	    && dev->class->pm->runtime_suspend) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->class->pm->runtime_suspend(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else {
>  		retval = -ENOSYS;
> @@ -257,11 +259,11 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
>  		__pm_runtime_idle(dev);
> 
>  	if (parent && !parent->power.ignore_children) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		pm_request_idle(parent);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	}
> 
>   out:
> @@ -278,9 +280,9 @@ int pm_runtime_suspend(struct device *dev)
>  {
>  	int retval;
> 
> -	spin_lock_irq(&dev->power.lock);
> +	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	retval = __pm_runtime_suspend(dev, false);
> -	spin_unlock_irq(&dev->power.lock);
> +	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
> 
>  	return retval;
>  }
> @@ -342,11 +344,13 @@ int __pm_runtime_resume(struct device *dev, bool
> from_wq)
>  			    && dev->power.runtime_status != RPM_SUSPENDING)
>  				break;
> 
> -			spin_unlock_irq(&dev->power.lock);
> +			spin_unlock_irqrestore(&dev->power.lock,
> +					       dev->power.lock_flags);
> 
>  			schedule();
> 
> -			spin_lock_irq(&dev->power.lock);
> +			spin_lock_irqsave(&dev->power.lock,
> +					  dev->power.lock_flags);
>  		}
>  		finish_wait(&dev->power.wait_queue, &wait);
>  		goto repeat;
> @@ -384,27 +388,27 @@ int __pm_runtime_resume(struct device *dev, bool
> from_wq)
>  	dev->power.runtime_status = RPM_RESUMING;
> 
>  	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->bus->pm->runtime_resume(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else if (dev->type && dev->type->pm
>  	    && dev->type->pm->runtime_resume) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->type->pm->runtime_resume(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else if (dev->class && dev->class->pm
>  	    && dev->class->pm->runtime_resume) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		retval = dev->class->pm->runtime_resume(dev);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  		dev->power.runtime_error = retval;
>  	} else {
>  		retval = -ENOSYS;
> @@ -425,11 +429,11 @@ int __pm_runtime_resume(struct device *dev, bool
> from_wq)
> 
>   out:
>  	if (parent) {
> -		spin_unlock_irq(&dev->power.lock);
> +		spin_unlock_irqrestore(&dev->power.lock, dev-
> >power.lock_flags);
> 
>  		pm_runtime_put(parent);
> 
> -		spin_lock_irq(&dev->power.lock);
> +		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	}
> 
>  	dev_dbg(dev, "__pm_runtime_resume() returns %d!\n", retval);
> @@ -445,9 +449,9 @@ int pm_runtime_resume(struct device *dev)
>  {
>  	int retval;
> 
> -	spin_lock_irq(&dev->power.lock);
> +	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	retval = __pm_runtime_resume(dev, false);
> -	spin_unlock_irq(&dev->power.lock);
> +	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
> 
>  	return retval;
>  }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8e258c7..1a3d514 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -464,6 +464,7 @@ struct dev_pm_info {
>  	struct work_struct	work;
>  	wait_queue_head_t	wait_queue;
>  	spinlock_t		lock;
> +	unsigned long           lock_flags;
>  	atomic_t		usage_count;
>  	atomic_t		child_count;
>  	unsigned int		disable_depth:3;
> --
> 1.7.2.1

Thanks Kevin. This looks good from interrupt locking standpoint. We will test this patch on GPIO and USB and revert back. 

One question. Is it OK to call Schedule() with interrupts disabled?

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Shilimkar, Santosh
> Sent: Monday, August 09, 2010 9:43 PM
> To: Kevin Hilman
> Cc: Basak, Partha; Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi,
> Hema; Raja, Govindraj; Varadarajan, Charulatha
> Subject: RE: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 

[Snip]

> > >> pm_runtime_put_sync()
> > >>     __pm_runtime_put()
> > >>         pm_runtime_idle()
> > >>             spin_lock_irq()
> > >>             __pm_runtime_idle()
> > >>                 spin_unlock_irq()   <--- interrupts unconditionally
> > >> enabled
> > >>                 dev->bus->pm->runtime_idle()
> > >>                 spin_lock_irq()
> > >>              spin_unlock_irq()
> > >>
> > >> To fix, use the save/restore versions of the spinlock API.
> > >>
> > > Similar thing was thought when this problem was encountered but
> > > there was a concern that it may lead to interrupts are being disable
> > > for longer times
> >
> > why?
> >
> > if interrupts are enabled when this API is used, this patch doesn't
> > change the amount of time interrupts are disabled.
> >
> > if interrupts are already disabled, then you *want* interrupts to be
> > disabled for the entire time.
> >
> Correct me if I am off the track here.
> 
> If these API's are _always_ called with interrupt disabled then probably
> we don't even need the new spinlock version locking.
> 
> Considering the API is called with interrupt enabled.
> 
> int pm_runtime_suspend(struct device *dev)
>  {
>  	int retval;
> 
> 	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
>  	retval = __pm_runtime_suspend(dev, false);
> 	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);c
> 
> The interrupt on local cpu remain disabled till "__pm_runtime_suspend"
> does its job and re-enables the interrupts. No?
> 
> 
Sorry Kevin for oversight. The existing used lock is already a irq version.
You are right. This change should be fine. Some how I was under impression
the existing code was using plain spin lock version.

Really sorry about the noise.

Regards,
Santosh

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Shilimkar, Santosh]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Monday, August 09, 2010 9:25 PM
> To: Shilimkar, Santosh
> Cc: Basak, Partha; Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi,
> Hema; Raja, Govindraj; Varadarajan, Charulatha
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> >> Sent: Monday, August 09, 2010 9:01 PM
> >> To: Basak, Partha
> >> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> >> Govindraj; Varadarajan, Charulatha
> >> Subject: Re: Issues with calling pm_runtime functions in
> >> platform_pm_suspend_noirq/IRQ disabled context.
> >>
> >> "Basak, Partha" <p-basak2@ti.com> writes:
> >>
> >> >> Finally, we have omap_sram_idle():
> >> >>
> >> >> > In particular, for USB, while executing SRAM_Idle, is we use
> >> pm_runtime
> >> >> > functions, we see spurious IO-Pad interrupts.
> >> >>
> >> >> Your message doesn't specify what PM runtime functions you are
> >> executing.
> >> >> But if those functions are calling mutex_lock(), then they obviously
> >> must
> >> >> not be called from omap_sram_idle() in interrupt context.
> >> >>
> >> >> It's not clear from your message why you need to call PM runtime
> >> functions
> >> >> from the idle loop.  I'd suggest that you post the problematic code
> in
> >> >> question to the list.
> >> >>
> >> >
> >> > USB issue:
> >> >
> >> > Please refer to the USB patch attached (will be sent to the list as
> well
> >> in a few minutes)
> >> > As I mentioned previously, few drivers like GPIO, USB & UART have
> clock-
> >> disable/enable + context save/restore in Idle path(omap_sram_idle()).
> >> >
> >> > In particular, I am referring to calling of the functions
> >> pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.
> >> >
> >> > Now, the following call sequence from omap_sram_idle()will enable
> IRQs:
> >> > pm_runtime_put_sync-->
> >> > 	__pm_runtime_put-->
> >> > 		pm_runtime_idle-->
> >> > 			spin_unlock_irq(),
> >> > 			__pm_runtime_idle(),-->
> >> > 			 spin_unlock_irq,
> >> > 				spin_unlock_irq
> >> >
> >> > The functions pm_runtime_idle() & __ pm_runtime_idle() do not use
> >> > spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of
> >> > the interrupts in omap_sram_idle unconditionally, which for USB in
> >> > particular is leading to IO-pad interrupt being triggered which does
> >> > not come otherwise.
> >>
> >> You seem to have correctly identified the problem.  Can you try the
> >> patch below (untested) which attempts to address the root cause you've
> >> identified?
> >>
> >> Kevin
> >>
> >> > To work around this problem, instead of using Runtime APIs, we are
> using
> >> omap_device_enable/disable, which in-turn call to
> __omap_hwmod_enable/idle
> >> >
> >> > Simply put, I am not talking about grabbing a mutex when interrupts
> are
> >> disabled, rather of a scenario where interrupts are getting enabled as
> a
> >> side-effect of calling these functions in   omap_sram_idle().
> >>
> >> From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
> >> From: Kevin Hilman <khilman@deeprootsystems.com>
> >> Date: Mon, 9 Aug 2010 08:12:39 -0700
> >> Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled
> >> context
> >>
> >> When using runtime PM in combination with CPUidle, the runtime PM
> >> transtions of some devices may be triggered during the idle path.
> >> Late in the idle sequence, interrupts may already be disabled when
> >> runtime PM for these devices is initiated (using the _sync versions of
> >> the runtime PM API.)
> >>
> >> Currently, the runtime PM core assumes methods are called with
> >> interrupts enabled.  However, if it is called with interrupts
> >> disabled, the internal locking unconditionally enables interrupts, for
> >> example:
> >>
> >> pm_runtime_put_sync()
> >>     __pm_runtime_put()
> >>         pm_runtime_idle()
> >>             spin_lock_irq()
> >>             __pm_runtime_idle()
> >>                 spin_unlock_irq()   <--- interrupts unconditionally
> >> enabled
> >>                 dev->bus->pm->runtime_idle()
> >>                 spin_lock_irq()
> >>              spin_unlock_irq()
> >>
> >> To fix, use the save/restore versions of the spinlock API.
> >>
> > Similar thing was thought when this problem was encountered but
> > there was a concern that it may lead to interrupts are being disable
> > for longer times
> 
> why?
> 
> if interrupts are enabled when this API is used, this patch doesn't
> change the amount of time interrupts are disabled.
> 
> if interrupts are already disabled, then you *want* interrupts to be
> disabled for the entire time.
> 
Correct me if I am off the track here.

If these API's are _always_ called with interrupt disabled then probably we don't even need the new spinlock version locking.

Considering the API is called with interrupt enabled.

int pm_runtime_suspend(struct device *dev)
 {
 	int retval;
 
	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_suspend(dev, false);
	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);c

The interrupt on local cpu remain disabled till "__pm_runtime_suspend" does its job and re-enables the interrupts. No?


Regards,
Santosh

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:

>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Kevin Hilman
>> Sent: Monday, August 09, 2010 9:01 PM
>> To: Basak, Partha
>> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
>> Govindraj; Varadarajan, Charulatha
>> Subject: Re: Issues with calling pm_runtime functions in
>> platform_pm_suspend_noirq/IRQ disabled context.
>> 
>> "Basak, Partha" <p-basak2@ti.com> writes:
>> 
>> >> Finally, we have omap_sram_idle():
>> >>
>> >> > In particular, for USB, while executing SRAM_Idle, is we use
>> pm_runtime
>> >> > functions, we see spurious IO-Pad interrupts.
>> >>
>> >> Your message doesn't specify what PM runtime functions you are
>> executing.
>> >> But if those functions are calling mutex_lock(), then they obviously
>> must
>> >> not be called from omap_sram_idle() in interrupt context.
>> >>
>> >> It's not clear from your message why you need to call PM runtime
>> functions
>> >> from the idle loop.  I'd suggest that you post the problematic code in
>> >> question to the list.
>> >>
>> >
>> > USB issue:
>> >
>> > Please refer to the USB patch attached (will be sent to the list as well
>> in a few minutes)
>> > As I mentioned previously, few drivers like GPIO, USB & UART have clock-
>> disable/enable + context save/restore in Idle path(omap_sram_idle()).
>> >
>> > In particular, I am referring to calling of the functions
>> pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.
>> >
>> > Now, the following call sequence from omap_sram_idle()will enable IRQs:
>> > pm_runtime_put_sync-->
>> > 	__pm_runtime_put-->
>> > 		pm_runtime_idle-->
>> > 			spin_unlock_irq(),
>> > 			__pm_runtime_idle(),-->
>> > 			 spin_unlock_irq,
>> > 				spin_unlock_irq
>> >
>> > The functions pm_runtime_idle() & __ pm_runtime_idle() do not use
>> > spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of
>> > the interrupts in omap_sram_idle unconditionally, which for USB in
>> > particular is leading to IO-pad interrupt being triggered which does
>> > not come otherwise.
>> 
>> You seem to have correctly identified the problem.  Can you try the
>> patch below (untested) which attempts to address the root cause you've
>> identified?
>> 
>> Kevin
>> 
>> > To work around this problem, instead of using Runtime APIs, we are using
>> omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle
>> >
>> > Simply put, I am not talking about grabbing a mutex when interrupts are
>> disabled, rather of a scenario where interrupts are getting enabled as a
>> side-effect of calling these functions in   omap_sram_idle().
>> 
>> From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@deeprootsystems.com>
>> Date: Mon, 9 Aug 2010 08:12:39 -0700
>> Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled
>> context
>> 
>> When using runtime PM in combination with CPUidle, the runtime PM
>> transtions of some devices may be triggered during the idle path.
>> Late in the idle sequence, interrupts may already be disabled when
>> runtime PM for these devices is initiated (using the _sync versions of
>> the runtime PM API.)
>> 
>> Currently, the runtime PM core assumes methods are called with
>> interrupts enabled.  However, if it is called with interrupts
>> disabled, the internal locking unconditionally enables interrupts, for
>> example:
>> 
>> pm_runtime_put_sync()
>>     __pm_runtime_put()
>>         pm_runtime_idle()
>>             spin_lock_irq()
>>             __pm_runtime_idle()
>>                 spin_unlock_irq()   <--- interrupts unconditionally
>> enabled
>>                 dev->bus->pm->runtime_idle()
>>                 spin_lock_irq()
>>              spin_unlock_irq()
>> 
>> To fix, use the save/restore versions of the spinlock API.
>>
> Similar thing was thought when this problem was encountered but 
> there was a concern that it may lead to interrupts are being disable
> for longer times

why?

if interrupts are enabled when this API is used, this patch doesn't
change the amount of time interrupts are disabled.

if interrupts are already disabled, then you *want* interrupts to be
disabled for the entire time.

what exactly is the concern?

Kevin

> Are all run time PM APIs are short enough to keep interrupts
> disabled without impacting interrupt latency?
>  

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Monday, August 09, 2010 9:01 PM
> To: Basak, Partha
> Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
> Govindraj; Varadarajan, Charulatha
> Subject: Re: Issues with calling pm_runtime functions in
> platform_pm_suspend_noirq/IRQ disabled context.
> 
> "Basak, Partha" <p-basak2@ti.com> writes:
> 
> >> Finally, we have omap_sram_idle():
> >>
> >> > In particular, for USB, while executing SRAM_Idle, is we use
> pm_runtime
> >> > functions, we see spurious IO-Pad interrupts.
> >>
> >> Your message doesn't specify what PM runtime functions you are
> executing.
> >> But if those functions are calling mutex_lock(), then they obviously
> must
> >> not be called from omap_sram_idle() in interrupt context.
> >>
> >> It's not clear from your message why you need to call PM runtime
> functions
> >> from the idle loop.  I'd suggest that you post the problematic code in
> >> question to the list.
> >>
> >
> > USB issue:
> >
> > Please refer to the USB patch attached (will be sent to the list as well
> in a few minutes)
> > As I mentioned previously, few drivers like GPIO, USB & UART have clock-
> disable/enable + context save/restore in Idle path(omap_sram_idle()).
> >
> > In particular, I am referring to calling of the functions
> pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.
> >
> > Now, the following call sequence from omap_sram_idle()will enable IRQs:
> > pm_runtime_put_sync-->
> > 	__pm_runtime_put-->
> > 		pm_runtime_idle-->
> > 			spin_unlock_irq(),
> > 			__pm_runtime_idle(),-->
> > 			 spin_unlock_irq,
> > 				spin_unlock_irq
> >
> > The functions pm_runtime_idle() & __ pm_runtime_idle() do not use
> > spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of
> > the interrupts in omap_sram_idle unconditionally, which for USB in
> > particular is leading to IO-pad interrupt being triggered which does
> > not come otherwise.
> 
> You seem to have correctly identified the problem.  Can you try the
> patch below (untested) which attempts to address the root cause you've
> identified?
> 
> Kevin
> 
> > To work around this problem, instead of using Runtime APIs, we are using
> omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle
> >
> > Simply put, I am not talking about grabbing a mutex when interrupts are
> disabled, rather of a scenario where interrupts are getting enabled as a
> side-effect of calling these functions in   omap_sram_idle().
> 
> From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@deeprootsystems.com>
> Date: Mon, 9 Aug 2010 08:12:39 -0700
> Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled
> context
> 
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts may already be disabled when
> runtime PM for these devices is initiated (using the _sync versions of
> the runtime PM API.)
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
> 
> pm_runtime_put_sync()
>     __pm_runtime_put()
>         pm_runtime_idle()
>             spin_lock_irq()
>             __pm_runtime_idle()
>                 spin_unlock_irq()   <--- interrupts unconditionally
> enabled
>                 dev->bus->pm->runtime_idle()
>                 spin_lock_irq()
>              spin_unlock_irq()
> 
> To fix, use the save/restore versions of the spinlock API.
>
Similar thing was thought when this problem was encountered but 
there was a concern that it may lead to interrupts are being disable
for longer times

Are all run time PM APIs are short enough to keep interrupts
disabled without impacting interrupt latency?
 

Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman]

"Basak, Partha" <p-basak2@ti.com> writes:

>> Finally, we have omap_sram_idle():
>> 
>> > In particular, for USB, while executing SRAM_Idle, is we use pm_runtime
>> > functions, we see spurious IO-Pad interrupts.
>> 
>> Your message doesn't specify what PM runtime functions you are executing.
>> But if those functions are calling mutex_lock(), then they obviously must
>> not be called from omap_sram_idle() in interrupt context.
>> 
>> It's not clear from your message why you need to call PM runtime functions
>> from the idle loop.  I'd suggest that you post the problematic code in
>> question to the list.
>>
>
> USB issue:
>
> Please refer to the USB patch attached (will be sent to the list as well in a few minutes)
> As I mentioned previously, few drivers like GPIO, USB & UART have clock- disable/enable + context save/restore in Idle path(omap_sram_idle()).
>
> In particular, I am referring to calling of the functions pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.
>
> Now, the following call sequence from omap_sram_idle()will enable IRQs: 
> pm_runtime_put_sync-->
> 	__pm_runtime_put-->
> 		pm_runtime_idle-->
> 			spin_unlock_irq(),
> 			__pm_runtime_idle(),-->
> 			 spin_unlock_irq,
> 				spin_unlock_irq 
>
> The functions pm_runtime_idle() & __ pm_runtime_idle() do not use
> spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of
> the interrupts in omap_sram_idle unconditionally, which for USB in
> particular is leading to IO-pad interrupt being triggered which does
> not come otherwise.

You seem to have correctly identified the problem.  Can you try the
patch below (untested) which attempts to address the root cause you've
identified?

Kevin

> To work around this problem, instead of using Runtime APIs, we are using omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle
>
> Simply put, I am not talking about grabbing a mutex when interrupts are disabled, rather of a scenario where interrupts are getting enabled as a side-effect of calling these functions in   omap_sram_idle().

>From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@deeprootsystems.com>
Date: Mon, 9 Aug 2010 08:12:39 -0700
Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled context

When using runtime PM in combination with CPUidle, the runtime PM
transtions of some devices may be triggered during the idle path.
Late in the idle sequence, interrupts may already be disabled when
runtime PM for these devices is initiated (using the _sync versions of
the runtime PM API.)

Currently, the runtime PM core assumes methods are called with
interrupts enabled.  However, if it is called with interrupts
disabled, the internal locking unconditionally enables interrupts, for
example:

pm_runtime_put_sync()
    __pm_runtime_put()
        pm_runtime_idle()
            spin_lock_irq()
            __pm_runtime_idle()
                spin_unlock_irq()   <--- interrupts unconditionally enabled
                dev->bus->pm->runtime_idle()
                spin_lock_irq()
             spin_unlock_irq()

To fix, use the save/restore versions of the spinlock API.

Reported-by: Partha Basak <p-basak2@ti.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 drivers/base/power/runtime.c |   68 ++++++++++++++++++++++-------------------
 include/linux/pm.h           |    1 +
 2 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b0ec0e9..b02ef35 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -80,24 +80,24 @@ static int __pm_runtime_idle(struct device *dev)
 	dev->power.idle_notification = true;
 
 	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->bus->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->type->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->class->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
 	dev->power.idle_notification = false;
@@ -115,9 +115,9 @@ int pm_runtime_idle(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_idle(dev);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
@@ -187,11 +187,13 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 			if (dev->power.runtime_status != RPM_SUSPENDING)
 				break;
 
-			spin_unlock_irq(&dev->power.lock);
+			spin_unlock_irqrestore(&dev->power.lock,
+					       dev->power.lock_flags);
 
 			schedule();
 
-			spin_lock_irq(&dev->power.lock);
+			spin_lock_irqsave(&dev->power.lock,
+					  dev->power.lock_flags);
 		}
 		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
@@ -201,27 +203,27 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 	dev->power.deferred_resume = false;
 
 	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->bus->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->type && dev->type->pm
 	    && dev->type->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->type->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->class->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -257,11 +259,11 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 		__pm_runtime_idle(dev);
 
 	if (parent && !parent->power.ignore_children) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		pm_request_idle(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
  out:
@@ -278,9 +280,9 @@ int pm_runtime_suspend(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_suspend(dev, false);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
@@ -342,11 +344,13 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 			    && dev->power.runtime_status != RPM_SUSPENDING)
 				break;
 
-			spin_unlock_irq(&dev->power.lock);
+			spin_unlock_irqrestore(&dev->power.lock,
+					       dev->power.lock_flags);
 
 			schedule();
 
-			spin_lock_irq(&dev->power.lock);
+			spin_lock_irqsave(&dev->power.lock,
+					  dev->power.lock_flags);
 		}
 		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
@@ -384,27 +388,27 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 	dev->power.runtime_status = RPM_RESUMING;
 
 	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->bus->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->type && dev->type->pm
 	    && dev->type->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->type->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->class->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -425,11 +429,11 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 
  out:
 	if (parent) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		pm_runtime_put(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
 	dev_dbg(dev, "__pm_runtime_resume() returns %d!\n", retval);
@@ -445,9 +449,9 @@ int pm_runtime_resume(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_resume(dev, false);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 8e258c7..1a3d514 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -464,6 +464,7 @@ struct dev_pm_info {
 	struct work_struct	work;
 	wait_queue_head_t	wait_queue;
 	spinlock_t		lock;
+	unsigned long           lock_flags;
 	atomic_t		usage_count;
 	atomic_t		child_count;
 	unsigned int		disable_depth:3;
-- 
1.7.2.1

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Basak, Partha]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

> Finally, we have omap_sram_idle():
> 
> > In particular, for USB, while executing SRAM_Idle, is we use pm_runtime
> > functions, we see spurious IO-Pad interrupts.
> 
> Your message doesn't specify what PM runtime functions you are executing.
> But if those functions are calling mutex_lock(), then they obviously must
> not be called from omap_sram_idle() in interrupt context.
> 
> It's not clear from your message why you need to call PM runtime functions
> from the idle loop.  I'd suggest that you post the problematic code in
> question to the list.
>

USB issue:

Please refer to the USB patch attached (will be sent to the list as well in a few minutes)
As I mentioned previously, few drivers like GPIO, USB & UART have clock- disable/enable + context save/restore in Idle path(omap_sram_idle()).

In particular, I am referring to calling of the functions pm_runtime_put_sync() & pm_runtime_get_sync() for USB around wfi.

Now, the following call sequence from omap_sram_idle()will enable IRQs: 
pm_runtime_put_sync-->
	__pm_runtime_put-->
		pm_runtime_idle-->
			spin_unlock_irq(),
			__pm_runtime_idle(),-->
			 spin_unlock_irq,
				spin_unlock_irq 

The functions pm_runtime_idle() & __ pm_runtime_idle() do not use spin_lock_irqsave & spin_unlock_irqrestore. This leads to enabling of the interrupts in omap_sram_idle unconditionally, which for USB in particular is leading to IO-pad interrupt being triggered which does not come otherwise.

To work around this problem, instead of using Runtime APIs, we are using omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle

Simply put, I am not talking about grabbing a mutex when interrupts are disabled, rather of a scenario where interrupts are getting enabled as a side-effect of calling these functions in   omap_sram_idle().
> 
> - Paul

[-- Attachment #2: 0008-runtime-pm-changes.patch --]
[-- Type: application/octet-stream, Size: 13065 bytes --]

From: Hema HK  <hemahk@ti.com>
Subject: [PATCH 8/8 ]usb : musb:Using runtime pm apis for musb.

Calling runtime pm APIs pm_runtime_put_sync() and pm_runtime_get_sync() 
for enabling/disabling the clocks,sysconfig settings.

used omap_hwmod_enable_wakeup & omap_hwmod_disable_wakeup apis to set/clear
the wakeup enable bit.
Also need to put the USB in force standby and force idle mode when usb not used
and set it back to smart idle and smart stndby after wakeup.
these cases are handled using the oh flags.
For omap3430 auto idle bit has to be disabled because of the errata.So using 
HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.

Signed-off-by: Hema HK <hemahk@ti.com>
Signed-off-by: Basak, Partha <p-basak2@ti.com>

Cc: Felipe Balbi <felipe.balbi@nokia.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---

 arch/arm/mach-omap2/pm34xx.c          |   10 +++-
 arch/arm/mach-omap2/usb-musb.c        |   72 ++++++++++++++++++++++++++++++-
 arch/arm/plat-omap/include/plat/usb.h |    9 +++
 drivers/usb/musb/musb_core.c          |   12 +++++
 drivers/usb/musb/omap2430.c           |   77 +++++-----------------------------
 include/linux/usb/musb.h              |   18 +++++++
 6 files changed, 126 insertions(+), 72 deletions(-)

Index: linux-omap-pm/arch/arm/mach-omap2/pm34xx.c
===================================================================
--- linux-omap-pm.orig/arch/arm/mach-omap2/pm34xx.c	2010-08-06 10:44:06.000000000 -0400
+++ linux-omap-pm/arch/arm/mach-omap2/pm34xx.c	2010-08-09 06:20:33.201904089 -0400
@@ -418,7 +418,9 @@
 			omap3_core_save_context();
 			omap3_prcm_save_context();
 			/* Save MUSB context */
-			musb_context_save_restore(1);
+			musb_context_save_restore(save_context);
+		} else {
+			musb_context_save_restore(disable_clk);
 		}
 	}
 
@@ -462,7 +464,9 @@
 			omap3_sram_restore_context();
 			omap2_sms_restore_context();
 			/* restore MUSB context */
-			musb_context_save_restore(0);
+			musb_context_save_restore(restore_context);
+		} else {
+			musb_context_save_restore(enable_clk);
 		}
 		omap_uart_resume_idle(0);
 		omap_uart_resume_idle(1);
Index: linux-omap-pm/arch/arm/mach-omap2/usb-musb.c
===================================================================
--- linux-omap-pm.orig/arch/arm/mach-omap2/usb-musb.c	2010-08-06 10:44:06.000000000 -0400
+++ linux-omap-pm/arch/arm/mach-omap2/usb-musb.c	2010-08-09 06:22:01.465914181 -0400
@@ -25,11 +25,13 @@
 #include <linux/io.h>
 
 #include <linux/usb/musb.h>
+#include <linux/pm_runtime.h>
 
 #include <mach/hardware.h>
 #include <mach/irqs.h>
 #include <plat/usb.h>
 #include <plat/omap_device.h>
+#include <plat/omap_hwmod.h>
 
 #ifdef CONFIG_USB_MUSB_SOC
 
@@ -63,12 +65,32 @@
 
 static u64 musb_dmamask = DMA_BIT_MASK(32);
 
+static int usb_idle_hwmod(struct omap_device *od)
+{
+	struct omap_hwmod *oh = *od->hwmods;
+	if (irqs_disabled())
+		_omap_hwmod_idle(oh);
+	else
+		omap_device_idle_hwmods(od);
+	return 0;
+}
+
+static int usb_enable_hwmod(struct omap_device *od)
+{
+	struct omap_hwmod *oh = *od->hwmods;
+	if (irqs_disabled())
+		_omap_hwmod_enable(oh);
+	else
+		omap_device_enable_hwmods(od);
+	return 0;
+}
+
 static struct omap_device_pm_latency omap_musb_latency[] = {
-	  {
-		.deactivate_func = omap_device_idle_hwmods,
-		.activate_func   = omap_device_enable_hwmods,
+	{
+		.deactivate_func = usb_idle_hwmod,
+		.activate_func	 = usb_enable_hwmod,
 		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
-	  },
+	},
 };
 
 void __init usb_musb_init(struct omap_musb_board_data *board_data)
@@ -99,6 +121,18 @@
 		musb_plat.board_data = board_data;
 		musb_plat.power = board_data->power >> 1;
 		musb_plat.mode = board_data->mode;
+		musb_plat.device_enable = omap_device_enable;
+		musb_plat.device_idle = omap_device_idle;
+		musb_plat.enable_wakeup = omap_device_enable_wakeup;
+		musb_plat.disable_wakeup = omap_device_disable_wakeup;
+		/*
+		 * Errata 1.166 idle_req/ack is broken in omap3430
+		 * workaround is to disable the autodile bit for omap3430.
+		 */
+		if (cpu_is_omap3430())
+			oh->flags |= HWMOD_NO_OCP_AUTOIDLE;
+
+		musb_plat.oh = oh;
 		pdata = &musb_plat;
 
 		od = omap_device_build(name, bus_id, oh, pdata,
@@ -120,7 +154,7 @@
 	}
 }
 
-void musb_context_save_restore(int save)
+void musb_context_save_restore(enum musb_state state)
 {
 	struct omap_hwmod *oh = omap_hwmod_lookup("usb_otg_hs");
 	struct omap_device *od = oh->od;
@@ -129,14 +163,64 @@
 	struct device_driver *drv = dev->driver;
 
 	if (drv) {
+#ifdef CONFIG_PM_RUNTIME
 		struct musb_hdrc_platform_data *pdata = dev->platform_data;
 		const struct dev_pm_ops *pm = drv->pm;
-		if (!pdata->is_usb_active(dev)) {
 
-			if (save)
+		if (!pdata->is_usb_active(dev)) {
+			switch (state) {
+			case save_context:
+				/* If the usb device not active then Save
+				 * the context,set the sysconfig setting to
+				 * force standby force idle during idle and
+				 * disable the clock.
+				 */
+				oh->flags |= HWMOD_SWSUP_SIDLE
+						| HWMOD_SWSUP_MSTANDBY;
 				pm->suspend(dev);
-			else
+				pdata->device_idle(pdev);
+
+				break;
+			case disable_clk:
+				/* If the usb device not active then
+				 * Save the context, set the sysconfig setting
+				 * to force standby force idle during idle and
+				 * disable the clock.
+				 */
+
+				oh->flags |= HWMOD_SWSUP_SIDLE
+					| HWMOD_SWSUP_MSTANDBY;
+				pdata->device_idle(pdev);
+
+				break;
+
+			case restore_context:
+				/* Enable the clock, set the sysconfig
+				 * back to smart idle and smart stndby after
+				 * wakeup. restore the context.
+				 */
+				oh->flags &= ~(HWMOD_SWSUP_SIDLE
+					| HWMOD_SWSUP_MSTANDBY);
+				pdata->device_enable(pdev);
 				pm->resume_noirq(dev);
+
+				break;
+
+			case enable_clk:
+				/* Set the sysconfig back to smart
+				 * idle and smart stndby after wakeup and
+				 * enable the clock
+				 */
+				oh->flags &= ~(HWMOD_SWSUP_SIDLE
+					| HWMOD_SWSUP_MSTANDBY);
+				pdata->device_enable(pdev);
+
+				break;
+
+			default:
+				break;
+			}
+#endif
 		}
 	}
 }
Index: linux-omap-pm/arch/arm/plat-omap/include/plat/usb.h
===================================================================
--- linux-omap-pm.orig/arch/arm/plat-omap/include/plat/usb.h	2010-08-06 10:44:06.000000000 -0400
+++ linux-omap-pm/arch/arm/plat-omap/include/plat/usb.h	2010-08-06 10:44:42.942112875 -0400
@@ -71,6 +71,13 @@
 	unsigned extvbus:1;
 };
 
+enum musb_state {
+	save_context = 1,
+	disable_clk,
+	restore_context,
+	enable_clk,
+	};
+
 enum musb_interface    {MUSB_INTERFACE_ULPI, MUSB_INTERFACE_UTMI};
 
 extern void usb_musb_init(struct omap_musb_board_data *board_data);
@@ -80,7 +87,7 @@
 extern void usb_ohci_init(const struct ohci_hcd_omap_platform_data *pdata);
 
 /* For saving and restoring the musb context during off/wakeup*/
-extern void musb_context_save_restore(int save);
+extern void musb_context_save_restore(enum musb_state state);
 #endif
 
 
Index: linux-omap-pm/drivers/usb/musb/musb_core.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/musb_core.c	2010-08-06 10:44:06.000000000 -0400
+++ linux-omap-pm/drivers/usb/musb/musb_core.c	2010-08-06 10:44:42.942112875 -0400
@@ -2447,9 +2447,21 @@
 	return 0;
 }
 
+static int musb_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int musb_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
 static const struct dev_pm_ops musb_dev_pm_ops = {
 	.suspend	= musb_suspend,
 	.resume_noirq	= musb_resume_noirq,
+	.runtime_suspend = musb_runtime_suspend,
+	.runtime_resume	 = musb_runtime_resume,
 };
 
 #define MUSB_DEV_PM_OPS (&musb_dev_pm_ops)
Index: linux-omap-pm/drivers/usb/musb/omap2430.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/omap2430.c	2010-08-06 10:44:30.093914217 -0400
+++ linux-omap-pm/drivers/usb/musb/omap2430.c	2010-08-06 10:44:42.942112875 -0400
@@ -31,6 +31,7 @@
 #include <linux/list.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
 
 #include <plat/mux.h>
 
@@ -209,10 +210,6 @@
 	struct musb_hdrc_platform_data *plat = dev->platform_data;
 	struct omap_musb_board_data *data = plat->board_data;
 
-#if defined(CONFIG_ARCH_OMAP2430)
-	omap_cfg_reg(AE5_2430_USB0HS_STP);
-#endif
-
 	/* We require some kind of external transceiver, hooked
 	 * up through ULPI.  TWL4030-family PMICs include one,
 	 * which needs a driver, drivers aren't always needed.
@@ -224,22 +221,6 @@
 	}
 
 	musb_platform_resume(musb);
-
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	l &= ~NOSTDBY;		/* remove possible nostdby */
-	l |= SMARTSTDBY;	/* enable smart standby */
-	l &= ~AUTOIDLE;		/* disable auto idle */
-	l &= ~NOIDLE;		/* remove possible noidle */
-	l |= SMARTIDLE;		/* enable smart idle */
-	/*
-	 * MUSB AUTOIDLE don't work in 3430.
-	 * Workaround by Richard Woodruff/TI
-	 */
-	if (!cpu_is_omap3430())
-		l |= AUTOIDLE;		/* enable auto idle */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	l = musb_readl(musb->mregs, OTG_INTERFSEL);
 
 	if (data->interface_type == MUSB_INTERFACE_UTMI) {
@@ -273,48 +254,26 @@
 void musb_platform_save_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	/*
-	 * As per the omap-usbotg specification, configure it to forced standby
-	 * and  force idle mode when no activity on usb.
-	 */
 	void __iomem *musb_base = musb->mregs;
 
-	musb_writel(musb_base, OTG_FORCESTDBY, 0);
-
-	musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
-				OTG_SYSCONFIG) & ~(NOSTDBY | SMARTSTDBY));
-
-	musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
-					OTG_SYSCONFIG) & ~(AUTOIDLE));
-
-	musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
-				OTG_SYSCONFIG) & ~(NOIDLE | SMARTIDLE));
-
 	musb_writel(musb_base, OTG_FORCESTDBY, 1);
 }
 
 void musb_platform_restore_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	/*
-	 * As per the omap-usbotg specification, configure it smart standby
-	 * and smart idle during operation.
-	 */
 	void __iomem *musb_base = musb->mregs;
 
 	musb_writel(musb_base, OTG_FORCESTDBY, 0);
-
-	musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
-				OTG_SYSCONFIG) | (SMARTSTDBY));
-
-	musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
-				OTG_SYSCONFIG) | (SMARTIDLE));
 }
 #endif
 
 static int musb_platform_suspend(struct musb *musb)
 {
 	u32 l;
+	struct device *dev = musb->controller;
+	struct musb_hdrc_platform_data *pdata = dev->platform_data;
+	struct omap_hwmod *oh = pdata->oh;
 
 	if (!musb->clock)
 		return 0;
@@ -324,16 +283,9 @@
 	l |= ENABLEFORCE;	/* enable MSTANDBY */
 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l |= ENABLEWAKEUP;	/* enable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
+	pdata->enable_wakeup(oh->od);
 	otg_set_suspend(musb->xceiv, 1);
-
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 0);
-	else
-		clk_disable(musb->clock);
+	pm_runtime_put_sync(dev);
 
 	return 0;
 }
@@ -341,21 +293,18 @@
 static int musb_platform_resume(struct musb *musb)
 {
 	u32 l;
+	struct device *dev = musb->controller;
+	struct musb_hdrc_platform_data *pdata = dev->platform_data;
+	struct omap_hwmod *oh = pdata->oh;
 
 	if (!musb->clock)
 		return 0;
 
 	otg_set_suspend(musb->xceiv, 0);
-
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 1);
-	else
-		clk_enable(musb->clock);
-
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	pdata->disable_wakeup(oh->od);
+	pdata->enable_wakeup(oh->od);
 	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
 	l &= ~ENABLEFORCE;	/* disable MSTANDBY */
 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
Index: linux-omap-pm/include/linux/usb/musb.h
===================================================================
--- linux-omap-pm.orig/include/linux/usb/musb.h	2010-08-06 10:44:06.000000000 -0400
+++ linux-omap-pm/include/linux/usb/musb.h	2010-08-06 10:44:42.946115274 -0400
@@ -10,6 +10,9 @@
 #ifndef __LINUX_USB_MUSB_H
 #define __LINUX_USB_MUSB_H
 
+#include <linux/platform_device.h>
+#include <plat/omap_hwmod.h>
+
 /* The USB role is defined by the connector used on the board, so long as
  * standards are being followed.  (Developer boards sometimes won't.)
  */
@@ -129,6 +132,21 @@
 
 	/* check usb device active state*/
 	int		(*is_usb_active)(struct device *dev);
+
+	/* omap hwmod data structure	 */
+	struct	omap_hwmod *oh;
+
+	/* enable clocks and set sysconfig register*/
+	int		(*device_enable)(struct platform_device *pdev);
+
+	/* Disable clock and reset the sysconfig register settings*/
+	int		(*device_idle)(struct platform_device *pdev);
+
+	/* set the enable wakeup bit of sysconfig register */
+	int		(*enable_wakeup)(struct omap_device *od);
+
+	/* Clear the enable wakeup bit  of sysconfig register */
+	int		(*disable_wakeup)(struct omap_device *od);
 };
 
 

RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Paul Walmsley]

On Sat, 7 Aug 2010, Basak, Partha wrote:

> > -----Original Message-----
> > From: Paul Walmsley [mailto:paul@pwsan.com]
> > Sent: Saturday, August 07, 2010 1:00 AM
> > 
> > On Tue, 3 Aug 2010, Basak, Partha wrote:
> > 
> > > I believe, it is not correct to call the pm_runtime APIs from interrupt
> > > locked context
> > 
> > Why do you think that the PM runtime APIs are being called from interrupt
> > context?  Could you point out the call chain that you're seeing?
> >
> I meant that they should not not be called in a context which has 
> interrupts disabled. SRAM_Idle as well as suspend_no_irq are the 
> instances I was referring to.

It would be really helpful if you would use the exact names of the 
functions that you are referring to. This message assumes that your use of 
"SRAM_Idle" refers to omap_sram_idle() in pm34xx.c, and that 
"suspend_no_irq" refers to platform_pm_suspend_noirq() in pm_bus.c.

You write that "[the PM runtime functions] should not not be called in a 
context which has interrupts disabled."  All interrupts?  Some interrupts?  
"Context" in the general sense, or in the strict sense used in Linux 
interrupt handling?  It is difficult to comment on this statement since it 
is unclear.  I will guess that your message refers to the fact that some
of the PM runtime functions take a mutex, and therefore it is invalid to 
call those functions when the timer interrupt is disabled.

If that's what you mean, then it's worth observing that it's valid to call 
some PM runtime functions, such as pm_runtime_get_noresume(), 
pm_runtime_suspended(), etc., no matter which interrupts are enabled, 
since they don't take any mutex.  Similarly, it seems quite valid to call 
pretty much any PM runtime function as long as the timer interrupt is 
still running.  This is the case with platform_pm_suspend_noirq(), at 
least, with this call chain:

dpm_suspend_noirq() ->
  device_suspend_noirq() ->
    pm_irq_op() ->
      dev_pm_ops.suspend_noirq ->
         platform_pm_suspend_noirq()

Unfortunately, your message didn't provide a call chain, so, not sure if 
we're even referring to the same code path.  Based on this chain, I don't 
see any interrupt-related problems with calling PM runtime functions from 
platform_pm_suspend_noirq().  If you are actually seeing some problem 
here, then you should post the warning that you're seeing and the call 
chain that's causing the problem.

Finally, we have omap_sram_idle():

> In particular, for USB, while executing SRAM_Idle, is we use pm_runtime 
> functions, we see spurious IO-Pad interrupts.

Your message doesn't specify what PM runtime functions you are executing.  
But if those functions are calling mutex_lock(), then they obviously must 
not be called from omap_sram_idle() in interrupt context.

It's not clear from your message why you need to call PM runtime functions 
from the idle loop.  I'd suggest that you post the problematic code in 
question to the list.


- Paul