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

[Kevin Hilman <khilman@deeprootsystems.com>]

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;