Hi Paul,
Thanks for the review. Do you plan to review the full set before I
start to address the comments.
On Mon, May 7, 2012 at 8:41 AM, Paul Walmsley <paul@xxxxxxxxx> wrote:
> Hi
>
> On Wed, 18 Apr 2012, jean.pihet@xxxxxxxxxxxxxx wrote:
>
>> From: Jean Pihet <j-pihet@xxxxxx>
>>
>> Signed-off-by: Jean Pihet <j-pihet@xxxxxx>
>
> This patch is missing a description, which would describe why this lock is
> needed and what it protects against. Please add this.
Ok
>
>> ---
>> arch/arm/mach-omap2/pm.c | 8 ++++++--
>> arch/arm/mach-omap2/powerdomain.c | 1 +
>> arch/arm/mach-omap2/powerdomain.h | 3 ++-
>> 3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
>> index d0c1c96..6918a13 100644
>> --- a/arch/arm/mach-omap2/pm.c
>> +++ b/arch/arm/mach-omap2/pm.c
>> @@ -100,15 +100,17 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
>> if (!pwrdm || IS_ERR(pwrdm))
>> return -EINVAL;
>>
>> + mutex_lock(&pwrdm->lock);
>> +
>> while (!(pwrdm->pwrsts & (1 << pwrst))) {
>> if (pwrst == PWRDM_POWER_OFF)
>> - return ret;
>> + goto out;
>> pwrst--;
>> }
>>
>> next_pwrst = pwrdm_read_next_pwrst(pwrdm);
>> if (next_pwrst == pwrst)
>> - return ret;
>> + goto out;
>>
>> curr_pwrst = pwrdm_read_pwrst(pwrdm);
>> if (curr_pwrst < PWRDM_POWER_ON) {
>> @@ -141,6 +143,8 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
>> break;
>> }
>>
>> +out:
>> + mutex_unlock(&pwrdm->lock);
>> return ret;
>> }
>>
>
> So this mutex would protect against simultaneous calls to
> omap_set_pwrdm_state(), but shouldn't this protection be extended to
> anything that would change the powerdomain's state? For example, couldn't
> other calls to pwrdm_set_next_pwrst() race against this function?
The intention behind this patch set is to change the API to only use
omap_set_pwrdm_state to change the power domains states. Probably I
should emphasize more on that in the cover letter and commits
description.
>
>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>> index 96ad3dbe..319b277 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -102,6 +102,7 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
>> INIT_LIST_HEAD(&pwrdm->voltdm_node);
>> voltdm_add_pwrdm(voltdm, pwrdm);
>>
>> + mutex_init(&pwrdm->lock);
>> list_add(&pwrdm->node, &pwrdm_list);
>>
>> /* Initialize the powerdomain's state counter */
>> diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
>> index 0d72a8a..6c6567d 100644
>> --- a/arch/arm/mach-omap2/powerdomain.h
>> +++ b/arch/arm/mach-omap2/powerdomain.h
>> @@ -19,7 +19,7 @@
>>
>> #include <linux/types.h>
>> #include <linux/list.h>
>> -
>> +#include <linux/mutex.h>
>> #include <linux/atomic.h>
>>
>> #include <plat/cpu.h>
>> @@ -116,6 +116,7 @@ struct powerdomain {
>> struct clockdomain *pwrdm_clkdms[PWRDM_MAX_CLKDMS];
>> struct list_head node;
>> struct list_head voltdm_node;
>> + struct mutex lock;
>> int state;
>> unsigned state_counter[PWRDM_MAX_PWRSTS];
>> unsigned ret_logic_off_counter;
>
> Please add a kerneldoc entry in the struct powerdomain documentation for
> this field.
Ok
>
>
> - Paul
Thanks,
Jean
--
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
