Re: [RFC][patch 02/12] remove clocksource inline functions

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Wed, 29 Jul 2009 08:57:13 -0600
dwalker@fifo99.com wrote:

> On Wed, 2009-07-29 at 16:44 +0200, Martin Schwidefsky wrote:
> > On Wed, 29 Jul 2009 08:15:19 -0600
> > dwalker@fifo99.com wrote:
> > 
> > > > Remove clocksource_read, clocksource_enable and clocksource_disable
> > > > inline functions. No functional change.
> > > > 
> > > 
> > > Your still not really explaining this one, is this suppose to be
> > > cleaner? Or is this related to some other part of your clean up?
> > 
> > The only one of the three inline functions that is a bit more
> > complicated is clocksource_enable() because of the mult_orig logic. But
> > that goes away with a later patch. The function aren't accessors either,
> > they are used exclusively by the timekeeping code. In short, they are
> > useless, don't you think?
> 
> Above is what should go in your patch description .. 

Ok, sounds reasonable.
 
> The reason that I'm not totally into this one is cause these inlines
> help to document to the code..
> 
> If you have ,
> 
> struct clocksource cs;
> 
> then several lines later you have
> 
> cs->read(); 
> 
> vs,
> 
> clocksource_read(cs);
> 
> The later is completely clear, and the former isn't.. Instead of "cs"
> you could pick any obscure name, and read() isn't exactly unique.. So
> really any function in the clocksource structure has the potential for a
> helper, and the inlines don't really cost anything ..

Hmm, you have an object of type struct clocksource and you do
cs->read(cs). If that is not clear enough then I don't know what is. We
do that all over the place in the linux kernel. And I personally find
these useless wrappers rather annoying. I don't like to have to jump to
another place to find out that it just calls the read function of the
object.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Will Newton]

On Wed, Jul 29, 2009 at 4:32 PM, Martin
Schwidefsky<schwidefsky@de.ibm.com> wrote:

> Hmm, you have an object of type struct clocksource and you do
> cs->read(cs). If that is not clear enough then I don't know what is. We
> do that all over the place in the linux kernel. And I personally find
> these useless wrappers rather annoying. I don't like to have to jump to
> another place to find out that it just calls the read function of the
> object.

An argument for the helper is that it eases grepability. Sure you can
search for "->read" but that's going to turn up all kinds of
non-clocksource code as well. grep clocksource_read will get you
exactly what you want.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Wed, 29 Jul 2009 16:36:46 +0100
Will Newton <will.newton@gmail.com> wrote:

> On Wed, Jul 29, 2009 at 4:32 PM, Martin
> Schwidefsky<schwidefsky@de.ibm.com> wrote:
> 
> > Hmm, you have an object of type struct clocksource and you do
> > cs->read(cs). If that is not clear enough then I don't know what is. We
> > do that all over the place in the linux kernel. And I personally find
> > these useless wrappers rather annoying. I don't like to have to jump to
> > another place to find out that it just calls the read function of the
> > object.
> 
> An argument for the helper is that it eases grepability. Sure you can
> search for "->read" but that's going to turn up all kinds of
> non-clocksource code as well. grep clocksource_read will get you
> exactly what you want.

That would make sense if the clocksource_read calls are littered all
over the kernel source. But they are not, the only user is
timekeeping.c

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Wed, 29 Jul 2009 18:27:54 +0200
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> On Wed, 29 Jul 2009 16:36:46 +0100
> Will Newton <will.newton@gmail.com> wrote:
> 
> > On Wed, Jul 29, 2009 at 4:32 PM, Martin
> > Schwidefsky<schwidefsky@de.ibm.com> wrote:
> > 
> > > Hmm, you have an object of type struct clocksource and you do
> > > cs->read(cs). If that is not clear enough then I don't know what is. We
> > > do that all over the place in the linux kernel. And I personally find
> > > these useless wrappers rather annoying. I don't like to have to jump to
> > > another place to find out that it just calls the read function of the
> > > object.
> > 
> > An argument for the helper is that it eases grepability. Sure you can
> > search for "->read" but that's going to turn up all kinds of
> > non-clocksource code as well. grep clocksource_read will get you
> > exactly what you want.
> 
> That would make sense if the clocksource_read calls are littered all
> over the kernel source. But they are not, the only user is
> timekeeping.c
 
And What I find odd as well is e.g. this:

offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask

A mixture of a wrapper to get something out of the clock and a direct
access to other fields of the clocksource. Either it is wrapped or it
isn't, no? A proper wrapper would do all of the above and not just the
call over the read function pointer.


-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On Wed, 29 Jul 2009 18:27:54 +0200, Martin Schwidefsky said:
> On Wed, 29 Jul 2009 16:36:46 +0100 Will Newton <will.newton@gmail.com> wrote:

> > An argument for the helper is that it eases grepability. Sure you can
> > search for "->read" but that's going to turn up all kinds of
> > non-clocksource code as well. grep clocksource_read will get you
> > exactly what you want.
> 
> That would make sense if the clocksource_read calls are littered all
> over the kernel source. But they are not, the only user is
> timekeeping.c

You know that a priori because you're familiar with that code. But there's
another use case: An idiot monkey like myself manages to break the kernel
*again* in some part of the kernel they're totally unfamiliar with, and
they need to discover for themselves that timekeeping.c is the only user.

(Of course, in another 5-6 years I'll probably have broken something in
every part of the kernel and whinged at Andrew about it, and that argument
won't apply anymore.. ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [RFC][patch 02/12] remove clocksource inline functions

[Christoph Hellwig]

> You know that a priori because you're familiar with that code. But there's
> another use case: An idiot monkey like myself manages to break the kernel
> *again* in some part of the kernel they're totally unfamiliar with, and
> they need to discover for themselves that timekeeping.c is the only user.
> 
> (Of course, in another 5-6 years I'll probably have broken something in
> every part of the kernel and whinged at Andrew about it, and that argument
> won't apply anymore.. ;)

Andthe poor grepping monkey can be sure that everyone used the useless
wrapper exactly how?  If you want to help people with grepping rename
the method from read to clocksource_read..

Re: [RFC][patch 02/12] remove clocksource inline functions

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Thu, 30 Jul 2009 17:48:36 EDT, Christoph Hellwig said:
> > You know that a priori because you're familiar with that code. But there's
> > another use case: An idiot monkey like myself manages to break the kernel
> > *again* in some part of the kernel they're totally unfamiliar with, and
> > they need to discover for themselves that timekeeping.c is the only user.
> > 
> > (Of course, in another 5-6 years I'll probably have broken something in
> > every part of the kernel and whinged at Andrew about it, and that argument
> > won't apply anymore.. ;)
> 
> Andthe poor grepping monkey can be sure that everyone used the useless
> wrapper exactly how?  If you want to help people with grepping rename
> the method from read to clocksource_read..

Even better. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Fri, 31 Jul 2009 07:50:22 -0400
Valdis.Kletnieks@vt.edu wrote:

> On Thu, 30 Jul 2009 17:48:36 EDT, Christoph Hellwig said:
> > > You know that a priori because you're familiar with that code. But there's
> > > another use case: An idiot monkey like myself manages to break the kernel
> > > *again* in some part of the kernel they're totally unfamiliar with, and
> > > they need to discover for themselves that timekeeping.c is the only user.
> > > 
> > > (Of course, in another 5-6 years I'll probably have broken something in
> > > every part of the kernel and whinged at Andrew about it, and that argument
> > > won't apply anymore.. ;)
> > 
> > Andthe poor grepping monkey can be sure that everyone used the useless
> > wrapper exactly how?  If you want to help people with grepping rename
> > the method from read to clocksource_read..
> 
> Even better. ;)
 
With the same reasoning you could argue that f_ops->read should be
renamed to f_ops->file_read. And why is it so important to be able
to grep for clocksource_read? Not that a patch to rename read to
clocksource_read is hard but before I go ahead an do the rename for 60+
clocksources in the kernel I would like to have a better reason than
"I might want to grep it".

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Daniel Walker]

On Wed, 2009-07-29 at 17:32 +0200, Martin Schwidefsky wrote:
> Hmm, you have an object of type struct clocksource and you do
> cs->read(cs). If that is not clear enough then I don't know what is.

It's not as clear as it could be .. In the case above you have to look
in at least two places to know what's going on.. First to see the
cs->read() , and second to see if "cs" is actually a clocksource or
something else.. "cs" could be declared anyplace with any name.

If you see clocksource_read(cs) , you might need to once check what
clocksource_read() is actually doing, but only once.. After that when
you see that function you know that variable is a clocksource, and it's
"read()" is getting called. So you only need to review one line in the
simplest case.

Daniel

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Wed, 29 Jul 2009 08:52:50 -0700
Daniel Walker <dwalker@fifo99.com> wrote:

> On Wed, 2009-07-29 at 17:32 +0200, Martin Schwidefsky wrote:
> > Hmm, you have an object of type struct clocksource and you do
> > cs->read(cs). If that is not clear enough then I don't know what is.
> 
> It's not as clear as it could be .. In the case above you have to look
> in at least two places to know what's going on.. First to see the
> cs->read() , and second to see if "cs" is actually a clocksource or
> something else.. "cs" could be declared anyplace with any name.

Well you have something like that in the code:

	struct clocksource *clock;

	clock = timekeeper.clock;
	cycle_now = clock->read(clock);

If I read the function top to bottom I immediately see that clock is a
clocksource and that the code does a read on it. That is not the case
if I need to lookup the clocksource_read wrapper.

> If you see clocksource_read(cs) , you might need to once check what
> clocksource_read() is actually doing, but only once.. After that when
> you see that function you know that variable is a clocksource, and it's
> "read()" is getting called. So you only need to review one line in the
> simplest case.

After you learned (once) that timekeeper.clock is a clock source you
have no trouble to understand the 6 occurrences of clock->read(clock)
there are in the code.

Anyway this seems to be a matter of personal preference, in the end I
don't care too much about the inline functions.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

On Wed, 29 Jul 2009 08:15:19 -0600
dwalker@fifo99.com wrote:

> > Remove clocksource_read, clocksource_enable and clocksource_disable
> > inline functions. No functional change.
> > 
> 
> Your still not really explaining this one, is this suppose to be
> cleaner? Or is this related to some other part of your clean up?

The only one of the three inline functions that is a bit more
complicated is clocksource_enable() because of the mult_orig logic. But
that goes away with a later patch. The function aren't accessors either,
they are used exclusively by the timekeeping code. In short, they are
useless, don't you think?

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Daniel Walker]

On Wed, 2009-07-29 at 16:44 +0200, Martin Schwidefsky wrote:
> On Wed, 29 Jul 2009 08:15:19 -0600
> dwalker@fifo99.com wrote:
> 
> > > Remove clocksource_read, clocksource_enable and clocksource_disable
> > > inline functions. No functional change.
> > > 
> > 
> > Your still not really explaining this one, is this suppose to be
> > cleaner? Or is this related to some other part of your clean up?
> 
> The only one of the three inline functions that is a bit more
> complicated is clocksource_enable() because of the mult_orig logic. But
> that goes away with a later patch. The function aren't accessors either,
> they are used exclusively by the timekeeping code. In short, they are
> useless, don't you think?

Above is what should go in your patch description .. 

The reason that I'm not totally into this one is cause these inlines
help to document to the code..

If you have ,

struct clocksource cs;

then several lines later you have

cs->read(); 

vs,

clocksource_read(cs);

The later is completely clear, and the former isn't.. Instead of "cs"
you could pick any obscure name, and read() isn't exactly unique.. So
really any function in the clocksource structure has the potential for a
helper, and the inlines don't really cost anything ..

Daniel

[RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

Greetings,
version 2 of the clocksource / timekeeping cleanup patches. The series
has grown quite a bit, what started with a simple idea to replace the
tick based clocksource update with stop_machine is now a full fledged
code rework.

The code is working on s390 and on my Athlon system at home which has
a broken tsc clocksource:

[    0.000000] Fast TSC calibration using PIT
[    0.000341] hpet clockevent registered
[    0.000343] HPET: 3 timers in total, 0 timers will be used for per-cpu timer
[    0.204021] hpet0: at MMIO 0xfefff000, IRQs 2, 8, 31
[    0.204027] hpet0: 3 comparators, 32-bit 25.000000 MHz counter
[    0.208007] Switching to clock hpet
[    0.211544] Switched to high resolution mode on CPU 0
[    0.211960] Switched to high resolution mode on CPU 1
[    8.000020] Clocksource tsc unstable (delta = -172310085 ns)

So the clocksource switch via stop_machine and the clocksource watchdog
are working. I keep the fingers crossed that nothing else breaks.

The patch set is based on todays upstream tree plus the patches from
the tip tree, if anyone wants to try them you need to pull from the
master branch of
    git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-tip

There is still more room for improvement. Some sore points are:

1) The cycle_last value still is in the struct clocksource. It should
   be in the struct timekeeper but the check against cycles_last in the
   read function of the TSC clock source makes it hard.
2) read_persistent_clock returns seconds. With a really good initial
   time source this is not very precise. read_persistent_clock should
   return a struct timespec.
3) xtime, raw_time, total_sleep_time, timekeeping_suspended, jiffies,
   the ntp state and probably a few other values may be better located
   in the struct timekeeper as well.

and a few more I forgot.

Many thanks to John who pushed me into the right directions.
-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

[-- Attachment #1: clocksource-inline.diff --]
[-- Type: text/plain, Size: 5591 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Remove clocksource_read, clocksource_enable and clocksource_disable
inline functions. No functional change.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 include/linux/clocksource.h |   46 --------------------------------------------
 kernel/time/timekeeping.c   |   32 +++++++++++++++++-------------
 2 files changed, 18 insertions(+), 60 deletions(-)

Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -79,7 +79,7 @@ static void clocksource_forward_now(void
 	cycle_t cycle_now, cycle_delta;
 	s64 nsec;
 
-	cycle_now = clocksource_read(clock);
+	cycle_now = clock->read(clock);
 	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 	clock->cycle_last = cycle_now;
 
@@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
 		*ts = xtime;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -146,7 +146,7 @@ ktime_t ktime_get(void)
 		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
 		tomono = wall_to_monotonic;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -274,16 +274,18 @@ static void change_clocksource(void)
 
 	clocksource_forward_now();
 
-	if (clocksource_enable(new))
+	if (new->enable && ! new->enable(new))
 		return;
+	/* save mult_orig on enable */
+	new->mult_orig = new->mult;
 
 	new->raw_time = clock->raw_time;
 	old = clock;
 	clock = new;
-	clocksource_disable(old);
+	if (old->disable)
+		old->disable(old);
 
-	clock->cycle_last = 0;
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 	clock->error = 0;
 	clock->xtime_nsec = 0;
 	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
@@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
 		seq = read_seqbegin(&xtime_lock);
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -435,9 +437,12 @@ void __init timekeeping_init(void)
 	ntp_init();
 
 	clock = clocksource_get_next();
-	clocksource_enable(clock);
+	if (clock->enable)
+		clock->enable(clock);
+	/* save mult_orig on enable */
+	clock->mult_orig = clock->mult;
 	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;
@@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
 	}
 	update_xtime_cache(0);
 	/* re-base the last cycle value */
-	clock->cycle_last = 0;
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 	clock->error = 0;
 	timekeeping_suspended = 0;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
@@ -630,7 +634,7 @@ void update_wall_time(void)
 		return;
 
 #ifdef CONFIG_GENERIC_TIME
-	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
+	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
 #else
 	offset = clock->cycle_interval;
 #endif
Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
 }
 
 /**
- * clocksource_read: - Access the clocksource's current cycle value
- * @cs:		pointer to clocksource being read
- *
- * Uses the clocksource to return the current cycle_t value
- */
-static inline cycle_t clocksource_read(struct clocksource *cs)
-{
-	return cs->read(cs);
-}
-
-/**
- * clocksource_enable: - enable clocksource
- * @cs:		pointer to clocksource
- *
- * Enables the specified clocksource. The clocksource callback
- * function should start up the hardware and setup mult and field
- * members of struct clocksource to reflect hardware capabilities.
- */
-static inline int clocksource_enable(struct clocksource *cs)
-{
-	int ret = 0;
-
-	if (cs->enable)
-		ret = cs->enable(cs);
-
-	/* save mult_orig on enable */
-	cs->mult_orig = cs->mult;
-
-	return ret;
-}
-
-/**
- * clocksource_disable: - disable clocksource
- * @cs:		pointer to clocksource
- *
- * Disables the specified clocksource. The clocksource callback
- * function should power down the now unused hardware block to
- * save power.
- */
-static inline void clocksource_disable(struct clocksource *cs)
-{
-	if (cs->disable)
-		cs->disable(cs);
-}
-
-/**
  * cyc2ns - converts clocksource cycles to nanoseconds
  * @cs:		Pointer to clocksource
  * @cycles:	Cycles

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][patch 02/12] remove clocksource inline functions

[Daniel Walker]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
> 

Your still not really explaining this one, is this suppose to be
cleaner? Or is this related to some other part of your clean up?

Daniel

Re: [RFC][patch 02/12] remove clocksource inline functions

[Christoph Hellwig]

On Wed, Jul 29, 2009 at 07:15:30AM -0700, Daniel Walker wrote:
> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (clocksource-inline.diff)
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > Remove clocksource_read, clocksource_enable and clocksource_disable
> > inline functions. No functional change.
> > 
> 
> Your still not really explaining this one, is this suppose to be
> cleaner? Or is this related to some other part of your clean up?

It removes an entirely useless layer of obsfucation.

Re: [RFC][patch 02/12] remove clocksource inline functions

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: john stultz <johnstul@us.ibm.com>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Again, Magnus's fix for mult_orig that Thomas has (or *should* have)
queued at this point for 2.6.31 will collide with this, but the fix is
trivial.

Acked-by: John Stultz <johnstul@us.ibm.com>


> ---
>  include/linux/clocksource.h |   46 --------------------------------------------
>  kernel/time/timekeeping.c   |   32 +++++++++++++++++-------------
>  2 files changed, 18 insertions(+), 60 deletions(-)
> 
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -79,7 +79,7 @@ static void clocksource_forward_now(void
>  	cycle_t cycle_now, cycle_delta;
>  	s64 nsec;
> 
> -	cycle_now = clocksource_read(clock);
> +	cycle_now = clock->read(clock);
>  	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
>  	clock->cycle_last = cycle_now;
> 
> @@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
>  		*ts = xtime;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -146,7 +146,7 @@ ktime_t ktime_get(void)
>  		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
>  		tomono = wall_to_monotonic;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -274,16 +274,18 @@ static void change_clocksource(void)
> 
>  	clocksource_forward_now();
> 
> -	if (clocksource_enable(new))
> +	if (new->enable && ! new->enable(new))
>  		return;
> +	/* save mult_orig on enable */
> +	new->mult_orig = new->mult;
> 
>  	new->raw_time = clock->raw_time;
>  	old = clock;
>  	clock = new;
> -	clocksource_disable(old);
> +	if (old->disable)
> +		old->disable(old);
> 
> -	clock->cycle_last = 0;
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
>  	clock->error = 0;
>  	clock->xtime_nsec = 0;
>  	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> @@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
>  		seq = read_seqbegin(&xtime_lock);
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -435,9 +437,12 @@ void __init timekeeping_init(void)
>  	ntp_init();
> 
>  	clock = clocksource_get_next();
> -	clocksource_enable(clock);
> +	if (clock->enable)
> +		clock->enable(clock);
> +	/* save mult_orig on enable */
> +	clock->mult_orig = clock->mult;
>  	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
> 
>  	xtime.tv_sec = sec;
>  	xtime.tv_nsec = 0;
> @@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
>  	}
>  	update_xtime_cache(0);
>  	/* re-base the last cycle value */
> -	clock->cycle_last = 0;
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
>  	clock->error = 0;
>  	timekeeping_suspended = 0;
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
> @@ -630,7 +634,7 @@ void update_wall_time(void)
>  		return;
> 
>  #ifdef CONFIG_GENERIC_TIME
> -	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> +	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
>  #else
>  	offset = clock->cycle_interval;
>  #endif
> Index: linux-2.6/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.orig/include/linux/clocksource.h
> +++ linux-2.6/include/linux/clocksource.h
> @@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
>  }
> 
>  /**
> - * clocksource_read: - Access the clocksource's current cycle value
> - * @cs:		pointer to clocksource being read
> - *
> - * Uses the clocksource to return the current cycle_t value
> - */
> -static inline cycle_t clocksource_read(struct clocksource *cs)
> -{
> -	return cs->read(cs);
> -}
> -
> -/**
> - * clocksource_enable: - enable clocksource
> - * @cs:		pointer to clocksource
> - *
> - * Enables the specified clocksource. The clocksource callback
> - * function should start up the hardware and setup mult and field
> - * members of struct clocksource to reflect hardware capabilities.
> - */
> -static inline int clocksource_enable(struct clocksource *cs)
> -{
> -	int ret = 0;
> -
> -	if (cs->enable)
> -		ret = cs->enable(cs);
> -
> -	/* save mult_orig on enable */
> -	cs->mult_orig = cs->mult;
> -
> -	return ret;
> -}
> -
> -/**
> - * clocksource_disable: - disable clocksource
> - * @cs:		pointer to clocksource
> - *
> - * Disables the specified clocksource. The clocksource callback
> - * function should power down the now unused hardware block to
> - * save power.
> - */
> -static inline void clocksource_disable(struct clocksource *cs)
> -{
> -	if (cs->disable)
> -		cs->disable(cs);
> -}
> -
> -/**
>   * cyc2ns - converts clocksource cycles to nanoseconds
>   * @cs:		Pointer to clocksource
>   * @cycles:	Cycles
>