Re: [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-10-08 Thread Tony Lindgren 
* Tony Lindgren t...@atomide.com [120925 08:19]:
 * R, Sricharan r.sricha...@ti.com [120925 00:44]:
  Hi Tony,
  
  [snip..]
  
index dbf1e03..2bc51fb 100644
--- a/arch/arm/plat-omap/counter_32k.c
+++ b/arch/arm/plat-omap/counter_32k.c
@@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
  * nsecs and adds to a monotonically increasing timespec.
  */
 static struct timespec persistent_ts;
-static cycles_t cycles, last_cycles;
+static cycles_t cycles;
 static unsigned int persistent_mult, persistent_shift;
+static DEFINE_SPINLOCK(read_persistent_clock_lock);
+
 static void omap_read_persistent_clock(struct timespec *ts)
 {
unsigned long long nsecs;
-   cycles_t delta;
-   struct timespec *tsp = persistent_ts;
+   cycles_t last_cycles;
+   unsigned long flags;
+
+   spin_lock_irqsave(read_persistent_clock_lock, flags);
   
last_cycles = cycles;
cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
-   delta = cycles - last_cycles;
   
-   nsecs = clocksource_cyc2ns(delta, persistent_mult, 
persistent_shift);
+   nsecs = clocksource_cyc2ns(cycles - last_cycles,
+   persistent_mult, persistent_shift);
  
   ..I think there's another bug here where cycles - last_cycles
   returns wrong value when the timer wraps around as cycles_t is
   64 bits and the counter is 32 bits. It seems it's been there
   since when the read_persistent_clock was added with commit
   d92cfcbe (OMAP: timekeeping: time should not stop during suspend)?
  
   It seems that after this patch cycles should not be cycles_t
   but u32, and the result of cycles - last_cycles should also
   be u32.
  
   cycles_t is defined as  typedef unsigned long cycles_t;
   Am i missing something here ?
 
 Oh right, cycles_t is unsigned long, it's OK then. Must
 have grepped for it from some other arch.

Applying to fixes with Cc stable.

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 V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-09-25 Thread R, Sricharan 
Hi Tony,

[snip..]

  index dbf1e03..2bc51fb 100644
  --- a/arch/arm/plat-omap/counter_32k.c
  +++ b/arch/arm/plat-omap/counter_32k.c
  @@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
* nsecs and adds to a monotonically increasing timespec.
*/
   static struct timespec persistent_ts;
  -static cycles_t cycles, last_cycles;
  +static cycles_t cycles;
   static unsigned int persistent_mult, persistent_shift;
  +static DEFINE_SPINLOCK(read_persistent_clock_lock);
  +
   static void omap_read_persistent_clock(struct timespec *ts)
   {
  unsigned long long nsecs;
  -   cycles_t delta;
  -   struct timespec *tsp = persistent_ts;
  +   cycles_t last_cycles;
  +   unsigned long flags;
  +
  +   spin_lock_irqsave(read_persistent_clock_lock, flags);
 
  last_cycles = cycles;
  cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
  -   delta = cycles - last_cycles;
 
  -   nsecs = clocksource_cyc2ns(delta, persistent_mult, persistent_shift);
  +   nsecs = clocksource_cyc2ns(cycles - last_cycles,
  +   persistent_mult, persistent_shift);

 ..I think there's another bug here where cycles - last_cycles
 returns wrong value when the timer wraps around as cycles_t is
 64 bits and the counter is 32 bits. It seems it's been there
 since when the read_persistent_clock was added with commit
 d92cfcbe (OMAP: timekeeping: time should not stop during suspend)?

 It seems that after this patch cycles should not be cycles_t
 but u32, and the result of cycles - last_cycles should also
 be u32.

 cycles_t is defined as  typedef unsigned long cycles_t;
 Am i missing something here ?

Thanks,
 Sricharan
--
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 V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-09-25 Thread Tony Lindgren 
* R, Sricharan r.sricha...@ti.com [120925 00:44]:
 Hi Tony,
 
 [snip..]
 
   index dbf1e03..2bc51fb 100644
   --- a/arch/arm/plat-omap/counter_32k.c
   +++ b/arch/arm/plat-omap/counter_32k.c
   @@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
 * nsecs and adds to a monotonically increasing timespec.
 */
static struct timespec persistent_ts;
   -static cycles_t cycles, last_cycles;
   +static cycles_t cycles;
static unsigned int persistent_mult, persistent_shift;
   +static DEFINE_SPINLOCK(read_persistent_clock_lock);
   +
static void omap_read_persistent_clock(struct timespec *ts)
{
   unsigned long long nsecs;
   -   cycles_t delta;
   -   struct timespec *tsp = persistent_ts;
   +   cycles_t last_cycles;
   +   unsigned long flags;
   +
   +   spin_lock_irqsave(read_persistent_clock_lock, flags);
  
   last_cycles = cycles;
   cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
   -   delta = cycles - last_cycles;
  
   -   nsecs = clocksource_cyc2ns(delta, persistent_mult, persistent_shift);
   +   nsecs = clocksource_cyc2ns(cycles - last_cycles,
   +   persistent_mult, persistent_shift);
 
  ..I think there's another bug here where cycles - last_cycles
  returns wrong value when the timer wraps around as cycles_t is
  64 bits and the counter is 32 bits. It seems it's been there
  since when the read_persistent_clock was added with commit
  d92cfcbe (OMAP: timekeeping: time should not stop during suspend)?
 
  It seems that after this patch cycles should not be cycles_t
  but u32, and the result of cycles - last_cycles should also
  be u32.
 
  cycles_t is defined as  typedef unsigned long cycles_t;
  Am i missing something here ?

Oh right, cycles_t is unsigned long, it's OK then. Must
have grepped for it from some other arch.

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 V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-09-24 Thread Kevin Hilman 
R Sricharan r.sricha...@ti.com writes:

 From: Colin Cross ccr...@android.com

 read_persistent_clock uses a global variable, use a spinlock to
 ensure non-atomic updates to the variable don't overlap and cause
 time to move backwards.

 Signed-off-by: Colin Cross ccr...@android.com
 Signed-off-by: R Sricharan r.sricha...@ti.com

Acked-by: Kevin Hilman khil...@ti.com

Tony, this should probably be queued up for v3.7-rc and flagged for stable.

Thanks,

Kevin

 ---
  [V2] Added signed-off-by and looped in linux-arm-kernel list

  arch/arm/plat-omap/counter_32k.c |   21 ++---
  1 file changed, 14 insertions(+), 7 deletions(-)

 diff --git a/arch/arm/plat-omap/counter_32k.c 
 b/arch/arm/plat-omap/counter_32k.c
 index dbf1e03..2bc51fb 100644
 --- a/arch/arm/plat-omap/counter_32k.c
 +++ b/arch/arm/plat-omap/counter_32k.c
 @@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
   * nsecs and adds to a monotonically increasing timespec.
   */
  static struct timespec persistent_ts;
 -static cycles_t cycles, last_cycles;
 +static cycles_t cycles;
  static unsigned int persistent_mult, persistent_shift;
 +static DEFINE_SPINLOCK(read_persistent_clock_lock);
 +
  static void omap_read_persistent_clock(struct timespec *ts)
  {
   unsigned long long nsecs;
 - cycles_t delta;
 - struct timespec *tsp = persistent_ts;
 + cycles_t last_cycles;
 + unsigned long flags;
 +
 + spin_lock_irqsave(read_persistent_clock_lock, flags);
  
   last_cycles = cycles;
   cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
 - delta = cycles - last_cycles;
  
 - nsecs = clocksource_cyc2ns(delta, persistent_mult, persistent_shift);
 + nsecs = clocksource_cyc2ns(cycles - last_cycles,
 + persistent_mult, persistent_shift);
 +
 + timespec_add_ns(persistent_ts, nsecs);
 +
 + *ts = persistent_ts;
  
 - timespec_add_ns(tsp, nsecs);
 - *ts = *tsp;
 + spin_unlock_irqrestore(read_persistent_clock_lock, flags);
  }
  
  /**
--
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 V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-09-24 Thread Tony Lindgren 
* Kevin Hilman khil...@deeprootsystems.com [120924 17:44]:
 R Sricharan r.sricha...@ti.com writes:
 
  From: Colin Cross ccr...@android.com
 
  read_persistent_clock uses a global variable, use a spinlock to
  ensure non-atomic updates to the variable don't overlap and cause
  time to move backwards.
 
  Signed-off-by: Colin Cross ccr...@android.com
  Signed-off-by: R Sricharan r.sricha...@ti.com
 
 Acked-by: Kevin Hilman khil...@ti.com
 
 Tony, this should probably be queued up for v3.7-rc and flagged for stable.

Yes I can see that happening. But then in addition..

  ---
   [V2] Added signed-off-by and looped in linux-arm-kernel list
 
   arch/arm/plat-omap/counter_32k.c |   21 ++---
   1 file changed, 14 insertions(+), 7 deletions(-)
 
  diff --git a/arch/arm/plat-omap/counter_32k.c 
  b/arch/arm/plat-omap/counter_32k.c
  index dbf1e03..2bc51fb 100644
  --- a/arch/arm/plat-omap/counter_32k.c
  +++ b/arch/arm/plat-omap/counter_32k.c
  @@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
* nsecs and adds to a monotonically increasing timespec.
*/
   static struct timespec persistent_ts;
  -static cycles_t cycles, last_cycles;
  +static cycles_t cycles;
   static unsigned int persistent_mult, persistent_shift;
  +static DEFINE_SPINLOCK(read_persistent_clock_lock);
  +
   static void omap_read_persistent_clock(struct timespec *ts)
   {
  unsigned long long nsecs;
  -   cycles_t delta;
  -   struct timespec *tsp = persistent_ts;
  +   cycles_t last_cycles;
  +   unsigned long flags;
  +
  +   spin_lock_irqsave(read_persistent_clock_lock, flags);
   
  last_cycles = cycles;
  cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
  -   delta = cycles - last_cycles;
   
  -   nsecs = clocksource_cyc2ns(delta, persistent_mult, persistent_shift);
  +   nsecs = clocksource_cyc2ns(cycles - last_cycles,
  +   persistent_mult, persistent_shift);

..I think there's another bug here where cycles - last_cycles
returns wrong value when the timer wraps around as cycles_t is
64 bits and the counter is 32 bits. It seems it's been there
since when the read_persistent_clock was added with commit
d92cfcbe (OMAP: timekeeping: time should not stop during suspend)?

It seems that after this patch cycles should not be cycles_t
but u32, and the result of cycles - last_cycles should also
be u32.

  +   timespec_add_ns(persistent_ts, nsecs);
  +
  +   *ts = persistent_ts;
   
  -   timespec_add_ns(tsp, nsecs);
  -   *ts = *tsp;
  +   spin_unlock_irqrestore(read_persistent_clock_lock, flags);
   }

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

[PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock

2012-09-12 Thread R Sricharan 
From: Colin Cross ccr...@android.com

read_persistent_clock uses a global variable, use a spinlock to
ensure non-atomic updates to the variable don't overlap and cause
time to move backwards.

Signed-off-by: Colin Cross ccr...@android.com
Signed-off-by: R Sricharan r.sricha...@ti.com
---
 [V2] Added signed-off-by and looped in linux-arm-kernel list

 arch/arm/plat-omap/counter_32k.c |   21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm/plat-omap/counter_32k.c b/arch/arm/plat-omap/counter_32k.c
index dbf1e03..2bc51fb 100644
--- a/arch/arm/plat-omap/counter_32k.c
+++ b/arch/arm/plat-omap/counter_32k.c
@@ -55,22 +55,29 @@ static u32 notrace omap_32k_read_sched_clock(void)
  * nsecs and adds to a monotonically increasing timespec.
  */
 static struct timespec persistent_ts;
-static cycles_t cycles, last_cycles;
+static cycles_t cycles;
 static unsigned int persistent_mult, persistent_shift;
+static DEFINE_SPINLOCK(read_persistent_clock_lock);
+
 static void omap_read_persistent_clock(struct timespec *ts)
 {
unsigned long long nsecs;
-   cycles_t delta;
-   struct timespec *tsp = persistent_ts;
+   cycles_t last_cycles;
+   unsigned long flags;
+
+   spin_lock_irqsave(read_persistent_clock_lock, flags);
 
last_cycles = cycles;
cycles = sync32k_cnt_reg ? __raw_readl(sync32k_cnt_reg) : 0;
-   delta = cycles - last_cycles;
 
-   nsecs = clocksource_cyc2ns(delta, persistent_mult, persistent_shift);
+   nsecs = clocksource_cyc2ns(cycles - last_cycles,
+   persistent_mult, persistent_shift);
+
+   timespec_add_ns(persistent_ts, nsecs);
+
+   *ts = persistent_ts;
 
-   timespec_add_ns(tsp, nsecs);
-   *ts = *tsp;
+   spin_unlock_irqrestore(read_persistent_clock_lock, flags);
 }
 
 /**
-- 
1.7.9.5

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