| Message ID | 355359386.nxMIlHNuqO@phil (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Heiko, On Fri, Jan 16, 2015 at 8:52 AM, Heiko Stübner <heiko@sntech.de> wrote: > 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 <heiko@sntech.de> > --- > drivers/clk/rockchip/clk-cpu.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c > index 75c8c45..3ecdf7d 100644 > --- a/drivers/clk/rockchip/clk-cpu.c > +++ b/drivers/clk/rockchip/clk-cpu.c > @@ -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 <dianders@chromium.org> -Doug
Quoting Doug Anderson (2015-01-16 09:11:06) > Heiko, > > On Fri, Jan 16, 2015 at 8:52 AM, Heiko Stübner <heiko@sntech.de> wrote: > > 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 <heiko@sntech.de> > > --- > > drivers/clk/rockchip/clk-cpu.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c > > index 75c8c45..3ecdf7d 100644 > > --- a/drivers/clk/rockchip/clk-cpu.c > > +++ b/drivers/clk/rockchip/clk-cpu.c > > @@ -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 <dianders@chromium.org> Applied to clk-fixes and removed flags initialization locally. Regards, Mike > > -Doug
diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c index 75c8c45..3ecdf7d 100644 --- a/drivers/clk/rockchip/clk-cpu.c +++ b/drivers/clk/rockchip/clk-cpu.c @@ -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; }
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 <heiko@sntech.de> --- drivers/clk/rockchip/clk-cpu.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)