Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit

[John Stultz]

On Wed, 2010-12-29 at 05:47 +0900, Kuwahara,T. wrote:
> On Tue, Dec 28, 2010 at 8:40 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > From: Richard Cochran <richardcochran@xxxxxxxxx>
> >
> > This patch adds a new mode bit into the timex structure. When set, the
> > bit instructs the kernel to add the given time value to the current time.
> 
> I came up with this simple solution: "Just use ADJ_OFFSET as usual,
> but don't forget to disable the kernel PLL."

Again, this seems to be trying to add new functionality into a corner
case of a existing mode.

I don't like this because it makes testing and validating that the code
is correct for its uses difficult. Especially given that a number of
combinations of modes might be set at once. What happens to freq
adjustments done at the same time as an ADJ_OFFSET - STA_PLL?

Personally, I see the adjtimex call as too flexible, as I'd prefer to
have the modes be exclusive (adjusting one thing per call). As it is
now, the kernel doesn't throw out enough invalid to invalid-ish cases.
ie: MOD_NANO|MOD_MICRO: totally cool! We should fix these and make the
input checking more strict, but in my mind moving to mode-numbers rather
then mode-flags would be much nicer (and more extendable).

Richard: Maybe this is a good thing to think about for clock_adjtime? If
we are adding a new syscall, maybe we should make sure we clean up some
of the old syscalls issues? It does add a good bit of complexity, as the
idea of clock_adjtime being a multiplexing adjtimex was nice and simple.
We'd also have to review the mode usage to see if multi-mode adjustments
in a single call are all that common or not.

Kuwahara: Maybe could you maybe better explain your passion against
using a new mode-flag for this new functionality? You seem dead set
against it, but have not expressed your rational well.

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/