[PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[Mike Chan]

Need to lock the res_mutex when traversing the res_list.

Signed-off-by: Mike Chan <mike@android.com>
---
 arch/arm/plat-omap/resource.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
index 25072cd..4631912 100644
--- a/arch/arm/plat-omap/resource.c
+++ b/arch/arm/plat-omap/resource.c
@@ -234,11 +234,13 @@ int resource_refresh(void)
 	struct shared_resource *resp = NULL;
 	int ret = 0;
 
+	down(&res_mutex);
 	list_for_each_entry(resp, &res_list, node) {
 		ret = update_resource_level(resp);
 		if (ret)
 			break;
 	}
+	up(&res_mutex);
 	return ret;
 }
 
-- 
1.5.4.5

Re: [PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> Need to lock the res_mutex when traversing the res_list.
>
> Signed-off-by: Mike Chan <mike@android.com>

Looks good, thanks.

Pushed to PM branch and pm-2.6.29.

Kevin

> ---
>  arch/arm/plat-omap/resource.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 25072cd..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -234,11 +234,13 @@ int resource_refresh(void)
>  	struct shared_resource *resp = NULL;
>  	int ret = 0;
>  
> +	down(&res_mutex);
>  	list_for_each_entry(resp, &res_list, node) {
>  		ret = update_resource_level(resp);
>  		if (ret)
>  			break;
>  	}
> +	up(&res_mutex);
>  	return ret;
>  }
>  
> -- 
> 1.5.4.5

Re: [PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[Mike Turquette]

Kevin Hilman wrote:
> Mike Chan <mike@android.com> writes:
> 
>> Need to lock the res_mutex when traversing the res_list.
>>
>> Signed-off-by: Mike Chan <mike@android.com>
> 
> Looks good, thanks.

This patch causes a hang for me when transitioning to OFF mode.  This 
was tested on the Android 2.6.29 tree and is 100% reproducible.  The 
moment a user runs 'echo 1 > /sys/power/enable_off_mode' the board hangs 
without any further output.

Reverting the patch allows me to hit OFF mode again.  I haven't yet 
tested this on vanilla 2.6.29 or latest L-O.

Mike

> Pushed to PM branch and pm-2.6.29.
> 
> Kevin
> 
>> ---
>>  arch/arm/plat-omap/resource.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>> index 25072cd..4631912 100644
>> --- a/arch/arm/plat-omap/resource.c
>> +++ b/arch/arm/plat-omap/resource.c
>> @@ -234,11 +234,13 @@ int resource_refresh(void)
>>  	struct shared_resource *resp = NULL;
>>  	int ret = 0;
>>  
>> +	down(&res_mutex);
>>  	list_for_each_entry(resp, &res_list, node) {
>>  		ret = update_resource_level(resp);
>>  		if (ret)
>>  			break;
>>  	}
>> +	up(&res_mutex);
>>  	return ret;
>>  }
>>  
>> -- 
>> 1.5.4.5
> --
> 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
> 

Re: [PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[Kevin Hilman]

Mike Turquette <mturquette@ti.com> writes:

> Kevin Hilman wrote:
>> Mike Chan <mike@android.com> writes:
>>
>>> Need to lock the res_mutex when traversing the res_list.
>>>
>>> Signed-off-by: Mike Chan <mike@android.com>
>>
>> Looks good, thanks.
>
> This patch causes a hang for me when transitioning to OFF mode.  This
> was tested on the Android 2.6.29 tree and is 100% reproducible.  The
> moment a user runs 'echo 1 > /sys/power/enable_off_mode' the board
> hangs without any further output.
>
> Reverting the patch allows me to hit OFF mode again.  I haven't yet
> tested this on vanilla 2.6.29 or latest L-O.
>

OK, reverting this in both PM branches.  Looks like a deadlock to me,
as update_resource_level() must cause a call to something else that
takes the mutex.

Kevin

>
>> Pushed to PM branch and pm-2.6.29.
>>
>> Kevin
>>
>>> ---
>>>  arch/arm/plat-omap/resource.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>>> index 25072cd..4631912 100644
>>> --- a/arch/arm/plat-omap/resource.c
>>> +++ b/arch/arm/plat-omap/resource.c
>>> @@ -234,11 +234,13 @@ int resource_refresh(void)
>>>  	struct shared_resource *resp = NULL;
>>>  	int ret = 0;
>>>  +	down(&res_mutex);
>>>  	list_for_each_entry(resp, &res_list, node) {
>>>  		ret = update_resource_level(resp);
>>>  		if (ret)
>>>  			break;
>>>  	}
>>> +	up(&res_mutex);
>>>  	return ret;
>>>  }
>>>  -- 
>>> 1.5.4.5
>> --
>> 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
>>

Re: [PATCH] [ARM] omap: resource: Make resource_refresh() thread safe.

[Mike Chan]

On Fri, Sep 11, 2009 at 2:10 PM, Kevin
Hilman<khilman@deeprootsystems.com> wrote:
> Mike Turquette <mturquette@ti.com> writes:
>
>> Kevin Hilman wrote:
>>> Mike Chan <mike@android.com> writes:
>>>
>>>> Need to lock the res_mutex when traversing the res_list.
>>>>
>>>> Signed-off-by: Mike Chan <mike@android.com>
>>>
>>> Looks good, thanks.
>>
>> This patch causes a hang for me when transitioning to OFF mode.  This
>> was tested on the Android 2.6.29 tree and is 100% reproducible.  The
>> moment a user runs 'echo 1 > /sys/power/enable_off_mode' the board
>> hangs without any further output.
>>
>> Reverting the patch allows me to hit OFF mode again.  I haven't yet
>> tested this on vanilla 2.6.29 or latest L-O.
>>
>
> OK, reverting this in both PM branches.  Looks like a deadlock to me,
> as update_resource_level() must cause a call to something else that
> takes the mutex.
>

Good catch, I see the deadlock are in the change_level function
pointers. Where set_xxx will call resource_request() or
resource_release() => resource_lookup() which grabs the list mutex we
grabbed in resource_refresh().

Thoughts on changing the mutex to a RW semaphore?

-- Mike

> Kevin
>
>>
>>> Pushed to PM branch and pm-2.6.29.
>>>
>>> Kevin
>>>
>>>> ---
>>>>  arch/arm/plat-omap/resource.c |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>>>> index 25072cd..4631912 100644
>>>> --- a/arch/arm/plat-omap/resource.c
>>>> +++ b/arch/arm/plat-omap/resource.c
>>>> @@ -234,11 +234,13 @@ int resource_refresh(void)
>>>>     struct shared_resource *resp = NULL;
>>>>     int ret = 0;
>>>>  +  down(&res_mutex);
>>>>     list_for_each_entry(resp, &res_list, node) {
>>>>             ret = update_resource_level(resp);
>>>>             if (ret)
>>>>                     break;
>>>>     }
>>>> +   up(&res_mutex);
>>>>     return ret;
>>>>  }
>>>>  --
>>>> 1.5.4.5
>>> --
>>> 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
>>>
>
--
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