lkml.org 
[lkml]   [2005]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH] new timeofday core subsystem (v. A3)
    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 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/

    \
     
     \ /
      Last update: 2005-03-22 14:11    [W:0.035 / U:60.568 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site