Re: [PATCH 4/8] OMAP2+: PM: idle clkdms only if already in idle

[Rajendra Nayak <rnayak@ti.com>]

On 6/10/2011 5:45 AM, Todd Poynor wrote:
>> +	int hwsup = 0;
>> >
>> >    	if (pwrdm == NULL || IS_ERR(pwrdm))
>> >    		return -EINVAL;
>> >  @@ -127,6 +128,7 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
>> >    			(pwrdm->flags&  PWRDM_HAS_LOWPOWERSTATECHANGE)) {
>> >    			sleep_switch = LOWPOWERSTATE_SWITCH;
>> >    		} else {
>> >  +			hwsup = clkdm_is_idle(pwrdm->pwrdm_clkdms[0]);
>> >    			clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
>> >    			pwrdm_wait_transition(pwrdm);
>> >    			sleep_switch = FORCEWAKEUP_SWITCH;
>> >  @@ -142,7 +144,7 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
>> >
>> >    	switch (sleep_switch) {
>> >    	case FORCEWAKEUP_SWITCH:
>> >  -		if (pwrdm->pwrdm_clkdms[0]->flags&  CLKDM_CAN_ENABLE_AUTO)
>> >  +		if (hwsup)
>> >    			clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
>> >    		else
>> >    			clkdm_sleep(pwrdm->pwrdm_clkdms[0]);
> Is concurrency protection needed here?  Not sure if it's expected that
> multiple threads would simultaneously manage the state of the same power
> domain, or that the associated clock domain would change state
> concurrently.

Yeah, maybe there is a need for a per-clkdm lock to prevent these
concurrency issues. This race between omap_set_pwrdm_state and
clk_enable/disable all programming clkdm state was one possibility
for a race (even without this series), but with this series to move the
clkdm handling into hwmod there are certainly more possibilities (across 
concurrent omap_hwmod_enable/idle) because the hwmod framework uses 
per-hwmod lock while the clock framework used a global lock.
Thanks for bringing this up.
Paul/Benoit any thoughts on if a per-clkdm lock seems reasonable?

I just did a quick patch to add per-clkdm locking, it also has a
couple instances of arch-specific clkdm calls making
calls back to generic clkdm framework fixed, which
otherwise would result in recursive locking.

----
From: Rajendra Nayak <rnayak@ti.com>
Date: Fri, 10 Jun 2011 15:42:25 +0530
Subject: [PATCH] OMAP: clockdomain: Add per clkdm lock to prevent 
concurrent state programming

Since the clkdm state programming is now done from
within the hwmod framework (which uses a per-hwmod
lock) instead of the being done from the clock
framework (which used a global lock), there is
now a need to have per-clkdm locking to prevent
races between different hwmods/modules belonging to the
same clock domain concurrently programming the
clkdm state.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
  arch/arm/mach-omap2/clockdomain.c          |   42 
++++++++++++++++++++++++++--
  arch/arm/mach-omap2/clockdomain.h          |    2 +
  arch/arm/mach-omap2/clockdomain2xxx_3xxx.c |    6 ++-
  arch/arm/mach-omap2/clockdomain44xx.c      |    7 ++--
  4 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/clockdomain.c 
b/arch/arm/mach-omap2/clockdomain.c
index b98a972..374e669 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -92,6 +92,8 @@ static int _clkdm_register(struct clockdomain *clkdm)

  	pwrdm_add_clkdm(pwrdm, clkdm);

+	spin_lock_init(&clkdm->lock);
+
  	pr_debug("clockdomain: registered %s\n", clkdm->name);

  	return 0;
@@ -690,6 +692,9 @@ int clkdm_clear_all_sleepdeps(struct clockdomain *clkdm)
   */
  int clkdm_sleep(struct clockdomain *clkdm)
  {
+	int ret;
+	unsigned long flags;
+
  	if (!clkdm)
  		return -EINVAL;

@@ -704,7 +709,10 @@ int clkdm_sleep(struct clockdomain *clkdm)

  	pr_debug("clockdomain: forcing sleep on %s\n", clkdm->name);

-	return arch_clkdm->clkdm_sleep(clkdm);
+	spin_lock_irqsave(&clkdm->lock, flags);
+	ret = arch_clkdm->clkdm_sleep(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);
+	return ret;
  }

  /**
@@ -718,6 +726,9 @@ int clkdm_sleep(struct clockdomain *clkdm)
   */
  int clkdm_wakeup(struct clockdomain *clkdm)
  {
+	int ret;
+	unsigned long flags;
+
  	if (!clkdm)
  		return -EINVAL;

@@ -732,7 +743,10 @@ int clkdm_wakeup(struct clockdomain *clkdm)

  	pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name);

-	return arch_clkdm->clkdm_wakeup(clkdm);
+	spin_lock_irqsave(&clkdm->lock, flags);
+	ret = arch_clkdm->clkdm_wakeup(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);
+	return ret;
  }

  /**
@@ -747,6 +761,8 @@ int clkdm_wakeup(struct clockdomain *clkdm)
   */
  void clkdm_allow_idle(struct clockdomain *clkdm)
  {
+	unsigned long flags;
+
  	if (!clkdm)
  		return;

@@ -762,8 +778,10 @@ void clkdm_allow_idle(struct clockdomain *clkdm)
  	pr_debug("clockdomain: enabling automatic idle transitions for %s\n",
  		 clkdm->name);

+	spin_lock_irqsave(&clkdm->lock, flags);
  	arch_clkdm->clkdm_allow_idle(clkdm);
  	pwrdm_clkdm_state_switch(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);
  }

  /**
@@ -777,6 +795,8 @@ void clkdm_allow_idle(struct clockdomain *clkdm)
   */
  void clkdm_deny_idle(struct clockdomain *clkdm)
  {
+	unsigned long flags;
+
  	if (!clkdm)
  		return;

@@ -792,7 +812,9 @@ void clkdm_deny_idle(struct clockdomain *clkdm)
  	pr_debug("clockdomain: disabling automatic idle transitions for %s\n",
  		 clkdm->name);

+	spin_lock_irqsave(&clkdm->lock, flags);
  	arch_clkdm->clkdm_deny_idle(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);
  }

  /**
@@ -805,6 +827,9 @@ void clkdm_deny_idle(struct clockdomain *clkdm)
   */
  int clkdm_is_idle(struct clockdomain *clkdm)
  {
+	int ret;
+	unsigned long flags;
+
  	if (!clkdm)
  		return -EINVAL;

@@ -813,7 +838,10 @@ int clkdm_is_idle(struct clockdomain *clkdm)

  	pr_debug("clockdomain: reading idle state for %s\n", clkdm->name);

-	return arch_clkdm->clkdm_is_idle(clkdm);
+	spin_lock_irqsave(&clkdm->lock, flags);
+	ret = arch_clkdm->clkdm_is_idle(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);
+	return ret;
  }


@@ -835,6 +863,8 @@ int clkdm_is_idle(struct clockdomain *clkdm)
   */
  int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
  {
+	unsigned long flags;
+
  	/*
  	 * XXX Rewrite this code to maintain a list of enabled
  	 * downstream clocks for debugging purposes?
@@ -859,9 +889,11 @@ int clkdm_clk_enable(struct clockdomain *clkdm, 
struct clk *clk)
  	pr_debug("clockdomain: clkdm %s: clk %s now enabled\n", clkdm->name,
  		 clk->name);

+	spin_lock_irqsave(&clkdm->lock, flags);
  	arch_clkdm->clkdm_clk_enable(clkdm);
  	pwrdm_wait_transition(clkdm->pwrdm.ptr);
  	pwrdm_clkdm_state_switch(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);

  	return 0;
  }
@@ -882,6 +914,8 @@ int clkdm_clk_enable(struct clockdomain *clkdm, 
struct clk *clk)
   */
  int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk)
  {
+	unsigned long flags;
+
  	/*
  	 * XXX Rewrite this code to maintain a list of enabled
  	 * downstream clocks for debugging purposes?
@@ -908,8 +942,10 @@ int clkdm_clk_disable(struct clockdomain *clkdm, 
struct clk *clk)
  	pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
  		 clk->name);

+	spin_lock_irqsave(&clkdm->lock, flags);
  	arch_clkdm->clkdm_clk_disable(clkdm);
  	pwrdm_clkdm_state_switch(clkdm);
+	spin_unlock_irqrestore(&clkdm->lock, flags);

  	return 0;
  }
diff --git a/arch/arm/mach-omap2/clockdomain.h 
b/arch/arm/mach-omap2/clockdomain.h
index 085ed82..9fd4eb5 100644
--- a/arch/arm/mach-omap2/clockdomain.h
+++ b/arch/arm/mach-omap2/clockdomain.h
@@ -17,6 +17,7 @@
  #define __ARCH_ARM_MACH_OMAP2_CLOCKDOMAIN_H

  #include <linux/init.h>
+#include <linux/spinlock.h>

  #include "powerdomain.h"
  #include <plat/clock.h>
@@ -122,6 +123,7 @@ struct clockdomain {
  	const struct omap_chip_id omap_chip;
  	atomic_t usecount;
  	struct list_head node;
+	spinlock_t lock;
  };

  /**
diff --git a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c 
b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
index e0c393f..f841ac8 100644
--- a/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
+++ b/arch/arm/mach-omap2/clockdomain2xxx_3xxx.c
@@ -193,7 +193,8 @@ static int omap2_clkdm_clk_enable(struct clockdomain 
*clkdm)
  		_clkdm_add_autodeps(clkdm);
  		_enable_hwsup(clkdm);
  	} else {
-		clkdm_wakeup(clkdm);
+		if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
+			omap2_clkdm_wakeup(clkdm);
  	}

  	return 0;
@@ -215,7 +216,8 @@ static int omap2_clkdm_clk_disable(struct 
clockdomain *clkdm)
  		_clkdm_del_autodeps(clkdm);
  		_enable_hwsup(clkdm);
  	} else {
-		clkdm_sleep(clkdm);
+		if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP)
+			omap2_clkdm_sleep(clkdm);
  	}

  	return 0;
diff --git a/arch/arm/mach-omap2/clockdomain44xx.c 
b/arch/arm/mach-omap2/clockdomain44xx.c
index e4b8e06..911770b 100644
--- a/arch/arm/mach-omap2/clockdomain44xx.c
+++ b/arch/arm/mach-omap2/clockdomain44xx.c
@@ -101,7 +101,8 @@ static int omap4_clkdm_is_idle(struct clockdomain 
*clkdm)

  static int omap4_clkdm_clk_enable(struct clockdomain *clkdm)
  {
-	clkdm_wakeup(clkdm);
+	if (clkdm->flags & CLKDM_CAN_FORCE_WAKEUP)
+		return omap4_clkdm_wakeup(clkdm);
  	return 0;
  }

@@ -112,8 +113,8 @@ static int omap4_clkdm_clk_disable(struct 
clockdomain *clkdm)
  	hwsup = omap4_cminst_is_clkdm_in_hwsup(clkdm->prcm_partition,
  					clkdm->cm_inst, clkdm->clkdm_offs);

-	if (!hwsup)
-		clkdm_sleep(clkdm);
+	if (!hwsup && (clkdm->flags & CLKDM_CAN_FORCE_SLEEP))
+		omap4_clkdm_sleep(clkdm);

  	return 0;
  }
-- 
1.7.0.4