* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
@ 2012-09-12 7:14 R Sricharan
2012-09-25 0:43 ` Kevin Hilman
0 siblings, 1 reply; 6+ messages in thread
From: R Sricharan @ 2012-09-12 7:14 UTC (permalink / raw)
To: linux-arm-kernel
From: Colin Cross <ccross@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 <ccross@android.com>
Signed-off-by: R Sricharan <r.sricharan@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
2012-09-12 7:14 [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock R Sricharan
@ 2012-09-25 0:43 ` Kevin Hilman
2012-09-25 1:22 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Hilman @ 2012-09-25 0:43 UTC (permalink / raw)
To: linux-arm-kernel
R Sricharan <r.sricharan@ti.com> writes:
> From: Colin Cross <ccross@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 <ccross@android.com>
> Signed-off-by: R Sricharan <r.sricharan@ti.com>
Acked-by: Kevin Hilman <khilman@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);
> }
>
> /**
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
2012-09-25 0:43 ` Kevin Hilman
@ 2012-09-25 1:22 ` Tony Lindgren
2012-09-25 7:43 ` R, Sricharan
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2012-09-25 1:22 UTC (permalink / raw)
To: linux-arm-kernel
* Kevin Hilman <khilman@deeprootsystems.com> [120924 17:44]:
> R Sricharan <r.sricharan@ti.com> writes:
>
> > From: Colin Cross <ccross@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 <ccross@android.com>
> > Signed-off-by: R Sricharan <r.sricharan@ti.com>
>
> Acked-by: Kevin Hilman <khilman@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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
2012-09-25 1:22 ` Tony Lindgren
@ 2012-09-25 7:43 ` R, Sricharan
2012-09-25 15:17 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: R, Sricharan @ 2012-09-25 7:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
2012-09-25 7:43 ` R, Sricharan
@ 2012-09-25 15:17 ` Tony Lindgren
2012-10-08 21:02 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2012-09-25 15:17 UTC (permalink / raw)
To: linux-arm-kernel
* R, Sricharan <r.sricharan@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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock
2012-09-25 15:17 ` Tony Lindgren
@ 2012-10-08 21:02 ` Tony Lindgren
0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2012-10-08 21:02 UTC (permalink / raw)
To: linux-arm-kernel
* Tony Lindgren <tony@atomide.com> [120925 08:19]:
> * R, Sricharan <r.sricharan@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-08 21:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12 7:14 [PATCH V2] ARM: OMAP: counter: add locking to read_persistent_clock R Sricharan
2012-09-25 0:43 ` Kevin Hilman
2012-09-25 1:22 ` Tony Lindgren
2012-09-25 7:43 ` R, Sricharan
2012-09-25 15:17 ` Tony Lindgren
2012-10-08 21:02 ` Tony Lindgren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).