[PATCH 0/2] ARM: OMAP: dmtimer: scheduling while atomic fixes

[PATCH 0/2] ARM: OMAP: dmtimer: scheduling while atomic fixes

[Omar Ramirez Luna]

Changes in dmtimer framework have introduced scheduling-while-atomic
and other lock state BUGs, these are related to the usage of clk_get
(internal path holds a mutex_lock) while holding a spin_lock_irqsave.

The other inconsistent lock state BUGs are caused when calling
omap_dm_timer_request* on softirq or hardirq, because code handling source
parent clocks is still using clk_get, since there is only one user of those
APIs that acquires a lock in a softirq context (tidspbridge) for now it
can be changed.

Omar Ramirez Luna (2):
  ARM: OMAP: dmtimer: fix sleeping function called from invalid context
  ARM: OMAP: dmtimer: reorganize omap_dm_timer_request_*

 arch/arm/plat-omap/dmtimer.c              |  104 +++++++++++------------------
 arch/arm/plat-omap/include/plat/dmtimer.h |    6 +-
 2 files changed, 44 insertions(+), 66 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context

[Omar Ramirez Luna]

omap_dm_timer_request* holds a spin_lock_irqsave while inner routines call
clk_get_sys which holds a mutex_lock, given that mutex can be put to sleep
a BUG message is triggered. This occurs in 2 ocassions.

1. When the fck is gotten at the beginning of omap_dm_timer_prepare by using
   clk_get (which will call clk_get_sys), this was fixed by getting the clock
   handles on probe.

2. When omap_dm_timer_set_source tries to get the clock handles (with clk_get)
   for the fck and source clock, this was moved to be made after
   spin_unlock_irqsave when the context is not atomic anymore.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/plat-omap/dmtimer.c |   69 +++++++++++++++++++++++++----------------
 1 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index af3b92b..2acd4de 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct omap_dm_timer *timer)
 int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 {
 	struct dmtimer_platform_data *pdata = timer->pdev->dev.platform_data;
-	int ret;
-
-	timer->fclk = clk_get(&timer->pdev->dev, "fck");
-	if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
-		timer->fclk = NULL;
-		dev_err(&timer->pdev->dev, ": No fclk handle.\n");
-		return -EINVAL;
-	}
 
 	if (pdata->needs_manual_reset)
 		omap_dm_timer_reset(timer);
 
-	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
-
 	timer->posted = 1;
-	return ret;
+
+	return 0;
 }
 
 struct omap_dm_timer *omap_dm_timer_request(void)
@@ -168,19 +159,26 @@ struct omap_dm_timer *omap_dm_timer_request(void)
 		break;
 	}
 
-	if (timer) {
-		ret = omap_dm_timer_prepare(timer);
-		if (ret) {
-			timer->reserved = 0;
-			timer = NULL;
-		}
+	if (!timer) {
+		spin_unlock_irqrestore(&dm_timer_lock, flags);
+		goto err_no_timer;
 	}
+
+	omap_dm_timer_prepare(timer);
+
 	spin_unlock_irqrestore(&dm_timer_lock, flags);
 
-	if (!timer)
-		pr_debug("%s: timer request failed!\n", __func__);
+	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
+	if (ret) {
+		timer->reserved = 0;
+		goto err_no_timer;
+	}
 
 	return timer;
+
+err_no_timer:
+	pr_debug("%s: timer request failed!\n", __func__);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_request);
 
@@ -199,19 +197,26 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
 		}
 	}
 
-	if (timer) {
-		ret = omap_dm_timer_prepare(timer);
-		if (ret) {
-			timer->reserved = 0;
-			timer = NULL;
-		}
+	if (!timer) {
+		spin_unlock_irqrestore(&dm_timer_lock, flags);
+		goto err_no_timer;
 	}
+
+	omap_dm_timer_prepare(timer);
+
 	spin_unlock_irqrestore(&dm_timer_lock, flags);
 
-	if (!timer)
-		pr_debug("%s: timer%d request failed!\n", __func__, id);
+	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
+	if (ret) {
+		timer->reserved = 0;
+		goto err_no_timer;
+	}
 
 	return timer;
+
+err_no_timer:
+	pr_debug("%s: timer%d request failed!\n", __func__, id);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific);
 
@@ -658,6 +663,14 @@ static int __devinit omap_dm_timer_probe(struct platform_device *pdev)
 		goto err_free_mem;
 	}
 
+	timer->fclk = clk_get(&pdev->dev, "fck");
+	if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
+		timer->fclk = NULL;
+		dev_err(&pdev->dev, ": No fclk handle for id %d.\n", pdev->id);
+		ret = -EINVAL;
+		goto err_clk_handle;
+	}
+
 	timer->id = pdev->id;
 	timer->irq = irq->start;
 	timer->reserved = pdata->reserved;
@@ -686,6 +699,8 @@ static int __devinit omap_dm_timer_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_clk_handle:
+	iounmap(timer->io_base);
 err_free_mem:
 	kfree(timer);
 
-- 
1.7.5.4

[PATCH 2/2] ARM: OMAP: dmtimer: reorganize omap_dm_timer_request_*

[Omar Ramirez Luna]

omap_dm_timer_request and omap_dm_timer_request_specific have almost
the same code except for a conditional check and the parameters they
receive.

omap_dm_timer_request_specific can be sent a -1 parameter to contain
the behavior of omap_dm_timer_request and thus get rid of some lines
of code.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/plat-omap/dmtimer.c              |   83 ++++++++---------------------
 arch/arm/plat-omap/include/plat/dmtimer.h |    6 ++-
 2 files changed, 26 insertions(+), 63 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 2acd4de..aa7e6da 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -131,88 +131,49 @@ static void omap_dm_timer_reset(struct omap_dm_timer *timer)
 	timer->posted = 1;
 }
 
-int omap_dm_timer_prepare(struct omap_dm_timer *timer)
-{
-	struct dmtimer_platform_data *pdata = timer->pdev->dev.platform_data;
-
-	if (pdata->needs_manual_reset)
-		omap_dm_timer_reset(timer);
-
-	timer->posted = 1;
-
-	return 0;
-}
-
-struct omap_dm_timer *omap_dm_timer_request(void)
+struct omap_dm_timer *omap_dm_timer_request_specific(int id)
 {
-	struct omap_dm_timer *timer = NULL, *t;
+	struct omap_dm_timer *t = NULL;
+	struct dmtimer_platform_data *pdata;
 	unsigned long flags;
 	int ret = 0;
 
 	spin_lock_irqsave(&dm_timer_lock, flags);
-	list_for_each_entry(t, &omap_timer_list, node) {
-		if (t->reserved)
-			continue;
-
-		timer = t;
-		timer->reserved = 1;
-		break;
-	}
 
-	if (!timer) {
-		spin_unlock_irqrestore(&dm_timer_lock, flags);
-		goto err_no_timer;
-	}
-
-	omap_dm_timer_prepare(timer);
-
-	spin_unlock_irqrestore(&dm_timer_lock, flags);
-
-	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
-	if (ret) {
-		timer->reserved = 0;
-		goto err_no_timer;
-	}
-
-	return timer;
-
-err_no_timer:
-	pr_debug("%s: timer request failed!\n", __func__);
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(omap_dm_timer_request);
-
-struct omap_dm_timer *omap_dm_timer_request_specific(int id)
-{
-	struct omap_dm_timer *timer = NULL, *t;
-	unsigned long flags;
-	int ret = 0;
+	if (id == -1) {
+		list_for_each_entry(t, &omap_timer_list, node) {
+			if (!t->reserved)
+				break;
+		}
 
-	spin_lock_irqsave(&dm_timer_lock, flags);
-	list_for_each_entry(t, &omap_timer_list, node) {
-		if (t->pdev->id == id && !t->reserved) {
-			timer = t;
-			timer->reserved = 1;
-			break;
+	} else {
+		list_for_each_entry(t, &omap_timer_list, node) {
+			if (t->pdev->id == id && !t->reserved)
+				break;
 		}
 	}
 
-	if (!timer) {
+	if (!t) {
 		spin_unlock_irqrestore(&dm_timer_lock, flags);
 		goto err_no_timer;
 	}
 
-	omap_dm_timer_prepare(timer);
+	t->reserved = 1;
+	t->posted = 1;
+
+	pdata = t->pdev->dev.platform_data;
+	if (pdata->needs_manual_reset)
+		omap_dm_timer_reset(t);
 
 	spin_unlock_irqrestore(&dm_timer_lock, flags);
 
-	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
+	ret = omap_dm_timer_set_source(t, OMAP_TIMER_SRC_32_KHZ);
 	if (ret) {
-		timer->reserved = 0;
+		t->reserved = 0;
 		goto err_no_timer;
 	}
 
-	return timer;
+	return t;
 
 err_no_timer:
 	pr_debug("%s: timer%d request failed!\n", __func__, id);
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index 9418f00..b8b563d 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -107,7 +107,6 @@ struct dmtimer_platform_data {
 	int (*get_context_loss_count)(struct device *dev);
 };
 
-struct omap_dm_timer *omap_dm_timer_request(void);
 struct omap_dm_timer *omap_dm_timer_request_specific(int timer_id);
 int omap_dm_timer_free(struct omap_dm_timer *timer);
 void omap_dm_timer_enable(struct omap_dm_timer *timer);
@@ -282,7 +281,10 @@ struct omap_dm_timer {
 	int (*get_context_loss_count)(struct device *dev);
 };
 
-int omap_dm_timer_prepare(struct omap_dm_timer *timer);
+static inline struct omap_dm_timer *omap_dm_timer_request(void)
+{
+	return omap_dm_timer_request_specific(-1);
+}
 
 static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
 						int posted)
-- 
1.7.5.4

[PATCH 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context

[Tony Lindgren]

* Omar Ramirez Luna <omar.ramirez@ti.com> [111124 19:37]:
> omap_dm_timer_request* holds a spin_lock_irqsave while inner routines call
> clk_get_sys which holds a mutex_lock, given that mutex can be put to sleep
> a BUG message is triggered. This occurs in 2 ocassions.
> 
> 1. When the fck is gotten at the beginning of omap_dm_timer_prepare by using
>    clk_get (which will call clk_get_sys), this was fixed by getting the clock
>    handles on probe.
> 
> 2. When omap_dm_timer_set_source tries to get the clock handles (with clk_get)
>    for the fck and source clock, this was moved to be made after
>    spin_unlock_irqsave when the context is not atomic anymore.
> 
> @@ -168,19 +159,26 @@ struct omap_dm_timer *omap_dm_timer_request(void)
>  		break;
>  	}
>  
> -	if (timer) {
> -		ret = omap_dm_timer_prepare(timer);
> -		if (ret) {
> -			timer->reserved = 0;
> -			timer = NULL;
> -		}
> +	if (!timer) {
> +		spin_unlock_irqrestore(&dm_timer_lock, flags);
> +		goto err_no_timer;
>  	}
> +
> +	omap_dm_timer_prepare(timer);
> +
>  	spin_unlock_irqrestore(&dm_timer_lock, flags);
>  
> -	if (!timer)
> -		pr_debug("%s: timer request failed!\n", __func__);
> +	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> +	if (ret) {
> +		timer->reserved = 0;
> +		goto err_no_timer;
> +	}
>  
>  	return timer;
> +
> +err_no_timer:
> +	pr_debug("%s: timer request failed!\n", __func__);
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(omap_dm_timer_request);

This does not seem right.. It seems that you're hardcoding the source
clock to 32 KiHz clock while other sources are available too?
  
> @@ -199,19 +197,26 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
>  		}
>  	}
>  
> -	if (timer) {
> -		ret = omap_dm_timer_prepare(timer);
> -		if (ret) {
> -			timer->reserved = 0;
> -			timer = NULL;
> -		}
> +	if (!timer) {
> +		spin_unlock_irqrestore(&dm_timer_lock, flags);
> +		goto err_no_timer;
>  	}
> +
> +	omap_dm_timer_prepare(timer);
> +
>  	spin_unlock_irqrestore(&dm_timer_lock, flags);
>  
> -	if (!timer)
> -		pr_debug("%s: timer%d request failed!\n", __func__, id);
> +	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> +	if (ret) {
> +		timer->reserved = 0;
> +		goto err_no_timer;
> +	}

And here too?

Regards,

Tony

[PATCH 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context

[Ramirez Luna, Omar]

On Fri, Dec 9, 2011 at 3:34 PM, Tony Lindgren <tony@atomide.com> wrote:
>> + ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...
>> ?EXPORT_SYMBOL_GPL(omap_dm_timer_request);
>
> This does not seem right.. It seems that you're hardcoding the source
> clock to 32 KiHz clock while other sources are available too?

Agree, but... (below)

>> + ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...
> And here too?

Agree but that is the default behavior set by dm timer framework:

@@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct
omap_dm_timer *timer)
 int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 {
...
-       ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...

All clocks requested are set to 32 KHz first, even with the current
approach, there exists another API to set a new source.

To be honest I don't know why the clocks are set to 32 KHz first,
maybe the default call path for users should be:

omap_dm_timer_request
omap_dm_timer_set_source (new explicit call)
omap_dm_timer_start

And remove setting the source to 32 KHz by default in omap_dm_timer_request.

Regards,

Omar

[PATCH 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context

[Tony Lindgren]

* Ramirez Luna, Omar <omar.ramirez@ti.com> [111209 13:38]:
> On Fri, Dec 9, 2011 at 3:34 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> + ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> ...
> >> ?EXPORT_SYMBOL_GPL(omap_dm_timer_request);
> >
> > This does not seem right.. It seems that you're hardcoding the source
> > clock to 32 KiHz clock while other sources are available too?
> 
> Agree, but... (below)
> 
> >> + ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> ...
> > And here too?
> 
> Agree but that is the default behavior set by dm timer framework:
> 
> @@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct
> omap_dm_timer *timer)
>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>  {
> ...
> -       ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> ...
> 
> All clocks requested are set to 32 KHz first, even with the current
> approach, there exists another API to set a new source.
> 
> To be honest I don't know why the clocks are set to 32 KHz first,
> maybe the default call path for users should be:

You need a functional clock for the timer registers to work
I believe.

> omap_dm_timer_request

Yes this would make sense. The omap_timer_list should be protected
by a mutex. There should not be a need for spinlock there as
omap_dm_timer_request should be only called during init. If that's
not the case, the the driver using it is broken.

> omap_dm_timer_set_source (new explicit call)
> omap_dm_timer_start

Once the timer has been requested, there should not be a need
for locking as there should be only one timer user using the
timer specific registers.
 
> And remove setting the source to 32 KHz by default in omap_dm_timer_request.

That you may need to be able to do anything with the timer :)

Regards,

Tony

[PATCH 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context

[Ramirez Luna, Omar]

On Mon, Dec 12, 2011 at 5:08 PM, Tony Lindgren <tony@atomide.com> wrote:
...
>> @@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct
>> omap_dm_timer *timer)
>> ?int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>> ?{
>> ...
>> - ? ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
>> ...
>>
>> All clocks requested are set to 32 KHz first, even with the current
>> approach, there exists another API to set a new source.
>>
>> To be honest I don't know why the clocks are set to 32 KHz first,
>> maybe the default call path for users should be:
>
> You need a functional clock for the timer registers to work
> I believe.

Sounds logic :)

>> omap_dm_timer_request
>
> Yes this would make sense. The omap_timer_list should be protected
> by a mutex. There should not be a need for spinlock there as
> omap_dm_timer_request should be only called during init. If that's
> not the case, the the driver using it is broken.

Ok, I made this patch thinking that 'request' could be called from any
context, but if that is not the case mutex should be fine.

>> omap_dm_timer_set_source (new explicit call)
>> omap_dm_timer_start
>
> Once the timer has been requested, there should not be a need
> for locking as there should be only one timer user using the
> timer specific registers.
>
>> And remove setting the source to 32 KHz by default in omap_dm_timer_request.
>
> That you may need to be able to do anything with the timer :)

Well the intention was for the user to call it explicitly so it didn't
look as a hard coded setting, but I can keep it.

IIUC, mutex should be held instead of spin lock, I can do the change
instead of this patch and see how it goes.

Thanks,

Omar