ARM: OMAP: counter: add locking to read_persistent_clock

PATCH Superseded R Sricharan 3 participants 6 messages Sep 12, 2012 – Oct 8, 2012

Subsystems (2)

Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

R Sricharan

author

2012-09-12 07:14 UTC #0 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

From: Colin Cross <[email protected]>

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 <[email protected]>

Signed-off-by R Sricharan <[email protected]>

---
[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(-)

arch/arm/plat-omap/counter_32k.c +14 -7

index dbf1e03..2bc51fb 100644
@@ -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);
 }
 
 /**

Signature · 1 line

1.7.9.5

Kevin Hilman

2012-09-25 00:43 UTC #1 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

Quoted 8 lines · R Sricharan

From: Colin Cross <[email protected]>
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 <[email protected]>
Signed-off-by R Sricharan <[email protected]>

Acked-by Kevin Hilman <[email protected]>

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

Thanks,

Kevin

Quoted 47 lines

---
[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(-)

arch/arm/plat-omap/counter_32k.c +14 -7

index dbf1e03..2bc51fb 100644
@@ -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);
 }
 
 /**

Tony Lindgren

2012-09-25 01:22 UTC #2 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

Quoted 8 lines · Kevin Hilman

Quoted 6 lines · R Sricharan
From: Colin Cross <[email protected]>
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.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Colin Cross <[email protected]>
Signed-off-by R Sricharan <[email protected]>
Acked-by Kevin Hilman <[email protected]>
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..

Quoted 29 lines

---
[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(-)

arch/arm/plat-omap/counter_32k.c +7 -3

index dbf1e03..2bc51fb 100644
@@ -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);

Quoted 7 lines

arch/arm/plat-omap/counter_32k.c static u32 notrace omap_32k_read_sched_clock(void) · line 74
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c +2 -2
 	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.

Quoted 8 lines

arch/arm/plat-omap/counter_32k.c static u32 notrace omap_32k_read_sched_clock(void) · line 81
Quoted 8 lines
arch/arm/plat-omap/counter_32k.c +4 -2
+	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

R, Sricharan

author

2012-09-25 07:43 UTC #3 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

Hi Tony,

[snip..]

Quoted 36 lines

Quoted 22 lines
Quoted 22 lines
index dbf1e03..2bc51fb 100644
arch/arm/plat-omap/counter_32k.c +7 -3
@@ -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);
 
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c static u32 notrace omap_32k_read_sched_clock(void) · line 74
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c +2 -2
    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)?

Quoted 4 lines

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

Tony Lindgren

2012-09-25 15:17 UTC #4 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

Quoted 40 lines · R, Sricharan

Hi Tony,
[snip..]
Quoted 36 lines
Quoted 22 lines
Quoted 22 lines
index dbf1e03..2bc51fb 100644
arch/arm/plat-omap/counter_32k.c +7 -3
@@ -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);
 
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c static u32 notrace omap_32k_read_sched_clock(void) · line 74
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c +2 -2
    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)?

Quoted 6 lines

Quoted 4 lines
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

Tony Lindgren

2012-10-08 21:02 UTC #5 lore raw

PATCH v2 1/1 ARM: OMAP: counter: add locking to read_persistent_clock

Quoted 45 lines · Tony Lindgren

Quoted 44 lines · R, Sricharan
Hi Tony,
[snip..]
Quoted 40 lines
Quoted 22 lines
Quoted 22 lines
index dbf1e03..2bc51fb 100644
arch/arm/plat-omap/counter_32k.c +7 -3
@@ -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);
 
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c static u32 notrace omap_32k_read_sched_clock(void) · line 74
Quoted 7 lines
arch/arm/plat-omap/counter_32k.c +2 -2
    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.

Quoted 5 lines

Quoted 2 lines
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