* [PATCH] omap: resource: Fix race in register_resource()
@ 2009-09-17 23:32 Mike Chan
2009-09-17 23:41 ` Mike Chan
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Mike Chan @ 2009-09-17 23:32 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, Mike Chan
Checking if the resource is already registered and adding to the list
must be atomic or bad things can happen.
Signed-off-by: Mike Chan <mike@android.com>
---
arch/arm/plat-omap/resource.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
index 111020a..4631912 100644
--- a/arch/arm/plat-omap/resource.c
+++ b/arch/arm/plat-omap/resource.c
@@ -255,6 +255,7 @@ int resource_refresh(void)
*/
int resource_register(struct shared_resource *resp)
{
+ int ret = 0;
if (!resp)
return -EINVAL;
@@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
return -EINVAL;
/* Verify that the resource is not already registered */
- if (resource_lookup(resp->name))
- return -EEXIST;
+ down(&res_mutex);
+ if (_resource_lookup(resp->name))
+ ret = -EEXIST;
+ goto out;
INIT_LIST_HEAD(&resp->users_list);
- down(&res_mutex);
/* Add the resource to the resource list */
list_add(&resp->node, &res_list);
@@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
if (resp->ops->init)
resp->ops->init(resp);
- up(&res_mutex);
pr_debug("resource: registered %s\n", resp->name);
- return 0;
+out:
+ up(&res_mutex);
+ return ret;
}
EXPORT_SYMBOL(resource_register);
--
1.5.4.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-17 23:32 [PATCH] omap: resource: Fix race in register_resource() Mike Chan
@ 2009-09-17 23:41 ` Mike Chan
2009-09-30 16:44 ` Kevin Hilman
2009-09-30 23:49 ` Kevin Hilman
2 siblings, 0 replies; 8+ messages in thread
From: Mike Chan @ 2009-09-17 23:41 UTC (permalink / raw)
To: linux-omap; +Cc: khilman
On Thu, Sep 17, 2009 at 4:32 PM, Mike Chan <mike@android.com> wrote:
> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
> ---
> arch/arm/plat-omap/resource.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
> */
> int resource_register(struct shared_resource *resp)
> {
> + int ret = 0;
> if (!resp)
> return -EINVAL;
>
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
> return -EINVAL;
>
> /* Verify that the resource is not already registered */
> - if (resource_lookup(resp->name))
> - return -EEXIST;
> + down(&res_mutex);
> + if (_resource_lookup(resp->name))
> + ret = -EEXIST;
> + goto out;
>
> INIT_LIST_HEAD(&resp->users_list);
>
> - down(&res_mutex);
> /* Add the resource to the resource list */
> list_add(&resp->node, &res_list);
>
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
> if (resp->ops->init)
> resp->ops->init(resp);
>
> - up(&res_mutex);
> pr_debug("resource: registered %s\n", resp->name);
>
> - return 0;
> +out:
> + up(&res_mutex);
> + return ret;
> }
> EXPORT_SYMBOL(resource_register);
>
> --
> 1.5.4.5
>
>
Kevin, this should be based off of your pm branch (2.6.30?)
-- Mike
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-17 23:32 [PATCH] omap: resource: Fix race in register_resource() Mike Chan
2009-09-17 23:41 ` Mike Chan
@ 2009-09-30 16:44 ` Kevin Hilman
2009-09-30 23:49 ` Kevin Hilman
2 siblings, 0 replies; 8+ messages in thread
From: Kevin Hilman @ 2009-09-30 16:44 UTC (permalink / raw)
To: Mike Chan; +Cc: linux-omap
Mike Chan <mike@android.com> writes:
> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
Thanks, this one applies. Pushing to PM branch and pm-2.6.29.
Kevin
> ---
> arch/arm/plat-omap/resource.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
> */
> int resource_register(struct shared_resource *resp)
> {
> + int ret = 0;
> if (!resp)
> return -EINVAL;
>
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
> return -EINVAL;
>
> /* Verify that the resource is not already registered */
> - if (resource_lookup(resp->name))
> - return -EEXIST;
> + down(&res_mutex);
> + if (_resource_lookup(resp->name))
> + ret = -EEXIST;
> + goto out;
>
> INIT_LIST_HEAD(&resp->users_list);
>
> - down(&res_mutex);
> /* Add the resource to the resource list */
> list_add(&resp->node, &res_list);
>
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
> if (resp->ops->init)
> resp->ops->init(resp);
>
> - up(&res_mutex);
> pr_debug("resource: registered %s\n", resp->name);
>
> - return 0;
> +out:
> + up(&res_mutex);
> + return ret;
> }
> EXPORT_SYMBOL(resource_register);
>
> --
> 1.5.4.5
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-17 23:32 [PATCH] omap: resource: Fix race in register_resource() Mike Chan
2009-09-17 23:41 ` Mike Chan
2009-09-30 16:44 ` Kevin Hilman
@ 2009-09-30 23:49 ` Kevin Hilman
2009-09-30 23:58 ` Mike Chan
2 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2009-09-30 23:49 UTC (permalink / raw)
To: Mike Chan; +Cc: linux-omap
Mike Chan <mike@android.com> writes:
> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
> ---
> arch/arm/plat-omap/resource.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
> */
> int resource_register(struct shared_resource *resp)
> {
> + int ret = 0;
> if (!resp)
> return -EINVAL;
>
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
> return -EINVAL;
>
> /* Verify that the resource is not already registered */
> - if (resource_lookup(resp->name))
> - return -EEXIST;
> + down(&res_mutex);
> + if (_resource_lookup(resp->name))
> + ret = -EEXIST;
> + goto out;
Ahem, you're rebased version has a little problem. Missing {} block
mans this goto is always executed and no resources are ever added.
I'll fix this up manually before applying, but some more testing next
time would be helpful
Thanks,
Kevin
> INIT_LIST_HEAD(&resp->users_list);
>
> - down(&res_mutex);
> /* Add the resource to the resource list */
> list_add(&resp->node, &res_list);
>
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
> if (resp->ops->init)
> resp->ops->init(resp);
>
> - up(&res_mutex);
> pr_debug("resource: registered %s\n", resp->name);
>
> - return 0;
> +out:
> + up(&res_mutex);
> + return ret;
> }
> EXPORT_SYMBOL(resource_register);
>
> --
> 1.5.4.5
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-30 23:49 ` Kevin Hilman
@ 2009-09-30 23:58 ` Mike Chan
0 siblings, 0 replies; 8+ messages in thread
From: Mike Chan @ 2009-09-30 23:58 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Wed, Sep 30, 2009 at 4:49 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Mike Chan <mike@android.com> writes:
>
>> Checking if the resource is already registered and adding to the list
>> must be atomic or bad things can happen.
>>
>> Signed-off-by: Mike Chan <mike@android.com>
>> ---
>> arch/arm/plat-omap/resource.c | 13 ++++++++-----
>> 1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>> index 111020a..4631912 100644
>> --- a/arch/arm/plat-omap/resource.c
>> +++ b/arch/arm/plat-omap/resource.c
>> @@ -255,6 +255,7 @@ int resource_refresh(void)
>> */
>> int resource_register(struct shared_resource *resp)
>> {
>> + int ret = 0;
>> if (!resp)
>> return -EINVAL;
>>
>> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
>> return -EINVAL;
>>
>> /* Verify that the resource is not already registered */
>> - if (resource_lookup(resp->name))
>> - return -EEXIST;
>> + down(&res_mutex);
>> + if (_resource_lookup(resp->name))
>> + ret = -EEXIST;
>> + goto out;
>
> Ahem, you're rebased version has a little problem. Missing {} block
> mans this goto is always executed and no resources are ever added.
>
> I'll fix this up manually before applying, but some more testing next
> time would be helpful
>
Opps, sorry about that :/ and thanks for cleaning it up.
-- Mike
> Thanks,
>
> Kevin
>
>
>> INIT_LIST_HEAD(&resp->users_list);
>>
>> - down(&res_mutex);
>> /* Add the resource to the resource list */
>> list_add(&resp->node, &res_list);
>>
>> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>> if (resp->ops->init)
>> resp->ops->init(resp);
>>
>> - up(&res_mutex);
>> pr_debug("resource: registered %s\n", resp->name);
>>
>> - return 0;
>> +out:
>> + up(&res_mutex);
>> + return ret;
>> }
>> EXPORT_SYMBOL(resource_register);
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] omap: resource: Fix race in register_resource()
@ 2009-09-03 19:37 Mike Chan
2009-09-08 22:39 ` Kevin Hilman
0 siblings, 1 reply; 8+ messages in thread
From: Mike Chan @ 2009-09-03 19:37 UTC (permalink / raw)
To: linux-omap; +Cc: khilman, Mike Chan
Checking if the resource is already registered and adding to the list
must be atomic or bad things can happen.
Signed-off-by: Mike Chan <mike@android.com>
---
arch/arm/plat-omap/resource.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
index e9ace13..9d20249 100644
--- a/arch/arm/plat-omap/resource.c
+++ b/arch/arm/plat-omap/resource.c
@@ -253,6 +253,7 @@ int resource_refresh(void)
*/
int resource_register(struct shared_resource *resp)
{
+ int ret = 0;
if (!resp)
return -EINVAL;
@@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
return -EINVAL;
/* Verify that the resource is not already registered */
- if (resource_lookup(resp->name))
- return -EEXIST;
+ down(&res_mutex);
+ if (_resource_lookup(resp->name)) {
+ ret = -EEXIST;
+ goto out;
+ }
INIT_LIST_HEAD(&resp->users_list);
mutex_init(&resp->resource_mutex);
- down(&res_mutex);
/* Add the resource to the resource list */
list_add(&resp->node, &res_list);
@@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
if (resp->ops->init)
resp->ops->init(resp);
- up(&res_mutex);
pr_debug("resource: registered %s\n", resp->name);
- return 0;
+out:
+ up(&res_mutex);
+ return ret;
}
EXPORT_SYMBOL(resource_register);
--
1.5.4.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-03 19:37 Mike Chan
@ 2009-09-08 22:39 ` Kevin Hilman
2009-09-08 23:55 ` Mike Chan
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2009-09-08 22:39 UTC (permalink / raw)
To: Mike Chan; +Cc: linux-omap
Mike Chan <mike@android.com> writes:
> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
Functionally, this looks fine. Some procedural details still to work
out:
Are you the original author of this? or was it Chunqiu or Limei? The
changelog should be prefixed with a From: line of the original author
(handled by git-format-patch) if it was not you. Also add the other's
signoffs if appropriate.
What tree does this apply to? It's not specified, and does not apply
to either current PM branch or pm-2.6.29.
Kevin
> ---
> arch/arm/plat-omap/resource.c | 14 +++++++++-----
> 1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index e9ace13..9d20249 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -253,6 +253,7 @@ int resource_refresh(void)
> */
> int resource_register(struct shared_resource *resp)
> {
> + int ret = 0;
> if (!resp)
> return -EINVAL;
>
> @@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
> return -EINVAL;
>
> /* Verify that the resource is not already registered */
> - if (resource_lookup(resp->name))
> - return -EEXIST;
> + down(&res_mutex);
> + if (_resource_lookup(resp->name)) {
> + ret = -EEXIST;
> + goto out;
> + }
>
> INIT_LIST_HEAD(&resp->users_list);
> mutex_init(&resp->resource_mutex);
>
> - down(&res_mutex);
> /* Add the resource to the resource list */
> list_add(&resp->node, &res_list);
>
> @@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
> if (resp->ops->init)
> resp->ops->init(resp);
>
> - up(&res_mutex);
> pr_debug("resource: registered %s\n", resp->name);
>
> - return 0;
> +out:
> + up(&res_mutex);
> + return ret;
> }
> EXPORT_SYMBOL(resource_register);
>
> --
> 1.5.4.5
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] omap: resource: Fix race in register_resource()
2009-09-08 22:39 ` Kevin Hilman
@ 2009-09-08 23:55 ` Mike Chan
0 siblings, 0 replies; 8+ messages in thread
From: Mike Chan @ 2009-09-08 23:55 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Tue, Sep 8, 2009 at 3:39 PM, Kevin Hilman<khilman@deeprootsystems.com> wrote:
> Mike Chan <mike@android.com> writes:
>
>> Checking if the resource is already registered and adding to the list
>> must be atomic or bad things can happen.
>>
>> Signed-off-by: Mike Chan <mike@android.com>
>
> Functionally, this looks fine. Some procedural details still to work
> out:
>
> Are you the original author of this? or was it Chunqiu or Limei? The
> changelog should be prefixed with a From: line of the original author
> (handled by git-format-patch) if it was not you. Also add the other's
> signoffs if appropriate.
>
I am the original author of this patch. Chunqiu sent two patches out
1) a per-resource level mutex (vs global).
2) Make update_resource_level() thread safe.
I've been meaning to send them out to l-o on behalf of Chunqiu.
> What tree does this apply to? It's not specified, and does not apply
> to either current PM branch or pm-2.6.29.
>
I thought I rebased this off of pm but I may have goofed. I can send a new one.
-- Mike
> Kevin
>
>> ---
>> arch/arm/plat-omap/resource.c | 14 +++++++++-----
>> 1 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>> index e9ace13..9d20249 100644
>> --- a/arch/arm/plat-omap/resource.c
>> +++ b/arch/arm/plat-omap/resource.c
>> @@ -253,6 +253,7 @@ int resource_refresh(void)
>> */
>> int resource_register(struct shared_resource *resp)
>> {
>> + int ret = 0;
>> if (!resp)
>> return -EINVAL;
>>
>> @@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
>> return -EINVAL;
>>
>> /* Verify that the resource is not already registered */
>> - if (resource_lookup(resp->name))
>> - return -EEXIST;
>> + down(&res_mutex);
>> + if (_resource_lookup(resp->name)) {
>> + ret = -EEXIST;
>> + goto out;
>> + }
>>
>> INIT_LIST_HEAD(&resp->users_list);
>> mutex_init(&resp->resource_mutex);
>>
>> - down(&res_mutex);
>> /* Add the resource to the resource list */
>> list_add(&resp->node, &res_list);
>>
>> @@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>> if (resp->ops->init)
>> resp->ops->init(resp);
>>
>> - up(&res_mutex);
>> pr_debug("resource: registered %s\n", resp->name);
>>
>> - return 0;
>> +out:
>> + up(&res_mutex);
>> + return ret;
>> }
>> EXPORT_SYMBOL(resource_register);
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-09-30 23:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-17 23:32 [PATCH] omap: resource: Fix race in register_resource() Mike Chan
2009-09-17 23:41 ` Mike Chan
2009-09-30 16:44 ` Kevin Hilman
2009-09-30 23:49 ` Kevin Hilman
2009-09-30 23:58 ` Mike Chan
-- strict thread matches above, loose matches on Subject: below --
2009-09-03 19:37 Mike Chan
2009-09-08 22:39 ` Kevin Hilman
2009-09-08 23:55 ` Mike Chan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.