Warning at kernel/mutex.c

Warning at kernel/mutex.c

[Fabio Estevam]

Hi,

I am running 3.1-rc10 kernel built with mxs_defconfig on a MX28EVK
board and I am getting the following:

[    0.200000] Switching to clocksource mxs_timer
[    0.220000] Switched to NOHz mode on CPU #0
[    0.220000] ------------[ cut here ]------------
[    0.220000] WARNING: at kernel/mutex.c:198 mutex_lock_nested+0x260/0x29c()
[    0.220000] Modules linked in:
[    0.220000] [<c0014478>] (unwind_backtrace+0x0/0xf4) from
[<c002630c>] (warn_slowpath_common+0x4c/0x64)
[    0.220000] [<c002630c>] (warn_slowpath_common+0x4c/0x64) from
[<c0026340>] (warn_slowpath_null+0x1c/0x24)
[    0.220000] [<c0026340>] (warn_slowpath_null+0x1c/0x24) from
[<c02957cc>] (mutex_lock_nested+0x260/0x29c)
[    0.220000] [<c02957cc>] (mutex_lock_nested+0x260/0x29c) from
[<c0018580>] (clk_enable+0x2c/0x4c)
[    0.220000] [<c0018580>] (clk_enable+0x2c/0x4c) from [<c01cd9e0>]
(pl011_console_write+0x20/0x78)
[    0.220000] [<c01cd9e0>] (pl011_console_write+0x20/0x78) from
[<c002650c>] (__call_console_drivers+0x7c/0x94)
[    0.220000] [<c002650c>] (__call_console_drivers+0x7c/0x94) from
[<c0026b98>] (console_unlock+0xf8/0x230)
[    0.220000] [<c0026b98>] (console_unlock+0xf8/0x230) from
[<c002724c>] (vprintk+0x2a8/0x42c)
[    0.220000] [<c002724c>] (vprintk+0x2a8/0x42c) from [<c02937e0>]
(printk+0x20/0x30)
[    0.220000] [<c02937e0>] (printk+0x20/0x30) from [<c0049a80>]
(hrtimer_run_pending+0x78/0xd8)
[    0.220000] [<c0049a80>] (hrtimer_run_pending+0x78/0xd8) from
[<c00344c4>] (run_timer_softirq+0x18/0x3bc)
[    0.220000] [<c00344c4>] (run_timer_softirq+0x18/0x3bc) from
[<c002cb00>] (__do_softirq+0xa0/0x1e0)
[    0.220000] [<c002cb00>] (__do_softirq+0xa0/0x1e0) from
[<c002cdd8>] (irq_exit+0x8c/0xa8)
[    0.220000] [<c002cdd8>] (irq_exit+0x8c/0xa8) from [<c000ff60>]
(handle_IRQ+0x34/0x84)
[    0.220000] [<c000ff60>] (handle_IRQ+0x34/0x84) from [<c000ecd8>]
(__irq_svc+0x38/0x60)
[    0.220000] [<c000ecd8>] (__irq_svc+0x38/0x60) from [<c00c10c4>]
(T.844+0x2c8/0x404)
[    0.220000] [<c00c10c4>] (T.844+0x2c8/0x404) from [<c00c1364>]
(kmem_cache_alloc+0x164/0x174)
[    0.220000] [<c00c1364>] (kmem_cache_alloc+0x164/0x174) from
[<c00dbbc8>] (alloc_inode+0x54/0x9c)
[    0.220000] [<c00dbbc8>] (alloc_inode+0x54/0x9c) from [<c00dbc18>]
(new_inode_pseudo+0x8/0x40)
[    0.220000] [<c00dbc18>] (new_inode_pseudo+0x8/0x40) from
[<c00dbc58>] (new_inode+0x8/0x1c)
[    0.220000] [<c00dbc58>] (new_inode+0x8/0x1c) from [<c017a940>]
(T.386+0x38/0x160)
[    0.220000] [<c017a940>] (T.386+0x38/0x160) from [<c017ab30>]
(debugfs_create_file+0xc8/0x1b4)
[    0.220000] [<c017ab30>] (debugfs_create_file+0xc8/0x1b4) from
[<c0075bf8>] (trace_create_file+0x18/0x40)
[    0.220000] [<c0075bf8>] (trace_create_file+0x18/0x40) from
[<c007fc6c>] (__trace_add_event_call+0xc4/0x3bc)
[    0.220000] [<c007fc6c>] (__trace_add_event_call+0xc4/0x3bc) from
[<c0392cc4>] (event_trace_init+0x1c4/0x2c8)
[    0.220000] [<c0392cc4>] (event_trace_init+0x1c4/0x2c8) from
[<c0008704>] (do_one_initcall+0x30/0x17c)
[    0.220000] [<c0008704>] (do_one_initcall+0x30/0x17c) from
[<c038b21c>] (kernel_init+0x7c/0x120)
[    0.220000] [<c038b21c>] (kernel_init+0x7c/0x120) from [<c0010000>]
(kernel_thread_exit+0x0/0x8)

Is this a known issue?

Regards,

Fabio Estevam

Warning at kernel/mutex.c

[Russell King - ARM Linux]

On Tue, Oct 18, 2011 at 02:43:27PM -0200, Fabio Estevam wrote:
> Hi,
> 
> I am running 3.1-rc10 kernel built with mxs_defconfig on a MX28EVK
> board and I am getting the following:
> 
> [    0.200000] Switching to clocksource mxs_timer
> [    0.220000] Switched to NOHz mode on CPU #0
> [    0.220000] ------------[ cut here ]------------
> [    0.220000] WARNING: at kernel/mutex.c:198 mutex_lock_nested+0x260/0x29c()
> [    0.220000] Modules linked in:
> [    0.220000] [<c0014478>] (unwind_backtrace+0x0/0xf4) from
> [<c002630c>] (warn_slowpath_common+0x4c/0x64)
> [    0.220000] [<c002630c>] (warn_slowpath_common+0x4c/0x64) from
> [<c0026340>] (warn_slowpath_null+0x1c/0x24)
> [    0.220000] [<c0026340>] (warn_slowpath_null+0x1c/0x24) from
> [<c02957cc>] (mutex_lock_nested+0x260/0x29c)
> [    0.220000] [<c02957cc>] (mutex_lock_nested+0x260/0x29c) from
> [<c0018580>] (clk_enable+0x2c/0x4c)
> [    0.220000] [<c0018580>] (clk_enable+0x2c/0x4c) from [<c01cd9e0>]
> (pl011_console_write+0x20/0x78)

clk_enable() shouldn't be taking a mutex because drivers can _and_ do
call it from non-schedulable contexts.  Unfortunately, some clk_enable
implementations do use a mutex.

We have a transition path for this, discussed quite a while ago -
introducing clk_prepare() to do the slow bits of enabling a clock,
leaving clk_enable() for the fast stuff.

Warning at kernel/mutex.c

[Uwe Kleine-König]

On Tue, Oct 18, 2011 at 06:37:44PM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 18, 2011 at 02:43:27PM -0200, Fabio Estevam wrote:
> > Hi,
> > 
> > I am running 3.1-rc10 kernel built with mxs_defconfig on a MX28EVK
> > board and I am getting the following:
> > 
> > [    0.200000] Switching to clocksource mxs_timer
> > [    0.220000] Switched to NOHz mode on CPU #0
> > [    0.220000] ------------[ cut here ]------------
> > [    0.220000] WARNING: at kernel/mutex.c:198 mutex_lock_nested+0x260/0x29c()
> > [    0.220000] Modules linked in:
> > [    0.220000] [<c0014478>] (unwind_backtrace+0x0/0xf4) from
> > [<c002630c>] (warn_slowpath_common+0x4c/0x64)
> > [    0.220000] [<c002630c>] (warn_slowpath_common+0x4c/0x64) from
> > [<c0026340>] (warn_slowpath_null+0x1c/0x24)
> > [    0.220000] [<c0026340>] (warn_slowpath_null+0x1c/0x24) from
> > [<c02957cc>] (mutex_lock_nested+0x260/0x29c)
> > [    0.220000] [<c02957cc>] (mutex_lock_nested+0x260/0x29c) from
> > [<c0018580>] (clk_enable+0x2c/0x4c)
> > [    0.220000] [<c0018580>] (clk_enable+0x2c/0x4c) from [<c01cd9e0>]
> > (pl011_console_write+0x20/0x78)
> 
> clk_enable() shouldn't be taking a mutex because drivers can _and_ do
> call it from non-schedulable contexts.  Unfortunately, some clk_enable
> implementations do use a mutex.
> 
> We have a transition path for this, discussed quite a while ago -
> introducing clk_prepare() to do the slow bits of enabling a clock,
> leaving clk_enable() for the fast stuff.
> 
And some time ago I posted a work-around with the obvious downside of
having the clock on longer than necessary. You can find it at

	http://mid.gmane.org/1293117682-18505-1-git-send-email-u.kleine-koenig at pengutronix.de

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Warning at kernel/mutex.c

[Fabio Estevam]

On Tue, Oct 18, 2011 at 3:37 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> clk_enable() shouldn't be taking a mutex because drivers can _and_ do
> call it from non-schedulable contexts. ?Unfortunately, some clk_enable
> implementations do use a mutex.
>
> We have a transition path for this, discussed quite a while ago -
> introducing clk_prepare() to do the slow bits of enabling a clock,
> leaving clk_enable() for the fast stuff.
>

Thanks for the explanation, Russell.

Sascha,

Would the conversion from mutex to spinlock in clk_enable/disable work
in the meantime?

Please see below.

OMAP/PXA/Tegra also use spinlock in clk_enable/disable.

If this is OK then I can submit a proper patch for mxc and mxs.

diff --git a/arch/arm/mach-mxs/clock.c b/arch/arm/mach-mxs/clock.c
index a7093c8..1cdc21a 100644
--- a/arch/arm/mach-mxs/clock.c
+++ b/arch/arm/mach-mxs/clock.c
@@ -42,6 +42,7 @@

 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
+static DEFINE_SPINLOCK(clock_lock);

 /*-------------------------------------------------------------------------
  * Standard clock functions defined in include/linux/clk.h
@@ -80,13 +81,14 @@ static int __clk_enable(struct clk *clk)
 int clk_enable(struct clk *clk)
 {
 	int ret = 0;
+	unsigned long flags;

 	if (clk == NULL || IS_ERR(clk))
 		return -EINVAL;

-	mutex_lock(&clocks_mutex);
+	spin_lock_irqsave(&clock_lock, flags);
 	ret = __clk_enable(clk);
-	mutex_unlock(&clocks_mutex);
+	spin_unlock_irqrestore(&clock_lock, flags);

 	return ret;
 }
@@ -98,12 +100,14 @@ EXPORT_SYMBOL(clk_enable);
  */
 void clk_disable(struct clk *clk)
 {
+	unsigned long flags;
+
 	if (clk == NULL || IS_ERR(clk))
 		return;

-	mutex_lock(&clocks_mutex);
+	spin_lock_irqsave(&clock_lock, flags);
 	__clk_disable(clk);
-	mutex_unlock(&clocks_mutex);
+	spin_unlock_irqrestore(&clock_lock, flags);
 }
 EXPORT_SYMBOL(clk_disable);

Warning at kernel/mutex.c

[Uwe Kleine-König]

Hello,

On Tue, Oct 18, 2011 at 04:33:07PM -0200, Fabio Estevam wrote:
> On Tue, Oct 18, 2011 at 3:37 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> > clk_enable() shouldn't be taking a mutex because drivers can _and_ do
> > call it from non-schedulable contexts. ?Unfortunately, some clk_enable
> > implementations do use a mutex.
> >
> > We have a transition path for this, discussed quite a while ago -
> > introducing clk_prepare() to do the slow bits of enabling a clock,
> > leaving clk_enable() for the fast stuff.
> >
> 
> Thanks for the explanation, Russell.
> 
> Sascha,
> 
> Would the conversion from mutex to spinlock in clk_enable/disable work
> in the meantime?
> 
> Please see below.
> 
> OMAP/PXA/Tegra also use spinlock in clk_enable/disable.
> 
> If this is OK then I can submit a proper patch for mxc and mxs.
> 
> diff --git a/arch/arm/mach-mxs/clock.c b/arch/arm/mach-mxs/clock.c
> index a7093c8..1cdc21a 100644
> --- a/arch/arm/mach-mxs/clock.c
> +++ b/arch/arm/mach-mxs/clock.c
> @@ -42,6 +42,7 @@
> 
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
> +static DEFINE_SPINLOCK(clock_lock);
If clocks_mutex is unused now, please remove it. If it's not unused you
probably introduced a problem with your patch.

Uwe
> 
>  /*-------------------------------------------------------------------------
>   * Standard clock functions defined in include/linux/clk.h
> @@ -80,13 +81,14 @@ static int __clk_enable(struct clk *clk)
>  int clk_enable(struct clk *clk)
>  {
>  	int ret = 0;
> +	unsigned long flags;
> 
>  	if (clk == NULL || IS_ERR(clk))
>  		return -EINVAL;
> 
> -	mutex_lock(&clocks_mutex);
> +	spin_lock_irqsave(&clock_lock, flags);
>  	ret = __clk_enable(clk);
> -	mutex_unlock(&clocks_mutex);
> +	spin_unlock_irqrestore(&clock_lock, flags);
> 
>  	return ret;
>  }
> @@ -98,12 +100,14 @@ EXPORT_SYMBOL(clk_enable);
>   */
>  void clk_disable(struct clk *clk)
>  {
> +	unsigned long flags;
> +
>  	if (clk == NULL || IS_ERR(clk))
>  		return;
> 
> -	mutex_lock(&clocks_mutex);
> +	spin_lock_irqsave(&clock_lock, flags);
>  	__clk_disable(clk);
> -	mutex_unlock(&clocks_mutex);
> +	spin_unlock_irqrestore(&clock_lock, flags);
>  }
>  EXPORT_SYMBOL(clk_disable);
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Warning at kernel/mutex.c

[Fabio Estevam]

2011/10/18 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:

>> ?static LIST_HEAD(clocks);
>> ?static DEFINE_MUTEX(clocks_mutex);
>> +static DEFINE_SPINLOCK(clock_lock);
> If clocks_mutex is unused now, please remove it. If it's not unused you
> probably introduced a problem with your patch.

clocks_mutex is still used in clk_set_rate and clk_set_parent.

Should it be converted to spinlocks too?

Warning at kernel/mutex.c

[Sascha Hauer]

On Tue, Oct 18, 2011 at 04:45:51PM -0200, Fabio Estevam wrote:
> 2011/10/18 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> 
> >> ?static LIST_HEAD(clocks);
> >> ?static DEFINE_MUTEX(clocks_mutex);
> >> +static DEFINE_SPINLOCK(clock_lock);
> > If clocks_mutex is unused now, please remove it. If it's not unused you
> > probably introduced a problem with your patch.
> 
> clocks_mutex is still used in clk_set_rate and clk_set_parent.
> 
> Should it be converted to spinlocks too?
>

The mutex currently used protects two things: The clock tree and the
clock registers. If we use a mutex for the parent/rate functions and
a spinlock for enable/disable we must make sure that both function sets
do not access the same registers. I *think* this is the case on all
i.MX, but I haven't really checked this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Warning at kernel/mutex.c

[Uwe Kleine-König]

On Wed, Oct 19, 2011 at 08:46:03AM +0200, Sascha Hauer wrote:
> On Tue, Oct 18, 2011 at 04:45:51PM -0200, Fabio Estevam wrote:
> > 2011/10/18 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > 
> > >> ?static LIST_HEAD(clocks);
> > >> ?static DEFINE_MUTEX(clocks_mutex);
> > >> +static DEFINE_SPINLOCK(clock_lock);
> > > If clocks_mutex is unused now, please remove it. If it's not unused you
> > > probably introduced a problem with your patch.
> > 
> > clocks_mutex is still used in clk_set_rate and clk_set_parent.
> > 
> > Should it be converted to spinlocks too?
> >
> 
> The mutex currently used protects two things: The clock tree and the
> clock registers. If we use a mutex for the parent/rate functions and
> a spinlock for enable/disable we must make sure that both function sets
> do not access the same registers.
... and that no strage things happen when there we're in the middle of a
parent/rate change and a clock is enabled or disabled.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |