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.
> ---
> 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?
> 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.
- Paul
--
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
