Hi On Wed, 18 Apr 2012, [email protected] wrote:
> From: Jean Pihet <[email protected]> > > Signed-off-by: Jean Pihet <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
