On Tue, Aug 18, 2009 at 8:04 AM, Kevin Hilman<khilman@xxxxxxxxxxxxxxxxxxx> wrote: > "Wang Limei-E12499" <E12499@xxxxxxxxxxxx> writes: > >> Seems like I did not submit the patch in the right way, before I setup >> my mail correctly, attach the patches in the mail. > > You're patches are still line-wrapped. > > I strongly recommend using git-format-patch and git-send-email to > submit patches. Chunqiu was able to do this. Please consult him. > > Also, no need to CC linux-omap-owner. linux-omap is all that is needed. > This patch has been reviewed and merged into our android-omap-2.6.29 tree http://android.git.kernel.org/?p=kernel/omap.git;a=commit;h=0b6a9b6514c7ccfa0c76e4defdaea3dcbc617633 Kevin if you're having line wrap problems feel free to pull it from here, assuming everyone's feedback has been addressed -- MIke > Thanks, > > Kevin > > >> PATCH1:0001-Add-per-resource-mutex-for-OMAP-resource-framework.patch >> >> From b4e9cc01f9d1aaeec39cc1ee794e5efaec61c781 Mon Sep 17 00:00:00 2001 >> From: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >> Date: Fri, 14 Aug 2009 17:34:32 +0800 >> Subject: [PATCH] Add per-resource mutex for OMAP resource framework >> >> Current OMAP resource fwk uses a global res_mutex >> for resource_request and resource_release calls >> for all the available resources.It may cause dead >> lock if resource_request/resource_release is called >> recursively. >> >> For current OMAP3 VDD1/VDD2 resource, the change_level >> implementation is mach-omap2/resource34xx.c/set_opp(), >> when using resource_release to remove vdd1 constraint, >> this function may call resource_release again to release >> Vdd2 constrait if target vdd1 level is less than OPP3. >> in this scenario, the global res_mutex down operation >> will be called again, this will cause the second >> down operation hang there. >> >> To fix the problem, per-resource mutex is added >> to avoid hangup when resource_request/resource_release >> is called recursively. >> >> Signed-off-by: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >> --- >> arch/arm/plat-omap/include/mach/resource.h | 2 ++ >> arch/arm/plat-omap/resource.c | 27 >> +++++++++++++++------------ >> 2 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/mach/resource.h >> b/arch/arm/plat-omap/include/mach/resource.h >> index f91d8ce..d482fb8 100644 >> --- a/arch/arm/plat-omap/include/mach/resource.h >> +++ b/arch/arm/plat-omap/include/mach/resource.h >> @@ -46,6 +46,8 @@ struct shared_resource { >> /* Shared resource operations */ >> struct shared_resource_ops *ops; >> struct list_head node; >> + /* Protect each resource */ >> + struct mutex resource_mutex; >> }; >> >> struct shared_resource_ops { >> diff --git a/arch/arm/plat-omap/resource.c >> b/arch/arm/plat-omap/resource.c >> index ec31727..5eae4e8 100644 >> --- a/arch/arm/plat-omap/resource.c >> +++ b/arch/arm/plat-omap/resource.c >> @@ -264,6 +264,7 @@ int resource_register(struct shared_resource *resp) >> return -EEXIST; >> >> INIT_LIST_HEAD(&resp->users_list); >> + mutex_init(&resp->resource_mutex); >> >> down(&res_mutex); >> /* Add the resource to the resource list */ >> @@ -326,14 +327,14 @@ int resource_request(const char *name, struct >> device *dev, >> struct users_list *user; >> int found = 0, ret = 0; >> >> - down(&res_mutex); >> - resp = _resource_lookup(name); >> + resp = resource_lookup(name); >> if (!resp) { >> printk(KERN_ERR "resource_request: Invalid resource >> name\n"); >> ret = -EINVAL; >> - goto res_unlock; >> + goto ret; >> } >> >> + mutex_lock(&resp->resource_mutex); >> /* Call the resource specific validate function */ >> if (resp->ops->validate_level) { >> ret = resp->ops->validate_level(resp, level); >> @@ -362,7 +363,7 @@ int resource_request(const char *name, struct device >> *dev, >> user->level = level; >> >> res_unlock: >> - up(&res_mutex); >> + mutex_unlock(&resp->resource_mutex); >> /* >> * Recompute and set the current level for the resource. >> * NOTE: update_resource level moved out of spin_lock, as it may >> call >> @@ -371,6 +372,7 @@ res_unlock: >> */ >> if (!ret) >> ret = update_resource_level(resp); >> +ret: >> return ret; >> } >> EXPORT_SYMBOL(resource_request); >> @@ -393,14 +395,14 @@ int resource_release(const char *name, struct >> device *dev) >> struct users_list *user; >> int found = 0, ret = 0; >> >> - down(&res_mutex); >> - resp = _resource_lookup(name); >> + resp = resource_lookup(name); >> if (!resp) { >> printk(KERN_ERR "resource_release: Invalid resource >> name\n"); >> ret = -EINVAL; >> - goto res_unlock; >> + goto ret; >> } >> >> + mutex_lock(&resp->resource_mutex); >> list_for_each_entry(user, &resp->users_list, node) { >> if (user->dev == dev) { >> found = 1; >> @@ -421,7 +423,9 @@ int resource_release(const char *name, struct device >> *dev) >> /* Recompute and set the current level for the resource */ >> ret = update_resource_level(resp); >> res_unlock: >> - up(&res_mutex); >> + mutex_unlock(&resp->resource_mutex); >> + >> +ret: >> return ret; >> } >> EXPORT_SYMBOL(resource_release); >> @@ -438,15 +442,14 @@ int resource_get_level(const char *name) >> struct shared_resource *resp; >> u32 ret; >> >> - down(&res_mutex); >> - resp = _resource_lookup(name); >> + resp = resource_lookup(name); >> if (!resp) { >> printk(KERN_ERR "resource_release: Invalid resource >> name\n"); >> - up(&res_mutex); >> return -EINVAL; >> } >> + mutex_lock(&resp->resource_mutex); >> ret = resp->curr_level; >> - up(&res_mutex); >> + mutex_unlock(&resp->resource_mutex); >> return ret; >> } >> EXPORT_SYMBOL(resource_get_level); >> -- >> 1.5.4.3 >> >> PATCH2:0002-Move-the-resource-level-update-into-mutex_lock-block.patch >> >> >> From 9cc371b5d7f2e049fe72bc946dcb8ec8e1de826c Mon Sep 17 00:00:00 2001 >> From: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >> Date: Fri, 14 Aug 2009 17:43:13 +0800 >> Subject: [PATCH] Move the resource level update into mutex_lock block. >> >> The update_resource_level is called outside of >> the mutex lock protection block due to an out of date >> spin lock mechanism, now mutex is used, so move >> the update_resource_level into mutex protection block. >> >> Signed-off-by: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >> --- >> arch/arm/plat-omap/resource.c | 11 +++-------- >> 1 files changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/plat-omap/resource.c >> b/arch/arm/plat-omap/resource.c >> index 5eae4e8..e2a003a 100644 >> --- a/arch/arm/plat-omap/resource.c >> +++ b/arch/arm/plat-omap/resource.c >> @@ -362,16 +362,11 @@ int resource_request(const char *name, struct >> device *dev, >> } >> user->level = level; >> >> + /* Recompute and set the current level for the resource */ >> + ret = update_resource_level(resp); >> + >> res_unlock: >> mutex_unlock(&resp->resource_mutex); >> - /* >> - * Recompute and set the current level for the resource. >> - * NOTE: update_resource level moved out of spin_lock, as it may >> call >> - * pm_qos_add_requirement, which does a kzmalloc. This won't be >> allowed >> - * in iterrupt context. The spin_lock still protects add/remove >> users. >> - */ >> - if (!ret) >> - ret = update_resource_level(resp); >> ret: >> return ret; >> } >> -- >> 1.5.4.3 >> >> >> -----Original Message----- >> From: Wang Limei-E12499 >> Sent: Monday, August 17, 2009 11:13 AM >> To: 'khilman@xxxxxxxxxxxxxxxxxxx' >> Cc: Wang Limei-E12499; Wang Sawsd-A24013 >> Subject: RE: Linux-omap PM: Fix dead lock condition in >> resource_release(vdd1_opp) >> >> >> Kevin, >> >> Seems like I did not submit the patch in the recommended way,I will try >> to be better in the future. >> >> If you can review the patch and feedback, I will apperciate it. >> >> Thanks, >> Limei >> >> -----Original Message----- >> From: Wang Limei-E12499 >> Sent: Friday, August 14, 2009 5:44 PM >> To: Kevin Hilman >> Cc: Romit Dasgupta; linux-omap@xxxxxxxxxxxxxxx; Wang Sawsd-A24013; Wang >> Limei-E12499 >> Subject: RE: Linux-omap PM: Fix dead lock condition in >> resource_release(vdd1_opp) >> >> >> Kevin, >> >> Thanks for reviewing the patch. >> >> Chunqiu and I revised the patch. Pls see the attachment. >> >> >> Thanks, >> Limei,chunqiu >> >> -----Original Message----- >> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >> Sent: Thursday, August 13, 2009 8:02 AM >> To: Wang Limei-E12499 >> Cc: Romit Dasgupta; linux-omap@xxxxxxxxxxxxxxx; Wang Sawsd-A24013 >> Subject: Re: Linux-omap PM: Fix dead lock condition in >> resource_release(vdd1_opp) >> >> "Wang Limei-E12499" <E12499@xxxxxxxxxxxx> writes: >> >>> >>> Kevin and Romit, >>> >>> I agreed with you, thanks Kevin and Romit for the comments! Chunqiu >>> Wang coded resource-based mutex, below is the patch. Pls review and >>> let us know your feedback. >>> >>> >>> From 31f87ffb8eb1f854a9adb7fd96011d490f4655fa Mon Sep 17 00:00:00 2001 >>> From: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >>> Date: Wed, 12 Aug 2009 16:22:09 +0800 >>> Subject: [PATCH] Fix resource framework mutex lock issue when >>> resource_get or resource_release are called nestedly. >>> >> >> Could use a shorter summary (subject) and a more detailed changelog. >> >> This patch is doing too many things in a single patch without enough >> explanation. >> >> Not only does it convert the global semaphore to a resource-specific >> semaphore, but it also changing the locking slightly by moving some >> things in/out of lock protection. That should be described in the >> changelog as well. >> >> Even better would be a first patch that simply converts the semaphore to >> a resource-specific *mutex* (not resource-specific semaphore.) IOW, use >> mutex API in <linux/mutex.h>: >> >> struct mutex; >> init_mutex() >> mutex_lock() >> mutex_unlock() >> mutex_is_lockec() >> ... >> >> Then, add a 2nd patch which does any reworking of the critical sections. >> >> Kevin >> >> >>> Signed-off-by: Chunqiu Wang <cqwang@xxxxxxxxxxxx> >>> --- >>> arch/arm/plat-omap/include/mach/resource.h | 2 + >>> arch/arm/plat-omap/resource.c | 38 >>> +++++++++++++-------------- >>> 2 files changed, 20 insertions(+), 20 deletions(-) >>> >>> diff --git a/arch/arm/plat-omap/include/mach/resource.h >>> b/arch/arm/plat-omap/include/mach/resource.h >>> index f91d8ce..389cb67 100644 >>> --- a/arch/arm/plat-omap/include/mach/resource.h >>> +++ b/arch/arm/plat-omap/include/mach/resource.h >>> @@ -22,6 +22,7 @@ >>> >>> #include <linux/list.h> >>> #include <linux/mutex.h> >>> +#include <linux/semaphore.h> >>> #include <linux/device.h> >>> #include <mach/cpu.h> >>> >>> @@ -46,6 +47,7 @@ struct shared_resource { >>> /* Shared resource operations */ >>> struct shared_resource_ops *ops; >>> struct list_head node; >>> + struct semaphore resource_mutex; >>> }; >>> >>> struct shared_resource_ops { >>> diff --git a/arch/arm/plat-omap/resource.c >>> b/arch/arm/plat-omap/resource.c index ec31727..758a138 100644 >>> --- a/arch/arm/plat-omap/resource.c >>> +++ b/arch/arm/plat-omap/resource.c >>> @@ -264,6 +264,7 @@ int resource_register(struct shared_resource >> *resp) >>> return -EEXIST; >>> >>> INIT_LIST_HEAD(&resp->users_list); >>> + sema_init(&resp->resource_mutex, 1); >>> >>> down(&res_mutex); >>> /* Add the resource to the resource list */ @@ -326,14 +327,14 >> @@ >>> int resource_request(const char *name, struct device *dev, >>> struct users_list *user; >>> int found = 0, ret = 0; >>> >>> - down(&res_mutex); >>> - resp = _resource_lookup(name); >>> + resp = resource_lookup(name); >>> if (!resp) { >>> printk(KERN_ERR "resource_request: Invalid resource >> name\n"); >>> ret = -EINVAL; >>> - goto res_unlock; >>> + goto ret; >>> } >>> >>> + down(&resp->resource_mutex); >>> /* Call the resource specific validate function */ >>> if (resp->ops->validate_level) { >>> ret = resp->ops->validate_level(resp, level); @@ -361,16 >> +362,12 @@ >>> int resource_request(const char *name, struct device *dev, >>> } >>> user->level = level; >>> >>> + /* Recompute and set the current level for the resource */ >>> + ret = update_resource_level(resp); >>> res_unlock: >>> - up(&res_mutex); >>> - /* >>> - * Recompute and set the current level for the resource. >>> - * NOTE: update_resource level moved out of spin_lock, as it may >>> call >>> - * pm_qos_add_requirement, which does a kzmalloc. This won't be >>> allowed >>> - * in iterrupt context. The spin_lock still protects add/remove >>> users. >>> - */ >>> - if (!ret) >>> - ret = update_resource_level(resp); >>> + up(&resp->resource_mutex); >>> + >>> +ret: >>> return ret; >>> } >>> EXPORT_SYMBOL(resource_request); >>> @@ -393,14 +390,14 @@ int resource_release(const char *name, struct >>> device *dev) >>> struct users_list *user; >>> int found = 0, ret = 0; >>> >>> - down(&res_mutex); >>> - resp = _resource_lookup(name); >>> + resp = resource_lookup(name); >>> if (!resp) { >>> printk(KERN_ERR "resource_release: Invalid resource >> name\n"); >>> ret = -EINVAL; >>> - goto res_unlock; >>> + goto ret; >>> } >>> >>> + down(&resp->resource_mutex); >>> list_for_each_entry(user, &resp->users_list, node) { >>> if (user->dev == dev) { >>> found = 1; >>> @@ -421,7 +418,9 @@ int resource_release(const char *name, struct >>> device >>> *dev) >>> /* Recompute and set the current level for the resource */ >>> ret = update_resource_level(resp); >>> res_unlock: >>> - up(&res_mutex); >>> + up(&resp->resource_mutex); >>> + >>> +ret: >>> return ret; >>> } >>> EXPORT_SYMBOL(resource_release); >>> @@ -438,15 +437,14 @@ int resource_get_level(const char *name) >>> struct shared_resource *resp; >>> u32 ret; >>> >>> - down(&res_mutex); >>> - resp = _resource_lookup(name); >>> + resp = resource_lookup(name); >>> if (!resp) { >>> printk(KERN_ERR "resource_release: Invalid resource >> name\n"); >>> - up(&res_mutex); >>> return -EINVAL; >>> } >>> + down(&resp->resource_mutex); >>> ret = resp->curr_level; >>> - up(&res_mutex); >>> + up(&resp->resource_mutex); >>> return ret; >>> } >>> EXPORT_SYMBOL(resource_get_level); >>> -- >>> 1.5.4.3 >>> >>> >>> >>> >>> >>> -----Original Message----- >>> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>> Sent: Monday, August 10, 2009 11:23 AM >>> To: Wang Limei-E12499 >>> Cc: linux-omap@xxxxxxxxxxxxxxx >>> Subject: Re: Linux-omap PM: Fix dead lock condition in >>> resource_release(vdd1_opp) >>> >>> "Wang Limei-E12499" <E12499@xxxxxxxxxxxx> writes: >>> >>>> I am using linux-omap pm-2.6.29 >>>> <http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git; >>>> a =s hortlog;h=pm-2.6.29> branch,found a dead lock condition in: >>>> arch/arm/plat-omap/resource.c->resource_release(). >>>> >>>> The dead lock happens when using >>>> resource_request("vdd1_opp",&dev,...) >>>> and resource_release("vdd1_opp", &dev) to set and release vdd1 opp >>>> constraint. The scenario is: >>>> >>>> plat-omap/resource.c/resource_release("vdd1_opp", >>>> &dev)==>resource.c/update_resource_level()=>mach-omap2/resource34xx.c >>>> / se t_opp(). In set_opp(), if the target_level of vdd1 is less >>>> than OPP3,will release the constraint set on VDD2 by calling >>>> resource_release(), but it will never return because can not get the >>>> mutex which is holding by the previous caller. >>>> >>>> int resource_release(const char *name, struct device *dev) >>>> { ....... >>>> down(&res_mutex); >>>> ........ >>>> /* Recompute and set the current level for the resource */ >>>> ret = update_resource_level(resp); >>>> res_unlock: >>>> up(&res_mutex); >>>> return ret; >>>> } >>>> >>>> int set_opp(struct shared_resource *resp, u32 target_level) { >>>> ..... >>>> if (resp == vdd1_resp) { >>>> if (target_level < 3) >>>> resource_release("vdd2_opp", &vdd2_dev); } >>>> >>>> The patch to fix this issue is below, will you pls review it and let >>>> me know your feedback? >>>> >>>> From: Limei Wang <e12499@xxxxxxxxxxxx> >>>> Date: Fri, 7 Aug 2009 11:40:35 -0500 >>>> Subject: [PATCH] OMAP PM: Fix dead lock bug in >>>> resourc_release(vdd1_opp). >>>> >>>> >>>> Signed-off-by: Limei Wang <e12499@xxxxxxxxxxxx> >>>> --- >>>> arch/arm/plat-omap/resource.c | 6 ++++-- >>>> 1 files changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/plat-omap/resource.c >>>> b/arch/arm/plat-omap/resource.c index ec31727..876fd12 100644 >>>> --- a/arch/arm/plat-omap/resource.c >>>> +++ b/arch/arm/plat-omap/resource.c >>>> @@ -418,10 +418,12 @@ int resource_release(const char *name, struct >>>> device *dev) >>>> list_del(&user->node); >>>> free_user(user); >>>> >>>> - /* Recompute and set the current level for the resource */ >>>> - ret = update_resource_level(resp); >>>> res_unlock: >>>> up(&res_mutex); >>>> + >>>> + /* Recompute and set the current level for the resource */ >>>> + if (!ret) >>>> + ret = update_resource_level(resp); >>>> return ret; >>>> } >>>> EXPORT_SYMBOL(resource_release); >>>> -- >>>> 1.5.6.3 >>> >>> This is wrong for several reasons. >>> >>> First, you're not fixing the problem, you're just moving the call >>> outside of the lock, thus creating other locking problems. >>> >>> Second, the various error paths would break because >>> update_resource_level() would be called on the 'res_unlock' error path >> >>> where it is not currently being called. >>> >>> A per-resource mutex as suggest by Romit seems like the right approach >> >>> to fixing this problem. >>> >>> Kevin > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
