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

2011-12-12 Thread Tony Lindgren 
* Ramirez Luna, Omar omar.rami...@ti.com [111209 13:38]:
 On Fri, Dec 9, 2011 at 3:34 PM, Tony Lindgren t...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-12-12 Thread Ramirez Luna, Omar 
On Mon, Dec 12, 2011 at 5:08 PM, Tony Lindgren t...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-12-09 Thread Tony Lindgren 
* Omar Ramirez Luna omar.rami...@ti.com [24 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-12-09 Thread Ramirez Luna, Omar 
On Fri, Dec 9, 2011 at 3:34 PM, Tony Lindgren t...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2011-11-24 Thread 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.rami...@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

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html