On Fri, Mar 11, 2005 at 05:24:15PM -0800, john stultz wrote:
> +struct timesource_t timesource_jiffies = {
> + .name = "jiffies",
> + .priority = 0, /* lowest priority*/
> + .type = TIMESOURCE_FUNCTION,
> + .read_fnct = jiffies_read,
> + .mask = (cycle_t)~0,
Not sure this is right. The type of 0 is 'int' and the ~ will happen
before the cast to a potentially longer type.
> + .mult = NSEC_PER_SEC/HZ,
Does rounding matter here? Alpha has HZ of 1024, so this comes out to
976562.5.
> +struct timesource_t {
> + char* name;
> + int priority;
> + enum {
> + TIMESOURCE_FUNCTION,
> + TIMESOURCE_CYCLES,
> + TIMESOURCE_MMIO_32,
> + TIMESOURCE_MMIO_64
> + } type;
> + cycle_t (*read_fnct)(void);
> + void __iomem* mmio_ptr;
Convention is * goes next to the variable name rather than the type.
> +/* XXX - this should go somewhere better! */
> +#ifndef readq
> +static inline unsigned long long readq(void __iomem *addr)
Somewhere in asm-generic..
> +static inline cycle_t read_timesource(struct timesource_t* ts)
> +{
> + switch (ts->type) {
> + case TIMESOURCE_MMIO_32:
> + return (cycle_t)readl(ts->mmio_ptr);
> + case TIMESOURCE_MMIO_64:
> + return (cycle_t)readq(ts->mmio_ptr);
> + case TIMESOURCE_CYCLES:
> + return (cycle_t)get_cycles();
> + default:/* case: TIMESOURCE_FUNCTION */
> + return ts->read_fnct();
> + }
> +}
Wouldn't it be better to change read_fnct to take a timesource * and
then change all the other guys to generic_timesource_<foo> helper
functions? This does away with the switch and makes it trivial to add
new generic sources. Change mmio_ptr to void *private.
> @@ -467,6 +468,7 @@
> pidhash_init();
> init_timers();
> softirq_init();
> + timeofday_init();
> time_init();
Can we push time_init inside of timeofday_init?
> +/* Chapter 5: Kernel Variables [RFC 1589 pg. 28] */
> +/* 5.1 Interface Variables */
> +static int ntp_status = STA_UNSYNC; /* status */
> +static long ntp_offset; /* usec */
> +static long ntp_constant = 2; /* ntp magic? */
> +static long ntp_maxerror = NTP_PHASE_LIMIT; /* usec */
> +static long ntp_esterror = NTP_PHASE_LIMIT; /* usec */
> +static const long ntp_tolerance = MAXFREQ; /* shifted ppm
> */
> +static const long ntp_precision = 1; /* constant */
> +
> +/* 5.2 Phase-Lock Loop Variables */
> +static long ntp_freq; /* shifted ppm
> */
> +static long ntp_reftime; /* sec */
You present a nice argument for not using tabs except at the beginning
of the line.
> +#define MILLION 1000000
Still a magic number despite being a define. Very meta. Unused.
> +/* int ntp_advance(nsec_t interval):
> + * Periodic hook which increments NTP state machine by interval.
> + * Returns the signed PPM adjustment to be used for the next interval.
> + * This is ntp_hardclock in the RFC.
Why is it not ntp_hardclock here?
> + */
> +int ntp_advance(nsec_t interval)
> +{
> + static u64 interval_sum=0;
Spaces.
> + /* decrement singleshot offset interval */
> + ss_offset_len =- interval;
Eh?
> + /* bound the adjustment to MAXPHASE/MINSEC */
> + if (tmp > (MAXPHASE / MINSEC) << SHIFT_UPDATE)
> + tmp = (MAXPHASE / MINSEC) << SHIFT_UPDATE;
> + if (tmp < -(MAXPHASE / MINSEC) << SHIFT_UPDATE)
> + tmp = -(MAXPHASE / MINSEC) << SHIFT_UPDATE;
max, min?
> + /* Make sure offset is bounded by MAXPHASE */
> + if (tmp > MAXPHASE)
> + tmp = MAXPHASE;
> + if (tmp < -MAXPHASE)
> + tmp = -MAXPHASE;
max, min.
> + if ((ntp_status & STA_FLL) && (interval >= MINSEC)) {
> + long damping;
> + tmp = (offset / interval); /* ppm (usec/sec)*/
(unnecessary parens)
> +/* int ntp_adjtimex(struct timex* tx)
> + * Interface to change NTP state machine
> + */
> +int ntp_adjtimex(struct timex* tx)
> +{
> + long save_offset;
> + int result;
> + unsigned long flags;
> +
> +/*=[Sanity checking]===============================*/
> + /* Check capabilities if we're trying to modify something */
> + if (tx->modes && !capable(CAP_SYS_TIME))
> + return -EPERM;
This is already done in do_adjtimex.
> + /* clear everything */
> + ntp_status |= STA_UNSYNC;
> + ntp_maxerror = NTP_PHASE_LIMIT;
> + ntp_esterror = NTP_PHASE_LIMIT;
> + ss_offset_len=0;
> + singleshot_adj=0;
> + tick_adj=0;
> + offset_adj =0;
Spacing.
> +/*[Nanosecond based variables]----------------
This comment style is weird. Kill the trailing dashes at least.
> +static enum { TIME_RUNNING, TIME_SUSPENDED } time_suspend_state =
> TIME_RUNNING;
Insert some line breaks.
> + /* convert timespec to ns */
> + nsec_t newtime = timespec2ns(tv);
> + /* clear NTP settings */
> + ntp_clear();
Pointless comments.
> +int do_adjtimex(struct timex *tx)
> +{
> + do_gettimeofday(&tx->time); /* set timex->time*/
Oh. Move the cap check back here..
> + if (time_suspend_state != TIME_RUNNING) {
> + printk(KERN_INFO "timeofday_suspend_hook: ACK! called while
> we're suspended!");
Line length. Perhaps BUG_ON instead.
> + /* finally, update legacy time values */
> + write_seqlock_irqsave(&xtime_lock, x_flags);
> + xtime = ns2timespec(system_time + wall_time_offset);
> + wall_to_monotonic = ns2timespec(wall_time_offset);
> + wall_to_monotonic.tv_sec = -wall_to_monotonic.tv_sec;
> + wall_to_monotonic.tv_nsec = -wall_to_monotonic.tv_nsec;
> + /* XXX - should jiffies be updated here? */
Excellent question.
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
