Re: [PATCH] Make the kernel NTP code hand 64-bit *unsigned* values to do_div()

[Roman Zippel]

Hi,

On Thu, 21 Feb 2008, David Howells wrote:

> The kernel NTP code shouldn't hand 64-bit *signed* values to do_div().  Make 
> it
> instead hand 64-bit unsigned values.  This gets rid of a couple of warnings.

I would actually prefer to introduce an explicit API for signed 64 
divides to get rid of the temps completely, something like below.
Right now it uses do_div as fallback. When all archs are converted, do_div 
can be single compatibility define and perhaps we can get rid of it
completely.
Bonus feature: implement the x86 version without the asm casts allowing 
gcc to generate better code.

bye, Roman

---
 include/asm-generic/div64.h |   14 ++
 include/asm-i386/div64.h|   20 
 include/linux/calc64.h  |   28 
 kernel/time.c   |   26 +++---
 kernel/time/ntp.c   |   21 +
 lib/div64.c |   21 -
 6 files changed, 94 insertions(+), 36 deletions(-)

Index: linux-2.6/include/asm-generic/div64.h
===
--- linux-2.6.orig/include/asm-generic/div64.h
+++ linux-2.6/include/asm-generic/div64.h
@@ -35,6 +35,20 @@ static inline uint64_t div64_64(uint64_t
return dividend / divisor;
 }
 
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   *remainder = dividend % divisor;
+   return dividend / divisor;
+}
+#define div_u64_remdiv_u64_rem
+
+static inline s64 div_s64_rem(s64 dividend, s32 divisor, s32 *remainder)
+{
+   *remainder = dividend % divisor;
+   return dividend / divisor;
+}
+#define div_s64_remdiv_s64_rem
+
 #elif BITS_PER_LONG == 32
 
 extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
Index: linux-2.6/include/asm-i386/div64.h
===
--- linux-2.6.orig/include/asm-i386/div64.h
+++ linux-2.6/include/asm-i386/div64.h
@@ -48,5 +48,25 @@ div_ll_X_l_rem(long long divs, long div,
 
 }
 
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   union {
+   u64 v64;
+   u32 v32[2];
+   } d = { dividend };
+   u32 upper;
+
+   upper = d.v32[1];
+   if (upper) {
+   upper = d.v32[1] % divisor;
+   d.v32[1] = d.v32[1] / divisor;
+   }
+   asm ("divl %2" : "=a" (d.v32[0]), "=d" (*remainder) :
+   "rm" (divisor), "0" (d.v32[0]), "1" (upper));
+   return d.v64;
+}
+#define div_u64_remdiv_u64_rem
+
 extern uint64_t div64_64(uint64_t dividend, uint64_t divisor);
+
 #endif
Index: linux-2.6/include/linux/calc64.h
===
--- linux-2.6.orig/include/linux/calc64.h
+++ linux-2.6/include/linux/calc64.h
@@ -46,4 +46,32 @@ static inline long div_long_long_rem_sig
return res;
 }
 
+#ifndef div_u64_rem
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   *remainder = do_div(dividend, divisor);
+   return dividend;
+}
+#endif
+
+#ifndef div_u64
+static inline u64 div_u64(u64 dividend, u32 divisor)
+{
+   u32 remainder;
+   return div_u64_rem(dividend, divisor, );
+}
+#endif
+
+#ifndef div_s64_rem
+extern s64 div_s64_rem(s64 dividend, s32 divisor, s32 *remainder);
+#endif
+
+#ifndef div_s64
+static inline s64 div_s64(s64 dividend, s32 divisor)
+{
+   s32 remainder;
+   return div_s64_rem(dividend, divisor, );
+}
+#endif
+
 #endif
Index: linux-2.6/kernel/time.c
===
--- linux-2.6.orig/kernel/time.c
+++ linux-2.6/kernel/time.c
@@ -661,9 +661,7 @@ clock_t jiffies_to_clock_t(long x)
 #if (TICK_NSEC % (NSEC_PER_SEC / USER_HZ)) == 0
return x / (HZ / USER_HZ);
 #else
-   u64 tmp = (u64)x * TICK_NSEC;
-   do_div(tmp, (NSEC_PER_SEC / USER_HZ));
-   return (long)tmp;
+   return div_u64((u64)x * TICK_NSEC, NSEC_PER_SEC / USER_HZ);
 #endif
 }
 EXPORT_SYMBOL(jiffies_to_clock_t);
@@ -675,16 +673,12 @@ unsigned long clock_t_to_jiffies(unsigne
return ~0UL;
return x * (HZ / USER_HZ);
 #else
-   u64 jif;
-
/* Don't worry about loss of precision here .. */
if (x >= ~0UL / HZ * USER_HZ)
return ~0UL;
 
/* .. but do try to contain it here */
-   jif = x * (u64) HZ;
-   do_div(jif, USER_HZ);
-   return jif;
+   return div_u64((u64)x * HZ, USER_HZ);
 #endif
 }
 EXPORT_SYMBOL(clock_t_to_jiffies);
@@ -692,17 +686,15 @@ EXPORT_SYMBOL(clock_t_to_jiffies);
 u64 jiffies_64_to_clock_t(u64 x)
 {
 #if (TICK_NSEC % (NSEC_PER_SEC / USER_HZ)) == 0
-   do_div(x, HZ / USER_HZ);
+   return div_u64(x, HZ / USER_HZ);
 #else
/*
 * There are better ways that don't overflow early,
 * but even this doesn't overflow in hundreds of years
 * in 64 bits, so..
 */
-   x *= 

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Thu, 21 Feb 2008, john stultz wrote:

> > Again, what kind of crappy hardware do you expect? Aren't clocks supposed 
> > to get better and not worse?
> 
> Well, while I've seen much worse, I consider crappy hardware to be 100
> +ppm error. So if the hardware is perfect and the system results in
> 153ppm error, I'd consider that pretty crappy, especially if its not the
> hardware's fault.

Nevertheless this error is real, why are you trying to hide it?
This is isn't an error we can't handle, it's still perfectly within the 
limit and except that NTP reports a somewhat larger drift than you'd like 
to see, everything works fine.

> > Where do you get this idea that the 500ppm are exclusively for hardware 
> > errors? If you have such bad hardware, there is another simple solution: 
> > change HZ to 100 and the error is reduced to 15ppm.
> 
> True its not exclusively for hardware errors, and if we were talking
> about only 15ppm I wouldn't really worry about it. But when we're saying
> the system is adding 30% of the maximum error, that's just not good.

Another 30% is required for normal to crappy hardware clocks and then 
there is still enough room left.

> > I would see the point if this problem had actually any practically 
> > relevance, but this error is not a problem for pretty much all existing 
> > standard hardware. Why are you insisting on redesigning timekeeping for 
> > broken hardware?
> 
> Remember my earlier data? Where I was talking about the acpi_pm being a
> multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a
> 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care,
> but this _does_ effect current hardware, so I'd call it relevant.

How exactly does it effect current hardware in a way that it breaks them? 
Despite this error everything still works fine, the hardware doesn't care.

> > There's nothing 'injected', that resolution error is very real and the 
> > 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by 
> > this.
> 
> Sure, 500ppm is enough for most people with good hardware. But remember
> the alpha example you brought up earlier? The HZ=1200 case, with the
> CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account,
> we end up with a **11230ppm** error from the granularity issue. NTP just
> won't work on those systems.
> 
> Now granted, the three types of alpha systems that actually use that HZ
> value is probably as close to "nobody" as you're going to get, but I
> don't think we can just throw the granularity issue aside.

That's actually a good example, why it's irrelevant. First it's using a 
cycle based clock, thus the rounding error is irrelevant. Second in the 
common case they already use 1024 as HZ to reduce this error, so something 
similiar could be done for the HZ=1200 case and I suspect that it was 
already done and only CLOCK_TICK_RATE is just wrong. This mail 
http://consortiumlibrary.org/axp-list/archive/2002-11/0101.html suggest 
that this is the right thing to do.

There is _no_ reason to artificially optimize this error value, there are 
still enough other ways to improve timekeeping. The granularity error is 
there no matter what you do and as long as it's within a reasonable limit 
there is nothing that needs fixing.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Thu, 21 Feb 2008, john stultz wrote:

  Again, what kind of crappy hardware do you expect? Aren't clocks supposed 
  to get better and not worse?
 
 Well, while I've seen much worse, I consider crappy hardware to be 100
 +ppm error. So if the hardware is perfect and the system results in
 153ppm error, I'd consider that pretty crappy, especially if its not the
 hardware's fault.

Nevertheless this error is real, why are you trying to hide it?
This is isn't an error we can't handle, it's still perfectly within the 
limit and except that NTP reports a somewhat larger drift than you'd like 
to see, everything works fine.

  Where do you get this idea that the 500ppm are exclusively for hardware 
  errors? If you have such bad hardware, there is another simple solution: 
  change HZ to 100 and the error is reduced to 15ppm.
 
 True its not exclusively for hardware errors, and if we were talking
 about only 15ppm I wouldn't really worry about it. But when we're saying
 the system is adding 30% of the maximum error, that's just not good.

Another 30% is required for normal to crappy hardware clocks and then 
there is still enough room left.

  I would see the point if this problem had actually any practically 
  relevance, but this error is not a problem for pretty much all existing 
  standard hardware. Why are you insisting on redesigning timekeeping for 
  broken hardware?
 
 Remember my earlier data? Where I was talking about the acpi_pm being a
 multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a
 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care,
 but this _does_ effect current hardware, so I'd call it relevant.

How exactly does it effect current hardware in a way that it breaks them? 
Despite this error everything still works fine, the hardware doesn't care.

  There's nothing 'injected', that resolution error is very real and the 
  500ppm limit is more than enough to deal with this. _Nobody_ is hurt by 
  this.
 
 Sure, 500ppm is enough for most people with good hardware. But remember
 the alpha example you brought up earlier? The HZ=1200 case, with the
 CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account,
 we end up with a **11230ppm** error from the granularity issue. NTP just
 won't work on those systems.
 
 Now granted, the three types of alpha systems that actually use that HZ
 value is probably as close to nobody as you're going to get, but I
 don't think we can just throw the granularity issue aside.

That's actually a good example, why it's irrelevant. First it's using a 
cycle based clock, thus the rounding error is irrelevant. Second in the 
common case they already use 1024 as HZ to reduce this error, so something 
similiar could be done for the HZ=1200 case and I suspect that it was 
already done and only CLOCK_TICK_RATE is just wrong. This mail 
http://consortiumlibrary.org/axp-list/archive/2002-11/0101.html suggest 
that this is the right thing to do.

There is _no_ reason to artificially optimize this error value, there are 
still enough other ways to improve timekeeping. The granularity error is 
there no matter what you do and as long as it's within a reasonable limit 
there is nothing that needs fixing.

bye, Roman
--
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/

Re: [PATCH] Make the kernel NTP code hand 64-bit *unsigned* values to do_div()

[Roman Zippel]

Hi,

On Thu, 21 Feb 2008, David Howells wrote:

 The kernel NTP code shouldn't hand 64-bit *signed* values to do_div().  Make 
 it
 instead hand 64-bit unsigned values.  This gets rid of a couple of warnings.

I would actually prefer to introduce an explicit API for signed 64 
divides to get rid of the temps completely, something like below.
Right now it uses do_div as fallback. When all archs are converted, do_div 
can be single compatibility define and perhaps we can get rid of it
completely.
Bonus feature: implement the x86 version without the asm casts allowing 
gcc to generate better code.

bye, Roman

---
 include/asm-generic/div64.h |   14 ++
 include/asm-i386/div64.h|   20 
 include/linux/calc64.h  |   28 
 kernel/time.c   |   26 +++---
 kernel/time/ntp.c   |   21 +
 lib/div64.c |   21 -
 6 files changed, 94 insertions(+), 36 deletions(-)

Index: linux-2.6/include/asm-generic/div64.h
===
--- linux-2.6.orig/include/asm-generic/div64.h
+++ linux-2.6/include/asm-generic/div64.h
@@ -35,6 +35,20 @@ static inline uint64_t div64_64(uint64_t
return dividend / divisor;
 }
 
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   *remainder = dividend % divisor;
+   return dividend / divisor;
+}
+#define div_u64_remdiv_u64_rem
+
+static inline s64 div_s64_rem(s64 dividend, s32 divisor, s32 *remainder)
+{
+   *remainder = dividend % divisor;
+   return dividend / divisor;
+}
+#define div_s64_remdiv_s64_rem
+
 #elif BITS_PER_LONG == 32
 
 extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
Index: linux-2.6/include/asm-i386/div64.h
===
--- linux-2.6.orig/include/asm-i386/div64.h
+++ linux-2.6/include/asm-i386/div64.h
@@ -48,5 +48,25 @@ div_ll_X_l_rem(long long divs, long div,
 
 }
 
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   union {
+   u64 v64;
+   u32 v32[2];
+   } d = { dividend };
+   u32 upper;
+
+   upper = d.v32[1];
+   if (upper) {
+   upper = d.v32[1] % divisor;
+   d.v32[1] = d.v32[1] / divisor;
+   }
+   asm (divl %2 : =a (d.v32[0]), =d (*remainder) :
+   rm (divisor), 0 (d.v32[0]), 1 (upper));
+   return d.v64;
+}
+#define div_u64_remdiv_u64_rem
+
 extern uint64_t div64_64(uint64_t dividend, uint64_t divisor);
+
 #endif
Index: linux-2.6/include/linux/calc64.h
===
--- linux-2.6.orig/include/linux/calc64.h
+++ linux-2.6/include/linux/calc64.h
@@ -46,4 +46,32 @@ static inline long div_long_long_rem_sig
return res;
 }
 
+#ifndef div_u64_rem
+static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+   *remainder = do_div(dividend, divisor);
+   return dividend;
+}
+#endif
+
+#ifndef div_u64
+static inline u64 div_u64(u64 dividend, u32 divisor)
+{
+   u32 remainder;
+   return div_u64_rem(dividend, divisor, remainder);
+}
+#endif
+
+#ifndef div_s64_rem
+extern s64 div_s64_rem(s64 dividend, s32 divisor, s32 *remainder);
+#endif
+
+#ifndef div_s64
+static inline s64 div_s64(s64 dividend, s32 divisor)
+{
+   s32 remainder;
+   return div_s64_rem(dividend, divisor, remainder);
+}
+#endif
+
 #endif
Index: linux-2.6/kernel/time.c
===
--- linux-2.6.orig/kernel/time.c
+++ linux-2.6/kernel/time.c
@@ -661,9 +661,7 @@ clock_t jiffies_to_clock_t(long x)
 #if (TICK_NSEC % (NSEC_PER_SEC / USER_HZ)) == 0
return x / (HZ / USER_HZ);
 #else
-   u64 tmp = (u64)x * TICK_NSEC;
-   do_div(tmp, (NSEC_PER_SEC / USER_HZ));
-   return (long)tmp;
+   return div_u64((u64)x * TICK_NSEC, NSEC_PER_SEC / USER_HZ);
 #endif
 }
 EXPORT_SYMBOL(jiffies_to_clock_t);
@@ -675,16 +673,12 @@ unsigned long clock_t_to_jiffies(unsigne
return ~0UL;
return x * (HZ / USER_HZ);
 #else
-   u64 jif;
-
/* Don't worry about loss of precision here .. */
if (x = ~0UL / HZ * USER_HZ)
return ~0UL;
 
/* .. but do try to contain it here */
-   jif = x * (u64) HZ;
-   do_div(jif, USER_HZ);
-   return jif;
+   return div_u64((u64)x * HZ, USER_HZ);
 #endif
 }
 EXPORT_SYMBOL(clock_t_to_jiffies);
@@ -692,17 +686,15 @@ EXPORT_SYMBOL(clock_t_to_jiffies);
 u64 jiffies_64_to_clock_t(u64 x)
 {
 #if (TICK_NSEC % (NSEC_PER_SEC / USER_HZ)) == 0
-   do_div(x, HZ / USER_HZ);
+   return div_u64(x, HZ / USER_HZ);
 #else
/*
 * There are better ways that don't overflow early,
 * but even this doesn't overflow in hundreds of years
 * in 64 bits, so..
 */
-   x 

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

w.

If you really need some kind of adjustment for your extremely broken 
hardware, below is the absolute maximum you need, which doesn't inflict 
more insanity on all the sane hardware.

bye, Roman


Revert bbe4d18ac2e058c56adb0cd71f49d9ed3216a405 and 
e13a2e61dd5152f5499d2003470acf9c838eab84 and remove CLOCK_TICK_ADJUST 
completely. Add a optional kernel parameter ntp_tick_adj instead to allow 
adjusting of a large base drift and thus keeping ntpd happy.
The CLOCK_TICK_ADJUST mechanism was introduced at a time PIT was the 
primary clock, but we have a varity of clock sources now, so a global PIT 
specific adjustment makes little sense anymore.

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>


---
 include/linux/timex.h |9 +
 kernel/time/ntp.c |   11 ++-
 kernel/time/timekeeping.c |6 ++
 3 files changed, 13 insertions(+), 13 deletions(-)

Index: linux-2.6/include/linux/timex.h
===
--- linux-2.6.orig/include/linux/timex.h
+++ linux-2.6/include/linux/timex.h
@@ -232,14 +232,7 @@ static inline int ntp_synced(void)
 #else
 #define NTP_INTERVAL_FREQ  (HZ)
 #endif
-
-#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
-#define CLOCK_TICK_ADJUST  (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
-   (s64)CLOCK_TICK_RATE)
-
-/* Because using NSEC_PER_SEC would be too easy */
-#define NTP_INTERVAL_LENGTH s64)TICK_USEC * NSEC_PER_USEC * USER_HZ) + \
- CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ)
+#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
 extern u64 current_tick_length(void);
Index: linux-2.6/kernel/time/ntp.c
===
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -42,12 +42,13 @@ long time_esterror = NTP_PHASE_LIMIT;   /*
 long time_freq;/* frequency offset (scaled 
ppm)*/
 static long time_reftime;  /* time at last adjustment (s)  */
 long time_adjust;
+long ntp_tick_adj;
 
 static void ntp_update_frequency(void)
 {
u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
<< TICK_LENGTH_SHIFT;
-   second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
+   second_length += (s64)ntp_tick_adj << TICK_LENGTH_SHIFT;
second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
tick_length_base = second_length;
@@ -400,3 +401,11 @@ leave: if ((time_status & (STA_UNSYNC|ST
notify_cmos_timer();
return(result);
 }
+
+static int __init ntp_tick_adj_setup(char *str)
+{
+   ntp_tick_adj = simple_strtol(str, NULL, 0);
+   return 1;
+}
+
+__setup("ntp_tick_adj=", ntp_tick_adj_setup);
Index: linux-2.6/kernel/time/timekeeping.c
===
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -187,8 +187,7 @@ static void change_clocksource(void)
 
clock->error = 0;
clock->xtime_nsec = 0;
-   clocksource_calculate_interval(clock,
-   (unsigned long)(current_tick_length()>>TICK_LENGTH_SHIFT));
+   clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 
tick_clock_notify();
 
@@ -245,8 +244,7 @@ void __init timekeeping_init(void)
ntp_clear();
 
clock = clocksource_get_next();
-   clocksource_calculate_interval(clock,
-   (unsigned long)(current_tick_length()>>TICK_LENGTH_SHIFT));
+   clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
clock->cycle_last = clocksource_read(clock);
 
xtime.tv_sec = sec;
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

 and 
e13a2e61dd5152f5499d2003470acf9c838eab84 and remove CLOCK_TICK_ADJUST 
completely. Add a optional kernel parameter ntp_tick_adj instead to allow 
adjusting of a large base drift and thus keeping ntpd happy.
The CLOCK_TICK_ADJUST mechanism was introduced at a time PIT was the 
primary clock, but we have a varity of clock sources now, so a global PIT 
specific adjustment makes little sense anymore.

Signed-off-by: Roman Zippel [EMAIL PROTECTED]


---
 include/linux/timex.h |9 +
 kernel/time/ntp.c |   11 ++-
 kernel/time/timekeeping.c |6 ++
 3 files changed, 13 insertions(+), 13 deletions(-)

Index: linux-2.6/include/linux/timex.h
===
--- linux-2.6.orig/include/linux/timex.h
+++ linux-2.6/include/linux/timex.h
@@ -232,14 +232,7 @@ static inline int ntp_synced(void)
 #else
 #define NTP_INTERVAL_FREQ  (HZ)
 #endif
-
-#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
-#define CLOCK_TICK_ADJUST  (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
-   (s64)CLOCK_TICK_RATE)
-
-/* Because using NSEC_PER_SEC would be too easy */
-#define NTP_INTERVAL_LENGTH s64)TICK_USEC * NSEC_PER_USEC * USER_HZ) + \
- CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ)
+#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
 extern u64 current_tick_length(void);
Index: linux-2.6/kernel/time/ntp.c
===
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -42,12 +42,13 @@ long time_esterror = NTP_PHASE_LIMIT;   /*
 long time_freq;/* frequency offset (scaled 
ppm)*/
 static long time_reftime;  /* time at last adjustment (s)  */
 long time_adjust;
+long ntp_tick_adj;
 
 static void ntp_update_frequency(void)
 {
u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
 TICK_LENGTH_SHIFT;
-   second_length += (s64)CLOCK_TICK_ADJUST  TICK_LENGTH_SHIFT;
+   second_length += (s64)ntp_tick_adj  TICK_LENGTH_SHIFT;
second_length += (s64)time_freq  (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
tick_length_base = second_length;
@@ -400,3 +401,11 @@ leave: if ((time_status  (STA_UNSYNC|ST
notify_cmos_timer();
return(result);
 }
+
+static int __init ntp_tick_adj_setup(char *str)
+{
+   ntp_tick_adj = simple_strtol(str, NULL, 0);
+   return 1;
+}
+
+__setup(ntp_tick_adj=, ntp_tick_adj_setup);
Index: linux-2.6/kernel/time/timekeeping.c
===
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -187,8 +187,7 @@ static void change_clocksource(void)
 
clock-error = 0;
clock-xtime_nsec = 0;
-   clocksource_calculate_interval(clock,
-   (unsigned long)(current_tick_length()TICK_LENGTH_SHIFT));
+   clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 
tick_clock_notify();
 
@@ -245,8 +244,7 @@ void __init timekeeping_init(void)
ntp_clear();
 
clock = clocksource_get_next();
-   clocksource_calculate_interval(clock,
-   (unsigned long)(current_tick_length()TICK_LENGTH_SHIFT));
+   clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
clock-cycle_last = clocksource_read(clock);
 
xtime.tv_sec = sec;
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 18 Feb 2008, john stultz wrote:

> If we are building a train station, and each train car is 60ft, it
> doesn't make much sense to build 1000ft stations, right? Instead you'll
> do better if you build a 1020ft station.

That would assume trains are always 60ft long, which is the basic error in 
your assumption.

Since we're using analogies: What you're doing is to put you winter 
clothes on your weight scale and reset the scale to zero to offset for the 
weigth of the clothes. If you stand now with your bathing clothes on the 
scale, does that mean you lost weight?
That's all you do - you change the scale and slightly screw the scale for 
everyone else trying to use it.

To keep in mind what time adjusting is supposed to do:

freq = 1sec + time_freq

What we do instead is:

freq + tick_adj = 1sec + time_freq

Where exactly is now the problem to integrate tick_adj into time_freq? The 
result is _exactly_ the same. The only visible difference is a slightly 
higher time_freq value and as long as it is within the 500 ppm limit there 
is simply no problem.

> And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the 
> (A!=B) issue, but it doesn't address the error from #2 below.
> [..]
> 2) We need a solution that handles granularity error well, as this is a
> moderate source of error for course clocksources such as jiffies.
> CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect
> we could do even better, but that will take some deeper changes.

How exactly does CLOCK_TICK_ADJUST address this problem? The error due to 
insufficient resolution is still there, all it does is shifting the scale, 
so it's not immediately visible.

> My understanding of your approach (removing CLOCK_TICK_ADJUST),
> addresses issues #1 and #3, but hurts issue #2.

What exactly is hurt?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 18 Feb 2008, john stultz wrote:

 If we are building a train station, and each train car is 60ft, it
 doesn't make much sense to build 1000ft stations, right? Instead you'll
 do better if you build a 1020ft station.

That would assume trains are always 60ft long, which is the basic error in 
your assumption.

Since we're using analogies: What you're doing is to put you winter 
clothes on your weight scale and reset the scale to zero to offset for the 
weigth of the clothes. If you stand now with your bathing clothes on the 
scale, does that mean you lost weight?
That's all you do - you change the scale and slightly screw the scale for 
everyone else trying to use it.

To keep in mind what time adjusting is supposed to do:

freq = 1sec + time_freq

What we do instead is:

freq + tick_adj = 1sec + time_freq

Where exactly is now the problem to integrate tick_adj into time_freq? The 
result is _exactly_ the same. The only visible difference is a slightly 
higher time_freq value and as long as it is within the 500 ppm limit there 
is simply no problem.

 And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the 
 (A!=B) issue, but it doesn't address the error from #2 below.
 [..]
 2) We need a solution that handles granularity error well, as this is a
 moderate source of error for course clocksources such as jiffies.
 CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect
 we could do even better, but that will take some deeper changes.

How exactly does CLOCK_TICK_ADJUST address this problem? The error due to 
insufficient resolution is still there, all it does is shifting the scale, 
so it's not immediately visible.

 My understanding of your approach (removing CLOCK_TICK_ADJUST),
 addresses issues #1 and #3, but hurts issue #2.

What exactly is hurt?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 13 Feb 2008, john stultz wrote:

> Oh! So your issue is that since time_freq is stored in ppm, or in effect
> a usecs per sec offset, when we add it to something other then 1 second
> we mis-apply what NTP tells us to. Is that right?

Pretty much everything is centered around that 1sec, so the closer the 
update frequency is to it the better.

> Right, so if NTP has us apply a 10ppm adjustment, instead of doing:
>   NSEC_PER_SEC + 10,000
> 
> We're doing:
>   NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000
> 
> Which, if I'm doing my math right, results in a 10.002ppm adjustment
> (using the 999847467ns number above), instead of just a 10ppm
> adjustment.
> 
> Now, true, this is an error, but it is a pretty small one. Even at the
> maximum 500ppm value, it only results in an error of 76 parts per
> billion. As you'll see below, that tends to be less error then what we
> get from the clock granularity. Is there something else I'm missing here
> or is this really the core issue you're concerned with?

The error accumulates and there is no good reason to do this for the 
common case.

> > In consequence this means, if we want to improve timekeeping, we first set 
> > the (update_cycles * NTP_INTERVAL_FREQ) interval as close as possible to 
> > the real frequency. It doesn't has to be perfect as we usually don't know 
> > the real frequency with 100% certainty anyway. 
> 
> This might need some more explanation, as I'm not certain I know what
> update_cycles refers to. Do you mean cycle_interval? I guess I'm not
> completely sure how you're suggesting we change things here.

clock->cycle_interval

> > Second, we drop the tick 
> > adjustment if possible and leave the adjustments to the NTP daemon and as 
> > long as the drift is within the 500ppm limit it has no problem to manage 
> > this.
> 
> Dropping the tick adjustment? By that do you mean the tick_usec value
> set by adjtimex()? I don't quite see why we want that. Could you expand
> here?

CLOCK_TICK_ADJUST

> HZ=1000 CLOCK_TICK_ADJUST=-152533
> jiffies   467 ppb error
> jiffies NOHZ  467 ppb error
> pit   0 ppb error
> pit NOHZ  0 ppb error
> acpi_pm   -280 ppb error
> acpi_pm NOHZ  279 ppb error
> 
> HZ=1000 CLOCK_TICK_ADJUST=0
> jiffies   153000 ppb error
> jiffies NOHZ  153000 ppb error
> pit   152533 ppb error
> pit NOHZ  0 ppb error
> acpi_pm   -127112 ppb error
> acpi_pm NOHZ  279 ppb error
> 
> So you are right, w/ pit & NO_HZ, the granularity error is always very
> small both with or without CLOCK_TICK_ADJUST. 

If you change the frequency of acpi_pm to 3579000 you'll get this:

HZ=1000 CLOCK_TICK_ADJUST=0
jiffies 153000 ppb error
jiffies NOHZ153000 ppb error
pit 152533 ppb error
pit NOHZ0 ppb error
acpi_pm 0 ppb error
acpi_pm NOHZ0 ppb error

HZ=1000 CLOCK_TICK_ADJUST=-152533
jiffies 0 ppb error
jiffies NOHZ466 ppb error
pit -467 ppb error
pit NOHZ-1 ppb error
acpi_pm 126407 ppb error
acpi_pm NOHZ22 ppb error

CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for 
jiffies). For every other clock you just add some random value, where 
it doesn't do _any_ good.
The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all 
you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't 
actually fix the error - it's still there.

> However, without CLOCK_TICK_ADJUST, the jiffies error increases for all
> values of HZ except 100 (which at first seems odd, but seems to be due
> to loss from rounding in the ACTHZ calculation).

jiffies depends on the timer resolution, so it will practically produce 
the same results as PIT (assuming it's used to generate the timer tick).

> One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the
> acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up
> being due to the acpi_pm being a very close to a multiple (3x) of the
> pit frequency, so CLOCK_TICK_ADJUST helps it as well.

What exactly does it help with?
All you are doing is number cosmetics, it has _no_ practically value and 
only decreases the quality of timekeeping.

> Further it seems to point that if we are going to be chasing down small
> sub-100ppb errors (which I think would be great to do, but lets not make
> users to endure 200+ppm errors while we debate the fine-tuning :) we
> might want to consider a method where we let ntp_update_freq take into
> account the current clocksource's interval length, so it becomes the
> base value against which we apply adjustments (scaled appropriately).

The error at least is real, the use value of CLOCK_TICK_ADJUST for the 
common case is not existent.

> There are 3 sources of error that we've discussed here:
> 1) The large (280ppm) error seen with no-NTP adjustment, caused by the
> inconsistent (A!=B) interval comparisons which started this discussion,
> which my patch does address.

Part of the error is caused by 

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 13 Feb 2008, john stultz wrote:

 Oh! So your issue is that since time_freq is stored in ppm, or in effect
 a usecs per sec offset, when we add it to something other then 1 second
 we mis-apply what NTP tells us to. Is that right?

Pretty much everything is centered around that 1sec, so the closer the 
update frequency is to it the better.

 Right, so if NTP has us apply a 10ppm adjustment, instead of doing:
   NSEC_PER_SEC + 10,000
 
 We're doing:
   NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000
 
 Which, if I'm doing my math right, results in a 10.002ppm adjustment
 (using the 999847467ns number above), instead of just a 10ppm
 adjustment.
 
 Now, true, this is an error, but it is a pretty small one. Even at the
 maximum 500ppm value, it only results in an error of 76 parts per
 billion. As you'll see below, that tends to be less error then what we
 get from the clock granularity. Is there something else I'm missing here
 or is this really the core issue you're concerned with?

The error accumulates and there is no good reason to do this for the 
common case.

  In consequence this means, if we want to improve timekeeping, we first set 
  the (update_cycles * NTP_INTERVAL_FREQ) interval as close as possible to 
  the real frequency. It doesn't has to be perfect as we usually don't know 
  the real frequency with 100% certainty anyway. 
 
 This might need some more explanation, as I'm not certain I know what
 update_cycles refers to. Do you mean cycle_interval? I guess I'm not
 completely sure how you're suggesting we change things here.

clock-cycle_interval

  Second, we drop the tick 
  adjustment if possible and leave the adjustments to the NTP daemon and as 
  long as the drift is within the 500ppm limit it has no problem to manage 
  this.
 
 Dropping the tick adjustment? By that do you mean the tick_usec value
 set by adjtimex()? I don't quite see why we want that. Could you expand
 here?

CLOCK_TICK_ADJUST

 HZ=1000 CLOCK_TICK_ADJUST=-152533
 jiffies   467 ppb error
 jiffies NOHZ  467 ppb error
 pit   0 ppb error
 pit NOHZ  0 ppb error
 acpi_pm   -280 ppb error
 acpi_pm NOHZ  279 ppb error
 
 HZ=1000 CLOCK_TICK_ADJUST=0
 jiffies   153000 ppb error
 jiffies NOHZ  153000 ppb error
 pit   152533 ppb error
 pit NOHZ  0 ppb error
 acpi_pm   -127112 ppb error
 acpi_pm NOHZ  279 ppb error
 
 So you are right, w/ pit  NO_HZ, the granularity error is always very
 small both with or without CLOCK_TICK_ADJUST. 

If you change the frequency of acpi_pm to 3579000 you'll get this:

HZ=1000 CLOCK_TICK_ADJUST=0
jiffies 153000 ppb error
jiffies NOHZ153000 ppb error
pit 152533 ppb error
pit NOHZ0 ppb error
acpi_pm 0 ppb error
acpi_pm NOHZ0 ppb error

HZ=1000 CLOCK_TICK_ADJUST=-152533
jiffies 0 ppb error
jiffies NOHZ466 ppb error
pit -467 ppb error
pit NOHZ-1 ppb error
acpi_pm 126407 ppb error
acpi_pm NOHZ22 ppb error

CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for 
jiffies). For every other clock you just add some random value, where 
it doesn't do _any_ good.
The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all 
you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't 
actually fix the error - it's still there.

 However, without CLOCK_TICK_ADJUST, the jiffies error increases for all
 values of HZ except 100 (which at first seems odd, but seems to be due
 to loss from rounding in the ACTHZ calculation).

jiffies depends on the timer resolution, so it will practically produce 
the same results as PIT (assuming it's used to generate the timer tick).

 One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the
 acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up
 being due to the acpi_pm being a very close to a multiple (3x) of the
 pit frequency, so CLOCK_TICK_ADJUST helps it as well.

What exactly does it help with?
All you are doing is number cosmetics, it has _no_ practically value and 
only decreases the quality of timekeeping.

 Further it seems to point that if we are going to be chasing down small
 sub-100ppb errors (which I think would be great to do, but lets not make
 users to endure 200+ppm errors while we debate the fine-tuning :) we
 might want to consider a method where we let ntp_update_freq take into
 account the current clocksource's interval length, so it becomes the
 base value against which we apply adjustments (scaled appropriately).

The error at least is real, the use value of CLOCK_TICK_ADJUST for the 
common case is not existent.

 There are 3 sources of error that we've discussed here:
 1) The large (280ppm) error seen with no-NTP adjustment, caused by the
 inconsistent (A!=B) interval comparisons which started this discussion,
 which my patch does address.

Part of the error is caused by CLOCK_TICK_ADJUST, but the other part of 
the error is real, all you do is hiding 

Re: Question on timekeeping subsystem

[Roman Zippel]

Hi,

On Wednesday 13. February 2008, Francis Moreau wrote:

> First I tried to find some documentation on the current implementation
> but haven't found any thing really usefull. Specially there's nothing about
> it in Documentation/ directory. Please correct me if I'm already wrong.
>
> Actually I read the implementation of update_wall_time() and I really fail
> to understand how it works. This is probably because I don't know
> what "xtime_nsec" and "error" fields in clocksource struct are for.
> These fields are not documented anywhere in the source code so it
> should be obvious but unfortunately not for me.

These mails should help to understand, what this code does:

http://lkml.org/lkml/2006/3/4/61
http://lkml.org/lkml/2006/4/3/205

bye, Roman
--
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/

Re: distributed module configuration [Was: Announce: Linux-next (Or Andrew's dream :-))]

[Roman Zippel]

Hi,

On Wednesday 13. February 2008, Sam Ravnborg wrote:

> config foo
>   tristate "do you want foo?"
>   depends on USB && BAR
>   module
> obj-$(CONFIG_FOO) += foo.o
> foo-y := file1.o file2.o
>   help
> foo will allow you to explode your PC

I'm more thinking about something like this:

module foo [FOO]
tristate "do you want foo?"
depends on USB && BAR
source file1.c
source file2.c if BAZ

Avoiding direct Makefile fragments would give us far more flexibility in the 
final Makefile output.

> And we could introduce support for
>
> source "drivers/net/Kconfig.*"
>
> But then we would have to make the kconfig step mandatory
> for each build as we would otherwise not know if there
> were added any Kconfig files.

That's a real problem and it would be a step back of what we have right now, 
so I'm not exactly comfortable with it.

bye, Roman
--
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/

Re: Question on timekeeping subsystem

[Roman Zippel]

Hi,

On Wednesday 13. February 2008, Francis Moreau wrote:

 First I tried to find some documentation on the current implementation
 but haven't found any thing really usefull. Specially there's nothing about
 it in Documentation/ directory. Please correct me if I'm already wrong.

 Actually I read the implementation of update_wall_time() and I really fail
 to understand how it works. This is probably because I don't know
 what xtime_nsec and error fields in clocksource struct are for.
 These fields are not documented anywhere in the source code so it
 should be obvious but unfortunately not for me.

These mails should help to understand, what this code does:

http://lkml.org/lkml/2006/3/4/61
http://lkml.org/lkml/2006/4/3/205

bye, Roman
--
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/

Re: distributed module configuration [Was: Announce: Linux-next (Or Andrew's dream :-))]

[Roman Zippel]

Hi,

On Wednesday 13. February 2008, Sam Ravnborg wrote:

 config foo
   tristate do you want foo?
   depends on USB  BAR
   module
 obj-$(CONFIG_FOO) += foo.o
 foo-y := file1.o file2.o
   help
 foo will allow you to explode your PC

I'm more thinking about something like this:

module foo [FOO]
tristate do you want foo?
depends on USB  BAR
source file1.c
source file2.c if BAZ

Avoiding direct Makefile fragments would give us far more flexibility in the 
final Makefile output.

 And we could introduce support for

 source drivers/net/Kconfig.*

 But then we would have to make the kconfig step mandatory
 for each build as we would otherwise not know if there
 were added any Kconfig files.

That's a real problem and it would be a step back of what we have right now, 
so I'm not exactly comfortable with it.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 11 Feb 2008, john stultz wrote:

> > I don't want to just send a patch, I want you to understand why your 
> > approach is wrong.
> 
> With all due respect, it also keeps the critique in one direction and
> makes your review less collaborative and more confrontational then I
> suspect (or maybe just hope) you intend.

I don't think that's necessarily a contradiction, if we keep it to 
confronting the problem. A simple patch wouldn't have provided any further 
understanding of the problem compared to what I already said. You would 
have seen what the patch does (which I described already differently), but 
not really why it does that.

In this sense I prefer to force the confrontation of the problem. I'm 
afraid a working patch would encourage to simply ignore the problem, as 
your problem at hand would be solved without completely understanding it.
The point is that I'd really like you to understand the problem, so I'm 
not the only one who understands this code :) and in the end it might 
allow better collaboration to further improve this code.

To make it very clear this is just about understanding the problem, I 
don't want to force a specific solution (which a patch would practically 
do). If we both understand the problem, we can also discuss the solution 
and maybe we find something better, but maybe I'm also totally wrong, 
which would be a little embarrassing :), but that would be fine too. There 
may be better ways to go about this problem, but IMO it would still be 
better than just ignoring the problem and force it with a patch.

> This fine grained error accounting is where the bug I'm trying to
> address is cropping up from. In order to have the comparison we need to
> have two values:
>  A: The clocksource's notion of how long the fixed interval is.
>  B: NTP's notion of how long the fixed interval is.
> 
> When no NTP adjustment is being made, these two values should be equal,
> but currently they are not. This is what causes the 280ppm error seen on
> my test system.
> 
> Part A is calculated in the following fashion:
>   #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
> 
>   Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the
> course of this discussion, lets ignore that.
> 
> Part B is calculated in ntp_update_frequency() as:
> 
>   u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
>   << TICK_LENGTH_SHIFT;
>   second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
>   second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
> 
>   tick_length_base = second_length;
>   do_div(tick_length_base, NTP_INTERVAL_FREQ);
> 
> 
> If we're assuming there is no NTP adjustment, and avoiding the
> TICK_LENGTH_SHIFT, this can be shorted to:
>   B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ)
>   + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ
> 
> 
> The A vs B comparison can be shortened further to:
>   NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ)
>   + CLOCK_TICK_ADJUST
> 
> So now on to what seems to be the point of contention:
>   If A != B, which side do we fix?
> 
> 
> My patches fix the A side so that it matches B, which on its face isn't
> terribly complicated, but you seem to be suggesting we fix the B side
> instead (Now I'm assuming here, because there's no patch. So I can only
> speak to your emails, which were not clear to me).

If we go from your base assumption above "there is no NTP adjustment", I 
would actually agree with you and it wouldn't matter much on which side 
to correct the equation.

The question is now what happens, if there are NTP adjustments, i.e. when 
the time_freq value isn't zero. Based on this initialization we tell the 
NTP daemon the base frequency, although not directly but it knows the 
length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon 
tells the kernel via time_freq how to change the speed so that freq1 == 
1 sec + time_freq.

The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we 
don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + 
tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec 
+ time_freq. Above initialization now calcalutes the base time length for 
an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested 
time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to 
freq2 cycles, so this finally means any adjustment made by the NTP daemon 
is slightly off.

To demonstrate this let's look at some real values and let's use the PIT 
for it (as this is where this originated and on which CLOCK_TICK_ADJUST is 
based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 
cycles and the actual update frequency is freq2=1193000. To adjust for 
this difference we change the length of a timer tick:

(NSEC_PER_SEC + CLOCK_TICK_ADJUST) / 

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 11 Feb 2008, john stultz wrote:

  I don't want to just send a patch, I want you to understand why your 
  approach is wrong.
 
 With all due respect, it also keeps the critique in one direction and
 makes your review less collaborative and more confrontational then I
 suspect (or maybe just hope) you intend.

I don't think that's necessarily a contradiction, if we keep it to 
confronting the problem. A simple patch wouldn't have provided any further 
understanding of the problem compared to what I already said. You would 
have seen what the patch does (which I described already differently), but 
not really why it does that.

In this sense I prefer to force the confrontation of the problem. I'm 
afraid a working patch would encourage to simply ignore the problem, as 
your problem at hand would be solved without completely understanding it.
The point is that I'd really like you to understand the problem, so I'm 
not the only one who understands this code :) and in the end it might 
allow better collaboration to further improve this code.

To make it very clear this is just about understanding the problem, I 
don't want to force a specific solution (which a patch would practically 
do). If we both understand the problem, we can also discuss the solution 
and maybe we find something better, but maybe I'm also totally wrong, 
which would be a little embarrassing :), but that would be fine too. There 
may be better ways to go about this problem, but IMO it would still be 
better than just ignoring the problem and force it with a patch.

 This fine grained error accounting is where the bug I'm trying to
 address is cropping up from. In order to have the comparison we need to
 have two values:
  A: The clocksource's notion of how long the fixed interval is.
  B: NTP's notion of how long the fixed interval is.
 
 When no NTP adjustment is being made, these two values should be equal,
 but currently they are not. This is what causes the 280ppm error seen on
 my test system.
 
 Part A is calculated in the following fashion:
   #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
   Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the
 course of this discussion, lets ignore that.
 
 Part B is calculated in ntp_update_frequency() as:
 
   u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
TICK_LENGTH_SHIFT;
   second_length += (s64)CLOCK_TICK_ADJUST  TICK_LENGTH_SHIFT;
   second_length += (s64)time_freq  (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
   tick_length_base = second_length;
   do_div(tick_length_base, NTP_INTERVAL_FREQ);
 
 
 If we're assuming there is no NTP adjustment, and avoiding the
 TICK_LENGTH_SHIFT, this can be shorted to:
   B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ)
   + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ
 
 
 The A vs B comparison can be shortened further to:
   NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ)
   + CLOCK_TICK_ADJUST
 
 So now on to what seems to be the point of contention:
   If A != B, which side do we fix?
 
 
 My patches fix the A side so that it matches B, which on its face isn't
 terribly complicated, but you seem to be suggesting we fix the B side
 instead (Now I'm assuming here, because there's no patch. So I can only
 speak to your emails, which were not clear to me).

If we go from your base assumption above there is no NTP adjustment, I 
would actually agree with you and it wouldn't matter much on which side 
to correct the equation.

The question is now what happens, if there are NTP adjustments, i.e. when 
the time_freq value isn't zero. Based on this initialization we tell the 
NTP daemon the base frequency, although not directly but it knows the 
length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon 
tells the kernel via time_freq how to change the speed so that freq1 == 
1 sec + time_freq.

The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we 
don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + 
tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec 
+ time_freq. Above initialization now calcalutes the base time length for 
an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested 
time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to 
freq2 cycles, so this finally means any adjustment made by the NTP daemon 
is slightly off.

To demonstrate this let's look at some real values and let's use the PIT 
for it (as this is where this originated and on which CLOCK_TICK_ADJUST is 
based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 
cycles and the actual update frequency is freq2=1193000. To adjust for 
this difference we change the length of a timer tick:

(NSEC_PER_SEC + CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ

or

(10^9 nsec - 152533 nsec) / 1000

This way 

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, john stultz wrote:

> -ENOPATCH
> 
> We're taking weeks to critique fairly small bug fix. I'm sure we both
> have better things to do then continue to misunderstand each other. I'll
> do the testing and will happily ack it if it resolves the issue.

I don't want to just send a patch, I want you to understand why your 
approach is wrong.

> Now, If you're disputing that I'm correcting the wrong side of the
> equation, then we're getting somewhere. But its still not clear to me
> how you're suggesting the other side (which is calculated in
> ntp_update_frequency) be changed.
> [..]
> You keep on bringing up NO_HZ, and again, the bug I'm trying to fix
> occurs with or without NO_HZ. The fix I proposed resolves the issue with
> or without NO_HZ.

The correction is incorrect for NO_HZ.
Let's try it the other way around, as my explanation seem to lack 
something.
Please try to explain what this correction actually means and why it 
should be correct for NO_HZ as well.

> > The only other alternative would be to calculate this correction 
> > dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when 
> > changing clocks you check whether "abs(((cs->xtime_interval * 
> > NTP_INTERVAL_FREQ) >> cs->shift) - NSEC_PER_SEC)" exceeds a certain limit 
> > (e.g. 200usec) and in this case you print a warning message, that the 
> > clock has large base drift value and is a bad ntp source and apply a 
> > correction value. This way the correction only hits the very few system 
> > which might need it and it would be the prefered solution, but it also 
> > requires a few more changes.
> 
> Uh, that seems to be just checking if the xtime_interval is off base, or
> if the ntp correction has gone too far. I just don't see how this
> connects to the issue at hand.

Above is the key to understanding the problem, if this difference is small 
enough there is no need to correct anything.

This is the original patch which introduced the correction:

http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860

Keep in mind that at that time PIT was used as the base clock (even if the 
tsc was used, it was relative to PIT). So how much of those assumptions 
are still valid today (especially with NO_HZ enabled)?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, john stultz wrote:

 -ENOPATCH
 
 We're taking weeks to critique fairly small bug fix. I'm sure we both
 have better things to do then continue to misunderstand each other. I'll
 do the testing and will happily ack it if it resolves the issue.

I don't want to just send a patch, I want you to understand why your 
approach is wrong.

 Now, If you're disputing that I'm correcting the wrong side of the
 equation, then we're getting somewhere. But its still not clear to me
 how you're suggesting the other side (which is calculated in
 ntp_update_frequency) be changed.
 [..]
 You keep on bringing up NO_HZ, and again, the bug I'm trying to fix
 occurs with or without NO_HZ. The fix I proposed resolves the issue with
 or without NO_HZ.

The correction is incorrect for NO_HZ.
Let's try it the other way around, as my explanation seem to lack 
something.
Please try to explain what this correction actually means and why it 
should be correct for NO_HZ as well.

  The only other alternative would be to calculate this correction 
  dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when 
  changing clocks you check whether abs(((cs-xtime_interval * 
  NTP_INTERVAL_FREQ)  cs-shift) - NSEC_PER_SEC) exceeds a certain limit 
  (e.g. 200usec) and in this case you print a warning message, that the 
  clock has large base drift value and is a bad ntp source and apply a 
  correction value. This way the correction only hits the very few system 
  which might need it and it would be the prefered solution, but it also 
  requires a few more changes.
 
 Uh, that seems to be just checking if the xtime_interval is off base, or
 if the ntp correction has gone too far. I just don't see how this
 connects to the issue at hand.

Above is the key to understanding the problem, if this difference is small 
enough there is no need to correct anything.

This is the original patch which introduced the correction:

http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860

Keep in mind that at that time PIT was used as the base clock (even if the 
tsc was used, it was relative to PIT). So how much of those assumptions 
are still valid today (especially with NO_HZ enabled)?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, Andrew Morton wrote:

> > Only now I noticed that the first patch had been merged without any 
> > further question. :-(
> > What point is there in reviewing patches, if everything is merged anyway. 
> > :-(
> > 
> 
> oops, mistake, sorry.  There's plenty of time to fix it though.

It has been signed off by both Ingo and Thomas and neither noticed 
anything? This makes me very afraid of the merging process...

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, Andrew Morton wrote:

  Only now I noticed that the first patch had been merged without any 
  further question. :-(
  What point is there in reviewing patches, if everything is merged anyway. 
  :-(
  
 
 oops, mistake, sorry.  There's plenty of time to fix it though.

It has been signed off by both Ingo and Thomas and neither noticed 
anything? This makes me very afraid of the merging process...

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, john stultz wrote:

>  
>   clock = clocksource_get_next();
> - clocksource_calculate_interval(clock,
> - (unsigned long)(current_tick_length()>>TICK_LENGTH_SHIFT));
> + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
>   clock->cycle_last = clocksource_read(clock);
>  

Only now I noticed that the first patch had been merged without any 
further question. :-(
What point is there in reviewing patches, if everything is merged anyway. :-(

bye, Roman
--
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/

Re: [patch 01/26] mount options: add documentation

[Roman Zippel]

Hi,

On Wed, 30 Jan 2008, Miklos Szeredi wrote:

> > How does this deal with certain special cases:
> > - chroot: how will mount/df only show the for chroot relevant mounts?
> 
> That is a very good question.  Andreas Gruenbacher had some patches
> for fixing behavior of /proc/mounts under a chroot, but people are
> paranoid about userspace ABI changes (unwarranted in this case, IMO).
> 
>   http://lkml.org/lkml/2007/4/20/147
> 
> Anyway, if we are going to have a new 'mountinfo' file, this could be
> easily fixed as well.
> 
> > - loop: how is the connection between file and loop device maintained?
> 
> We also discussed this with Karel, maybe it didn't make it onto lkml.
> 
> The proposed solution was to store the "loop" flag separately in a
> file under /var.  It could just be an empty file for each such loop
> device:
> 
>   /var/lib/mount/loops/loop0
> 
> This file is created by mount(8) if the '-oloop' option is given.  And
> umount(8) automatically tears down the loop device if it finds this
> file.

My question was maybe a little short. I don't doubt that we can shove a 
lot into the kernel, the question is rather how much of this will be 
unnecessary information, which the kernel doesn't really need itself.

> > Could also please explain why you want to go via user mounts. Other OS use 
> > a 
> > daemon for that, which e.g. can maintain access controls. How do you want 
> > to 
> > manage this?
> 
> The unprivileged mounts patches do contain a simple form of access
> control.  I don't think anything more is needed, but of course, having
> unprivileged mounts in the kernel does not prevent the use of a more
> sophisticated access control daemon in userspace, if that becomes
> necessary.

A "I don't think anything more is needed" lets go off all sorts of warning 
lights. Most things start out simple, so IMO it's very worth it to check 
where it might go to to know the limits beforehand. The main question here 
is why should a kernel based solution be preferable over a daemon based 
solution?

If we look for example look at OS X, it has no need for user mounts but 
has a daemon instead, which also provides an interesting notification 
system for new devices, mounts or unmount requests. All this could also be 
done in the kernel, but where would be the advantage in doing so? The 
kernel implementation would be either rather limited or only bloat the 
kernel. What is the feature that would make user mounts more than just a 
cool kernel hack?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 1 Feb 2008, John Stultz wrote:

> > CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't 
> > based on HZ, there is no point in using it!
> 
> Hey Roman,
> 
> Again, I'm sorry I don't seem to be following your objections. If you
> want to suggest a different patch to fix the issue, it might help.

I already gave you the necessary details for how to set 
NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. 
I really don't understand what's your problem with it. Why do you try to 
make it more complex than necessary?

> The big issue for me, is that we have to initialize the clocksource
> cycle interval so it matches the base tick_length that NTP uses.
> 
> To be clear, the issue I'm trying to address is only this:
> Assuming there is no NTP adjustment yet to be made, if we initialize the
> clocksource interval to X, then compare it with Y when we accumulate, we
> introduce error if X and Y are not the same.
> 
> It really doesn't matter how long the length is, if we're including
> CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or
> not. The issue is that we have to be consistent. If we're not, then we
> introduce error that ntpd has to additionally correct for.

You don't create consistency by adding corrections all over the place 
until it adds up to the right sum.
The current correction is already somewhat of a hack and I'd rather get 
rid of it than to let it spread all over the place (it's really only 
needed so that people with weird HZ settings don't hit the 500ppm limit 
and we're basically cheating to the ntpd by not telling it the real 
frequency). Please keep the knowledge about this crutch at a single place 
and don't spread it.
Anyway, for NO_HZ this correction is completely irrelevant, so again 
there's no point in adding random values all over the place until you get 
the correct result.

The only other alternative would be to calculate this correction 
dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when 
changing clocks you check whether "abs(((cs->xtime_interval * 
NTP_INTERVAL_FREQ) >> cs->shift) - NSEC_PER_SEC)" exceeds a certain limit 
(e.g. 200usec) and in this case you print a warning message, that the 
clock has large base drift value and is a bad ntp source and apply a 
correction value. This way the correction only hits the very few system 
which might need it and it would be the prefered solution, but it also 
requires a few more changes.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 8 Feb 2008, john stultz wrote:

  
   clock = clocksource_get_next();
 - clocksource_calculate_interval(clock,
 - (unsigned long)(current_tick_length()TICK_LENGTH_SHIFT));
 + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
   clock-cycle_last = clocksource_read(clock);
  

Only now I noticed that the first patch had been merged without any 
further question. :-(
What point is there in reviewing patches, if everything is merged anyway. :-(

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Fri, 1 Feb 2008, John Stultz wrote:

  CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't 
  based on HZ, there is no point in using it!
 
 Hey Roman,
 
 Again, I'm sorry I don't seem to be following your objections. If you
 want to suggest a different patch to fix the issue, it might help.

I already gave you the necessary details for how to set 
NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. 
I really don't understand what's your problem with it. Why do you try to 
make it more complex than necessary?

 The big issue for me, is that we have to initialize the clocksource
 cycle interval so it matches the base tick_length that NTP uses.
 
 To be clear, the issue I'm trying to address is only this:
 Assuming there is no NTP adjustment yet to be made, if we initialize the
 clocksource interval to X, then compare it with Y when we accumulate, we
 introduce error if X and Y are not the same.
 
 It really doesn't matter how long the length is, if we're including
 CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or
 not. The issue is that we have to be consistent. If we're not, then we
 introduce error that ntpd has to additionally correct for.

You don't create consistency by adding corrections all over the place 
until it adds up to the right sum.
The current correction is already somewhat of a hack and I'd rather get 
rid of it than to let it spread all over the place (it's really only 
needed so that people with weird HZ settings don't hit the 500ppm limit 
and we're basically cheating to the ntpd by not telling it the real 
frequency). Please keep the knowledge about this crutch at a single place 
and don't spread it.
Anyway, for NO_HZ this correction is completely irrelevant, so again 
there's no point in adding random values all over the place until you get 
the correct result.

The only other alternative would be to calculate this correction 
dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when 
changing clocks you check whether abs(((cs-xtime_interval * 
NTP_INTERVAL_FREQ)  cs-shift) - NSEC_PER_SEC) exceeds a certain limit 
(e.g. 200usec) and in this case you print a warning message, that the 
clock has large base drift value and is a bad ntp source and apply a 
correction value. This way the correction only hits the very few system 
which might need it and it would be the prefered solution, but it also 
requires a few more changes.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 30 Jan 2008, john stultz wrote:

> My concern is we state the accumulation interval is X ns long. Then
> current_tick_length() is to return (X + ntp_adjustment), so each
> accumulation interval we can keep track of the error and adjust our
> interval length.
> 
> So if ntp_update_frequency() sets tick_length_base to be:
> 
>   u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
>   << TICK_LENGTH_SHIFT;
>   second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
>   second_length += (s64)time_freq
>   << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
> 
>   tick_length_base = second_length;
>   do_div(tick_length_base, NTP_INTERVAL_FREQ);
> 
> 
> The above is basically (X + part of ntp_adjustment)

CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't 
based on HZ, there is no point in using it!

Let's look at what actually needs to be done:

1. initializing clock interval:

clock_cycle_interval = timer_cycle_interval * clock_frequency / 
timer_frequency

It's simply about converting timer cycles into clock cycles, so they're 
about the same time interval.
We already make it a bit more complicated than necessary as we go via 
nsec:

ntp_interval = timer_cycle_interval * 10^9nsec / timer_frequency

and in clocksource_calculate_interval() basically:

clock_cycle_interval = ntp_interval * clock_frequency / 10^9nsec

Without a fixed timer tick it's actually even easier, then we use the same 
frequency for clock and timer and the cycle interval is simply:

clock_cycle_interval = timer_cycle_interval = clock_frequency / 
NTP_INTERVAL_FREQ

There is no need to use the adjustment here, you'll only cause a mismatch 
between the clock and timer cycle interval, which had to be corrected by 
NTP.

2. initializing clock adjustment:

clock_adjust = timer_cycle_interval * NTP_INTERVAL_FREQ / 
timer_frequency - 1sec

This adjustment is used make up for the difference that the timer 
frequency isn't evenly divisible by HZ, so that the clock is advanced by 
1sec after timer_frequency cycles.

Like above the clock frequency is used for the timer frequency for this 
calculation for CONFIG_NO_HZ, so it would be incorrect to use 
CLOCK_TICK_RATE/LATCH/HZ here and since NTP_INTERVAL_FREQ is quite small 
the resulting adjustment would be rather small, it's easier not to bother 
in this case.

What you're basically trying is to add an error to the clock 
initialization, so that we can later compensate for it. The correct 
solution is really to not add the error in first place, so that there is 
no need to compensate for it.

bye. Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Tue, 29 Jan 2008, john stultz wrote:

> +/* Because using NSEC_PER_SEC would be too easy */
> +#define NTP_INTERVAL_LENGTH 
> s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ)

Why are you using USER_HZ? Did you test this with HZ!=100?
Anyway, please don't make more complicated than it already is.
What I said previously about the update interval is still valid, so the 
correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation 
from my last mail and to omit the correction for NO_HZ.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Tue, 29 Jan 2008, john stultz wrote:

 +/* Because using NSEC_PER_SEC would be too easy */
 +#define NTP_INTERVAL_LENGTH 
 s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ)

Why are you using USER_HZ? Did you test this with HZ!=100?
Anyway, please don't make more complicated than it already is.
What I said previously about the update interval is still valid, so the 
correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation 
from my last mail and to omit the correction for NO_HZ.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 30 Jan 2008, john stultz wrote:

 My concern is we state the accumulation interval is X ns long. Then
 current_tick_length() is to return (X + ntp_adjustment), so each
 accumulation interval we can keep track of the error and adjust our
 interval length.
 
 So if ntp_update_frequency() sets tick_length_base to be:
 
   u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
TICK_LENGTH_SHIFT;
   second_length += (s64)CLOCK_TICK_ADJUST  TICK_LENGTH_SHIFT;
   second_length += (s64)time_freq
(TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
   tick_length_base = second_length;
   do_div(tick_length_base, NTP_INTERVAL_FREQ);
 
 
 The above is basically (X + part of ntp_adjustment)

CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't 
based on HZ, there is no point in using it!

Let's look at what actually needs to be done:

1. initializing clock interval:

clock_cycle_interval = timer_cycle_interval * clock_frequency / 
timer_frequency

It's simply about converting timer cycles into clock cycles, so they're 
about the same time interval.
We already make it a bit more complicated than necessary as we go via 
nsec:

ntp_interval = timer_cycle_interval * 10^9nsec / timer_frequency

and in clocksource_calculate_interval() basically:

clock_cycle_interval = ntp_interval * clock_frequency / 10^9nsec

Without a fixed timer tick it's actually even easier, then we use the same 
frequency for clock and timer and the cycle interval is simply:

clock_cycle_interval = timer_cycle_interval = clock_frequency / 
NTP_INTERVAL_FREQ

There is no need to use the adjustment here, you'll only cause a mismatch 
between the clock and timer cycle interval, which had to be corrected by 
NTP.

2. initializing clock adjustment:

clock_adjust = timer_cycle_interval * NTP_INTERVAL_FREQ / 
timer_frequency - 1sec

This adjustment is used make up for the difference that the timer 
frequency isn't evenly divisible by HZ, so that the clock is advanced by 
1sec after timer_frequency cycles.

Like above the clock frequency is used for the timer frequency for this 
calculation for CONFIG_NO_HZ, so it would be incorrect to use 
CLOCK_TICK_RATE/LATCH/HZ here and since NTP_INTERVAL_FREQ is quite small 
the resulting adjustment would be rather small, it's easier not to bother 
in this case.

What you're basically trying is to add an error to the clock 
initialization, so that we can later compensate for it. The correct 
solution is really to not add the error in first place, so that there is 
no need to compensate for it.

bye. Roman
--
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/

Re: [patch 01/26] mount options: add documentation

[Roman Zippel]

Hi,

On Thursday 24. January 2008, Miklos Szeredi wrote:

> Q: Why do we need correct option showing in /proc/mounts?
> A: We want /proc/mounts to fully replace /etc/mtab.  The reasons for
>this are:
> - unprivileged mounters won't be able to update /etc/mtab
> - /etc/mtab doesn't work with private mount namespaces
> - /etc/mtab can become out-of-sync with reality

I asked this before and I don't remember getting an answer:
How does this deal with certain special cases:
- chroot: how will mount/df only show the for chroot relevant mounts?
- loop: how is the connection between file and loop device maintained?

I don't quite see how you want to achieve a _full_ replacement.

Could also please explain why you want to go via user mounts. Other OS use a 
daemon for that, which e.g. can maintain access controls. How do you want to 
manage this?

bye, Roman
--
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/

Re: [patch 01/26] mount options: add documentation

[Roman Zippel]

Hi,

On Thursday 24. January 2008, Miklos Szeredi wrote:

 Q: Why do we need correct option showing in /proc/mounts?
 A: We want /proc/mounts to fully replace /etc/mtab.  The reasons for
this are:
 - unprivileged mounters won't be able to update /etc/mtab
 - /etc/mtab doesn't work with private mount namespaces
 - /etc/mtab can become out-of-sync with reality

I asked this before and I don't remember getting an answer:
How does this deal with certain special cases:
- chroot: how will mount/df only show the for chroot relevant mounts?
- loop: how is the connection between file and loop device maintained?

I don't quite see how you want to achieve a _full_ replacement.

Could also please explain why you want to go via user mounts. Other OS use a 
daemon for that, which e.g. can maintain access controls. How do you want to 
manage this?

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 28 Jan 2008, john stultz wrote:

> Regardless, current_tick_length() really is the base interval we're
> using in the error accumulation loop, so it seems the cleanest interface
> to use (just to avoid redundancy at least) when establishing the
> clocksource's interval length. Or do you not agree?

I see, what you need to use in timex.h for !CONFIG_NO_HZ is:

#define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) / (s64)CLOCK_TICK_RATE)

this calculates the base length of a clock tick in nsec.

current_tick_length() would only work during boot. If you switch clocks 
later, it would include random adjustments specific to the old clock.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Mon, 28 Jan 2008, john stultz wrote:

 Regardless, current_tick_length() really is the base interval we're
 using in the error accumulation loop, so it seems the cleanest interface
 to use (just to avoid redundancy at least) when establishing the
 clocksource's interval length. Or do you not agree?

I see, what you need to use in timex.h for !CONFIG_NO_HZ is:

#define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) / (s64)CLOCK_TICK_RATE)

this calculates the base length of a clock tick in nsec.

current_tick_length() would only work during boot. If you switch clocks 
later, it would include random adjustments specific to the old clock.

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 23 Jan 2008, john stultz wrote:

> This difference in calculation was causing the clocksource correction
> code to apply a correction factor to the clocksource so the two
> intervals were the same, however this results in the actual frequency of
> the clocksource to be made incorrect. I believe this difference would
> affect all clocksources, although to differing degrees depending on the
> clocksource resolution.

Let's look at why the correction is done in first place. The update steps 
don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small 
addjustment is used to make up for it. The problem here is that if the 
update frequency changes, the addjustment isn't correct anymore.
The simple fix is to just omit the addjustment in these cases in ntp.c:

#if NTP_INTERVAL_FREQ == HZ
...
#else
#define CLOCK_TICK_ADJUST   0
#endif

bye, Roman
--
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/

Re: [PATCH] correct inconsistent ntp interval/tick_length usage

[Roman Zippel]

Hi,

On Wed, 23 Jan 2008, john stultz wrote:

 This difference in calculation was causing the clocksource correction
 code to apply a correction factor to the clocksource so the two
 intervals were the same, however this results in the actual frequency of
 the clocksource to be made incorrect. I believe this difference would
 affect all clocksources, although to differing degrees depending on the
 clocksource resolution.

Let's look at why the correction is done in first place. The update steps 
don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small 
addjustment is used to make up for it. The problem here is that if the 
update frequency changes, the addjustment isn't correct anymore.
The simple fix is to just omit the addjustment in these cases in ntp.c:

#if NTP_INTERVAL_FREQ == HZ
...
#else
#define CLOCK_TICK_ADJUST   0
#endif

bye, Roman
--
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/

Re: non-choice related config entries within choice

[Roman Zippel]

Hi,

On Wed, 16 Jan 2008, Sam Ravnborg wrote:

> But one feature I really would like to see is named chocies so we can do 
> stuff like:
> 
> choice X86_PROCESSOR
> 
> config GENERIC_PROCESSOR
>   bool "A generic X86 processor"
> endchoice
> 
> 
> ...
> 
> choice PPC_PROCESSOR
> 
> config GENERIC_PROCESSOR
>   bool "A generic PowerPC processor
> 
> endchoice
> 
> The issue here is that we do not today allow the same config option
> to appear if more than one choice.

What I have in mind is slightly different, above choices would simply be 
called PROCESSOR, which would tell kconfig that all choices belong to the 
same group.

bye, Roman
--
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/

Re: non-choice related config entries within choice

[Roman Zippel]

Hi,

On Wed, 16 Jan 2008, Jan Beulich wrote:

> now that I finally found time to look into the problems that caused the
> patch changing boolean/tristate choice behavior to be reverted I find
> that due to the way things worked in the past there are a couple of
> cases where config options not really belonging to the choice are inside
> the choice scope (drivers/usb/gadget/Kconfig, arch/ppc/Kconfig, and
> arch/mips/Kconfig are where I found such cases, and I hope this is a
> complete list).
> 
> The question is: Is it intended for this to work the way it used to, or
> is it rather reasonable to change these scripts so that stuff dependent
> upon the choice selection is being dealt with outside the choice scope?

This is really a feature, try it with a visible option there which depends 
on a choice option.
First for the choice type I think it's simpler to just look at the first 
choice option, anything more complex simply has to specify the type 
explicitly.

The bigger problem is that menu_finalize() is little complex which makes 
such changes more difficult, basically it does two things (updating the 
dependencies and generating the menu structure) in one pass and it depends 
on a specific order, which is nonobvious. I really should clean this up to 
make it easier to follow what's happening.
For now this means the dependency to the choice symbol has to be added a 
little later right before the call to menu_add_symbol().

bye, Roman
--
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/

[PATCH 2/3] environment symbol support

[Roman Zippel]

Add the possibility to import a value from the environment into kconfig
via the option syntax. Beside flexibility this has the advantage
providing proper dependencies.

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 Documentation/kbuild/kconfig-language.txt |   21 ++
 scripts/kconfig/expr.h|3 +-
 scripts/kconfig/lkc.h |5 +++
 scripts/kconfig/menu.c|5 ++-
 scripts/kconfig/qconf.cc  |   16 +++---
 scripts/kconfig/symbol.c  |   45 ++
 scripts/kconfig/util.c|   23 ++-
 scripts/kconfig/zconf.gperf   |1 
 scripts/kconfig/zconf.hash.c_shipped  |   45 --
 9 files changed, 129 insertions(+), 35 deletions(-)

Index: linux-2.6/Documentation/kbuild/kconfig-language.txt
===
--- linux-2.6.orig/Documentation/kbuild/kconfig-language.txt
+++ linux-2.6/Documentation/kbuild/kconfig-language.txt
@@ -127,6 +127,27 @@ applicable everywhere (see syntax).
   used to help visually separate configuration logic from help within
   the file as an aid to developers.
 
+- misc options: "option" [=]
+  Various less common options can be defined via this option syntax,
+  which can modify the behaviour of the menu entry and its config
+  symbol. These options are currently possible:
+
+  - "defconfig_list"
+This declares a list of default entries which can be used when
+looking for the default configuration (which is used when the main
+.config doesn't exists yet.)
+
+  - "modules"
+This declares the symbol to be used as the MODULES symbol, which
+enables the third modular state for all config symbols.
+
+  - "env"=
+This imports the environment variable into Kconfig. It behaves like
+a default, except that the value comes from the environment, this
+also means that the behaviour when mixing it with normal defaults is
+undefined at this point. The symbol is currently not exported back
+to the build environment (if this is desired, it can be done via
+another symbol).
 
 Menu dependencies
 -
Index: linux-2.6/scripts/kconfig/expr.h
===
--- linux-2.6.orig/scripts/kconfig/expr.h
+++ linux-2.6/scripts/kconfig/expr.h
@@ -108,7 +108,8 @@ struct symbol {
 #define SYMBOL_HASHMASK0xff
 
 enum prop_type {
-   P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE, P_SELECT, 
P_RANGE
+   P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE,
+   P_SELECT, P_RANGE, P_ENV
 };
 
 struct property {
Index: linux-2.6/scripts/kconfig/lkc.h
===
--- linux-2.6.orig/scripts/kconfig/lkc.h
+++ linux-2.6/scripts/kconfig/lkc.h
@@ -44,6 +44,7 @@ extern "C" {
 
 #define T_OPT_MODULES  1
 #define T_OPT_DEFCONFIG_LIST   2
+#define T_OPT_ENV  3
 
 struct kconf_id {
int name;
@@ -74,6 +75,7 @@ void kconfig_load(void);
 
 /* menu.c */
 void menu_init(void);
+void menu_warn(struct menu *menu, const char *fmt, ...);
 struct menu *menu_add_menu(void);
 void menu_end_menu(void);
 void menu_add_entry(struct symbol *sym);
@@ -103,6 +105,8 @@ void str_printf(struct gstr *gs, const c
 const char *str_get(struct gstr *gs);
 
 /* symbol.c */
+extern struct expr *sym_env_list;
+
 void sym_init(void);
 void sym_clear_all_valid(void);
 void sym_set_all_changed(void);
@@ -110,6 +114,7 @@ void sym_set_changed(struct symbol *sym)
 struct symbol *sym_check_deps(struct symbol *sym);
 struct property *prop_alloc(enum prop_type type, struct symbol *sym);
 struct symbol *prop_get_symbol(struct property *prop);
+struct property *sym_get_env_prop(struct symbol *sym);
 
 static inline tristate sym_get_tristate_value(struct symbol *sym)
 {
Index: linux-2.6/scripts/kconfig/menu.c
===
--- linux-2.6.orig/scripts/kconfig/menu.c
+++ linux-2.6/scripts/kconfig/menu.c
@@ -15,7 +15,7 @@ static struct menu **last_entry_ptr;
 struct file *file_list;
 struct file *current_file;
 
-static void menu_warn(struct menu *menu, const char *fmt, ...)
+void menu_warn(struct menu *menu, const char *fmt, ...)
 {
va_list ap;
va_start(ap, fmt);
@@ -172,6 +172,9 @@ void menu_add_option(int token, char *ar
else if (sym_defconfig_list != current_entry->sym)
zconf_error("trying to redefine defconfig symbol");
break;
+   case T_OPT_ENV:
+   prop_add_env(arg);
+   break;
}
 }
 
Index: linux-2.6/scripts/kconfig/qconf.cc
===
--- linux-2.6.orig/scripts/kconfig/qconf.cc
+++ linux-2.6/s

[PATCH 3/3] use environment option

[Roman Zippel]

Use the environment option to provide the ARCH symbol.
Remove the unused KERNELVERSION symbol.

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 init/Kconfig |4 
 scripts/kconfig/symbol.c |   14 --
 2 files changed, 4 insertions(+), 14 deletions(-)

Index: linux-2.6/init/Kconfig
===
--- linux-2.6.orig/init/Kconfig
+++ linux-2.6/init/Kconfig
@@ -1,3 +1,7 @@
+config ARCH
+   string
+   option env="ARCH"
+
 config DEFCONFIG_LIST
string
depends on !UML
Index: linux-2.6/scripts/kconfig/symbol.c
===
--- linux-2.6.orig/scripts/kconfig/symbol.c
+++ linux-2.6/scripts/kconfig/symbol.c
@@ -56,20 +56,6 @@ void sym_init(void)
 
uname();
 
-   sym = sym_lookup("ARCH", 0);
-   sym->type = S_STRING;
-   sym->flags |= SYMBOL_AUTO;
-   p = getenv("ARCH");
-   if (p)
-   sym_add_default(sym, p);
-
-   sym = sym_lookup("KERNELVERSION", 0);
-   sym->type = S_STRING;
-   sym->flags |= SYMBOL_AUTO;
-   p = getenv("KERNELVERSION");
-   if (p)
-   sym_add_default(sym, p);
-
sym = sym_lookup("UNAME_RELEASE", 0);
sym->type = S_STRING;
sym->flags |= SYMBOL_AUTO;
--
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/

[PATCH 1/3] explicitly introduce expression list

[Roman Zippel]

Rename E_CHOICE to E_LIST to explicitly add support for expression
lists. Add a helper macro expr_list_for_each_sym to more easily iterate
over the list.

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 scripts/kconfig/confdata.c |8 
 scripts/kconfig/expr.c |   16 
 scripts/kconfig/expr.h |5 -
 scripts/kconfig/menu.c |2 +-
 scripts/kconfig/symbol.c   |   13 +++--
 5 files changed, 24 insertions(+), 20 deletions(-)

Index: linux-2.6/scripts/kconfig/confdata.c
===
--- linux-2.6.orig/scripts/kconfig/confdata.c
+++ linux-2.6/scripts/kconfig/confdata.c
@@ -316,7 +316,7 @@ load:
 
 int conf_read(const char *name)
 {
-   struct symbol *sym;
+   struct symbol *sym, *choice_sym;
struct property *prop;
struct expr *e;
int i, flags;
@@ -357,9 +357,9 @@ int conf_read(const char *name)
 */
prop = sym_get_choice_prop(sym);
flags = sym->flags;
-   for (e = prop->expr; e; e = e->left.expr)
-   if (e->right.sym->visible != no)
-   flags &= e->right.sym->flags;
+   expr_list_for_each_sym(prop->expr, e, choice_sym)
+   if (choice_sym->visible != no)
+   flags &= choice_sym->flags;
sym->flags &= flags | ~SYMBOL_DEF_USER;
}
 
Index: linux-2.6/scripts/kconfig/expr.c
===
--- linux-2.6.orig/scripts/kconfig/expr.c
+++ linux-2.6/scripts/kconfig/expr.c
@@ -87,7 +87,7 @@ struct expr *expr_copy(struct expr *org)
break;
case E_AND:
case E_OR:
-   case E_CHOICE:
+   case E_LIST:
e->left.expr = expr_copy(org->left.expr);
e->right.expr = expr_copy(org->right.expr);
break;
@@ -217,7 +217,7 @@ int expr_eq(struct expr *e1, struct expr
expr_free(e2);
trans_count = old_count;
return res;
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -648,7 +648,7 @@ struct expr *expr_transform(struct expr 
case E_EQUAL:
case E_UNEQUAL:
case E_SYMBOL:
-   case E_CHOICE:
+   case E_LIST:
break;
default:
e->left.expr = expr_transform(e->left.expr);
@@ -932,7 +932,7 @@ struct expr *expr_trans_compare(struct e
break;
case E_SYMBOL:
return expr_alloc_comp(type, e->left.sym, sym);
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -1000,9 +1000,9 @@ int expr_compare_type(enum expr_type t1,
if (t2 == E_OR)
return 1;
case E_OR:
-   if (t2 == E_CHOICE)
+   if (t2 == E_LIST)
return 1;
-   case E_CHOICE:
+   case E_LIST:
if (t2 == 0)
return 1;
default:
@@ -1053,11 +1053,11 @@ void expr_print(struct expr *e, void (*f
fn(data, NULL, " && ");
expr_print(e->right.expr, fn, data, E_AND);
break;
-   case E_CHOICE:
+   case E_LIST:
fn(data, e->right.sym, e->right.sym->name);
if (e->left.expr) {
fn(data, NULL, " ^ ");
-   expr_print(e->left.expr, fn, data, E_CHOICE);
+   expr_print(e->left.expr, fn, data, E_LIST);
}
break;
case E_RANGE:
Index: linux-2.6/scripts/kconfig/expr.h
===
--- linux-2.6.orig/scripts/kconfig/expr.h
+++ linux-2.6/scripts/kconfig/expr.h
@@ -32,7 +32,7 @@ typedef enum tristate {
 } tristate;
 
 enum expr_type {
-   E_NONE, E_OR, E_AND, E_NOT, E_EQUAL, E_UNEQUAL, E_CHOICE, E_SYMBOL, 
E_RANGE
+   E_NONE, E_OR, E_AND, E_NOT, E_EQUAL, E_UNEQUAL, E_LIST, E_SYMBOL, 
E_RANGE
 };
 
 union expr_data {
@@ -49,6 +49,9 @@ struct expr {
 #define E_AND(dep1, dep2)  (((dep1)<(dep2))?(dep1):(dep2))
 #define E_NOT(dep) (2-(dep))
 
+#define expr_list_for_each_sym(l, e, s) \
+   for (e = (l); e && (s = e->right.sym); e = e->left.expr)
+
 struct expr_value {
struct expr *expr;
tristate tri;
Index: linux-2.6/scripts/kconfig/menu.c
===
--- linux-2.6.orig/scripts/kconfig/menu.c
+++ linux-2.6/scripts/kconfig/menu.c
@@ -331,7 +331,7 @@ void menu_finalize(struct menu *parent)
prop = sym_get_choice_prop(sym);
   

Re: kconfig: support option env="" [Was: kconfig: use $K64BIT to set 64BIT with all*config targets]

[Roman Zippel]

Hi,

On Sun, 6 Jan 2008, Sam Ravnborg wrote:

> Please get back to me so we can finsih this patch and have it applied.
> I will split the patch in two btw.

I reworked the patch a little and split it into three.

> > +   if (sym->flags & SYMBOL_AUTO)
> > +   sym->flags &= ~SYMBOL_WRITE;
> > +
> 
> Why is this change needed?
> It is non-obvious to me so please explain and I will add a comment.

Automatically generated symbols are not saved, this was previously not 
needed as they weren't in the menu structure.

> I did it like this:
>   menu_warn(current_entry,
> "config %s: redefining environment symbol 
> from '%s' to '%s'",
> sym->name, env, sym2->name);

I omitted the prefix, it's inconsistent with other warnings.

bye, Roman
--
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/

Re: kconfig: support option env= [Was: kconfig: use $K64BIT to set 64BIT with all*config targets]

[Roman Zippel]

Hi,

On Sun, 6 Jan 2008, Sam Ravnborg wrote:

 Please get back to me so we can finsih this patch and have it applied.
 I will split the patch in two btw.

I reworked the patch a little and split it into three.

  +   if (sym-flags  SYMBOL_AUTO)
  +   sym-flags = ~SYMBOL_WRITE;
  +
 
 Why is this change needed?
 It is non-obvious to me so please explain and I will add a comment.

Automatically generated symbols are not saved, this was previously not 
needed as they weren't in the menu structure.

 I did it like this:
   menu_warn(current_entry,
 config %s: redefining environment symbol 
 from '%s' to '%s',
 sym-name, env, sym2-name);

I omitted the prefix, it's inconsistent with other warnings.

bye, Roman
--
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/

[PATCH 1/3] explicitly introduce expression list

[Roman Zippel]

Rename E_CHOICE to E_LIST to explicitly add support for expression
lists. Add a helper macro expr_list_for_each_sym to more easily iterate
over the list.

Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 scripts/kconfig/confdata.c |8 
 scripts/kconfig/expr.c |   16 
 scripts/kconfig/expr.h |5 -
 scripts/kconfig/menu.c |2 +-
 scripts/kconfig/symbol.c   |   13 +++--
 5 files changed, 24 insertions(+), 20 deletions(-)

Index: linux-2.6/scripts/kconfig/confdata.c
===
--- linux-2.6.orig/scripts/kconfig/confdata.c
+++ linux-2.6/scripts/kconfig/confdata.c
@@ -316,7 +316,7 @@ load:
 
 int conf_read(const char *name)
 {
-   struct symbol *sym;
+   struct symbol *sym, *choice_sym;
struct property *prop;
struct expr *e;
int i, flags;
@@ -357,9 +357,9 @@ int conf_read(const char *name)
 */
prop = sym_get_choice_prop(sym);
flags = sym-flags;
-   for (e = prop-expr; e; e = e-left.expr)
-   if (e-right.sym-visible != no)
-   flags = e-right.sym-flags;
+   expr_list_for_each_sym(prop-expr, e, choice_sym)
+   if (choice_sym-visible != no)
+   flags = choice_sym-flags;
sym-flags = flags | ~SYMBOL_DEF_USER;
}
 
Index: linux-2.6/scripts/kconfig/expr.c
===
--- linux-2.6.orig/scripts/kconfig/expr.c
+++ linux-2.6/scripts/kconfig/expr.c
@@ -87,7 +87,7 @@ struct expr *expr_copy(struct expr *org)
break;
case E_AND:
case E_OR:
-   case E_CHOICE:
+   case E_LIST:
e-left.expr = expr_copy(org-left.expr);
e-right.expr = expr_copy(org-right.expr);
break;
@@ -217,7 +217,7 @@ int expr_eq(struct expr *e1, struct expr
expr_free(e2);
trans_count = old_count;
return res;
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -648,7 +648,7 @@ struct expr *expr_transform(struct expr 
case E_EQUAL:
case E_UNEQUAL:
case E_SYMBOL:
-   case E_CHOICE:
+   case E_LIST:
break;
default:
e-left.expr = expr_transform(e-left.expr);
@@ -932,7 +932,7 @@ struct expr *expr_trans_compare(struct e
break;
case E_SYMBOL:
return expr_alloc_comp(type, e-left.sym, sym);
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -1000,9 +1000,9 @@ int expr_compare_type(enum expr_type t1,
if (t2 == E_OR)
return 1;
case E_OR:
-   if (t2 == E_CHOICE)
+   if (t2 == E_LIST)
return 1;
-   case E_CHOICE:
+   case E_LIST:
if (t2 == 0)
return 1;
default:
@@ -1053,11 +1053,11 @@ void expr_print(struct expr *e, void (*f
fn(data, NULL,   );
expr_print(e-right.expr, fn, data, E_AND);
break;
-   case E_CHOICE:
+   case E_LIST:
fn(data, e-right.sym, e-right.sym-name);
if (e-left.expr) {
fn(data, NULL,  ^ );
-   expr_print(e-left.expr, fn, data, E_CHOICE);
+   expr_print(e-left.expr, fn, data, E_LIST);
}
break;
case E_RANGE:
Index: linux-2.6/scripts/kconfig/expr.h
===
--- linux-2.6.orig/scripts/kconfig/expr.h
+++ linux-2.6/scripts/kconfig/expr.h
@@ -32,7 +32,7 @@ typedef enum tristate {
 } tristate;
 
 enum expr_type {
-   E_NONE, E_OR, E_AND, E_NOT, E_EQUAL, E_UNEQUAL, E_CHOICE, E_SYMBOL, 
E_RANGE
+   E_NONE, E_OR, E_AND, E_NOT, E_EQUAL, E_UNEQUAL, E_LIST, E_SYMBOL, 
E_RANGE
 };
 
 union expr_data {
@@ -49,6 +49,9 @@ struct expr {
 #define E_AND(dep1, dep2)  (((dep1)(dep2))?(dep1):(dep2))
 #define E_NOT(dep) (2-(dep))
 
+#define expr_list_for_each_sym(l, e, s) \
+   for (e = (l); e  (s = e-right.sym); e = e-left.expr)
+
 struct expr_value {
struct expr *expr;
tristate tri;
Index: linux-2.6/scripts/kconfig/menu.c
===
--- linux-2.6.orig/scripts/kconfig/menu.c
+++ linux-2.6/scripts/kconfig/menu.c
@@ -331,7 +331,7 @@ void menu_finalize(struct menu *parent)
prop = sym_get_choice_prop(sym);
for (ep = prop-expr; *ep; ep = (*ep)-left.expr)
;
-   *ep = expr_alloc_one(E_CHOICE, NULL);
+   *ep

[PATCH 2/3] environment symbol support

[Roman Zippel]

Add the possibility to import a value from the environment into kconfig
via the option syntax. Beside flexibility this has the advantage
providing proper dependencies.

Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 Documentation/kbuild/kconfig-language.txt |   21 ++
 scripts/kconfig/expr.h|3 +-
 scripts/kconfig/lkc.h |5 +++
 scripts/kconfig/menu.c|5 ++-
 scripts/kconfig/qconf.cc  |   16 +++---
 scripts/kconfig/symbol.c  |   45 ++
 scripts/kconfig/util.c|   23 ++-
 scripts/kconfig/zconf.gperf   |1 
 scripts/kconfig/zconf.hash.c_shipped  |   45 --
 9 files changed, 129 insertions(+), 35 deletions(-)

Index: linux-2.6/Documentation/kbuild/kconfig-language.txt
===
--- linux-2.6.orig/Documentation/kbuild/kconfig-language.txt
+++ linux-2.6/Documentation/kbuild/kconfig-language.txt
@@ -127,6 +127,27 @@ applicable everywhere (see syntax).
   used to help visually separate configuration logic from help within
   the file as an aid to developers.
 
+- misc options: option symbol[=value]
+  Various less common options can be defined via this option syntax,
+  which can modify the behaviour of the menu entry and its config
+  symbol. These options are currently possible:
+
+  - defconfig_list
+This declares a list of default entries which can be used when
+looking for the default configuration (which is used when the main
+.config doesn't exists yet.)
+
+  - modules
+This declares the symbol to be used as the MODULES symbol, which
+enables the third modular state for all config symbols.
+
+  - env=value
+This imports the environment variable into Kconfig. It behaves like
+a default, except that the value comes from the environment, this
+also means that the behaviour when mixing it with normal defaults is
+undefined at this point. The symbol is currently not exported back
+to the build environment (if this is desired, it can be done via
+another symbol).
 
 Menu dependencies
 -
Index: linux-2.6/scripts/kconfig/expr.h
===
--- linux-2.6.orig/scripts/kconfig/expr.h
+++ linux-2.6/scripts/kconfig/expr.h
@@ -108,7 +108,8 @@ struct symbol {
 #define SYMBOL_HASHMASK0xff
 
 enum prop_type {
-   P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE, P_SELECT, 
P_RANGE
+   P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE,
+   P_SELECT, P_RANGE, P_ENV
 };
 
 struct property {
Index: linux-2.6/scripts/kconfig/lkc.h
===
--- linux-2.6.orig/scripts/kconfig/lkc.h
+++ linux-2.6/scripts/kconfig/lkc.h
@@ -44,6 +44,7 @@ extern C {
 
 #define T_OPT_MODULES  1
 #define T_OPT_DEFCONFIG_LIST   2
+#define T_OPT_ENV  3
 
 struct kconf_id {
int name;
@@ -74,6 +75,7 @@ void kconfig_load(void);
 
 /* menu.c */
 void menu_init(void);
+void menu_warn(struct menu *menu, const char *fmt, ...);
 struct menu *menu_add_menu(void);
 void menu_end_menu(void);
 void menu_add_entry(struct symbol *sym);
@@ -103,6 +105,8 @@ void str_printf(struct gstr *gs, const c
 const char *str_get(struct gstr *gs);
 
 /* symbol.c */
+extern struct expr *sym_env_list;
+
 void sym_init(void);
 void sym_clear_all_valid(void);
 void sym_set_all_changed(void);
@@ -110,6 +114,7 @@ void sym_set_changed(struct symbol *sym)
 struct symbol *sym_check_deps(struct symbol *sym);
 struct property *prop_alloc(enum prop_type type, struct symbol *sym);
 struct symbol *prop_get_symbol(struct property *prop);
+struct property *sym_get_env_prop(struct symbol *sym);
 
 static inline tristate sym_get_tristate_value(struct symbol *sym)
 {
Index: linux-2.6/scripts/kconfig/menu.c
===
--- linux-2.6.orig/scripts/kconfig/menu.c
+++ linux-2.6/scripts/kconfig/menu.c
@@ -15,7 +15,7 @@ static struct menu **last_entry_ptr;
 struct file *file_list;
 struct file *current_file;
 
-static void menu_warn(struct menu *menu, const char *fmt, ...)
+void menu_warn(struct menu *menu, const char *fmt, ...)
 {
va_list ap;
va_start(ap, fmt);
@@ -172,6 +172,9 @@ void menu_add_option(int token, char *ar
else if (sym_defconfig_list != current_entry-sym)
zconf_error(trying to redefine defconfig symbol);
break;
+   case T_OPT_ENV:
+   prop_add_env(arg);
+   break;
}
 }
 
Index: linux-2.6/scripts/kconfig/qconf.cc
===
--- linux-2.6.orig/scripts/kconfig/qconf.cc
+++ linux-2.6/scripts/kconfig/qconf.cc
@@ -1083,7 +1083,11 @@ QString

[PATCH 3/3] use environment option

[Roman Zippel]

Use the environment option to provide the ARCH symbol.
Remove the unused KERNELVERSION symbol.

Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 init/Kconfig |4 
 scripts/kconfig/symbol.c |   14 --
 2 files changed, 4 insertions(+), 14 deletions(-)

Index: linux-2.6/init/Kconfig
===
--- linux-2.6.orig/init/Kconfig
+++ linux-2.6/init/Kconfig
@@ -1,3 +1,7 @@
+config ARCH
+   string
+   option env=ARCH
+
 config DEFCONFIG_LIST
string
depends on !UML
Index: linux-2.6/scripts/kconfig/symbol.c
===
--- linux-2.6.orig/scripts/kconfig/symbol.c
+++ linux-2.6/scripts/kconfig/symbol.c
@@ -56,20 +56,6 @@ void sym_init(void)
 
uname(uts);
 
-   sym = sym_lookup(ARCH, 0);
-   sym-type = S_STRING;
-   sym-flags |= SYMBOL_AUTO;
-   p = getenv(ARCH);
-   if (p)
-   sym_add_default(sym, p);
-
-   sym = sym_lookup(KERNELVERSION, 0);
-   sym-type = S_STRING;
-   sym-flags |= SYMBOL_AUTO;
-   p = getenv(KERNELVERSION);
-   if (p)
-   sym_add_default(sym, p);
-
sym = sym_lookup(UNAME_RELEASE, 0);
sym-type = S_STRING;
sym-flags |= SYMBOL_AUTO;
--
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/

Re: "mconf" and help texts

[Roman Zippel]

Hi,

On Thursday 3. January 2008, Ph. Marek wrote:

> So I took a look at "Help", and saw that blob:
>
>   Selected by: NETFILTER_XT_TARGET_CONNMARK && NET && INET && NETFILTER &&
>  NETFILTER_XTABLES && (IP_NF_MANGLE || IP6_NF_MANGLE) && NF_CONNTRACK
> || NETFILTER_XT_MATCH_CONNMARK && NET && INET && NETFILTER &&
>  NETFILTER_XTABLES && NF_CONNTRACK || IP_NF_TARGET_CLUSTERIP && NET &&
>  INET && NETFILTER && IP_NF_MANGLE && EXPERIMENTAL && NF_CONNTRACK_IPV4
>
> That is a _bit_ unreadable.

What you see here is the internal representation of the select expression. To 
make it more easily readable, you could just cut off everything between && 
and || (it's the dependency of the symbol which does the select, the one 
before &&).
The readable expression could be generated when needed, but it might be easier 
to just generate at the same time as the full expression (in menu_finalize).

> As a side-node - I cannot get xconfig to work (pkg-config);

Why exactly? What's the error message?

> is there some 
> way in menuconfig to see why some config option is disallowed? The location
> tree shows some data ("-> Networking support (NET [=y])"), but not for all
> dependencies.

I don't quite understand. The dependency for option itself is above the 
location tree and the for dependencies of the dependencies you have to look 
at their individual info.

> [Does xconfig allow enabling them while seeing this option?] 

Only if you enable the missing dependency. This is insofar a little easier as 
they are linked if you enable the deubg info, so the prompt may be a little 
easier to find.

bye, Roman
--
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/

Re: [PATCH] UPDATED2: hfs: handle more on-disk corruptions without oopsing

[Roman Zippel]

Hi,

On Wednesday 2. January 2008, Eric Sandeen wrote:

> Roman, with this on top does it look better to you?

Looks good, thanks.

> I'll get hfsplus done in a bit.

I'm mainly concerned about brec.c/bfind.c, the patch should be pretty much 
identical.

bye, Roman
--
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/

Re: mconf and help texts

[Roman Zippel]

Hi,

On Thursday 3. January 2008, Ph. Marek wrote:

 So I took a look at Help, and saw that blob:

   Selected by: NETFILTER_XT_TARGET_CONNMARK  NET  INET  NETFILTER 
  NETFILTER_XTABLES  (IP_NF_MANGLE || IP6_NF_MANGLE)  NF_CONNTRACK
 || NETFILTER_XT_MATCH_CONNMARK  NET  INET  NETFILTER 
  NETFILTER_XTABLES  NF_CONNTRACK || IP_NF_TARGET_CLUSTERIP  NET 
  INET  NETFILTER  IP_NF_MANGLE  EXPERIMENTAL  NF_CONNTRACK_IPV4

 That is a _bit_ unreadable.

What you see here is the internal representation of the select expression. To 
make it more easily readable, you could just cut off everything between  
and || (it's the dependency of the symbol which does the select, the one 
before ).
The readable expression could be generated when needed, but it might be easier 
to just generate at the same time as the full expression (in menu_finalize).

 As a side-node - I cannot get xconfig to work (pkg-config);

Why exactly? What's the error message?

 is there some 
 way in menuconfig to see why some config option is disallowed? The location
 tree shows some data (- Networking support (NET [=y])), but not for all
 dependencies.

I don't quite understand. The dependency for option itself is above the 
location tree and the for dependencies of the dependencies you have to look 
at their individual info.

 [Does xconfig allow enabling them while seeing this option?] 

Only if you enable the missing dependency. This is insofar a little easier as 
they are linked if you enable the deubg info, so the prompt may be a little 
easier to find.

bye, Roman
--
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/

Re: [PATCH] UPDATED2: hfs: handle more on-disk corruptions without oopsing

[Roman Zippel]

Hi,

On Wednesday 2. January 2008, Eric Sandeen wrote:

 Roman, with this on top does it look better to you?

Looks good, thanks.

 I'll get hfsplus done in a bit.

I'm mainly concerned about brec.c/bfind.c, the patch should be pretty much 
identical.

bye, Roman
--
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/

Re: [PATCH] Avoid overflows in kernel/time.c (version 5)

[Roman Zippel]

Hi,

On Monday 17 December 2007, H. Peter Anvin wrote:

>  kernel/timeconst.pl   |  340

I agree with Jan that it would be better to put this into scripts.
In the long term we could also detect some of the simple special cases, so we 
can finally inline some of these functions again, this would mean the 
generated file would go into include/linux/ and the script should be in 
scripts/.

> + my ($b,$n,$d) = @_;

I noticed this already in the old script, using something else than single 
letter variables would increase readability considerably.

bye, Roman
--
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/

Re: [PATCH] UPDATED: hfs: handle more on-disk corruptions without oopsing

[Roman Zippel]

Hi,

On Thursday 20 December 2007, Eric Sandeen wrote:

> Index: linux-2.6.24-rc3/fs/hfs/brec.c
> ===
> --- linux-2.6.24-rc3.orig/fs/hfs/brec.c
> +++ linux-2.6.24-rc3/fs/hfs/brec.c
> @@ -44,10 +44,21 @@ u16 hfs_brec_keylen(struct hfs_bnode *no
>   recoff = hfs_bnode_read_u16(node, node->tree->node_size - (rec 
> + 1) *
> 2); if (!recoff)
>   return 0;
> - if (node->tree->attributes & HFS_TREE_BIGKEYS)
> + if (node->tree->attributes & HFS_TREE_BIGKEYS) {
>   retval = hfs_bnode_read_u16(node, recoff) + 2;
> - else
> + if (retval > node->tree->max_key_len + 2) {
> + printk(KERN_ERR "hfs: keylen %d too large\n",
> + retval);
> + retval = HFS_BAD_KEYLEN;
> + }
> + } else {
>   retval = (hfs_bnode_read_u8(node, recoff) | 1) + 1;
> + if (retval > node->tree->max_key_len + 1) {
> + printk(KERN_ERR "hfs: keylen %d too large\n",
> + retval);
> + retval = HFS_BAD_KEYLEN;
> + }
> + }
>   }
>   return retval;
>  }

You can reuse 0 as failure value, a key has to be of nonzero size.

> Index: linux-2.6.24-rc3/fs/hfs/btree.c
> ===
> --- linux-2.6.24-rc3.orig/fs/hfs/btree.c
> +++ linux-2.6.24-rc3/fs/hfs/btree.c
> @@ -81,6 +81,17 @@ struct hfs_btree *hfs_btree_open(struct
>   goto fail_page;
>   if (!tree->node_count)
>   goto fail_page;
> + if ((id == HFS_EXT_CNID) && (tree->max_key_len != HFS_MAX_EXT_KEYLEN)) {
> + printk(KERN_ERR "hfs: invalid extent max_key_len %d\n",
> + tree->max_key_len);
> + goto fail_page;
> + }
> + if ((id == HFS_CAT_CNID) && (tree->max_key_len != HFS_MAX_CAT_KEYLEN)) {
> + printk(KERN_ERR "hfs: invalid catalog max_key_len %d\n",
> + tree->max_key_len);
> + goto fail_page;
> + }
> +
>   tree->node_size_shift = ffs(size) - 1;
>   tree->pages_per_bnode = (tree->node_size + PAGE_CACHE_SIZE - 1) >>
> PAGE_CACHE_SHIFT;
>

I'd prefer a switch statement here.

It would be nice if you could do the same changes for hfsplus, so both stay in 
sync.
Thanks.

bye, Roman
--
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/

Re: [PATCH] UPDATED: hfs: handle more on-disk corruptions without oopsing

[Roman Zippel]

Hi,

On Thursday 20 December 2007, Eric Sandeen wrote:

 Index: linux-2.6.24-rc3/fs/hfs/brec.c
 ===
 --- linux-2.6.24-rc3.orig/fs/hfs/brec.c
 +++ linux-2.6.24-rc3/fs/hfs/brec.c
 @@ -44,10 +44,21 @@ u16 hfs_brec_keylen(struct hfs_bnode *no
   recoff = hfs_bnode_read_u16(node, node-tree-node_size - (rec 
 + 1) *
 2); if (!recoff)
   return 0;
 - if (node-tree-attributes  HFS_TREE_BIGKEYS)
 + if (node-tree-attributes  HFS_TREE_BIGKEYS) {
   retval = hfs_bnode_read_u16(node, recoff) + 2;
 - else
 + if (retval  node-tree-max_key_len + 2) {
 + printk(KERN_ERR hfs: keylen %d too large\n,
 + retval);
 + retval = HFS_BAD_KEYLEN;
 + }
 + } else {
   retval = (hfs_bnode_read_u8(node, recoff) | 1) + 1;
 + if (retval  node-tree-max_key_len + 1) {
 + printk(KERN_ERR hfs: keylen %d too large\n,
 + retval);
 + retval = HFS_BAD_KEYLEN;
 + }
 + }
   }
   return retval;
  }

You can reuse 0 as failure value, a key has to be of nonzero size.

 Index: linux-2.6.24-rc3/fs/hfs/btree.c
 ===
 --- linux-2.6.24-rc3.orig/fs/hfs/btree.c
 +++ linux-2.6.24-rc3/fs/hfs/btree.c
 @@ -81,6 +81,17 @@ struct hfs_btree *hfs_btree_open(struct
   goto fail_page;
   if (!tree-node_count)
   goto fail_page;
 + if ((id == HFS_EXT_CNID)  (tree-max_key_len != HFS_MAX_EXT_KEYLEN)) {
 + printk(KERN_ERR hfs: invalid extent max_key_len %d\n,
 + tree-max_key_len);
 + goto fail_page;
 + }
 + if ((id == HFS_CAT_CNID)  (tree-max_key_len != HFS_MAX_CAT_KEYLEN)) {
 + printk(KERN_ERR hfs: invalid catalog max_key_len %d\n,
 + tree-max_key_len);
 + goto fail_page;
 + }
 +
   tree-node_size_shift = ffs(size) - 1;
   tree-pages_per_bnode = (tree-node_size + PAGE_CACHE_SIZE - 1) 
 PAGE_CACHE_SHIFT;


I'd prefer a switch statement here.

It would be nice if you could do the same changes for hfsplus, so both stay in 
sync.
Thanks.

bye, Roman
--
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/

Re: [PATCH] Avoid overflows in kernel/time.c (version 5)

[Roman Zippel]

Hi,

On Monday 17 December 2007, H. Peter Anvin wrote:

  kernel/timeconst.pl   |  340

I agree with Jan that it would be better to put this into scripts.
In the long term we could also detect some of the simple special cases, so we 
can finally inline some of these functions again, this would mean the 
generated file would go into include/linux/ and the script should be in 
scripts/.

 + my ($b,$n,$d) = @_;

I noticed this already in the old script, using something else than single 
letter variables would increase readability considerably.

bye, Roman
--
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/

Re: [PATCH] initrd: Fix virtual/physical mix-up in overwrite test

[Roman Zippel]

Hi,

On Sunday 16 December 2007, Geert Uytterhoeven wrote:

> --- a/init/main.c
> +++ b/init/main.c
> @@ -598,9 +598,9 @@ asmlinkage void __init start_kernel(void
>
>  #ifdef CONFIG_BLK_DEV_INITRD
>   if (initrd_start && !initrd_below_start_ok &&
> - initrd_start < min_low_pfn << PAGE_SHIFT) {
> + virt_to_pfn(initrd_start) < min_low_pfn) {
>   printk(KERN_CRIT "initrd overwritten (0x%08lx < 0x%08lx) - "
> - "disabling it.\n",initrd_start,min_low_pfn << PAGE_SHIFT);
> + "disabling it.\n", virt_to_pfn(initrd_start), min_low_pfn);
>   initrd_start = 0;
>   }
>  #endif

BTW this is some really old code, so another option might be to remove this 
check completely as the same check is already done via bootmem.

bye, Roman
--
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/

Re: [PATCH] initrd: Fix virtual/physical mix-up in overwrite test

[Roman Zippel]

Hi,

On Sunday 16 December 2007, Geert Uytterhoeven wrote:

 --- a/init/main.c
 +++ b/init/main.c
 @@ -598,9 +598,9 @@ asmlinkage void __init start_kernel(void

  #ifdef CONFIG_BLK_DEV_INITRD
   if (initrd_start  !initrd_below_start_ok 
 - initrd_start  min_low_pfn  PAGE_SHIFT) {
 + virt_to_pfn(initrd_start)  min_low_pfn) {
   printk(KERN_CRIT initrd overwritten (0x%08lx  0x%08lx) - 
 - disabling it.\n,initrd_start,min_low_pfn  PAGE_SHIFT);
 + disabling it.\n, virt_to_pfn(initrd_start), min_low_pfn);
   initrd_start = 0;
   }
  #endif

BTW this is some really old code, so another option might be to remove this 
check completely as the same check is already done via bootmem.

bye, Roman
--
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/

Re: [PATCH] kconfig: Make KCONFIG_ALLCONFIG work with randconfig.

[Roman Zippel]

Hi,

On Wed, 28 Nov 2007, Paul Mundt wrote:

> Adrian mentioned a few weeks ago that KCONFIG_ALLCONFIG is the way to
> go to ensure that things like allyes/allmod/allnoconfig work with a
> constrained set of symbols, with the implication that this holds
> true for randconfig as well.

BTW another possibility is to use all{no,mod,yes,random,}.config.

> While allyes/mod/noconfigs do seem to work fine with KCONFIG_ALLCONFIG
> provisions, randconfig tramples all over the provided values at perhaps
> not surprisingly, random.

Please be careful with such broad statements, there is only an issue with 
choice values.

> Debugging this a bit, there seemed to be two issues:
> 
>   - SYMBOL_DEF and SYMBOL_DEF_USER overlap, which made
> def_sym->flags the same regardless of whether we came from an
> KCONFIG_ALLCONFIG path or not.

Look at how SYMBOL_DEF is used in confdata.c.

>   - clobbering of the fixed value in conf_choice() by way of
> random() def assignment.

Simply add a test for is_new there.

bye, Roman
-
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/

Re: [PATCH] kconfig: Make KCONFIG_ALLCONFIG work with randconfig.

[Roman Zippel]

Hi,

On Wed, 28 Nov 2007, Paul Mundt wrote:

 Adrian mentioned a few weeks ago that KCONFIG_ALLCONFIG is the way to
 go to ensure that things like allyes/allmod/allnoconfig work with a
 constrained set of symbols, with the implication that this holds
 true for randconfig as well.

BTW another possibility is to use all{no,mod,yes,random,}.config.

 While allyes/mod/noconfigs do seem to work fine with KCONFIG_ALLCONFIG
 provisions, randconfig tramples all over the provided values at perhaps
 not surprisingly, random.

Please be careful with such broad statements, there is only an issue with 
choice values.

 Debugging this a bit, there seemed to be two issues:
 
   - SYMBOL_DEF and SYMBOL_DEF_USER overlap, which made
 def_sym-flags the same regardless of whether we came from an
 KCONFIG_ALLCONFIG path or not.

Look at how SYMBOL_DEF is used in confdata.c.

   - clobbering of the fixed value in conf_choice() by way of
 random() def assignment.

Simply add a test for is_new there.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Randy Dunlap wrote:

> This all began (AFAIK) because some of us want to continue to be
> able to specify ARCH={i386,x86_64} on the (make) command line --
> not by using a .config file.  Taking away ARCH= on the command line
> is a regression (in some minds, at least), so Sam provided that
> capability.  Is that capability still present after this patch?

My patch does exactly that.
Sam's patch only made a subtle difference for all*config, but otherwise it 
would have made little difference, i.e. you still had to edit .config.

The maybe only remaining issue is which default config to use for 
defconfig, when ARCH isn't specified, but that is purely a Kbuild issue.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Fri, 16 Nov 2007, Sam Ravnborg wrote:

> 1) make all*config, randconfig, defconfig is broken on 64-bit boxes

With your approach you made it impossible to set 64BIT from all*.config,
which is the proper way to set the defaults.

> 2) A pure code refactoring patch is reverted for no obvious reason

It was done for the wrong reasons, otherwise the warning before it should 
have been included as well and then the function could have been reused 
for the "# ... is not set" case as well.

> 3) Behavioral changes are not documented:
>- 32-bit/64-bit can only be selected in config is you specify ARCH=x86

Which is now the default and thus it behaves more like other archs.

>- ARCH= takes precedence over kernel config for a configured kernel

What point is there in being able to specify ARCH=x86_64 and then still 
produce a 32bit kernel?

> 4) The changelogs miss title on reverted patches

Seriously?

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Fri, 16 Nov 2007, Sam Ravnborg wrote:

 1) make all*config, randconfig, defconfig is broken on 64-bit boxes

With your approach you made it impossible to set 64BIT from all*.config,
which is the proper way to set the defaults.

 2) A pure code refactoring patch is reverted for no obvious reason

It was done for the wrong reasons, otherwise the warning before it should 
have been included as well and then the function could have been reused 
for the # ... is not set case as well.

 3) Behavioral changes are not documented:
- 32-bit/64-bit can only be selected in config is you specify ARCH=x86

Which is now the default and thus it behaves more like other archs.

- ARCH= takes precedence over kernel config for a configured kernel

What point is there in being able to specify ARCH=x86_64 and then still 
produce a 32bit kernel?

 4) The changelogs miss title on reverted patches

Seriously?

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Randy Dunlap wrote:

 This all began (AFAIK) because some of us want to continue to be
 able to specify ARCH={i386,x86_64} on the (make) command line --
 not by using a .config file.  Taking away ARCH= on the command line
 is a regression (in some minds, at least), so Sam provided that
 capability.  Is that capability still present after this patch?

My patch does exactly that.
Sam's patch only made a subtle difference for all*config, but otherwise it 
would have made little difference, i.e. you still had to edit .config.

The maybe only remaining issue is which default config to use for 
defconfig, when ARCH isn't specified, but that is purely a Kbuild issue.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

> You suggest just to check ARCH value and not apply your patch. This was
> not my initial understanding as was hopefully obvious from my reply.

This patch only adds some extra features.

> If user did NOT specify ARCH we should use the kernel configuration - which
> your solution fail to do.

To make this easy I attached the patch which reverts the problematic 
changes and then you only need this simple change to force the 64BIT value 
for ARCH={i386,x86_64}, otherwise it's set by the user:

bye, Roman

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 Makefile |3 ++-
 arch/x86/Kconfig |4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -165,7 +165,8 @@ export srctree objtree VPATH TOPDIR
 # then ARCH is assigned, getting whatever value it gets normally, and 
 # SUBARCH is subsequently ignored.
 
-SUBARCH := $(shell uname -m | sed -e s/i.86/i386/ -e s/sun4u/sparc64/ \
+SUBARCH := $(shell uname -m | sed -e s/i.86/x86/ -e s/x86_64/x86/ \
+ -e s/sun4u/sparc64/ \
  -e s/arm.*/arm/ -e s/sa110/arm/ \
  -e s/s390x/s390/ -e s/parisc64/parisc/ \
  -e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
Index: linux-2.6/arch/x86/Kconfig
===
--- linux-2.6.orig/arch/x86/Kconfig
+++ linux-2.6/arch/x86/Kconfig
@@ -3,8 +3,8 @@ mainmenu "Linux Kernel Configuration for
 
 # Select 32 or 64 bit
 config 64BIT
-   bool "64-bit kernel"
-   default n
+   bool "64-bit kernel" if ARCH="x86"
+   default ARCH="x86_64"
help
  Say yes to build a 64-bit kernel - formerly known as x86_64
  Say no to build a 32-bit kernel - formerly known as i386Revert

9c900a9c9d9351e55ab6a84e12e3a52c474c7c8b
0f855aa64b3f63d35a891510cf7db932a435c116
2a113281f5cd2febbab21a93c8943f8d3eece4d3

and K64BIT parts of

daa93fab824f2b8c35bd11670c7fab2f32b2de5f

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 Makefile|4 -
 README  |2 
 scripts/kconfig/conf.c  |1 
 scripts/kconfig/confdata.c  |  146 
 scripts/kconfig/lkc_proto.h |1 
 5 files changed, 56 insertions(+), 98 deletions(-)

Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -200,11 +200,9 @@ SRCARCH:= $(ARCH)
 # Additional ARCH settings for x86
 ifeq ($(ARCH),i386)
 SRCARCH := x86
-K64BIT  := n
 endif
 ifeq ($(ARCH),x86_64)
 SRCARCH := x86
-K64BIT  := y
 endif
 
 KCONFIG_CONFIG ?= .config
@@ -341,7 +339,7 @@ KERNELRELEASE = $(shell cat include/conf
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
 
 export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH SRCARCH K64BIT CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD 
CC
+export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
Index: linux-2.6/README
===
--- linux-2.6.orig/README
+++ linux-2.6/README
@@ -194,8 +194,6 @@ CONFIGURING the kernel:
"make *config" checks for a file named "all{yes/mod/no/random}.config"
for symbol values that are to be forced.  If this file is not found,
it checks for a file named "all.config" to contain forced values.
-   Finally it checks the environment variable K64BIT and if found, sets
-   the config symbol "64BIT" to the value of the K64BIT variable.

NOTES on "make config":
- having unnecessary drivers will make the kernel bigger, and can
Index: linux-2.6/scripts/kconfig/conf.c
===
--- linux-2.6.orig/scripts/kconfig/conf.c
+++ linux-2.6/scripts/kconfig/conf.c
@@ -591,7 +591,6 @@ int main(int ac, char **av)
conf_read_simple(name, S_DEF_USER);
else if (!stat("all.config", ))
conf_read_simple("all.config", S_DEF_USER);
-   conf_set_env_sym("K64BIT", "64BIT", S_DEF_USER);
break;
default:
break;
Index: linux-2.6/scripts/kconfig/confdata.c
===
--- linux-2.6.orig/scripts/kconfig/confdata.c
+++ linux-2.6/scripts/kconfig/confdata.c
@@ -83,95 +83,6 @@ char *c

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

> > Can we please can get some consistency in this?
> > We have a .config file for a reason, what's wrong with using it?
> 
> We need to set a selected few values in a few cases where we do
> not have a .config file.
> allmodconfig for x86 for instance. We would like to generate a
> 32-bit and a 64-bit version.

This can already be set via all.config/allmod.config.
Where is this need coming from? The point is that I don't like to add an 
interface, which is maybe used by two people, who should be perfectly 
capable using an existing superior mechanism.

> > > > Please revert the K64BIT changes and use this instead.
> > > 
> > > I will finish up your patch and target it for next merge window.
> > 
> > Why can't this be fixed properly now? You don't even need this patch, just 
> > use ARCH to set 64BIT in the Kconfig as I've shown.
> Because the patch is in mainline and has been tested by a lot of people
> during the last week. And as the functionality is almost equal I do not
> see it as a big deal to have the less-perfect solution in one kernel release.
> 
> And the only reason the patch were applied to mainline was to fix the build
> of the merged x86 architecute - otherwise it was in no way -rc material.

I showed you a solution, which requires no patch at all, while your patch 
adds extra functionality which is questionable.
Why is a quick hack preferable over a proper solution? 
Either explain why my solution isn't usable or _please_ revert the K64BIT 
changes.

> > > > These are two different uses, when reading a .config only the basic 
> > > > syntax 
> > > > is checked, but not the value itself.
> > > This is wrong considering the amount of people that hand edit the .config 
> > > file.
> > 
> > It's not, the actual symbol value is set later depending on the dependency 
> > constraints.
> My point is that the .config file is handedited so the syntax should be 
> checked
> to best possible extent. If someone specify CONFIG_64BIT=64 we should error 
> out.

The other function doesn't complain about it either. There is already 
only limited error checking, e.g. there is no complaint that the value 
isn't really set (because it was selected by something else), otherwise 
the .config check during a kernel upgrades would be a lot noisier than it 
already is. Anyone directly editing .config should know what he is doing.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

> > > The value can be supplied on the command-line so we need to validate 
> > > input.
> > 
> > Is there a need for this?
> Yes. We would like to set 64BIT or not in other than x86 cases.
> And way forward was not to override ARCH as in the x86 case.

Can we please can get some consistency in this?
We have a .config file for a reason, what's wrong with using it?

> > Please revert the K64BIT changes and use this instead.
> 
> I will finish up your patch and target it for next merge window.

Why can't this be fixed properly now? You don't even need this patch, just 
use ARCH to set 64BIT in the Kconfig as I've shown.

> > These are two different uses, when reading a .config only the basic syntax 
> > is checked, but not the value itself.
> This is wrong considering the amount of people that hand edit the .config 
> file.

It's not, the actual symbol value is set later depending on the dependency 
constraints.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Wed, 14 Nov 2007, Sam Ravnborg wrote:

> The value can be supplied on the command-line so we need to validate input.

Is there a need for this?
BTW ARCH was already available as a value in the Kconfig files, so setting 
the 64BIT option was already possible without any changes the kconfig 
system, e.g.:

config 64BIT
bool "64 Bit kernel" if ARCH!="i386" && ARCH!="x86_64"
default ARCH="x86_64"

The patch below adds some features to it:
- it allows to import any environment variable by specifying "option env=..."
- it generates a dependency on it, so the kernel config is updated if it 
changes.

Please revert the K64BIT changes and use this instead.

> The code is a copy of what happen when reading a all.config file and
> the functionality should be equal.
> Can we make that part simpler too?

These are two different uses, when reading a .config only the basic syntax 
is checked, but not the value itself.

> By the way - I have never understood the purpose of the flags (S_DEF_USER 
> etc.)
> Can we have a few comments added to their definition?

It allows to hold multiple configs, a user of it is conf_split_config() 
which loads another config and compares to the current config and updates 
the files under include/config as needed.
It could also be used by front ends to display what actually changed 
compared to e.g. the saved config.

> One of the blockers are that kconfig does not support more than one prompt
> for a choice symbol. Is this something you can fix - or sketch how to fix it?

The basic idea is to add a name to the choice, so multiple choices can be 
grouped together. This requires some changes to the dependency check to 
make sure one choice option doesn't depend on another (which is currently 
enforced by the syntax).

bye, Roman


Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

---
 init/Kconfig |4 ++
 scripts/kconfig/expr.c   |   16 +-
 scripts/kconfig/expr.h   |5 +--
 scripts/kconfig/lkc.h|5 +++
 scripts/kconfig/menu.c   |7 +++-
 scripts/kconfig/qconf.cc |   15 ++---
 scripts/kconfig/symbol.c |   53 +--
 scripts/kconfig/util.c   |   25 +++-
 scripts/kconfig/zconf.gperf  |1 
 scripts/kconfig/zconf.hash.c_shipped |   45 -
 10 files changed, 123 insertions(+), 53 deletions(-)

Index: linux-2.6/init/Kconfig
===
--- linux-2.6.orig/init/Kconfig
+++ linux-2.6/init/Kconfig
@@ -7,6 +7,10 @@ config DEFCONFIG_LIST
default "/boot/config-$UNAME_RELEASE"
default "arch/$ARCH/defconfig"
 
+config ARCH
+   string
+   option env="ARCH"
+
 menu "General setup"
 
 config EXPERIMENTAL
Index: linux-2.6/scripts/kconfig/expr.c
===
--- linux-2.6.orig/scripts/kconfig/expr.c
+++ linux-2.6/scripts/kconfig/expr.c
@@ -87,7 +87,7 @@ struct expr *expr_copy(struct expr *org)
break;
case E_AND:
case E_OR:
-   case E_CHOICE:
+   case E_LIST:
e->left.expr = expr_copy(org->left.expr);
e->right.expr = expr_copy(org->right.expr);
break;
@@ -217,7 +217,7 @@ int expr_eq(struct expr *e1, struct expr
expr_free(e2);
trans_count = old_count;
return res;
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -648,7 +648,7 @@ struct expr *expr_transform(struct expr 
case E_EQUAL:
case E_UNEQUAL:
case E_SYMBOL:
-   case E_CHOICE:
+   case E_LIST:
break;
default:
e->left.expr = expr_transform(e->left.expr);
@@ -932,7 +932,7 @@ struct expr *expr_trans_compare(struct e
break;
case E_SYMBOL:
return expr_alloc_comp(type, e->left.sym, sym);
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -1000,9 +1000,9 @@ int expr_compare_type(enum expr_type t1,
if (t2 == E_OR)
return 1;
case E_OR:
-   if (t2 == E_CHOICE)
+   if (t2 == E_LIST)
return 1;
-   case E_CHOICE:
+   case E_LIST:
if (t2 == 0)
return 1;
default:
@@ -1053,11 +1053,11 @@ void expr_print(struct expr *e, void (*f
fn(data, NULL, " && ");
expr_print(e->right.expr, fn, data, E_AND);
break;
-   case E_CHOICE:
+   case E_LIST:
fn(da

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Wed, 14 Nov 2007, Sam Ravnborg wrote:

 The value can be supplied on the command-line so we need to validate input.

Is there a need for this?
BTW ARCH was already available as a value in the Kconfig files, so setting 
the 64BIT option was already possible without any changes the kconfig 
system, e.g.:

config 64BIT
bool 64 Bit kernel if ARCH!=i386  ARCH!=x86_64
default ARCH=x86_64

The patch below adds some features to it:
- it allows to import any environment variable by specifying option env=...
- it generates a dependency on it, so the kernel config is updated if it 
changes.

Please revert the K64BIT changes and use this instead.

 The code is a copy of what happen when reading a all.config file and
 the functionality should be equal.
 Can we make that part simpler too?

These are two different uses, when reading a .config only the basic syntax 
is checked, but not the value itself.

 By the way - I have never understood the purpose of the flags (S_DEF_USER 
 etc.)
 Can we have a few comments added to their definition?

It allows to hold multiple configs, a user of it is conf_split_config() 
which loads another config and compares to the current config and updates 
the files under include/config as needed.
It could also be used by front ends to display what actually changed 
compared to e.g. the saved config.

 One of the blockers are that kconfig does not support more than one prompt
 for a choice symbol. Is this something you can fix - or sketch how to fix it?

The basic idea is to add a name to the choice, so multiple choices can be 
grouped together. This requires some changes to the dependency check to 
make sure one choice option doesn't depend on another (which is currently 
enforced by the syntax).

bye, Roman


Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 init/Kconfig |4 ++
 scripts/kconfig/expr.c   |   16 +-
 scripts/kconfig/expr.h   |5 +--
 scripts/kconfig/lkc.h|5 +++
 scripts/kconfig/menu.c   |7 +++-
 scripts/kconfig/qconf.cc |   15 ++---
 scripts/kconfig/symbol.c |   53 +--
 scripts/kconfig/util.c   |   25 +++-
 scripts/kconfig/zconf.gperf  |1 
 scripts/kconfig/zconf.hash.c_shipped |   45 -
 10 files changed, 123 insertions(+), 53 deletions(-)

Index: linux-2.6/init/Kconfig
===
--- linux-2.6.orig/init/Kconfig
+++ linux-2.6/init/Kconfig
@@ -7,6 +7,10 @@ config DEFCONFIG_LIST
default /boot/config-$UNAME_RELEASE
default arch/$ARCH/defconfig
 
+config ARCH
+   string
+   option env=ARCH
+
 menu General setup
 
 config EXPERIMENTAL
Index: linux-2.6/scripts/kconfig/expr.c
===
--- linux-2.6.orig/scripts/kconfig/expr.c
+++ linux-2.6/scripts/kconfig/expr.c
@@ -87,7 +87,7 @@ struct expr *expr_copy(struct expr *org)
break;
case E_AND:
case E_OR:
-   case E_CHOICE:
+   case E_LIST:
e-left.expr = expr_copy(org-left.expr);
e-right.expr = expr_copy(org-right.expr);
break;
@@ -217,7 +217,7 @@ int expr_eq(struct expr *e1, struct expr
expr_free(e2);
trans_count = old_count;
return res;
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -648,7 +648,7 @@ struct expr *expr_transform(struct expr 
case E_EQUAL:
case E_UNEQUAL:
case E_SYMBOL:
-   case E_CHOICE:
+   case E_LIST:
break;
default:
e-left.expr = expr_transform(e-left.expr);
@@ -932,7 +932,7 @@ struct expr *expr_trans_compare(struct e
break;
case E_SYMBOL:
return expr_alloc_comp(type, e-left.sym, sym);
-   case E_CHOICE:
+   case E_LIST:
case E_RANGE:
case E_NONE:
/* panic */;
@@ -1000,9 +1000,9 @@ int expr_compare_type(enum expr_type t1,
if (t2 == E_OR)
return 1;
case E_OR:
-   if (t2 == E_CHOICE)
+   if (t2 == E_LIST)
return 1;
-   case E_CHOICE:
+   case E_LIST:
if (t2 == 0)
return 1;
default:
@@ -1053,11 +1053,11 @@ void expr_print(struct expr *e, void (*f
fn(data, NULL,   );
expr_print(e-right.expr, fn, data, E_AND);
break;
-   case E_CHOICE:
+   case E_LIST:
fn(data, e-right.sym, e-right.sym-name);
if (e-left.expr) {
fn(data, NULL,  ^ );
-   expr_print(e-left.expr, fn, data, E_CHOICE

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

   The value can be supplied on the command-line so we need to validate 
   input.
  
  Is there a need for this?
 Yes. We would like to set 64BIT or not in other than x86 cases.
 And way forward was not to override ARCH as in the x86 case.

Can we please can get some consistency in this?
We have a .config file for a reason, what's wrong with using it?

  Please revert the K64BIT changes and use this instead.
 
 I will finish up your patch and target it for next merge window.

Why can't this be fixed properly now? You don't even need this patch, just 
use ARCH to set 64BIT in the Kconfig as I've shown.

  These are two different uses, when reading a .config only the basic syntax 
  is checked, but not the value itself.
 This is wrong considering the amount of people that hand edit the .config 
 file.

It's not, the actual symbol value is set later depending on the dependency 
constraints.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

  Can we please can get some consistency in this?
  We have a .config file for a reason, what's wrong with using it?
 
 We need to set a selected few values in a few cases where we do
 not have a .config file.
 allmodconfig for x86 for instance. We would like to generate a
 32-bit and a 64-bit version.

This can already be set via all.config/allmod.config.
Where is this need coming from? The point is that I don't like to add an 
interface, which is maybe used by two people, who should be perfectly 
capable using an existing superior mechanism.

Please revert the K64BIT changes and use this instead.
   
   I will finish up your patch and target it for next merge window.
  
  Why can't this be fixed properly now? You don't even need this patch, just 
  use ARCH to set 64BIT in the Kconfig as I've shown.
 Because the patch is in mainline and has been tested by a lot of people
 during the last week. And as the functionality is almost equal I do not
 see it as a big deal to have the less-perfect solution in one kernel release.
 
 And the only reason the patch were applied to mainline was to fix the build
 of the merged x86 architecute - otherwise it was in no way -rc material.

I showed you a solution, which requires no patch at all, while your patch 
adds extra functionality which is questionable.
Why is a quick hack preferable over a proper solution? 
Either explain why my solution isn't usable or _please_ revert the K64BIT 
changes.

These are two different uses, when reading a .config only the basic 
syntax 
is checked, but not the value itself.
   This is wrong considering the amount of people that hand edit the .config 
   file.
  
  It's not, the actual symbol value is set later depending on the dependency 
  constraints.
 My point is that the .config file is handedited so the syntax should be 
 checked
 to best possible extent. If someone specify CONFIG_64BIT=64 we should error 
 out.

The other function doesn't complain about it either. There is already 
only limited error checking, e.g. there is no complaint that the value 
isn't really set (because it was selected by something else), otherwise 
the .config check during a kernel upgrades would be a lot noisier than it 
already is. Anyone directly editing .config should know what he is doing.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Thu, 15 Nov 2007, Sam Ravnborg wrote:

 You suggest just to check ARCH value and not apply your patch. This was
 not my initial understanding as was hopefully obvious from my reply.

This patch only adds some extra features.

 If user did NOT specify ARCH we should use the kernel configuration - which
 your solution fail to do.

To make this easy I attached the patch which reverts the problematic 
changes and then you only need this simple change to force the 64BIT value 
for ARCH={i386,x86_64}, otherwise it's set by the user:

bye, Roman

Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 Makefile |3 ++-
 arch/x86/Kconfig |4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -165,7 +165,8 @@ export srctree objtree VPATH TOPDIR
 # then ARCH is assigned, getting whatever value it gets normally, and 
 # SUBARCH is subsequently ignored.
 
-SUBARCH := $(shell uname -m | sed -e s/i.86/i386/ -e s/sun4u/sparc64/ \
+SUBARCH := $(shell uname -m | sed -e s/i.86/x86/ -e s/x86_64/x86/ \
+ -e s/sun4u/sparc64/ \
  -e s/arm.*/arm/ -e s/sa110/arm/ \
  -e s/s390x/s390/ -e s/parisc64/parisc/ \
  -e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
Index: linux-2.6/arch/x86/Kconfig
===
--- linux-2.6.orig/arch/x86/Kconfig
+++ linux-2.6/arch/x86/Kconfig
@@ -3,8 +3,8 @@ mainmenu Linux Kernel Configuration for
 
 # Select 32 or 64 bit
 config 64BIT
-   bool 64-bit kernel
-   default n
+   bool 64-bit kernel if ARCH=x86
+   default ARCH=x86_64
help
  Say yes to build a 64-bit kernel - formerly known as x86_64
  Say no to build a 32-bit kernel - formerly known as i386Revert

9c900a9c9d9351e55ab6a84e12e3a52c474c7c8b
0f855aa64b3f63d35a891510cf7db932a435c116
2a113281f5cd2febbab21a93c8943f8d3eece4d3

and K64BIT parts of

daa93fab824f2b8c35bd11670c7fab2f32b2de5f

Signed-off-by: Roman Zippel [EMAIL PROTECTED]

---
 Makefile|4 -
 README  |2 
 scripts/kconfig/conf.c  |1 
 scripts/kconfig/confdata.c  |  146 
 scripts/kconfig/lkc_proto.h |1 
 5 files changed, 56 insertions(+), 98 deletions(-)

Index: linux-2.6/Makefile
===
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -200,11 +200,9 @@ SRCARCH:= $(ARCH)
 # Additional ARCH settings for x86
 ifeq ($(ARCH),i386)
 SRCARCH := x86
-K64BIT  := n
 endif
 ifeq ($(ARCH),x86_64)
 SRCARCH := x86
-K64BIT  := y
 endif
 
 KCONFIG_CONFIG ?= .config
@@ -341,7 +339,7 @@ KERNELRELEASE = $(shell cat include/conf
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
 
 export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH SRCARCH K64BIT CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD 
CC
+export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP MAKE AWK GENKSYMS PERL UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
Index: linux-2.6/README
===
--- linux-2.6.orig/README
+++ linux-2.6/README
@@ -194,8 +194,6 @@ CONFIGURING the kernel:
make *config checks for a file named all{yes/mod/no/random}.config
for symbol values that are to be forced.  If this file is not found,
it checks for a file named all.config to contain forced values.
-   Finally it checks the environment variable K64BIT and if found, sets
-   the config symbol 64BIT to the value of the K64BIT variable.

NOTES on make config:
- having unnecessary drivers will make the kernel bigger, and can
Index: linux-2.6/scripts/kconfig/conf.c
===
--- linux-2.6.orig/scripts/kconfig/conf.c
+++ linux-2.6/scripts/kconfig/conf.c
@@ -591,7 +591,6 @@ int main(int ac, char **av)
conf_read_simple(name, S_DEF_USER);
else if (!stat(all.config, tmpstat))
conf_read_simple(all.config, S_DEF_USER);
-   conf_set_env_sym(K64BIT, 64BIT, S_DEF_USER);
break;
default:
break;
Index: linux-2.6/scripts/kconfig/confdata.c
===
--- linux-2.6.orig/scripts/kconfig/confdata.c
+++ linux-2.6/scripts/kconfig/confdata.c
@@ -83,95 +83,6 @@ char *conf_get_default_confname(void)
return name;
 }
 
-static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char 
*p)
-{
-   char *p2;
-
-   switch (sym-type) {
-   case

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Sat, 10 Nov 2007, Sam Ravnborg wrote:

> + if (p) {
> + char warning[100];
> + sprintf(warning, "Environment variable (%s = \"%s\")", env, p);
> + conf_filename = warning;
> + def_flags = SYMBOL_DEF << def;
> + if (def == S_DEF_USER) {
> + sym = sym_find(symname);
> + if (!sym)
> + return 1;
> + } else {
> + sym = sym_lookup(symname, 0);
> + if (sym->type == S_UNKNOWN)
> + sym->type = S_OTHER;
> + }
> + conf_set_sym_val(sym, def, def_flags, p);
> + }
> + return 0;

This is more complex than necessary, this should be enough:

sym = sym_find(symname);
if (sym)
sym_set_string_value(sym, p);

This is not a direct user interface, so the potential stricter error 
checking is not really needed.

In general I think it's problematic that this is only checked, when the 
config system is called, i.e. with a configured kernel adding ARCH would 
have no effect, what makes this more confusing is that one can later omit 
the ARCH variable, since it's saved in the .config. I think it would be 
better to check for this directly in the Makefile and then use a separate 
tool to set the variable directly (which could be simply sed or a simple 
helper program).

bye, Roman
-
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/

Re: [PATCH 0/11 v3] enable "make ARCH=x86"

[Roman Zippel]

Hi,

On Fri, 9 Nov 2007, Jeff Garzik wrote:

> > > I switch between other cross-compiled arches (alpha, usually) on the
> > > makefile command line
> > > 
> > > Yes, I know other 32/64-bit arches require .config editing.  That
> > > doesn't change the basic fact that this is a workflow regression.
> > > 
> > > Jeff
> > 
> > You can use:
> > 
> > make i386_defconfig
> > make x86_64_defconfig
> 
> Does that work for alpha too?
> 
> 
> > In any other case you'd be editing the .config anyways.
> 
> No, that's a logic rathole down which I will not follow :)
> 
> You can make any argument along those lines command line usage is really an
> art, not a science.  Its a user interface, and that involves human taste
> rather than logic.
> 
> I've been bouncing between architectures using ARCH= for years, and my fingers
> and brain have been trained.  It's just disappointing and a pain to change
> this nice user interface that has served so well for years.

I disagree that this is a regression. You can still bounce between archs 
as before, but the fact is that these are not separate archs anymore. The 
sooner we'll get used to the fact the better.
You also don't bounce between archs just by changing ARCH, you also have 
to reconfigure the kernel and that's there you can change the 64bit 
option. This means for the normal user not much is changing and from an 
experienced user I'd expect to know the difference.

If we look at this as a new feature, we have to look at what is needed to 
support this properly. Should the arch name imply certain config options? 
This wouldn't have to be limited to 64BIT - SMP or MMU could be other 
options. I think it would also make sense to hide the corresponding config 
option, otherwise one could change an option, which is a little later 
ignored anyway. Another question would be if and how it affects other 
information like uname information.

What I'd like to avoid is to add potential cruft, which is only used to 
avoid the inevitable to properly learn how to configure the kernel. A user 
interface has a lot to do with logic, especially consistency - an 
inconsistent mess also doesn't taste very good.

bye, Roman
-
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/

Re: [PATCH 0/11 v3] enable make ARCH=x86

[Roman Zippel]

Hi,

On Fri, 9 Nov 2007, Jeff Garzik wrote:

   I switch between other cross-compiled arches (alpha, usually) on the
   makefile command line
   
   Yes, I know other 32/64-bit arches require .config editing.  That
   doesn't change the basic fact that this is a workflow regression.
   
   Jeff
  
  You can use:
  
  make i386_defconfig
  make x86_64_defconfig
 
 Does that work for alpha too?
 
 
  In any other case you'd be editing the .config anyways.
 
 No, that's a logic rathole down which I will not follow :)
 
 You can make any argument along those lines command line usage is really an
 art, not a science.  Its a user interface, and that involves human taste
 rather than logic.
 
 I've been bouncing between architectures using ARCH= for years, and my fingers
 and brain have been trained.  It's just disappointing and a pain to change
 this nice user interface that has served so well for years.

I disagree that this is a regression. You can still bounce between archs 
as before, but the fact is that these are not separate archs anymore. The 
sooner we'll get used to the fact the better.
You also don't bounce between archs just by changing ARCH, you also have 
to reconfigure the kernel and that's there you can change the 64bit 
option. This means for the normal user not much is changing and from an 
experienced user I'd expect to know the difference.

If we look at this as a new feature, we have to look at what is needed to 
support this properly. Should the arch name imply certain config options? 
This wouldn't have to be limited to 64BIT - SMP or MMU could be other 
options. I think it would also make sense to hide the corresponding config 
option, otherwise one could change an option, which is a little later 
ignored anyway. Another question would be if and how it affects other 
information like uname information.

What I'd like to avoid is to add potential cruft, which is only used to 
avoid the inevitable to properly learn how to configure the kernel. A user 
interface has a lot to do with logic, especially consistency - an 
inconsistent mess also doesn't taste very good.

bye, Roman
-
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/

Re: [PATCH] kconfig: use $K64BIT to set 64BIT with all*config targets

[Roman Zippel]

Hi,

On Sat, 10 Nov 2007, Sam Ravnborg wrote:

 + if (p) {
 + char warning[100];
 + sprintf(warning, Environment variable (%s = \%s\), env, p);
 + conf_filename = warning;
 + def_flags = SYMBOL_DEF  def;
 + if (def == S_DEF_USER) {
 + sym = sym_find(symname);
 + if (!sym)
 + return 1;
 + } else {
 + sym = sym_lookup(symname, 0);
 + if (sym-type == S_UNKNOWN)
 + sym-type = S_OTHER;
 + }
 + conf_set_sym_val(sym, def, def_flags, p);
 + }
 + return 0;

This is more complex than necessary, this should be enough:

sym = sym_find(symname);
if (sym)
sym_set_string_value(sym, p);

This is not a direct user interface, so the potential stricter error 
checking is not really needed.

In general I think it's problematic that this is only checked, when the 
config system is called, i.e. with a configured kernel adding ARCH would 
have no effect, what makes this more confusing is that one can later omit 
the ARCH variable, since it's saved in the .config. I think it would be 
better to check for this directly in the Makefile and then use a separate 
tool to set the variable directly (which could be simply sed or a simple 
helper program).

bye, Roman
-
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/

Re: [PATCH 0/5] introduce K64BIT=y and backward compatibility ARCH={i386,x86_64} for x86

[Roman Zippel]

Hi,

On Sat, 10 Nov 2007, Sam Ravnborg wrote:

> As discussed in another thread the right thing is to add a generic solution
> to select between 32 and 64 bit - useable for powerpc, s390, ppc et al.

Could you please point me to this discussion?
Thanks.

bye, Roman
-
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/

Re: [PATCH 0/5] introduce K64BIT=y and backward compatibility ARCH={i386,x86_64} for x86

[Roman Zippel]

Hi,

On Sat, 10 Nov 2007, Sam Ravnborg wrote:

 As discussed in another thread the right thing is to add a generic solution
 to select between 32 and 64 bit - useable for powerpc, s390, ppc et al.

Could you please point me to this discussion?
Thanks.

bye, Roman
-
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/

Re: CLOCK_TICK_RATE in NTP code

[Roman Zippel]

Hi,

On Thursday 01 November 2007, Ralf Baechle wrote:

> kernel/time/ntp.c contains the following piece of code:
>
> #define CLOCK_TICK_OVERFLOW (LATCH * HZ - CLOCK_TICK_RATE)
> #define CLOCK_TICK_ADJUST   (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC)
> / \ (s64)CLOCK_TICK_RATE)
>
> static void ntp_update_frequency(void)
> {
> u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
> << TICK_LENGTH_SHIFT;
> second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
> second_length += (s64)time_freq << (TICK_LENGTH_SHIFT -
> SHIFT_NSEC);
>
> tick_length_base = second_length;
>
> do_div(second_length, HZ);
> tick_nsec = second_length >> TICK_LENGTH_SHIFT;
>
> do_div(tick_length_base, NTP_INTERVAL_FREQ);
> }
>
> So it uses CLOCK_TICK_RATE which on many systems but not all is defined to
> the i8253 input clock.  But timekeeping on anything remotely modern makes
> little use of the i8253 so I wonder the intent was here.

The basic idea is to provide a base frequency adjustment, when I wrote this I 
already wasn't entirely happy that it was hardcoded like this, but in the end 
I simply reimplemented what the old code did.
It's not strictly needed, so if someone wants to add something like:

#ifndef CLOCK_TICK_RATE
#define CLOCK_TICK_ADJUST 0
#else
...

it would be fine with me.

bye, Roman
-
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/

Re: CLOCK_TICK_RATE in NTP code

[Roman Zippel]

Hi,

On Thursday 01 November 2007, Ralf Baechle wrote:

 kernel/time/ntp.c contains the following piece of code:

 #define CLOCK_TICK_OVERFLOW (LATCH * HZ - CLOCK_TICK_RATE)
 #define CLOCK_TICK_ADJUST   (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC)
 / \ (s64)CLOCK_TICK_RATE)

 static void ntp_update_frequency(void)
 {
 u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
  TICK_LENGTH_SHIFT;
 second_length += (s64)CLOCK_TICK_ADJUST  TICK_LENGTH_SHIFT;
 second_length += (s64)time_freq  (TICK_LENGTH_SHIFT -
 SHIFT_NSEC);

 tick_length_base = second_length;

 do_div(second_length, HZ);
 tick_nsec = second_length  TICK_LENGTH_SHIFT;

 do_div(tick_length_base, NTP_INTERVAL_FREQ);
 }

 So it uses CLOCK_TICK_RATE which on many systems but not all is defined to
 the i8253 input clock.  But timekeeping on anything remotely modern makes
 little use of the i8253 so I wonder the intent was here.

The basic idea is to provide a base frequency adjustment, when I wrote this I 
already wasn't entirely happy that it was hardcoded like this, but in the end 
I simply reimplemented what the old code did.
It's not strictly needed, so if someone wants to add something like:

#ifndef CLOCK_TICK_RATE
#define CLOCK_TICK_ADJUST 0
#else
...

it would be fine with me.

bye, Roman
-
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/

Re: [PATCH] proc_fs.h redux

[Roman Zippel]

Hi,

On Sunday 28 October 2007, Russell King wrote:

> On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote:
> > and forward declarations of
> >
> > struct proc_dir_entry;
> > struct file_operations;
> >
> > As a general rule, I think it better to use includes
> > than use naked forward declarations.
>
> If you go down that route, you end up with _lots_ of circular
> dependencies - header file X needs Y needs Z which needs X.  We've
> been there, several times.  It very quickly becomes quite
> unmaintainable - you end up with hard to predict behaviour from
> include files.
>
> The only realistic solution is to use forward declarations.

It's unfortunately more complicated than that. What basically needs to be done 
is to separate declarations from its users (usually inline functions). The 
problem is to correctly pull the declarations from the  and 
 header in the correct order. A typical mistake is to put 
declarations and inline functions in the same linux header and then include 
some additional from an asm header. For most high level header it's not much 
of a problem, but let's take  as example. struct list_head is 
used everywhere, but just to get this one definition one also gets quite a 
few other dependencies as well.

bye, Roman
-
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/

Re: [PATCH] proc_fs.h redux

[Roman Zippel]

Hi,

On Sunday 28 October 2007, Russell King wrote:

 On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote:
  and forward declarations of
 
  struct proc_dir_entry;
  struct file_operations;
 
  As a general rule, I think it better to use includes
  than use naked forward declarations.

 If you go down that route, you end up with _lots_ of circular
 dependencies - header file X needs Y needs Z which needs X.  We've
 been there, several times.  It very quickly becomes quite
 unmaintainable - you end up with hard to predict behaviour from
 include files.

 The only realistic solution is to use forward declarations.

It's unfortunately more complicated than that. What basically needs to be done 
is to separate declarations from its users (usually inline functions). The 
problem is to correctly pull the declarations from the asm/... and 
linux/... header in the correct order. A typical mistake is to put 
declarations and inline functions in the same linux header and then include 
some additional from an asm header. For most high level header it's not much 
of a problem, but let's take linux/list.h as example. struct list_head is 
used everywhere, but just to get this one definition one also gets quite a 
few other dependencies as well.

bye, Roman
-
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/

Re: Kconfig: conf segfault (with invalid kconfig contents)

[Roman Zippel]

Hi,

On Thursday 25 October 2007, Sam Ravnborg wrote:

> > It's clearly invalid in that it depends on what it selects, but it should
> > just abort instead.
>
> Thanks for the report and especially for the testcase!
> I will try to look at it a bit later if noone bites me (I'm afraid not).

Well, you're also responsible for it. :)
http://lkml.org/lkml/2007/5/6/14

It actually finds the recursive dependency, but it crashes because sym->prop 
is NULL.

bye, Roman
-
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/

Re: tristate and bool not enogh for Kconfig anymore

[Roman Zippel]

Hi,

On Monday 22 October 2007, Randy Dunlap wrote:

> ~
> Another common idiom that we see (and sometimes have problems
> with) is this:
>
> When B (module or subsystem) uses interfaces from A (module or
> subsystem), A can be linked statically into the kernel image or
> can be built as loadable module(s).   This limits how B can be
> built.  If A is linked statically into the kernel image, B can be
> built statically or as loadable module(s).  However, if A is built
> as loadable module(s), then B must be restricted to loadable
> module(s) also.  This can be expressed in kconfig language as:
>
> config B
>   depends on A = y || A = B

What you describe is a simple "depends on A" and your example won't work 
because it adds a recursive dependency.

bye, Roman
-
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/

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

[Roman Zippel]

Hi,

On Sun, 28 Oct 2007, Alexey Dobriyan wrote:

> > If it's just about the type checking, something like below should pretty 
> > much do the same.
> 
> It won't catch, the following if both variables are unsigned long:
> 
>   spin_lock_irqsave(, flags);
>   [stuff]
>   spin_unlock_irqrestore(, foo->flags);
> 
> It won't catch "static unsigned long flags;". With sparse, we can
> eventually mark type as "on-stack only".

It should be on the stack, but we have cases where a pointer to it is used 
(e.g. lock_timer_base). How do you want to deal with this?

bye, Roman
-
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/

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

[Roman Zippel]

Hi,

On Sun, 21 Oct 2007, Alexey Dobriyan wrote:

> So far remedies were:
> a) grep(1) -- obviously fragile. I tried at some point grepping for
>spin_lock_irqsave(), found quite a few, but it became bring quickly.
> b) BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)) -- was tried,
>brutally broke some arches, survived one commit before revert :^)
>Doesn't work on i386 where sizeof(unsigned int) == sizeof(unsigned long).
> 
> So it would be nice to have something more robust.

If it's just about the type checking, something like below should pretty 
much do the same.

> * irq_flags_t allows arch maintainers to eventually switch to something
>   smaller than "unsigned long" if they want to.

Considering how painful this conversion could be, the question is whether 
this is really needed. Comments so far suggest some archs don't need all 
of it, but does someone need more?

bye, Roman

---
 include/linux/irqflags.h |   11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/irqflags.h
===
--- linux-2.6.orig/include/linux/irqflags.h
+++ linux-2.6/include/linux/irqflags.h
@@ -41,6 +41,10 @@
 # define INIT_TRACE_IRQFLAGS
 #endif
 
+static __always_inline void __irq_flags_check(unsigned long *flags)
+{
+}
+
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
 
 #include 
@@ -50,10 +54,11 @@
 #define local_irq_disable() \
do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
 #define local_irq_save(flags) \
-   do { raw_local_irq_save(flags); trace_hardirqs_off(); } while (0)
+   do { __irq_flags_check(); raw_local_irq_save(flags); 
trace_hardirqs_off(); } while (0)
 
 #define local_irq_restore(flags)   \
do {\
+   __irq_flags_check();  \
if (raw_irqs_disabled_flags(flags)) {   \
raw_local_irq_restore(flags);   \
trace_hardirqs_off();   \
@@ -69,8 +74,8 @@
  */
 # define raw_local_irq_disable()   local_irq_disable()
 # define raw_local_irq_enable()local_irq_enable()
-# define raw_local_irq_save(flags) local_irq_save(flags)
-# define raw_local_irq_restore(flags)  local_irq_restore(flags)
+# define raw_local_irq_save(flags) ({ __irq_flags_check(); 
local_irq_save(flags); })
+# define raw_local_irq_restore(flags)  ({ __irq_flags_check(); 
local_irq_restore(flags); })
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
-
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/

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

[Roman Zippel]

Hi,

On Sun, 21 Oct 2007, Alexey Dobriyan wrote:

 So far remedies were:
 a) grep(1) -- obviously fragile. I tried at some point grepping for
spin_lock_irqsave(), found quite a few, but it became bring quickly.
 b) BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)) -- was tried,
brutally broke some arches, survived one commit before revert :^)
Doesn't work on i386 where sizeof(unsigned int) == sizeof(unsigned long).
 
 So it would be nice to have something more robust.

If it's just about the type checking, something like below should pretty 
much do the same.

 * irq_flags_t allows arch maintainers to eventually switch to something
   smaller than unsigned long if they want to.

Considering how painful this conversion could be, the question is whether 
this is really needed. Comments so far suggest some archs don't need all 
of it, but does someone need more?

bye, Roman

---
 include/linux/irqflags.h |   11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/irqflags.h
===
--- linux-2.6.orig/include/linux/irqflags.h
+++ linux-2.6/include/linux/irqflags.h
@@ -41,6 +41,10 @@
 # define INIT_TRACE_IRQFLAGS
 #endif
 
+static __always_inline void __irq_flags_check(unsigned long *flags)
+{
+}
+
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
 
 #include asm/irqflags.h
@@ -50,10 +54,11 @@
 #define local_irq_disable() \
do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
 #define local_irq_save(flags) \
-   do { raw_local_irq_save(flags); trace_hardirqs_off(); } while (0)
+   do { __irq_flags_check(flags); raw_local_irq_save(flags); 
trace_hardirqs_off(); } while (0)
 
 #define local_irq_restore(flags)   \
do {\
+   __irq_flags_check(flags);  \
if (raw_irqs_disabled_flags(flags)) {   \
raw_local_irq_restore(flags);   \
trace_hardirqs_off();   \
@@ -69,8 +74,8 @@
  */
 # define raw_local_irq_disable()   local_irq_disable()
 # define raw_local_irq_enable()local_irq_enable()
-# define raw_local_irq_save(flags) local_irq_save(flags)
-# define raw_local_irq_restore(flags)  local_irq_restore(flags)
+# define raw_local_irq_save(flags) ({ __irq_flags_check(flags); 
local_irq_save(flags); })
+# define raw_local_irq_restore(flags)  ({ __irq_flags_check(flags); 
local_irq_restore(flags); })
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
-
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/

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

[Roman Zippel]

Hi,

On Sun, 28 Oct 2007, Alexey Dobriyan wrote:

  If it's just about the type checking, something like below should pretty 
  much do the same.
 
 It won't catch, the following if both variables are unsigned long:
 
   spin_lock_irqsave(lock, flags);
   [stuff]
   spin_unlock_irqrestore(lock, foo-flags);
 
 It won't catch static unsigned long flags;. With sparse, we can
 eventually mark type as on-stack only.

It should be on the stack, but we have cases where a pointer to it is used 
(e.g. lock_timer_base). How do you want to deal with this?

bye, Roman
-
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/

Re: tristate and bool not enogh for Kconfig anymore

[Roman Zippel]

Hi,

On Monday 22 October 2007, Randy Dunlap wrote:

 ~
 Another common idiom that we see (and sometimes have problems
 with) is this:

 When B (module or subsystem) uses interfaces from A (module or
 subsystem), A can be linked statically into the kernel image or
 can be built as loadable module(s).   This limits how B can be
 built.  If A is linked statically into the kernel image, B can be
 built statically or as loadable module(s).  However, if A is built
 as loadable module(s), then B must be restricted to loadable
 module(s) also.  This can be expressed in kconfig language as:

 config B
   depends on A = y || A = B

What you describe is a simple depends on A and your example won't work 
because it adds a recursive dependency.

bye, Roman
-
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/

Re: Kconfig: conf segfault (with invalid kconfig contents)

[Roman Zippel]

Hi,

On Thursday 25 October 2007, Sam Ravnborg wrote:

  It's clearly invalid in that it depends on what it selects, but it should
  just abort instead.

 Thanks for the report and especially for the testcase!
 I will try to look at it a bit later if noone bites me (I'm afraid not).

Well, you're also responsible for it. :)
http://lkml.org/lkml/2007/5/6/14

It actually finds the recursive dependency, but it crashes because sym-prop 
is NULL.

bye, Roman
-
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/

Re: [patch 3/8] m68k: ignore restart_syscall

[Roman Zippel]

Hi,

On Sat, 13 Oct 2007, Geert Uytterhoeven wrote:

> m68k: ignore restart_syscall, which is not needed on m68k.

It's somewhat needed, but nobody has gotten around to actually implement 
it yet...

bye, Roman
-
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/

Re: [patch 3/8] m68k: ignore restart_syscall

[Roman Zippel]

Hi,

On Sat, 13 Oct 2007, Geert Uytterhoeven wrote:

 m68k: ignore restart_syscall, which is not needed on m68k.

It's somewhat needed, but nobody has gotten around to actually implement 
it yet...

bye, Roman
-
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/

Re: [kbuild-devel] A bit of kconfig rewrite (Re: [PATCH] 9p: fix compile error if !CONFIG_SYSCTL)

[Roman Zippel]

Hi,

On Mon, 1 Oct 2007, Oleg Verych wrote:

> Today's kconfig was proposed and accepted in a very unpleasant
> circumstances, has very poor design, development and no working
> alternative (for 5+ years now).

If you want to make such statements, you have to offer a little more than 
the hot air you're producing right now...
If you want to improve the design, you're more than welcome. I'm the first 
one to admit that there's still lots of room for improvement, but if you 
want to claim this can only be done via a rewrite, then you have to be 
a lot more specific what's wrong the current design and why it's 
unfixable.
Quite some thought has been put into this design and if you were a little 
more specific, I could actually tell you why it is this way and maybe how 
to improve it incrementally instead of trying to reinvent everything.

>   + shell-like[0] (not like CML1, which is just shell) scripting, allowing
> to extend easily (if there is no one available) capabilities,
> config values or actions for particular sub-system or compilation
> unit,

Just to pick this one point as example: I like scripting and maybe I 
should just update the swig wrapper script I already have and merge it, 
which would make it easier to play with the kconfig database in whatever 
language you like.
OTOH due to the necessary build dependencies I don't see this become a 
mandatory feature, so unless there is a compelling reason a certain set of 
base function will remain in C.

bye, Roman
-
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/

Re: [PATCH 2.6.23-rc4] qconf ("make xconfig") Search Dialog Enhancement (rev8)

[Roman Zippel]

Hi,

On Thu, 20 Sep 2007, Shlomi Fish wrote:

> Which specific problems do you see with the coding style of the patch? Can 
> you 
> comment on it?

Mostly whitespace around any braces, please keep it close to the other 
source.

> > I would also prefer to move more of the search functionality into the
> > generic code, so it can be used by other front ends as well, otherwise a
> > lot of this had to be duplicated.
> 
> That would be a good idea, but I cannot use Qt there, which makes my job 
> harder.

Where is the problem with implementing it in C? Just try to keep it a 
simple at first.

> > I think a filter function makes it maybe a bit to flexible, if a front
> > end wants to do some weird filtering, it can still access the symbol
> > data base directly. 
> 
> A filter function would still be convenient in this context, as the symbol 
> data base API may change, and the filter function has a little logic in it.

This API is not really fixed at the moment, so it's not really a problem.

> > So what I have in mind is something like this: 
> >
> > struct symbol **sym_generic_search(const char *pattern, unsigned int
> > flags);
> >
> > This means the back end provides a basic search facility for the most
> > common search operations. The flags would specify what to search (e.g.
> > symbol name, help text, prompts) and how to do it.
> 
> I suggest we don't call it sym_generic_search, as generic implies it is a 
> generic filter. We can call it "sym_string_search" or whatever. Then, I 
> suggest we have separate arguments for every parameter (i.e: search type, 
> case sensitivity, what to search, etc.).

I don't care much about the name, but please keep it as a simple flag, 
which is a lot easier to extend.

bye, Roman

-
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/

Re: [PATCH 2.6.23-rc4] qconf (make xconfig) Search Dialog Enhancement (rev8)

[Roman Zippel]

Hi,

On Thu, 20 Sep 2007, Shlomi Fish wrote:

 Which specific problems do you see with the coding style of the patch? Can 
 you 
 comment on it?

Mostly whitespace around any braces, please keep it close to the other 
source.

  I would also prefer to move more of the search functionality into the
  generic code, so it can be used by other front ends as well, otherwise a
  lot of this had to be duplicated.
 
 That would be a good idea, but I cannot use Qt there, which makes my job 
 harder.

Where is the problem with implementing it in C? Just try to keep it a 
simple at first.

  I think a filter function makes it maybe a bit to flexible, if a front
  end wants to do some weird filtering, it can still access the symbol
  data base directly. 
 
 A filter function would still be convenient in this context, as the symbol 
 data base API may change, and the filter function has a little logic in it.

This API is not really fixed at the moment, so it's not really a problem.

  So what I have in mind is something like this: 
 
  struct symbol **sym_generic_search(const char *pattern, unsigned int
  flags);
 
  This means the back end provides a basic search facility for the most
  common search operations. The flags would specify what to search (e.g.
  symbol name, help text, prompts) and how to do it.
 
 I suggest we don't call it sym_generic_search, as generic implies it is a 
 generic filter. We can call it sym_string_search or whatever. Then, I 
 suggest we have separate arguments for every parameter (i.e: search type, 
 case sensitivity, what to search, etc.).

I don't care much about the name, but please keep it as a simple flag, 
which is a lot easier to extend.

bye, Roman

-
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/

Re: [kbuild-devel] A bit of kconfig rewrite (Re: [PATCH] 9p: fix compile error if !CONFIG_SYSCTL)

[Roman Zippel]

Hi,

On Mon, 1 Oct 2007, Oleg Verych wrote:

 Today's kconfig was proposed and accepted in a very unpleasant
 circumstances, has very poor design, development and no working
 alternative (for 5+ years now).

If you want to make such statements, you have to offer a little more than 
the hot air you're producing right now...
If you want to improve the design, you're more than welcome. I'm the first 
one to admit that there's still lots of room for improvement, but if you 
want to claim this can only be done via a rewrite, then you have to be 
a lot more specific what's wrong the current design and why it's 
unfixable.
Quite some thought has been put into this design and if you were a little 
more specific, I could actually tell you why it is this way and maybe how 
to improve it incrementally instead of trying to reinvent everything.

   + shell-like[0] (not like CML1, which is just shell) scripting, allowing
 to extend easily (if there is no one available) capabilities,
 config values or actions for particular sub-system or compilation
 unit,

Just to pick this one point as example: I like scripting and maybe I 
should just update the swig wrapper script I already have and merge it, 
which would make it easier to play with the kconfig database in whatever 
language you like.
OTOH due to the necessary build dependencies I don't see this become a 
mandatory feature, so unless there is a compelling reason a certain set of 
base function will remain in C.

bye, Roman
-
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/

Re: [PATCH v2] menuconfig: distinguish between selected-by-another options and comments

[Roman Zippel]

Hi,

On Sun, 16 Sep 2007, Sam Ravnborg wrote:

> > On Sun, 16 Sep 2007, Sam Ravnborg wrote:
> > 
> > > But can you take a look at distingushing between non-selectable options
> > > due to dependency issues and seleted-by symbols.
> > 
> > Do you have an example in mind? If a symbol is not changable, but still 
> > visible, a select is usually involved.
> 
> On i86_64 (but I think the arch does not matter).
> make defconfig
> make menuconfig
> 
> At top-level menu I see:
> --- Enable the block layer  --->
> 
> In block/Kconfig we have:
> menuconfig BLOCK
>bool "Enable the block layer" if EMBEDDED
>default y
> 
> If EMBEDDED == n then we see the above.
> And this was my first experience with this patch - and it
> took me some thoughts to realise it was the "if EMBEDDED" part
> that made it look like -*-

Indeed, I forgot about this case. Here it's actually the menu entry of a 
symbol that is forced to be visible, because a child entry is visible.

bye, Roman
-
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/

Re: [PATCH v3] menuconfig: distinguish between selected-by-another options and comments

[Roman Zippel]

Hi,

On Sun, 16 Sep 2007, Matej Laitl wrote:

> The v2 was maybe more intuitive, but had at least one flaw, where it claimed
> the option was selected by another, while it was in fact only made
> unchangeable by 'bool "Enable block layer" if EMBEDDED', defaulting to y.

The point is that I'm getting more concerned about overloading the 
interface with nontrivial information.
Another direction to consider would be to add this information to the help 
text, e.g. choose one syntax for nonchangable symbols and then the user 
can press help to find more detailed information.

> > This maximum value is overridden  
> > by the minimum value, so I wouldn't like it to be exported like this.
> 
> Where exactly does this happen?

sym_tristate_within_range() checks whether the new value is ok and 
sym_calc_value() uses rev_dep to override the value when necessary.

> There are cases when maximum < minimum, for
> example when you  THINKPAD_ACPI, then  BACKLIGHT_LCD_SUPPORT. After 
> that,
> BACKLIGHT_CLASS_DEVICE has minimum of 1 (selected by THINKPAD_ACPI) and 
> maximum
> 0 (depends on BACKLIGHT_LCD_SUPPORT, which is n)

The actual maximum value is then 1.

> The function names are maybe suboptimal, I agree.

The variable name is already correct, it's the visibility value of a 
symbol not its maximum. In the case of the "if EMBEDDED" then individual 
menu entries can still be visible, if any child entry is visible (see 
menu_is_visible()). 

bye, Roman
-
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/

Re: [PATCH v3] menuconfig: distinguish between selected-by-another options and comments

[Roman Zippel]

Hi,

On Sun, 16 Sep 2007, Matej Laitl wrote:

> +tristate sym_get_minimal_value(struct symbol *sym)
> +{
> + return sym->rev_dep.tri;
> +}
> +
> +tristate sym_get_maximal_value(struct symbol *sym)
> +{
> + return sym->visible;
> +}
> +

Right now I prefer the previous version. This maximum value is overridden 
by the minimum value, so I wouldn't like it to be exported like this.

I would really like to see an example, where the new changes make a 
difference.

bye, Roman
-
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/

Re: [PATCH] tristate choices with mixed tristate and boolean values

[Roman Zippel]

Hi,

On Mon, 10 Sep 2007, Jan Beulich wrote:

> --- linux-2.6.23-rc5/scripts/kconfig/menu.c   2007-09-07 16:48:23.0 
> +0200
> +++ 2.6.23-rc5-tristate-choices/scripts/kconfig/menu.c2007-09-03 
> 10:29:41.0 +0200
> @@ -235,16 +235,23 @@ void menu_finalize(struct menu *parent)
>   sym = parent->sym;
>   if (parent->list) {
>   if (sym && sym_is_choice(sym)) {
> - /* find the first choice value and find out choice type 
> */
> + /* find out choice type */
> + enum symbol_type type = S_UNKNOWN;
> +
>   for (menu = parent->list; menu; menu = menu->next) {
> - if (menu->sym) {
> - current_entry = parent;
> - menu_set_type(menu->sym->type);
> - current_entry = menu;
> - menu_set_type(sym->type);
> - break;
> + if (menu->sym && menu->sym->type != S_UNKNOWN) {
> + if (type == S_UNKNOWN)
> + type = menu->sym->type;
> + if (type != S_BOOLEAN)
> + break;
> + if (menu->sym->type == S_TRISTATE) {
> + type = S_TRISTATE;
> + break;
> + }
>   }
>   }
> + current_entry = parent;
> + menu_set_type(type);
>   parentdep = expr_alloc_symbol(sym);
>   } else if (parent->prompt)
>   parentdep = parent->prompt->visible.expr;

If I understand it correctly this makes possible to override the type of 
choice symbol, so this loop should only be done if the type is not 
S_UNKNOWN.

> @@ -253,7 +260,16 @@ void menu_finalize(struct menu *parent)
>  
>   for (menu = parent->list; menu; menu = menu->next) {
>   basedep = expr_transform(menu->dep);
> - basedep = expr_alloc_and(expr_copy(parentdep), basedep);
> + dep = parentdep;
> + if (sym && sym_is_choice(sym) && menu->sym) {
> + enum symbol_type type = menu->sym->type;
> +
> + if (type == S_UNKNOWN)
> + type = sym->type;
> +  if (type != S_TRISTATE)
> + dep = expr_alloc_comp(E_EQUAL, sym, 
> _yes);
> + }
> + basedep = expr_alloc_and(expr_copy(dep), basedep);
>   basedep = expr_eliminate_dups(basedep);
>   menu->dep = basedep;
>   if (menu->sym)

Hmm, if you want to do it this way, I'd prefer you add a parentdep_bool 
and simply do:

dep = (menu->sym && menu->sym->type != S_TRISTATE) ? 
parentdep_bool : parentdep;

Otherwise it looks ok.

bye, Roman
-
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/

Re: [PATCH v2] menuconfig: distinguish between selected-by-another options and comments

[Roman Zippel]

Hi,

On Sun, 16 Sep 2007, Sam Ravnborg wrote:

> But can you take a look at distingushing between non-selectable options
> due to dependency issues and seleted-by symbols.

Do you have an example in mind? If a symbol is not changable, but still 
visible, a select is usually involved.

bye, Roman
-
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/

Re: [PATCH v2] menuconfig: distinguish between selected-by-another options and comments

[Roman Zippel]

Hi,

On Sat, 15 Sep 2007, Matej Laitl wrote:

> menuconfig currently represents options implied by another option ('select'
> directive in Kconfig) by prefixing them with '---'. Unfortunately the same
> notation is used for comments.
> 
> This patch changes notation of selected-by-another items by introducing 2 new
> representations for implied options:
> {*} or {M} for options selected by another modularized one, thus builtin or
> module capable,
> -*- or -M- for options that cannot be at the moment changed by user.

I shortly considered doing something like this, but initially I wanted to
keep it simple. It's a compromise between overloading the user interface
with information and keeping it simple. That's the only concern I have
with it, but if everyone else likes I don't mind either. :)

> Signed-off-by: Matěj Laitl <[EMAIL PROTECTED]>

Signed-off-by: Roman Zippel <[EMAIL PROTECTED]>

bye, Roman

Re: [PATCH 2.6.23-rc4] qconf ("make xconfig") Search Dialog Enhancement (rev8)

[Roman Zippel]

Hi,

On Thu, 13 Sep 2007, Shlomi Fish wrote:

> This patch adds an option to use either substring match, regular expression 
> match, or keywords search in the "make xconfig" Edit->Find dialog.
> 
> It also allows searching on the "help" field of the menus as well as 
> the "name" of the symbol.
> 
> This change involved adding the sym_generic_search function to the LKC, and 
> implementing sym_re_search using it.

I like the direction, but first of all please fix the coding style.
I would also prefer to move more of the search functionality into the
generic code, so it can be used by other front ends as well, otherwise a
lot of this had to be duplicated.
I think a filter function makes it maybe a bit to flexible, if a front
end wants to do some weird filtering, it can still access the symbol
data base directly. So what I have in mind is something like this:

struct symbol **sym_generic_search(const char *pattern, unsigned int flags);

This means the back end provides a basic search facility for the most
common search operations. The flags would specify what to search (e.g.
symbol name, help text, prompts) and how to do it.


> @@ -1199,6 +1199,23 @@
>   layout2->addWidget(searchButton);
>   layout1->addLayout(layout2);
>  
> + // Initialize the "Search Type" button group.
> + searchType = new QVButtonGroup("Search Type", this, "searchType");
> +
> + substringSearch = new QRadioButton(
> + "Substring Match", searchType, "Substring Match"
> + );
> +
> + keywordsSearch = new QRadioButton("Keywords", searchType, "Keywords");
> +
> + regexSearch = new QRadioButton(
> + "Regular Expression", searchType, "Regular Expression"
> + );
> +
> + substringSearch->setChecked(TRUE);
> + layout1->addWidget(searchType);
> +
> + // Initialize the ConfigView and ConfigInfoView
>   split = new QSplitter(this);
>   split->setOrientation(QSplitter::Vertical);
>   list = new ConfigView(split, name);
> @@ -1245,6 +1262,20 @@
>   }
>  }
>  
> +SEARCH_TYPE ConfigSearchWindow::getSearchType()
> +{
> + return  substringSearch->isChecked() ? SUBSTRING :
> + regexSearch->isChecked() ? REGEX :
> + KEYWORDS;
> +}
>

If you use QButtonGroup::insert() you can assign id's to the indivual
buttons and then use selectedId to make this part simpler.

bye, Roman
-
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/