Patchwork Warning at kernel/mutex.c

login
register
mail settings
Submitter Fabio Estevam
Date Oct. 18, 2011, 6:33 p.m.
Message ID <CAOMZO5BKTzeV0f0=frQT-4uHESh2-cEQtc2UzniFT6dVSi_0EQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/120484/
State New
Headers show

Comments

Fabio Estevam - Oct. 18, 2011, 6:33 p.m.
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.
Uwe Kleine-König - Oct. 18, 2011, 6:40 p.m.
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);
>
Fabio Estevam - Oct. 18, 2011, 6:45 p.m.
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?
Sascha Hauer - Oct. 19, 2011, 6:46 a.m.
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
Uwe Kleine-König - Oct. 19, 2011, 6:54 a.m.
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

Patch

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);