[PATCH] OMAP: clock: fix race in disable all clocks

clk_disable_unused is invoked when CONFIG_OMAP_RESET_CLOCKS=y.
Since clk_disable_unused is called as lateinitcall, there can
be more than a few workqueues executing off secondary CPU(s).
The current code does the following:
a) checks if clk is unused
b) holds lock
c) disables clk
d) unlocks

Between (a) and (b) being executed on CPU0, It is possible to
have a driver executing on CPU1 which could do a get_sync->clk_get
(and increase the use_count) of the clock which was just about
to be disabled by clk_disable_unused.

We ensure instead that the entire list traversal is protected by
the lock allowing for parent child clock traversal which could be
potentially be done by runtime operations to be safe as well.

Reported-by: Todd Poynor <redacted>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
To confirm this in reality happens, I setup a trap as follows:
  @@ -450,6 +450,8 @@ static int __init clk_disable_unused(void)
  			continue;
  
  		spin_lock_irqsave(&clockfw_lock, flags);
 +		WARN(ck->usecount, "XXXX->RACECONDITION FOR CLOCK %s\n",
 +			ck->name);
  		arch_clock->clk_disable_unused(ck);
  		spin_unlock_irqrestore(&clockfw_lock, flags);
  	}
Resultant log: http://pastebin.pandaboard.org/index.php/view/66400792

 arch/arm/plat-omap/clock.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index 567e4b5..8030cc1 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c

@@ -442,6 +442,8 @@ static int __init clk_disable_unused(void)
 		return 0;
 
 	pr_info("clock: disabling unused clocks to save power\n");
+
+	spin_lock_irqsave(&clockfw_lock, flags);
 	list_for_each_entry(ck, &clocks, node) {
 		if (ck->ops == &clkops_null)
 			continue;

@@ -449,10 +451,9 @@ static int __init clk_disable_unused(void)
 		if (ck->usecount > 0 || !ck->enable_reg)
 			continue;
 
-		spin_lock_irqsave(&clockfw_lock, flags);
 		arch_clock->clk_disable_unused(ck);
-		spin_unlock_irqrestore(&clockfw_lock, flags);
 	}
+	spin_unlock_irqrestore(&clockfw_lock, flags);
 
 	return 0;
 }

-- 
1.7.0.4

Hi

adding Todd to cc

On Thu, 15 Mar 2012, Nishanth Menon wrote:

clk_disable_unused is invoked when CONFIG_OMAP_RESET_CLOCKS=y.
Since clk_disable_unused is called as lateinitcall, there can
be more than a few workqueues executing off secondary CPU(s).
The current code does the following:
a) checks if clk is unused
b) holds lock
c) disables clk
d) unlocks

Between (a) and (b) being executed on CPU0, It is possible to
have a driver executing on CPU1 which could do a get_sync->clk_get
(and increase the use_count) of the clock which was just about
to be disabled by clk_disable_unused.

We ensure instead that the entire list traversal is protected by
the lock allowing for parent child clock traversal which could be
potentially be done by runtime operations to be safe as well.

Reported-by: Todd Poynor <redacted>
Signed-off-by: Nishanth Menon <nm@ti.com>

Thanks, I'll queue this for v3.4-rc1.

In the medium term, we should get rid of CONFIG_OMAP_RESET_CLOCKS, at 
least for the OMAP2+ chips, and just do this by default.  But this will 
need to wait until all of the OMAP2+ devices and drivers have been 
converted to use hwmod.  Patches to that end are also gladly accepted :-)
In 3.5 we should be in pretty good shape for OMAP4, but OMAP2/3 will still 
need some work.



- Paul

cc Todd

On Thu, 15 Mar 2012, Nishanth Menon wrote:

clk_disable_unused is invoked when CONFIG_OMAP_RESET_CLOCKS=y.
Since clk_disable_unused is called as lateinitcall, there can
be more than a few workqueues executing off secondary CPU(s).
The current code does the following:
a) checks if clk is unused
b) holds lock
c) disables clk
d) unlocks

Between (a) and (b) being executed on CPU0, It is possible to
have a driver executing on CPU1 which could do a get_sync->clk_get
(and increase the use_count) of the clock which was just about
to be disabled by clk_disable_unused.

We ensure instead that the entire list traversal is protected by
the lock allowing for parent child clock traversal which could be
potentially be done by runtime operations to be safe as well.

Reported-by: Todd Poynor <redacted>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
To confirm this in reality happens, I setup a trap as follows:
  @@ -450,6 +450,8 @@ static int __init clk_disable_unused(void)
  			continue;
  
  		spin_lock_irqsave(&clockfw_lock, flags);
 +		WARN(ck->usecount, "XXXX->RACECONDITION FOR CLOCK %s\n",
 +			ck->name);
  		arch_clock->clk_disable_unused(ck);
  		spin_unlock_irqrestore(&clockfw_lock, flags);
  	}
Resultant log: http://pastebin.pandaboard.org/index.php/view/66400792

Thanks Nishanth & Todd, queued for 3.4-rc.


- Paul