Re: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)

[Kevin Hilman <khilman@deeprootsystems.com>]

Mike Chan <mike@android.com> writes:

> On Tue, Aug 18, 2009 at 8:04 AM, Kevin
> Hilman<khilman@deeprootsystems.com> wrote:
>> "Wang Limei-E12499" <E12499@motorola.com> 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

Hmm, I don't see any other review/signoff that the authors on that
link.

> Kevin if you're having line wrap problems feel free to pull it from
> here, assuming everyone's feedback has been addressed

It's not me who has the line-wrap problem. I could unwrap pretty
easily myself, but it gets very old working around various email
client problems, so I choose to reject patches until they can be sent
in a usable form. 

I'm still waiting for this so it can get a full review on-list.

Thanks,

Kevin

>
>> 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@motorola.com>
>>> 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@motorola.com>
>>> ---
>>>  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@motorola.com>
>>> 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@motorola.com>
>>> ---
>>>  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@deeprootsystems.com'
>>> 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@vger.kernel.org; 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@deeprootsystems.com]
>>> Sent: Thursday, August 13, 2009 8:02 AM
>>> To: Wang Limei-E12499
>>> Cc: Romit Dasgupta; linux-omap@vger.kernel.org; Wang Sawsd-A24013
>>> Subject: Re: Linux-omap PM: Fix dead lock condition in
>>> resource_release(vdd1_opp)
>>>
>>> "Wang Limei-E12499" <E12499@motorola.com> 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@motorola.com>
>>>> 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@motorola.com>
>>>> ---
>>>>  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@deeprootsystems.com]
>>>> Sent: Monday, August 10, 2009 11:23 AM
>>>> To: Wang Limei-E12499
>>>> Cc: linux-omap@vger.kernel.org
>>>> Subject: Re: Linux-omap PM: Fix dead lock condition in
>>>> resource_release(vdd1_opp)
>>>>
>>>> "Wang Limei-E12499" <E12499@motorola.com> 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@motorola.com>
>>>>> 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@motorola.com>
>>>>> ---
>>>>>  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@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