[RFC][PATCH] ppc misusing NTP's time_offset value

[RFC][PATCH] ppc misusing NTP's time_offset value

[john stultz]

Hey everyone,

	As part of my timeofday rework, I've been looking at the NTP code and I
noticed that the PPC architecture is apparently misusing the NTP's
time_offset (it is a terrible name!) value as some form of timezone
offset. This could cause problems when time_offset changed by the NTP
code.
	
	This patch changes the PPC code so it uses a more clear local variable:
timezone_offset.

Could a PPC maintainer verify this is correct?

Let me know if you have any comments or feedback.

thanks
-john

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

diff --git a/arch/ppc/kernel/time.c b/arch/ppc/kernel/time.c
--- a/arch/ppc/kernel/time.c
+++ b/arch/ppc/kernel/time.c
@@ -90,6 +90,9 @@ unsigned long tb_to_ns_scale;
 
 extern unsigned long wall_jiffies;
 
+/* used for timezone offset */
+static long timezone_offset;
+
 DEFINE_SPINLOCK(rtc_lock);
 
 EXPORT_SYMBOL(rtc_lock);
@@ -171,7 +174,7 @@ void timer_interrupt(struct pt_regs * re
 		     xtime.tv_sec - last_rtc_update >= 659 &&
 		     abs((xtime.tv_nsec / 1000) - (1000000-1000000/HZ)) < 500000/HZ &&
 		     jiffies - wall_jiffies == 1) {
-		  	if (ppc_md.set_rtc_time(xtime.tv_sec+1 + time_offset) == 0)
+		  	if (ppc_md.set_rtc_time(xtime.tv_sec+1 + timezone_offset) == 0)
 				last_rtc_update = xtime.tv_sec+1;
 			else
 				/* Try again one minute later */
@@ -284,7 +287,7 @@ void __init time_init(void)
 	unsigned old_stamp, stamp, elapsed;
 
         if (ppc_md.time_init != NULL)
-                time_offset = ppc_md.time_init();
+                timezone_offset = ppc_md.time_init();
 
 	if (__USE_RTC()) {
 		/* 601 processor: dec counts down by 128 every 128ns */
@@ -329,10 +332,10 @@ void __init time_init(void)
 	set_dec(tb_ticks_per_jiffy);
 
 	/* If platform provided a timezone (pmac), we correct the time */
-        if (time_offset) {
-		sys_tz.tz_minuteswest = -time_offset / 60;
+        if (timezone_offset) {
+		sys_tz.tz_minuteswest = -timezone_offset / 60;
 		sys_tz.tz_dsttime = 0;
-		xtime.tv_sec -= time_offset;
+		xtime.tv_sec -= timezone_offset;
         }
         set_normalized_timespec(&wall_to_monotonic,
                                 -xtime.tv_sec, -xtime.tv_nsec);

Re: [RFC][PATCH] ppc misusing NTP's time_offset value

[Tom Rini]

On Wed, Jun 29, 2005 at 03:05:51PM -0700, john stultz wrote:
> Hey everyone,
> 
> 	As part of my timeofday rework, I've been looking at the NTP code and I
> noticed that the PPC architecture is apparently misusing the NTP's
> time_offset (it is a terrible name!) value as some form of timezone
> offset. This could cause problems when time_offset changed by the NTP
> code.
> 	
> 	This patch changes the PPC code so it uses a more clear local variable:
> timezone_offset.
> 
> Could a PPC maintainer verify this is correct?

As mentioned by Mikael, it's odd that this problem came up, since i
guess the wrong patch made it in.  Andrew, please grab this if you
haven't already.  Thanks.

Acked-by: Tom Rini <trini@kernel.crashing.org>

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC][PATCH] ppc misusing NTP's time_offset value

[Mikael Pettersson]

On Wed, 29 Jun 2005 15:05:51 -0700, john stultz wrote:
>	As part of my timeofday rework, I've been looking at the NTP code and I
>noticed that the PPC architecture is apparently misusing the NTP's
>time_offset (it is a terrible name!) value as some form of timezone
>offset. This could cause problems when time_offset changed by the NTP
>code.
>	
>	This patch changes the PPC code so it uses a more clear local variable:
>timezone_offset.
>
>Could a PPC maintainer verify this is correct?
>
>Let me know if you have any comments or feedback.

arch/ppc/kernel/time.c used to have a 'static long time_offset;'
variable. Ulthough unrelated, this declaration clashed with the
one for kernel/time.c, causing compile-time errors with gcc4.
I submitted a fix for this in February, which renamed ppc's local
variable, and it was ACKed by Tom Rini and queued for 2.6.12.

However, the patch that actually went into 2.6.12 was different:
it just removed ppc's local variable, making arch/ppc/kernel/time.c
now share kernel/time.c's variable. At the time I assumed someone
had proved that the two modules _should_ share state, so I didn't
make a fuss about it.

Your patch brings the semantics back to what it was prior to 2.6.12.

/Mikael

Re: [RFC][PATCH] ppc misusing NTP's time_offset value

[Kumar Gala]

I think you forgot the patch.

- kumar

On Jun 30, 2005, at 5:15 AM, Mikael Pettersson wrote:

> On Wed, 29 Jun 2005 15:05:51 -0700, john stultz wrote:
>
>>     As part of my timeofday rework, I've been looking at the NTP
>>
> code and I
>
>> noticed that the PPC architecture is apparently misusing the NTP's
>> time_offset (it is a terrible name!) value as some form of timezone
>> offset. This could cause problems when time_offset changed by the NTP
>> code.
>>
>>     This patch changes the PPC code so it uses a more clear local
>>
> variable:
>
>> timezone_offset.
>>
>> Could a PPC maintainer verify this is correct?
>>
>> Let me know if you have any comments or feedback.
>>
>
> arch/ppc/kernel/time.c used to have a 'static long time_offset;'
> variable. Ulthough unrelated, this declaration clashed with the
> one for kernel/time.c, causing compile-time errors with gcc4.
> I submitted a fix for this in February, which renamed ppc's local
> variable, and it was ACKed by Tom Rini and queued for 2.6.12.
>
> However, the patch that actually went into 2.6.12 was different:
> it just removed ppc's local variable, making arch/ppc/kernel/time.c
> now share kernel/time.c's variable. At the time I assumed someone
> had proved that the two modules _should_ share state, so I didn't
> make a fuss about it.
>
> Your patch brings the semantics back to what it was prior to 2.6.12.
>
> /Mikael
> -
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>