diff mbox

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

Message ID 1384297710-29694-1-git-send-email-nm@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nishanth Menon Nov. 12, 2013, 11:08 p.m. UTC 
OMAP device hooks around suspend|resume_noirq ensures that hwmod
devices are forced to idle using omap_device_idle/enable as part of
the last stage of suspend activity.

For a device such as i2c who uses autosuspend, it is possible to enter
the suspend path with dev->power.runtime_status = RPM_ACTIVE.

As part of the suspend flow, the generic runtime logic would increment
it's dev->power.disable_depth to 1. This should prevent further
pm_runtime_get_sync from succeeding once the runtime_status has been
set to RPM_SUSPENDED.

Now, as part of the suspend_noirq handler in omap_device, we force the
following: if the device status is !suspended, we force the device
to idle using omap_device_idle (clocks are cut etc..). This ensures
that from a hardware perspective, the device is "suspended". However,
runtime_status is left to be active.

*if* an operation is attempted after this point to
pm_runtime_get_sync, runtime framework depends on runtime_status to
indicate accurately the device status, and since it sees it to be
ACTIVE, it assumes the module is functional and returns a non-error
value. As a result the user will see pm_runtime_get succeed, however a
register access will crash due to the lack of clocks.

To prevent this from happening, we should ensure that runtime_status
exactly indicates the device status. As a result of this change
any further calls to pm_runtime_get* would return -EACCES (since
disable_depth is 1). On resume, we restore the clocks and runtime
status exactly as we suspended with.

Reported-by: J Keerthy <j-keerthy@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Acked-by: Rajendra Nayak <rnayak@ti.com>
Acked-by: Kevin Hilman <khilman@linaro.org>
---
Changes in V2 (resend):
	- dropped the unnecessary flag for runtime status restore
	- picked up kevin's ack from https://patchwork.kernel.org/patch/3168371/

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

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

Logs from 3.12 based vendor kernel:
Before: http://pastebin.com/m5KxnB7a
After: http://pastebin.com/8AfX4e7r
The discussion on cpufreq side of the story which triggered this scenario:
http://marc.info/?l=linux-pm&m=138263811321921&w=2

Tested on TI vendor kernel (with dt boot):
AM335x: evm, BBB, sk, BBW
OMAP5uEVM, DRA7-evm

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

Comments

Felipe Balbi Nov. 13, 2013, 12:51 p.m. UTC | #1 
Hi,

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

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

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

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

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

> 
>> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
>>  	struct platform_device *pdev = to_platform_device(dev);
>>  	struct omap_device *od = to_omap_device(pdev);
>>  
>> -	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>> -	    !pm_runtime_status_suspended(dev)) {
>> +	if (od->flags & OMAP_DEVICE_SUSPENDED) {
>>  		od->flags &= ~OMAP_DEVICE_SUSPENDED;
>>  		omap_device_enable(pdev);
>> +		pm_runtime_set_active(dev);
> 
> ditto, also pm_runtime_set_active() may fail.
> 
again, pm_runtime is not yet active here yet - we just restore the pm
runtime state with which we went down with -> and that is not expected
to fail either - So, how about just adding a WARN if our expectation
of balanced operation was somehow broken in the future with changes to
runtime framework?
Felipe Balbi Nov. 13, 2013, 3:20 p.m. UTC | #3 
On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
> On 11/13/2013 06:51 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
> >> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> >> index b69dd9a..f97b34b 100644
> >> --- a/arch/arm/mach-omap2/omap_device.c
> >> +++ b/arch/arm/mach-omap2/omap_device.c
> >> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
> >>  
> >>  	if (!ret && !pm_runtime_status_suspended(dev)) {
> >>  		if (pm_generic_runtime_suspend(dev) == 0) {
> >> +			pm_runtime_set_suspended(dev);
> > 
> > don't you have to disable pm_runtime around status changes ? Or is
> > pm_runtime already disabled by the time we get here ?
> 
> pm_runtime is already disabled by the time no_irq suspend is invoked.
> 
> > 
> >> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
> >>  	struct platform_device *pdev = to_platform_device(dev);
> >>  	struct omap_device *od = to_omap_device(pdev);
> >>  
> >> -	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
> >> -	    !pm_runtime_status_suspended(dev)) {
> >> +	if (od->flags & OMAP_DEVICE_SUSPENDED) {
> >>  		od->flags &= ~OMAP_DEVICE_SUSPENDED;
> >>  		omap_device_enable(pdev);
> >> +		pm_runtime_set_active(dev);
> > 
> > ditto, also pm_runtime_set_active() may fail.
> > 
> again, pm_runtime is not yet active here yet - we just restore the pm
> runtime state with which we went down with -> and that is not expected
> to fail either - So, how about just adding a WARN if our expectation
> of balanced operation was somehow broken in the future with changes to
> runtime framework?

you mean:

WARN(pm_runtime_set_active(dev));  ?

sounds good

thanks
Nishanth Menon Nov. 13, 2013, 3:28 p.m. UTC | #4 
On 11/13/2013 09:20 AM, Felipe Balbi wrote:
> On Wed, Nov 13, 2013 at 08:56:06AM -0600, Nishanth Menon wrote:
>> On 11/13/2013 06:51 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
>>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>>> index b69dd9a..f97b34b 100644
>>>> --- a/arch/arm/mach-omap2/omap_device.c
>>>> +++ b/arch/arm/mach-omap2/omap_device.c
>>>> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
>>>>  
>>>>  	if (!ret && !pm_runtime_status_suspended(dev)) {
>>>>  		if (pm_generic_runtime_suspend(dev) == 0) {
>>>> +			pm_runtime_set_suspended(dev);
>>>
>>> don't you have to disable pm_runtime around status changes ? Or is
>>> pm_runtime already disabled by the time we get here ?
>>
>> pm_runtime is already disabled by the time no_irq suspend is invoked.
>>
>>>
>>>> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
>>>>  	struct platform_device *pdev = to_platform_device(dev);
>>>>  	struct omap_device *od = to_omap_device(pdev);
>>>>  
>>>> -	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>>>> -	    !pm_runtime_status_suspended(dev)) {
>>>> +	if (od->flags & OMAP_DEVICE_SUSPENDED) {
>>>>  		od->flags &= ~OMAP_DEVICE_SUSPENDED;
>>>>  		omap_device_enable(pdev);
>>>> +		pm_runtime_set_active(dev);
>>>
>>> ditto, also pm_runtime_set_active() may fail.
>>>
>> again, pm_runtime is not yet active here yet - we just restore the pm
>> runtime state with which we went down with -> and that is not expected
>> to fail either - So, how about just adding a WARN if our expectation
>> of balanced operation was somehow broken in the future with changes to
>> runtime framework?
> 
> you mean:
> 
> WARN(pm_runtime_set_active(dev));  ?

yes

> 
> sounds good

Thanks. Will post a v3 tomorrow morning to give a chance for
discussions on further comments if any.
Kevin Hilman Nov. 13, 2013, 3:45 p.m. UTC | #5 
Nishanth Menon <nm@ti.com> writes:

> On 11/13/2013 06:51 AM, Felipe Balbi wrote:
>> Hi,
>> 
>> On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>> index b69dd9a..f97b34b 100644
>>> --- a/arch/arm/mach-omap2/omap_device.c
>>> +++ b/arch/arm/mach-omap2/omap_device.c
>>> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
>>>  
>>>  	if (!ret && !pm_runtime_status_suspended(dev)) {
>>>  		if (pm_generic_runtime_suspend(dev) == 0) {
>>> +			pm_runtime_set_suspended(dev);
>> 
>> don't you have to disable pm_runtime around status changes ? Or is
>> pm_runtime already disabled by the time we get here ?
>
> pm_runtime is already disabled by the time no_irq suspend is invoked.
>
>> 
>>> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
>>>  	struct platform_device *pdev = to_platform_device(dev);
>>>  	struct omap_device *od = to_omap_device(pdev);
>>>  
>>> -	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>>> -	    !pm_runtime_status_suspended(dev)) {
>>> +	if (od->flags & OMAP_DEVICE_SUSPENDED) {
>>>  		od->flags &= ~OMAP_DEVICE_SUSPENDED;
>>>  		omap_device_enable(pdev);
>>> +		pm_runtime_set_active(dev);
>> 
>> ditto, also pm_runtime_set_active() may fail.
>> 
> again, pm_runtime is not yet active here yet - we just restore the pm
> runtime state with which we went down with -> and that is not expected
> to fail either - So, how about just adding a WARN if our expectation
> of balanced operation was somehow broken in the future with changes to
> runtime framework?

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

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Nov. 13, 2013, 4:19 p.m. UTC | #6 
On Wed, Nov 13, 2013 at 9:45 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> On 11/13/2013 06:51 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Nov 12, 2013 at 05:08:30PM -0600, Nishanth Menon wrote:
>>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>>> index b69dd9a..f97b34b 100644
>>>> --- a/arch/arm/mach-omap2/omap_device.c
>>>> +++ b/arch/arm/mach-omap2/omap_device.c
>>>> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev)
>>>>
>>>>     if (!ret && !pm_runtime_status_suspended(dev)) {
>>>>             if (pm_generic_runtime_suspend(dev) == 0) {
>>>> +                   pm_runtime_set_suspended(dev);
>>>
>>> don't you have to disable pm_runtime around status changes ? Or is
>>> pm_runtime already disabled by the time we get here ?
>>
>> pm_runtime is already disabled by the time no_irq suspend is invoked.
>>
>>>
>>>> @@ -634,10 +635,10 @@ static int _od_resume_noirq(struct device *dev)
>>>>     struct platform_device *pdev = to_platform_device(dev);
>>>>     struct omap_device *od = to_omap_device(pdev);
>>>>
>>>> -   if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>>>> -       !pm_runtime_status_suspended(dev)) {
>>>> +   if (od->flags & OMAP_DEVICE_SUSPENDED) {
>>>>             od->flags &= ~OMAP_DEVICE_SUSPENDED;
>>>>             omap_device_enable(pdev);
>>>> +           pm_runtime_set_active(dev);
>>>
>>> ditto, also pm_runtime_set_active() may fail.
>>>
>> again, pm_runtime is not yet active here yet - we just restore the pm
>> runtime state with which we went down with -> and that is not expected
>> to fail either - So, how about just adding a WARN if our expectation
>> of balanced operation was somehow broken in the future with changes to
>> runtime framework?
>
> And also a note in the changelog (or comment at the WARN) about the
> assumption that runtime PM is disabled at this point.

Ofcourse. will do.

--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index b69dd9a..f97b34b 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -621,6 +621,7 @@  static int _od_suspend_noirq(struct device *dev)
 
 	if (!ret && !pm_runtime_status_suspended(dev)) {
 		if (pm_generic_runtime_suspend(dev) == 0) {
+			pm_runtime_set_suspended(dev);
 			omap_device_idle(pdev);
 			od->flags |= OMAP_DEVICE_SUSPENDED;
 		}
@@ -634,10 +635,10 @@  static int _od_resume_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
 
-	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
-	    !pm_runtime_status_suspended(dev)) {
+	if (od->flags & OMAP_DEVICE_SUSPENDED) {
 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
 		omap_device_enable(pdev);
+		pm_runtime_set_active(dev);
 		pm_generic_runtime_resume(dev);
 	}