Skip to contentSkip to conversation

clk: rockchip: fix deadlock possibility in cpuclk

PATCH New Heiko Stübner 3 participants 3 messages Jan 16, 2015 – Jan 17, 2015

Subsystems (2)
Common clk Arm Rockchip
lore
Share & tools
lore

Heiko Stübner

author maintainer
#0 lore raw

PATCH 1/1 clk: rockchip: fix deadlock possibility in cpuclk

Lockdep reported a possible deadlock between the cpuclk lock and for example the i2c driver.

       CPU0                    CPU1
       ----                    ----
  lock(clk_lock);
                               local_irq_disable();
                               lock(&(&i2c->lock)->rlock);
                               lock(clk_lock);
  <Interrupt>
    lock(&(&i2c->lock)->rlock);

*** DEADLOCK ***

The generic clock-types of the core ccf already use spin_lock_irqsave when touching clock registers, so do the same for the cpuclk.

Signed-off-by Heiko Stuebner <[email protected]>
---
 drivers/clk/rockchip/clk-cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/clk/rockchip/clk-cpu.c +6 -4
index 75c8c45..3ecdf7d 100644
@@ -124,10 +124,11 @@ static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
 {
 	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
 	unsigned long alt_prate, alt_div;
+	unsigned long flags = 0;
 
 	alt_prate = clk_get_rate(cpuclk->alt_parent);
 
-	spin_lock(cpuclk->lock);
+	spin_lock_irqsave(cpuclk->lock, flags);
 
 	/*
 	 * If the old parent clock speed is less than the clock speed

@@ -164,7 +165,7 @@ static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
 			cpuclk->reg_base + reg_data->core_reg);
 	}
 
-	spin_unlock(cpuclk->lock);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
 

@@ -173,6 +174,7 @@ static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
 {
 	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
 	const struct rockchip_cpuclk_rate_table *rate;
+	unsigned long flags = 0;
 
 	rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
 	if (!rate) {

@@ -181,7 +183,7 @@ static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
 		return -EINVAL;
 	}
 
-	spin_lock(cpuclk->lock);
+	spin_lock_irqsave(cpuclk->lock, flags);
 
 	if (ndata->old_rate < ndata->new_rate)
 		rockchip_cpuclk_set_dividers(cpuclk, rate);

@@ -201,7 +203,7 @@ static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
 	if (ndata->old_rate > ndata->new_rate)
 		rockchip_cpuclk_set_dividers(cpuclk, rate);
 
-	spin_unlock(cpuclk->lock);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
Signature · 1 line
2.1.1
#1 lore raw

PATCH 1/1 clk: rockchip: fix deadlock possibility in cpuclk

Heiko,

Quoted 27 lines · Heiko St?bner

Lockdep reported a possible deadlock between the cpuclk lock and for example the i2c driver.

       CPU0                    CPU1
       ----                    ----
  lock(clk_lock);
                               local_irq_disable();
                               lock(&(&i2c->lock)->rlock);
                               lock(clk_lock);
  <Interrupt>
    lock(&(&i2c->lock)->rlock);

*** DEADLOCK ***

The generic clock-types of the core ccf already use spin_lock_irqsave when touching clock registers, so do the same for the cpuclk.

Signed-off-by Heiko Stuebner <[email protected]>
---
 drivers/clk/rockchip/clk-cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/clk/rockchip/clk-cpu.c +0 -0
index 75c8c45..3ecdf7d 100644

drivers/clk/rockchip/clk-cpu.c static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk, · line 127

Quoted 4 lines
drivers/clk/rockchip/clk-cpu.c +1 -0
@@ -124,10 +124,11 @@ static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
 {
        const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
        unsigned long alt_prate, alt_div;
+       unsigned long flags = 0;

nit: I don't often see flags initted to 0 here when using spin_lock_irqsave(). I don't think it's needed...

I doubt it really matters though, and this looks fine to me.

Reviewed-by Doug Anderson <[email protected]>

-Doug

#2 lore raw

PATCH 1/1 clk: rockchip: fix deadlock possibility in cpuclk

Quoting Doug Anderson (2015-01-16 09:11:06)

Quoted 38 lines

Heiko,

Quoted 27 lines · Heiko St?bner

Lockdep reported a possible deadlock between the cpuclk lock and for example the i2c driver.

       CPU0                    CPU1
       ----                    ----
  lock(clk_lock);
                               local_irq_disable();
                               lock(&(&i2c->lock)->rlock);
                               lock(clk_lock);
  <Interrupt>
    lock(&(&i2c->lock)->rlock);

*** DEADLOCK ***

The generic clock-types of the core ccf already use spin_lock_irqsave when touching clock registers, so do the same for the cpuclk.

Signed-off-by Heiko Stuebner <[email protected]>
---
 drivers/clk/rockchip/clk-cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/clk/rockchip/clk-cpu.c +0 -0
index 75c8c45..3ecdf7d 100644

drivers/clk/rockchip/clk-cpu.c static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk, · line 127

Quoted 4 lines
drivers/clk/rockchip/clk-cpu.c +1 -0
@@ -124,10 +124,11 @@ static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
 {
        const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
        unsigned long alt_prate, alt_div;
+       unsigned long flags = 0;

nit: I don't often see flags initted to 0 here when using spin_lock_irqsave(). I don't think it's needed...

Quoted 3 lines

I doubt it really matters though, and this looks fine to me.

Reviewed-by Doug Anderson <[email protected]>

Applied to clk-fixes and removed flags initialization locally.

Regards,
Mike

Quoted 2 lines

-Doug