Skip to contentSkip to conversation

ARM: OMAP: dmtimer: scheduling while atomic fixes

PATCH New Omar Ramirez Luna 2 participants 7 messages Nov 25, 2011 – Dec 13, 2011

Subsystems (2)
Arm (32-bit) Arm sub-architectures
lore
Share & tools
lore

Omar Ramirez Luna

author
#0 lore raw

cover ARM: OMAP: dmtimer: scheduling while atomic fixes

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(-)
Signature · 1 line
1.7.5.4
author
#1 lore raw

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

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 <[email protected]>
---
 arch/arm/plat-omap/dmtimer.c |   69 +++++++++++++++++++++++++----------------
 1 files changed, 42 insertions(+), 27 deletions(-)
arch/arm/plat-omap/dmtimer.c +42 -27
index af3b92b..2acd4de 100644
@@ -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);
Signature · 1 line
1.7.5.4
#2 lore raw

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

Quoted 36 lines · 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.

arch/arm/plat-omap/dmtimer.c +9 -8
@@ -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;

arch/arm/plat-omap/dmtimer.c struct omap_dm_timer *omap_dm_timer_request(void) · line 183

Quoted 10 lines
arch/arm/plat-omap/dmtimer.c +6 -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?

Quoted 16 lines
arch/arm/plat-omap/dmtimer.c +5 -6
@@ -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);

arch/arm/plat-omap/dmtimer.c struct omap_dm_timer *omap_dm_timer_request_specific(int id) · line 213

Quoted 10 lines
arch/arm/plat-omap/dmtimer.c +6 -2
+
 	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

author
#3 lore raw

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

Quoted 1 line · Tony Lindgren
Quoted 1 line

+ ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);

...

Quoted 4 lines
Quoted 1 line

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

Quoted 1 line
Quoted 1 line

+ ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);

...

Quoted 1 line

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

#4 lore raw

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

Quoted 24 lines · Ramirez Luna, Omar
Quoted 1 line · Tony Lindgren
Quoted 1 line

+ ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);

...

Quoted 4 lines
Quoted 1 line

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

Quoted 1 line
Quoted 1 line

+ ? ? ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);

...

Quoted 1 line

And here too?

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

arch/arm/plat-omap/dmtimer.c +0 -1
@@ -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);
...
Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

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

author
#5 lore raw

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

On Mon, Dec 12, 2011 at 5:08 PM, Tony Lindgren <[email protected]> wrote:
...

Quoted 8 lines
Quoted 8 lines
(quoted diff) +0 -1
@@ -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);
...
Quoted 8 lines
Quoted 5 lines

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

Quoted 6 lines
Quoted 1 line

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.

Quoted 10 lines
Quoted 2 lines

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.

Quoted 1 line

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

author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm/plat-omap/dmtimer.c              |   83 ++++++++---------------------
 arch/arm/plat-omap/include/plat/dmtimer.h |    6 ++-
 2 files changed, 26 insertions(+), 63 deletions(-)
arch/arm/plat-omap/dmtimer.c +22 -61
index 2acd4de..aa7e6da 100644
@@ -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);
arch/arm/plat-omap/include/plat/dmtimer.h +4 -2
index 9418f00..b8b563d 100644
@@ -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)
Signature · 1 line
1.7.5.4