2002-02-08 05:46:17

by Troy Benjegerdes

[permalink] [raw]
Subject: [PATCH] bring sanity to div64.h and do_div usage

This patch introduced include/linux/div64.h and removes asm/div64.h from
all the architectures that don't have optimized ASM.

I also played janitor and found every file that used asm/div64.h. There
was some mips stuff that I didn't touch, but now everything else should
actually work on all platforms linux supports, instead of just those that
are either 64 bit, or have optimized asm versions.

(This patch is against 2.4.17, please let me know if it does not apply to
current 2.4 or 2.5)

Index: drivers/acpi/include/platform/aclinux.h
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/acpi/include/platform/aclinux.h,v
retrieving revision 1.1
diff -u -r1.1 aclinux.h
--- drivers/acpi/include/platform/aclinux.h 2001/11/30 22:29:16 1.1
+++ drivers/acpi/include/platform/aclinux.h 2002/02/08 05:32:48
@@ -36,9 +36,9 @@
#include <linux/string.h>
#include <linux/kernel.h>
#include <linux/ctype.h>
+
#include <asm/system.h>
#include <asm/atomic.h>
-#include <asm/div64.h>

#define strtoul simple_strtoul

Index: drivers/net/sk98lin/skproc.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/net/sk98lin/skproc.c,v
retrieving revision 1.1
diff -u -r1.1 skproc.c
--- drivers/net/sk98lin/skproc.c 2001/11/30 22:29:08 1.1
+++ drivers/net/sk98lin/skproc.c 2002/02/08 05:32:57
@@ -43,6 +43,8 @@
******************************************************************************/

#include <linux/proc_fs.h>
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>

#include "h/skdrv1st.h"
#include "h/skdrv2nd.h"
@@ -296,60 +298,6 @@
return (min_t(int, buffer_length, len - offset));
}

-
-
-
-
-/*****************************************************************************
- *
- * SkDoDiv - convert 64bit number
- *
- * Description:
- * This function "converts" a long long number.
- *
- * Returns:
- * remainder of division
- */
-static long SkDoDiv (long long Dividend, int Divisor, long long *pErg)
-{
- long Rest;
- long long Ergebnis;
- long Akku;
-
-
- Akku = Dividend >> 32;
-
- Ergebnis = ((long long) (Akku / Divisor)) << 32;
- Rest = Akku % Divisor ;
-
- Akku = Rest << 16;
- Akku |= ((Dividend & 0xFFFF0000) >> 16);
-
-
- Ergebnis += ((long long) (Akku / Divisor)) << 16;
- Rest = Akku % Divisor ;
-
- Akku = Rest << 16;
- Akku |= (Dividend & 0xFFFF);
-
- Ergebnis += (Akku / Divisor);
- Rest = Akku % Divisor ;
-
- *pErg = Ergebnis;
- return (Rest);
-}
-
-
-#if 0
-#define do_div(n,base) ({ \
-long long __res; \
-__res = ((unsigned long long) n) % (unsigned) base; \
-n = ((unsigned long long) n) / (unsigned) base; \
-__res; })
-
-#endif
-
-
/*****************************************************************************
*
* SkNumber - Print results
@@ -398,7 +346,7 @@
if (num == 0)
tmp[i++]='0';
else while (num != 0)
- tmp[i++] = digits[SkDoDiv(num,base, &num)];
+ tmp[i++] = digits[do_div(&num,base)];

if (i > precision)
precision = i;
Index: drivers/video/matrox/matroxfb_maven.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/video/matrox/matroxfb_maven.c,v
retrieving revision 1.1
diff -u -r1.1 matroxfb_maven.c
--- drivers/video/matrox/matroxfb_maven.c 2001/11/30 22:29:14 1.1
+++ drivers/video/matrox/matroxfb_maven.c 2002/02/08 05:33:02
@@ -15,7 +15,8 @@
#include "matroxfb_DAC1064.h"
#include <linux/i2c.h>
#include <linux/matroxfb.h>
-#include <asm/div64.h>
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>
#include <asm/uaccess.h>

#define MAVEN_I2CID (0x1B)
@@ -698,9 +699,8 @@
int vdec;
int vlen;

-#define MATROX_USE64BIT_DIVIDE
if (mt->VTotal) {
-#ifdef MATROX_USE64BIT_DIVIDE
+#ifdef USE_SLOW_64BIT_DIVIDES
u64 f1;
u32 a;
u32 b;
@@ -709,7 +709,7 @@
b = (mt->VTotal - 1) * (m->htotal + 2) + m->hcorr + 2;

f1 = ((u64)a) << 15; /* *32768 */
- do_div(f1, b);
+ do_div(&f1, b);
vdec = f1;
#else
vdec = m->vlines * 32768 / mt->VTotal;
Index: fs/affs/file.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/affs/file.c,v
retrieving revision 1.1
diff -u -r1.1 file.c
--- fs/affs/file.c 2001/11/30 22:28:58 1.1
+++ fs/affs/file.c 2002/02/08 05:33:03
@@ -12,7 +12,6 @@
* affs regular file handling primitives
*/

-#include <asm/div64.h>
#include <asm/uaccess.h>
#include <asm/system.h>
#include <linux/sched.h>
Index: fs/affs/inode.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/affs/inode.c,v
retrieving revision 1.1
diff -u -r1.1 inode.c
--- fs/affs/inode.c 2001/11/30 22:28:58 1.1
+++ fs/affs/inode.c 2002/02/08 05:33:04
@@ -10,7 +10,6 @@
* (C) 1991 Linus Torvalds - minix filesystem
*/

-#include <asm/div64.h>
#include <linux/errno.h>
#include <linux/fs.h>
#include <linux/slab.h>
Index: fs/ntfs/util.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/ntfs/util.c,v
retrieving revision 1.1
diff -u -r1.1 util.c
--- fs/ntfs/util.c 2001/11/30 22:28:59 1.1
+++ fs/ntfs/util.c 2002/02/08 05:33:05
@@ -13,7 +13,8 @@
#include "util.h"
#include <linux/string.h>
#include <linux/errno.h>
-#include <asm/div64.h> /* For do_div(). */
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h> /* For do_div(). */
#include "support.h"

/*
@@ -233,7 +234,7 @@
{
/* Subtract the NTFS time offset, then convert to 1s intervals. */
ntfs_time64_t t = ntutc - NTFS_TIME_OFFSET;
- do_div(t, 10000000);
+ do_div(&t, 10000000);
return (ntfs_time_t)t;
}

Index: fs/smbfs/proc.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/smbfs/proc.c,v
retrieving revision 1.1
diff -u -r1.1 proc.c
--- fs/smbfs/proc.c 2001/11/30 22:28:58 1.1
+++ fs/smbfs/proc.c 2002/02/08 05:33:07
@@ -18,12 +18,14 @@
#include <linux/dirent.h>
#include <linux/nls.h>

+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>
+
#include <linux/smb_fs.h>
#include <linux/smbno.h>
#include <linux/smb_mount.h>

#include <asm/string.h>
-#include <asm/div64.h>

#include "smb_debug.h"
#include "proto.h"
@@ -375,7 +377,7 @@
/* FIXME: what about the timezone difference? */
/* Subtract the NTFS time offset, then convert to 1s intervals. */
u64 t = ntutc - NTFS_TIME_OFFSET;
- do_div(t, 10000000);
+ do_div(&t, 10000000);
return (time_t)t;
}

Index: include/asm-alpha/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvs9lkcGH Thu Feb 7 21:33:39 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __ALPHA_DIV64
-#define __ALPHA_DIV64
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- __res; })
-
-#endif
Index: include/asm-arm/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvs6NHMn8 Thu Feb 7 21:33:39 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __ASM_ARM_DIV64
-#define __ASM_ARM_DIV64
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) \
-({ \
- int __res; \
- __res = ((unsigned long)n) % (unsigned int)base; \
- n = ((unsigned long)n) / (unsigned int)base; \
- __res; \
-})
-
-#endif
-
Index: include/asm-cris/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsA6IpzQ Thu Feb 7 21:33:40 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,16 +0,0 @@
-#ifndef __ASM_CRIS_DIV64
-#define __ASM_CRIS_DIV64
-
-/* copy from asm-arm */
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) \
-({ \
- int __res; \
- __res = ((unsigned long)n) % (unsigned int)base; \
- n = ((unsigned long)n) / (unsigned int)base; \
- __res; \
-})
-
-#endif
-
Index: include/asm-ia64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsKee8nJ Thu Feb 7 21:33:40 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,20 +0,0 @@
-#ifndef _ASM_IA64_DIV64_H
-#define _ASM_IA64_DIV64_H
-
-/*
- * Copyright (C) 1999 Hewlett-Packard Co
- * Copyright (C) 1999 David Mosberger-Tang <[email protected]>
- *
- * vsprintf uses this to divide a 64-bit integer N by a small integer BASE.
- * This is incredibly hard on IA-64...
- */
-
-#define do_div(n,base) \
-({ \
- int _res; \
- _res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- _res; \
-})
-
-#endif /* _ASM_IA64_DIV64_H */
Index: include/asm-mips64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvstklJ6k Thu Feb 7 21:33:41 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,19 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License. See the file "COPYING" in the main directory of this archive
- * for more details.
- */
-#ifndef _ASM_DIV64_H
-#define _ASM_DIV64_H
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* _ASM_DIV64_H */
Index: include/asm-parisc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsUOPlWi Thu Feb 7 21:33:41 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,54 +0,0 @@
-#ifndef __ASM_PARISC_DIV64
-#define __ASM_PARISC_DIV64
-
-#ifdef __LP64__
-
-/*
- * Copyright (C) 1999 Hewlett-Packard Co
- * Copyright (C) 1999 David Mosberger-Tang <[email protected]>
- *
- * vsprintf uses this to divide a 64-bit integer N by a small integer BASE.
- * This is incredibly hard on IA-64 and HPPA
- */
-
-#define do_div(n,base) \
-({ \
- int _res; \
- _res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- _res; \
-})
-
-#else
-/*
- * unsigned long long division. Yuck Yuck! What is Linux coming to?
- * This is 100% disgusting
- */
-#define do_div(n,base) \
-({ \
- unsigned long __low, __low2, __high, __rem; \
- __low = (n) & 0xffffffff; \
- __high = (n) >> 32; \
- if (__high) { \
- __rem = __high % (unsigned long)base; \
- __high = __high / (unsigned long)base; \
- __low2 = __low >> 16; \
- __low2 += __rem << 16; \
- __rem = __low2 % (unsigned long)base; \
- __low2 = __low2 / (unsigned long)base; \
- __low = __low & 0xffff; \
- __low += __rem << 16; \
- __rem = __low % (unsigned long)base; \
- __low = __low / (unsigned long)base; \
- n = __low + ((long long)__low2 << 16) + \
- ((long long) __high << 32); \
- } else { \
- __rem = __low % (unsigned long)base; \
- n = (__low / (unsigned long)base); \
- } \
- __rem; \
-})
-#endif
-
-#endif
-
Index: include/asm-ppc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvs3bbXXn Thu Feb 7 21:33:41 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,13 +0,0 @@
-/*
- * BK Id: SCCS/s.div64.h 1.5 05/17/01 18:14:24 cort
- */
-#ifndef __PPC_DIV64
-#define __PPC_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif
Index: include/asm-s390x/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsxz2zsC Thu Feb 7 21:33:42 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,10 +0,0 @@
-#ifndef __S390_DIV64
-#define __S390_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif
Index: include/asm-sh/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsLubD1X Thu Feb 7 21:33:42 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,10 +0,0 @@
-#ifndef __ASM_SH_DIV64
-#define __ASM_SH_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif /* __ASM_SH_DIV64 */
Index: include/asm-sparc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsdjFoKq Thu Feb 7 21:33:42 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,11 +0,0 @@
-#ifndef __SPARC_DIV64
-#define __SPARC_DIV64
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* __SPARC_DIV64 */
Index: include/asm-sparc64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsBXaTu3 Thu Feb 7 21:33:42 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __SPARC64_DIV64
-#define __SPARC64_DIV64
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* __SPARC64_DIV64 */
Index: include/linux/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /dev/null Tue May 5 13:32:27 1998
+++ div64.h Thu Feb 7 21:33:12 2002
@@ -0,0 +1,81 @@
+/*
+ * include/linux/div64.h
+ *
+ * Primarily used by vsprintf to divide a 64 bit int N by a small integer base
+ * We really do NOT want to encourage people to do slow 64 bit divides in
+ * the kernel, so the 'default' version of this function panics if you
+ * try and divide a 64 bit number by anything other than 8 or 16.
+ *
+ * If you really *really* need this, and are prepared to be flamed by
+ * lkml, #define USE_SLOW_64BIT_DIVIDES before including this file.
+ */
+#ifndef __DIV64
+#define __DIV64
+
+#include <linux/config.h>
+
+/* configurable */
+#undef __USE_ASM
+
+
+#ifdef __USE_ASM
+/* yeah, this is a mess, and leaves out m68k.... */
+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) || defined(CONFIG_MIPS)
+# define __USE_ASM__
+# endif
+#endif
+
+#ifdef __USE_ASM__
+#include <asm/div64.h>
+#else /* __USE_ASM__ */
+static inline int do_div(unsigned long long * n, unsigned long base)
+{
+ int res = 0;
+ unsigned long long t = *n;
+ if ( t == (unsigned long)t ){ /* this should handle 64 bit platforms */
+ res = ((unsigned long) t) % base;
+ t = ((unsigned long) t) / base;
+ } else {
+#ifndef USE_SLOW_64BIT_DIVIDES
+ switch (base) {
+ case 8:
+ res = ((unsigned long) t & 0x7);
+ t = t >> 3;
+ break;
+ case 16:
+ res = ((unsigned long) t & 0xf);
+ t = t >> 4;
+ break;
+ default:
+ panic("do_div called with 64 bit arg and unsupported base\n", base);
+ }
+#else /* USE_SLOW_64BIT_DIVIDES */
+ /*
+ * Nasty ugly generic C 64 bit divide on 32 bit machine
+ * No one seems to want to take credit for this
+ */
+ unsigned long low, low2, high;
+ low = (t) & 0xffffffff;
+ high = (t) >> 32;
+ res = high % base;
+ high = high / base;
+ low2 = low >> 16;
+ low2 += res << 16;
+ res = low2 % base;
+ low2 = low2 / base;
+ low = low & 0xffff;
+ low += res << 16;
+ res = low % base;
+ low = low / base;
+ t = low + ((long long)low2 << 16) +
+ ((long long) high << 32);
+#endif /* USE_SLOW_64BIT_DIVIDES */
+ }
+ *n = t;
+ return res;
+}
+
+
+#endif /* __USE_ASM__ */
+
+#endif
Index: lib/vsprintf.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/lib/vsprintf.c,v
retrieving revision 1.1
diff -u -r1.1 vsprintf.c
--- lib/vsprintf.c 2001/11/30 22:28:59 1.1
+++ lib/vsprintf.c 2002/02/08 05:33:15
@@ -19,9 +19,9 @@
#include <linux/string.h>
#include <linux/ctype.h>
#include <linux/kernel.h>
+/* #define USE_SLOW_64BIT_DIVIDE */
+#include <linux/div64.h>

-#include <asm/div64.h>
-
/**
* simple_strtoul - convert a string to an unsigned long
* @cp: The start of the string
@@ -165,7 +165,7 @@
if (num == 0)
tmp[i++]='0';
else while (num != 0)
- tmp[i++] = digits[do_div(num,base)];
+ tmp[i++] = digits[do_div(&num,base)];
if (i > precision)
precision = i;
size -= precision;
@@ -426,22 +426,31 @@
}
continue;
}
- if (qualifier == 'L')
+
+ switch (qualifier) {
+ case 'L':
num = va_arg(args, long long);
- else if (qualifier == 'l') {
- num = va_arg(args, unsigned long);
+ break;
+ case 'l':
if (flags & SIGN)
- num = (signed long) num;
- } else if (qualifier == 'Z') {
+ num = (signed long long) va_arg(args, long);
+ else
+ num = va_arg(args, unsigned long);
+ break;
+ case 'Z':
num = va_arg(args, size_t);
- } else if (qualifier == 'h') {
- num = (unsigned short) va_arg(args, int);
+ break;
+ case 'h':
if (flags & SIGN)
- num = (signed short) num;
- } else {
- num = va_arg(args, unsigned int);
+ num = (signed long long) va_arg(args, int);
+ else
+ num = va_arg(args, unsigned int);
+ break;
+ default:
if (flags & SIGN)
- num = (signed int) num;
+ num = (signed long long) va_arg(args, int);
+ else
+ num = va_arg(args, unsigned int);
}
str = number(str, end, num, base,
field_width, precision, flags);


2002-02-08 12:16:01

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Hi,

This patch is broken. Please don't apply as it is.

At 05:45 08/02/02, Troy Benjegerdes wrote:
>This patch introduced include/linux/div64.h and removes asm/div64.h from
>all the architectures that don't have optimized ASM.
>
>I also played janitor and found every file that used asm/div64.h. There
>was some mips stuff that I didn't touch, but now everything else should
>actually work on all platforms linux supports, instead of just those that
>are either 64 bit, or have optimized asm versions.
[snip]
>diff -N div64.h
>--- /dev/null Tue May 5 13:32:27 1998
>+++ div64.h Thu Feb 7 21:33:12 2002
>@@ -0,0 +1,81 @@
>+/*
>+ * include/linux/div64.h
>+ *
>+ * Primarily used by vsprintf to divide a 64 bit int N by a small integer
>base
>+ * We really do NOT want to encourage people to do slow 64 bit divides in
>+ * the kernel, so the 'default' version of this function panics if you
>+ * try and divide a 64 bit number by anything other than 8 or 16.
>+ *
>+ * If you really *really* need this, and are prepared to be flamed by
>+ * lkml, #define USE_SLOW_64BIT_DIVIDES before including this file.
>+ */
>+#ifndef __DIV64
>+#define __DIV64
>+
>+#include <linux/config.h>
>+
>+/* configurable */
>+#undef __USE_ASM
>+
>+
>+#ifdef __USE_ASM
>+/* yeah, this is a mess, and leaves out m68k.... */
>+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) || defined(CONFIG_MIPS)
>+# define __USE_ASM__
>+# endif
>+#endif

Er, what exactly do you think the above does?!?

It NEVER will define __USE_ASM because you make the definition dependent on
the UNDEFINED __USE_ASM! If anything you need to do instead:

+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) ||
defined(CONFIG_MIPS) || defined(CONFIG_M68K)
+# define __USE_ASM__
+# endif

I.e. remove the #ifdef __USE_ASM and add CONFIG_M68K dependence.

>+#ifdef __USE_ASM__
>+#include <asm/div64.h>
>+#else /* __USE_ASM__ */
>+static inline int do_div(unsigned long long * n, unsigned long base)
>+{
>+ int res = 0;
>+ unsigned long long t = *n;
>+ if ( t == (unsigned long)t ){ /* this should handle 64 bit
>platforms */
>+ res = ((unsigned long) t) % base;
>+ t = ((unsigned long) t) / base;
>+ } else {

This check is silly. It is _way_ more efficient to do:

if (BITS_PER_LONG == 64) {
res = ((unsigned long)t) % base;
t = ((unsigned long)t) / base;
} else {

This actually only compiles one or the other but not both.

>+#ifndef USE_SLOW_64BIT_DIVIDES

This is stupid. If someone knows in advance they are only doing a division
by 8 or 16 they would not be using do_div in the first place, they would be
using a shift or even just normal division sign which gcc is supposed to
optimize to a shift itself (assuming division by constant).

Please remove this. People using do_div() are using it for a reason.

>+ switch (base) {
>+ case 8:
>+ res = ((unsigned long) t & 0x7);
>+ t = t >> 3;
>+ break;
>+ case 16:
>+ res = ((unsigned long) t & 0xf);
>+ t = t >> 4;
>+ break;
>+ default:
>+ panic("do_div called with 64 bit arg and
>unsupported base\n", base);

Anyway, why do you randomly pick 8 and 16? You could do any power of two
via shift by simply doing something along these lines:

if (!(base & (base - 1))) {
res = t & (base - 1);
t >>= ffs(base) - 1;
} else
panic("blah");

But I still think this is a really stupid thing to do.

>+ }
>+#else /* USE_SLOW_64BIT_DIVIDES */
>+ /*
>+ * Nasty ugly generic C 64 bit divide on 32 bit machine
>+ * No one seems to want to take credit for this
>+ */
>+ unsigned long low, low2, high;
>+ low = (t) & 0xffffffff;
>+ high = (t) >> 32;

Look at asm-parisc/div64.h which has an optimized version of this for t
actually being 32bit.

>+ res = high % base;
>+ high = high / base;
>+ low2 = low >> 16;
>+ low2 += res << 16;
>+ res = low2 % base;
>+ low2 = low2 / base;
>+ low = low & 0xffff;
>+ low += res << 16;
>+ res = low % base;
>+ low = low / base;
>+ t = low + ((long long)low2 << 16) +
>+ ((long long) high << 32);
>+#endif /* USE_SLOW_64BIT_DIVIDES */
>+ }
>+ *n = t;
>+ return res;
>+}
>+
>+
>+#endif /* __USE_ASM__ */
>+
>+#endif
[snip]

I think the patch is generally a good idea but it has to be done right...

Best regards,

Anton


--
"I've not lost my mind. It's backed up on tape somewhere." - Unknown
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

2002-02-08 12:30:13

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

At 12:15 08/02/02, Anton Altaparmakov wrote:
>Hi,
>
>This patch is broken. Please don't apply as it is.
>
>At 05:45 08/02/02, Troy Benjegerdes wrote:
>>This patch introduced include/linux/div64.h and removes asm/div64.h from
>>all the architectures that don't have optimized ASM.
>>
>>I also played janitor and found every file that used asm/div64.h. There
>>was some mips stuff that I didn't touch, but now everything else should
>>actually work on all platforms linux supports, instead of just those that
>>are either 64 bit, or have optimized asm versions.
>[snip]
>>diff -N div64.h
>>--- /dev/null Tue May 5 13:32:27 1998
>>+++ div64.h Thu Feb 7 21:33:12 2002
>>@@ -0,0 +1,81 @@
>>+/*
>>+ * include/linux/div64.h
>>+ *
>>+ * Primarily used by vsprintf to divide a 64 bit int N by a small
>>integer base
>>+ * We really do NOT want to encourage people to do slow 64 bit divides in
>>+ * the kernel, so the 'default' version of this function panics if you
>>+ * try and divide a 64 bit number by anything other than 8 or 16.
>>+ *
>>+ * If you really *really* need this, and are prepared to be flamed by
>>+ * lkml, #define USE_SLOW_64BIT_DIVIDES before including this file.
>>+ */
>>+#ifndef __DIV64
>>+#define __DIV64
>>+
>>+#include <linux/config.h>
>>+
>>+/* configurable */
>>+#undef __USE_ASM
>>+
>>+
>>+#ifdef __USE_ASM
>>+/* yeah, this is a mess, and leaves out m68k.... */
>>+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) || defined(CONFIG_MIPS)
>>+# define __USE_ASM__
>>+# endif
>>+#endif
>
>Er, what exactly do you think the above does?!?
>
>It NEVER will define __USE_ASM because you make the definition dependent
>on the UNDEFINED __USE_ASM! If anything you need to do instead:

Ooops. Sorry, didn't notice the second "__". This part will work. But rest
of comments remain.

Best regards,

Anton

>+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) ||
>defined(CONFIG_MIPS) || defined(CONFIG_M68K)
>+# define __USE_ASM__
>+# endif
>
>I.e. remove the #ifdef __USE_ASM and add CONFIG_M68K dependence.
>
>>+#ifdef __USE_ASM__
>>+#include <asm/div64.h>
>>+#else /* __USE_ASM__ */
>>+static inline int do_div(unsigned long long * n, unsigned long base)
>>+{
>>+ int res = 0;
>>+ unsigned long long t = *n;
>>+ if ( t == (unsigned long)t ){ /* this should handle 64 bit
>>platforms */
>>+ res = ((unsigned long) t) % base;
>>+ t = ((unsigned long) t) / base;
>>+ } else {
>
>This check is silly. It is _way_ more efficient to do:
>
>if (BITS_PER_LONG == 64) {
> res = ((unsigned long)t) % base;
> t = ((unsigned long)t) / base;
>} else {
>
>This actually only compiles one or the other but not both.
>
>>+#ifndef USE_SLOW_64BIT_DIVIDES
>
>This is stupid. If someone knows in advance they are only doing a division
>by 8 or 16 they would not be using do_div in the first place, they would
>be using a shift or even just normal division sign which gcc is supposed
>to optimize to a shift itself (assuming division by constant).
>
>Please remove this. People using do_div() are using it for a reason.
>
>>+ switch (base) {
>>+ case 8:
>>+ res = ((unsigned long) t & 0x7);
>>+ t = t >> 3;
>>+ break;
>>+ case 16:
>>+ res = ((unsigned long) t & 0xf);
>>+ t = t >> 4;
>>+ break;
>>+ default:
>>+ panic("do_div called with 64 bit arg and
>>unsupported base\n", base);
>
>Anyway, why do you randomly pick 8 and 16? You could do any power of two
>via shift by simply doing something along these lines:
>
>if (!(base & (base - 1))) {
> res = t & (base - 1);
> t >>= ffs(base) - 1;
>} else
> panic("blah");
>
>But I still think this is a really stupid thing to do.
>
>>+ }
>>+#else /* USE_SLOW_64BIT_DIVIDES */
>>+ /*
>>+ * Nasty ugly generic C 64 bit divide on 32 bit machine
>>+ * No one seems to want to take credit for this
>>+ */
>>+ unsigned long low, low2, high;
>>+ low = (t) & 0xffffffff;
>>+ high = (t) >> 32;
>
>Look at asm-parisc/div64.h which has an optimized version of this for t
>actually being 32bit.
>
>>+ res = high % base;
>>+ high = high / base;
>>+ low2 = low >> 16;
>>+ low2 += res << 16;
>>+ res = low2 % base;
>>+ low2 = low2 / base;
>>+ low = low & 0xffff;
>>+ low += res << 16;
>>+ res = low % base;
>>+ low = low / base;
>>+ t = low + ((long long)low2 << 16) +
>>+ ((long long) high << 32);
>>+#endif /* USE_SLOW_64BIT_DIVIDES */
>>+ }
>>+ *n = t;
>>+ return res;
>>+}
>>+
>>+
>>+#endif /* __USE_ASM__ */
>>+
>>+#endif
>[snip]
>
>I think the patch is generally a good idea but it has to be done right...
>
>Best regards,
>
>Anton
>
>
>--
> "I've not lost my mind. It's backed up on tape somewhere." - Unknown
>--
>Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
>Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
>ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/
>
>-
>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/

--
"I've not lost my mind. It's backed up on tape somewhere." - Unknown
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

2002-02-08 17:58:04

by Troy Benjegerdes

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

[snip]

> >+
> >+#ifdef __USE_ASM
> >+/* yeah, this is a mess, and leaves out m68k.... */
> >+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) || defined(CONFIG_MIPS)
> >+# define __USE_ASM__
> >+# endif
> >+#endif
>

[snip]

> I.e. remove the #ifdef __USE_ASM and add CONFIG_M68K dependence.

Well, there's a reason I left out CONFIG_M68K deps.. Go tell me where
CONFIG_M68K is defined.. ;)


> >+#ifdef __USE_ASM__
> >+#include <asm/div64.h>
> >+#else /* __USE_ASM__ */
> >+static inline int do_div(unsigned long long * n, unsigned long base)
> >+{
> >+ int res = 0;
> >+ unsigned long long t = *n;
> >+ if ( t == (unsigned long)t ){ /* this should handle 64 bit
> >platforms */
> >+ res = ((unsigned long) t) % base;
> >+ t = ((unsigned long) t) / base;
> >+ } else {
>
> This check is silly. It is _way_ more efficient to do:
>
> if (BITS_PER_LONG == 64) {
> res = ((unsigned long)t) % base;
> t = ((unsigned long)t) / base;
> } else {
>
> This actually only compiles one or the other but not both.

The intent of this is to have one check that handles both 64 and 32 bit
platforms nicely, and handle degenerate cases where N is less than 32
bits on 32 bit platforms.

I could be persuaded to add a (BITS_PER_LONG) check for 64 bit machines
if someone shows me ASM showing gcc isn't smart enough to figure out that

(unsigned long long)t == (unsigned long)t

is always true on 64 bit.

>
> >+#ifndef USE_SLOW_64BIT_DIVIDES
>
> This is stupid. If someone knows in advance they are only doing a division
> by 8 or 16 they would not be using do_div in the first place, they would be
> using a shift or even just normal division sign which gcc is supposed to
> optimize to a shift itself (assuming division by constant).
>
> Please remove this. People using do_div() are using it for a reason.

I am trying to provide a cleanup that is acceptable to most people.

Several people I have talked to on the issue specifically asked for the
panic(), as people using do_div() should really know better than to do 64
bit divides in the kernel.

I provided the #define option so we can have *one* sane cross platform 64
bit divide, but one that still makes people think before using do_div.

> Anyway, why do you randomly pick 8 and 16? You could do any power of two
> via shift by simply doing something along these lines:
>
> if (!(base & (base - 1))) {
> res = t & (base - 1);
> t >>= ffs(base) - 1;
> } else
> panic("blah");
>
> But I still think this is a really stupid thing to do.

using do_div() blindly is also considered a really stupid thing to do.
YMMV.

This patch is primarily intended to fix vsprintf of long longs on all
platforms, once and for all, and vsprintf only uses base 8 and 16. Anyone
doing a printk and wanting a decimal representation of a long long
probably deserves a panic anyway. (IHMO)


[snip]

> Look at asm-parisc/div64.h which has an optimized version of this for t
> actually being 32bit.

That came from asm-parisc/div64 originally.

The t being 32bit case is handled earlier

>
> I think the patch is generally a good idea but it has to be done right...
>
> Best regards,

Gabriel Paubert also pointed out one problem I'm not sure how to deal
with..

The generic C algorithm only handles base < 65536.

I can think of a couple ways around this..

1) Make the base argument be a 'u16 base', and people with too large a
base would get compile warnings/errors.

2) run-time check on base, and panic if too large

3) run-time check on base, print dmesg warning if too large



I'll re-post a new patch once I hear more feedback, and after I get some
suggestions on how to deal with base > 65535 problem


--
Troy Benjegerdes | master of mispeeling | 'da hozer' | [email protected]
-----"If this message isn't misspelled, I didn't write it" -- Me -----
"Why do musicians compose symphonies and poets write poems? They do it
because life wouldn't have any meaning for them if they didn't. That's
why I draw cartoons. It's my life." -- Charles Schulz

2002-02-08 18:54:14

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

On Fri, 8 Feb 2002, Troy Benjegerdes wrote:

> Several people I have talked to on the issue specifically asked for the
> panic(), as people using do_div() should really know better than to do 64
> bit divides in the kernel.

There are legitimate cases where you cannot avoid a double-precision
division and the inefficiency is negligible. For example for MIPS it's
used in do_*_gettimeoffset() and at most once a jiffy (actually we use
do_div64_32() to reduce work, as we know the quotient will *always* fit in
32 bits).

> The generic C algorithm only handles base < 65536.
>
> I can think of a couple ways around this..
>
> 1) Make the base argument be a 'u16 base', and people with too large a
> base would get compile warnings/errors.
>
> 2) run-time check on base, and panic if too large
>
> 3) run-time check on base, print dmesg warning if too large

4) Use a generic division algorithm using shifts and subtracts such as
one of these described in academic books. You may port the implementation
from include/asm-mips/div64.h. ;-)

Note that in do_*_gettimeoffset() the divisor is an arbitrary 32-bit
number, mostly depending on the uptime.

--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +

2002-02-08 19:05:34

by Roman Zippel

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Hi,

Troy Benjegerdes wrote:

> +#else /* USE_SLOW_64BIT_DIVIDES */
> + /*
> + * Nasty ugly generic C 64 bit divide on 32 bit machine
> + * No one seems to want to take credit for this
> + */
> + unsigned long low, low2, high;
> + low = (t) & 0xffffffff;
> + high = (t) >> 32;
> + res = high % base;
> + high = high / base;
> + low2 = low >> 16;
> + low2 += res << 16;
> + res = low2 % base;
> + low2 = low2 / base;
> + low = low & 0xffff;
> + low += res << 16;
> + res = low % base;
> + low = low / base;
> + t = low + ((long long)low2 << 16) +
> + ((long long) high << 32);


IMO this is large enough to put under lib/..., the rest could also be
put into asm-generic, from where the archs could include it, this would
avoid this:

> +#ifdef __USE_ASM
> +/* yeah, this is a mess, and leaves out m68k.... */
> +# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) ||
defined(CONFIG_MIPS)
> +# define __USE_ASM__
> +# endif
> +#endif

bye, Roman

2002-02-08 19:33:46

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

At 17:57 08/02/02, Troy Benjegerdes wrote:
>[snip]
>
> > >+
> > >+#ifdef __USE_ASM
> > >+/* yeah, this is a mess, and leaves out m68k.... */
> > >+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) ||

Just noticed there is a typo. It should be "defined(CONFIG_ARCH_S390)" not
"define"... Just spotted it

>defined(CONFIG_MIPS)
> > >+# define __USE_ASM__
> > >+# endif
> > >+#endif
> >
>
>[snip]
>
> > I.e. remove the #ifdef __USE_ASM and add CONFIG_M68K dependence.
>
>Well, there's a reason I left out CONFIG_M68K deps.. Go tell me where
>CONFIG_M68K is defined.. ;)

Appologies, it's in Configure.help but that is not a too useful places to
have it. However the kernel seems to be using:

#if defined(__mc68000__) so just use that instead. Any m68k people reading
this care to comment?

> > >+#ifdef __USE_ASM__
> > >+#include <asm/div64.h>
> > >+#else /* __USE_ASM__ */
> > >+static inline int do_div(unsigned long long * n, unsigned long base)
> > >+{
> > >+ int res = 0;
> > >+ unsigned long long t = *n;
> > >+ if ( t == (unsigned long)t ){ /* this should handle 64 bit
> > >platforms */
> > >+ res = ((unsigned long) t) % base;
> > >+ t = ((unsigned long) t) / base;
> > >+ } else {
> >
> > This check is silly. It is _way_ more efficient to do:
> >
> > if (BITS_PER_LONG == 64) {
> > res = ((unsigned long)t) % base;
> > t = ((unsigned long)t) / base;
> > } else {
> >
> > This actually only compiles one or the other but not both.
>
>The intent of this is to have one check that handles both 64 and 32 bit
>platforms nicely, and handle degenerate cases where N is less than 32
>bits on 32 bit platforms.
>
>I could be persuaded to add a (BITS_PER_LONG) check for 64 bit machines
>if someone shows me ASM showing gcc isn't smart enough to figure out that
>
> (unsigned long long)t == (unsigned long)t
>
>is always true on 64 bit.

Sorry can't say. Don't have 64 bit computers/gcc. )-: But as a matter of
principle I wouldn't trust gcc. I remember at least one case being
discussed on lkml before where gcc was not optimizing away obviously
optimizable code...

> > >+#ifndef USE_SLOW_64BIT_DIVIDES
> >
> > This is stupid. If someone knows in advance they are only doing a division
> > by 8 or 16 they would not be using do_div in the first place, they
> would be
> > using a shift or even just normal division sign which gcc is supposed to
> > optimize to a shift itself (assuming division by constant).
> >
> > Please remove this. People using do_div() are using it for a reason.
>
>I am trying to provide a cleanup that is acceptable to most people.
>
>Several people I have talked to on the issue specifically asked for the
>panic(), as people using do_div() should really know better than to do 64
>bit divides in the kernel.

This is ridiculous. There are GENUINE uses for 64-bit division. NTFS and
SMBFS and anything else having to interoperate with Windows requires 64-bit
divisions. Best example is time conversion. Time is 64-bit and obviously
has to be considered as a random number for all intents and purposes. And
converting UNIX <-> Windows times REQUIRES 64-bit arithmetic.

And this is not the only use. Fortunately most 64-bit divisions in the
kernel are by a power of 2 so shifts are used instead.

In fact I would say that anyone using do_div() for a base of 8 or 16 should
be shot. I would put the panic() in do_div() but only IF the base IS a
constant and IS a power of 2 and not otherwise. Then anyone using do_div()
on constants will learn the lesson and do it properly instead.

>I provided the #define option so we can have *one* sane cross platform 64
>bit divide, but one that still makes people think before using do_div.

You are just obfuscating the kernel code.

> > Anyway, why do you randomly pick 8 and 16? You could do any power of two
> > via shift by simply doing something along these lines:
> >
> > if (!(base & (base - 1))) {
> > res = t & (base - 1);
> > t >>= ffs(base) - 1;
> > } else
> > panic("blah");
> >
> > But I still think this is a really stupid thing to do.
>
>using do_div() blindly is also considered a really stupid thing to do.
>YMMV.

Yes, indeed. But do_div() exists because there ARE genuine uses for it. And
these will only increase over time as everything is becoming too large to
fit in 32-bits. I think that even inodes will have to become 64 bit within
the next few years.

>This patch is primarily intended to fix vsprintf of long longs on all
>platforms, once and for all, and vsprintf only uses base 8 and 16. Anyone
>doing a printk and wanting a decimal representation of a long long
>probably deserves a panic anyway. (IHMO)

Why? You would kill the NTFS driver's debug/error outputs completely then.
Why should I have to use hex numbers when they are not exactly natural to
the human brain. (Although there is a case for saying that the hex is
shorter so easier to parse by eye...but that is not reason enough to
panic() if someone does it). In fact IMO panic() should pretty much never
be used anywhere as it just shows lack of proper error handling but that is
a different issue and let us not enter into that argument...

>[snip]
>
> > Look at asm-parisc/div64.h which has an optimized version of this for t
> > actually being 32bit.
>
>That came from asm-parisc/div64 originally.
>
>The t being 32bit case is handled earlier
>
> >
> > I think the patch is generally a good idea but it has to be done right...
> >
> > Best regards,
>
>Gabriel Paubert also pointed out one problem I'm not sure how to deal
>with..
>
>The generic C algorithm only handles base < 65536.

Eeek. I hadn't even noticed that. That is not very generic is it?!?

>I can think of a couple ways around this..
>
>1) Make the base argument be a 'u16 base', and people with too large a
> base would get compile warnings/errors.
>
>2) run-time check on base, and panic if too large
>
>3) run-time check on base, print dmesg warning if too large

No, neither of these will do. NTFS needs to divide by 10000000 (1x10^7)
which is way above 16-bits.

>I'll re-post a new patch once I hear more feedback, and after I get some
>suggestions on how to deal with base > 65535 problem

Rewrite the algorithm is the obvious choice. The old NTFS driver used to
have one but I got rid of it in favour of the nice do_div() assuming it
would always work... But even that had certain restrictions put in place
because of the special uses it had so it is not generic enough for
do_div(). Considering do_div() is not useful in current state I may have to
resurrect the original code and put it into NTFS (and SMBFS, which has the
same problem). But it would be much better to have a proper do_div()
instead. Otherwise we will just get proliferation of various do_div()
implementations in various drivers.

Best regards,

Anton


--
"I've not lost my mind. It's backed up on tape somewhere." - Unknown
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

2002-02-08 21:17:40

by Troy Benjegerdes

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

> > > >+/* yeah, this is a mess, and leaves out m68k.... */
> > > >+# if defined(CONFIG_X86) || define(CONFIG_ARCH_S390) ||
>
> Just noticed there is a typo. It should be "defined(CONFIG_ARCH_S390)" not
> "define"... Just spotted it

fixed.

> #if defined(__mc68000__) so just use that instead. Any m68k people reading
> this care to comment?

fixed.

> Sorry can't say. Don't have 64 bit computers/gcc. )-: But as a matter of
> principle I wouldn't trust gcc. I remember at least one case being
> discussed on lkml before where gcc was not optimizing away obviously
> optimizable code...

anyone else have comments on this? (for now I'm adding a '(BITS_PER_LONG ==
64) to the first if..

> >I provided the #define option so we can have *one* sane cross platform 64
> >bit divide, but one that still makes people think before using do_div.
>
> You are just obfuscating the kernel code.

Obviously, there are good reasons for real 64 bit divides.

Would you rather have a 'do_div64()' function that does full divisions?

For ntfs/smbfs, can you just make two consecutive calls?

do_div(&val, 10000)
do_div(&val, 1000)

> Rewrite the algorithm is the obvious choice. The old NTFS driver used to
> have one but I got rid of it in favour of the nice do_div() assuming it
> would always work... But even that had certain restrictions put in place
> because of the special uses it had so it is not generic enough for
> do_div(). Considering do_div() is not useful in current state I may have to
> resurrect the original code and put it into NTFS (and SMBFS, which has the
> same problem). But it would be much better to have a proper do_div()
> instead. Otherwise we will just get proliferation of various do_div()
> implementations in various drivers.

Yes, I'd like to avoid 16 different degenerate case implementations of
do_div() in various drivers.

Linus (or anyone else in a position to send stuff to Linus), could you please
make a statement about what would be acceptable? Should we have two
functions, rewrite vsprintf, or what?

I could just punt and fix powerpc with asm, but I really don't think do_div
gets called enough to be worth the trouble of asm code. (ASM is for mucking
with MMU's, atomic ops, etc, not 64 bit divides in-kernel)

--
Troy Benjegerdes | master of mispeeling | 'da hozer' | [email protected]
-----"If this message isn't misspelled, I didn't write it" -- Me -----
"Why do musicians compose symphonies and poets write poems? They do it
because life wouldn't have any meaning for them if they didn't. That's
why I draw cartoons. It's my life." -- Charles Schulz

2002-02-15 18:48:29

by Troy Benjegerdes

[permalink] [raw]
Subject: Updated div64.h cleanup

Here's anothing try at an acceptable div64.h cleanup.

I would appreciate if someone could verify that what I did in the
matrox_maven.c file is correct.

Index: drivers/acpi/include/platform/aclinux.h
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/acpi/include/platform/aclinux.h,v
retrieving revision 1.1
diff -u -r1.1 aclinux.h
--- drivers/acpi/include/platform/aclinux.h 2001/11/30 22:29:16 1.1
+++ drivers/acpi/include/platform/aclinux.h 2002/02/15 00:47:05
@@ -36,9 +36,9 @@
#include <linux/string.h>
#include <linux/kernel.h>
#include <linux/ctype.h>
+
#include <asm/system.h>
#include <asm/atomic.h>
-#include <asm/div64.h>

#define strtoul simple_strtoul

Index: drivers/net/sk98lin/skproc.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/net/sk98lin/skproc.c,v
retrieving revision 1.1
diff -u -r1.1 skproc.c
--- drivers/net/sk98lin/skproc.c 2001/11/30 22:29:08 1.1
+++ drivers/net/sk98lin/skproc.c 2002/02/15 00:47:13
@@ -43,6 +43,8 @@
******************************************************************************/

#include <linux/proc_fs.h>
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>

#include "h/skdrv1st.h"
#include "h/skdrv2nd.h"
@@ -296,60 +298,6 @@
return (min_t(int, buffer_length, len - offset));
}

-
-
-
-
-/*****************************************************************************
- *
- * SkDoDiv - convert 64bit number
- *
- * Description:
- * This function "converts" a long long number.
- *
- * Returns:
- * remainder of division
- */
-static long SkDoDiv (long long Dividend, int Divisor, long long *pErg)
-{
- long Rest;
- long long Ergebnis;
- long Akku;
-
-
- Akku = Dividend >> 32;
-
- Ergebnis = ((long long) (Akku / Divisor)) << 32;
- Rest = Akku % Divisor ;
-
- Akku = Rest << 16;
- Akku |= ((Dividend & 0xFFFF0000) >> 16);
-
-
- Ergebnis += ((long long) (Akku / Divisor)) << 16;
- Rest = Akku % Divisor ;
-
- Akku = Rest << 16;
- Akku |= (Dividend & 0xFFFF);
-
- Ergebnis += (Akku / Divisor);
- Rest = Akku % Divisor ;
-
- *pErg = Ergebnis;
- return (Rest);
-}
-
-
-#if 0
-#define do_div(n,base) ({ \
-long long __res; \
-__res = ((unsigned long long) n) % (unsigned) base; \
-n = ((unsigned long long) n) / (unsigned) base; \
-__res; })
-
-#endif
-
-
/*****************************************************************************
*
* SkNumber - Print results
@@ -398,7 +346,7 @@
if (num == 0)
tmp[i++]='0';
else while (num != 0)
- tmp[i++] = digits[SkDoDiv(num,base, &num)];
+ tmp[i++] = digits[do_div(&num,base)];

if (i > precision)
precision = i;
Index: drivers/video/matrox/matroxfb_maven.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/drivers/video/matrox/matroxfb_maven.c,v
retrieving revision 1.1
diff -u -r1.1 matroxfb_maven.c
--- drivers/video/matrox/matroxfb_maven.c 2001/11/30 22:29:14 1.1
+++ drivers/video/matrox/matroxfb_maven.c 2002/02/15 00:47:17
@@ -15,7 +15,8 @@
#include "matroxfb_DAC1064.h"
#include <linux/i2c.h>
#include <linux/matroxfb.h>
-#include <asm/div64.h>
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>
#include <asm/uaccess.h>

#define MAVEN_I2CID (0x1B)
@@ -698,9 +699,8 @@
int vdec;
int vlen;

-#define MATROX_USE64BIT_DIVIDE
if (mt->VTotal) {
-#ifdef MATROX_USE64BIT_DIVIDE
+#ifdef USE_SLOW_64BIT_DIVIDES
u64 f1;
u32 a;
u32 b;
@@ -709,7 +709,11 @@
b = (mt->VTotal - 1) * (m->htotal + 2) + m->hcorr + 2;

f1 = ((u64)a) << 15; /* *32768 */
- do_div(f1, b);
+ if (b > 65535){ /* messy stuff to make sure b is less than 65536 */
+ do_div(&f1, b & 0xffff);
+ b = (b & 0xffff0000)/(b & 0xffff) + 1;
+ }
+ do_div(&f1, b);
vdec = f1;
#else
vdec = m->vlines * 32768 / mt->VTotal;
Index: fs/affs/file.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/affs/file.c,v
retrieving revision 1.1
diff -u -r1.1 file.c
--- fs/affs/file.c 2001/11/30 22:28:58 1.1
+++ fs/affs/file.c 2002/02/15 00:47:18
@@ -12,7 +12,6 @@
* affs regular file handling primitives
*/

-#include <asm/div64.h>
#include <asm/uaccess.h>
#include <asm/system.h>
#include <linux/sched.h>
Index: fs/affs/inode.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/affs/inode.c,v
retrieving revision 1.1
diff -u -r1.1 inode.c
--- fs/affs/inode.c 2001/11/30 22:28:58 1.1
+++ fs/affs/inode.c 2002/02/15 00:47:19
@@ -10,7 +10,6 @@
* (C) 1991 Linus Torvalds - minix filesystem
*/

-#include <asm/div64.h>
#include <linux/errno.h>
#include <linux/fs.h>
#include <linux/slab.h>
Index: fs/ntfs/util.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/ntfs/util.c,v
retrieving revision 1.1
diff -u -r1.1 util.c
--- fs/ntfs/util.c 2001/11/30 22:28:59 1.1
+++ fs/ntfs/util.c 2002/02/15 00:47:20
@@ -13,7 +13,8 @@
#include "util.h"
#include <linux/string.h>
#include <linux/errno.h>
-#include <asm/div64.h> /* For do_div(). */
+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h> /* For do_div(). */
#include "support.h"

/*
@@ -233,7 +234,8 @@
{
/* Subtract the NTFS time offset, then convert to 1s intervals. */
ntfs_time64_t t = ntutc - NTFS_TIME_OFFSET;
- do_div(t, 10000000);
+ do_div(&t, 10000);
+ do_div(&t, 1000); /* divide by 10000000 */
return (ntfs_time_t)t;
}

Index: fs/smbfs/proc.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/fs/smbfs/proc.c,v
retrieving revision 1.1
diff -u -r1.1 proc.c
--- fs/smbfs/proc.c 2001/11/30 22:28:58 1.1
+++ fs/smbfs/proc.c 2002/02/15 00:47:23
@@ -18,12 +18,14 @@
#include <linux/dirent.h>
#include <linux/nls.h>

+#define USE_SLOW_64BIT_DIVIDES
+#include <linux/div64.h>
+
#include <linux/smb_fs.h>
#include <linux/smbno.h>
#include <linux/smb_mount.h>

#include <asm/string.h>
-#include <asm/div64.h>

#include "smb_debug.h"
#include "proto.h"
@@ -375,7 +377,8 @@
/* FIXME: what about the timezone difference? */
/* Subtract the NTFS time offset, then convert to 1s intervals. */
u64 t = ntutc - NTFS_TIME_OFFSET;
- do_div(t, 10000000);
+ do_div(&t, 10000);
+ do_div(&t, 1000);
return (time_t)t;
}

Index: include/asm-alpha/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvs4OfhPk Thu Feb 14 16:48:20 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __ALPHA_DIV64
-#define __ALPHA_DIV64
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- __res; })
-
-#endif
Index: include/asm-arm/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsFC1yur Thu Feb 14 16:48:20 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __ASM_ARM_DIV64
-#define __ASM_ARM_DIV64
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) \
-({ \
- int __res; \
- __res = ((unsigned long)n) % (unsigned int)base; \
- n = ((unsigned long)n) / (unsigned int)base; \
- __res; \
-})
-
-#endif
-
Index: include/asm-cris/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsYOw3W7 Thu Feb 14 16:48:20 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,16 +0,0 @@
-#ifndef __ASM_CRIS_DIV64
-#define __ASM_CRIS_DIV64
-
-/* copy from asm-arm */
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) \
-({ \
- int __res; \
- __res = ((unsigned long)n) % (unsigned int)base; \
- n = ((unsigned long)n) / (unsigned int)base; \
- __res; \
-})
-
-#endif
-
Index: include/asm-ia64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvshL7A10 Thu Feb 14 16:48:20 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,20 +0,0 @@
-#ifndef _ASM_IA64_DIV64_H
-#define _ASM_IA64_DIV64_H
-
-/*
- * Copyright (C) 1999 Hewlett-Packard Co
- * Copyright (C) 1999 David Mosberger-Tang <[email protected]>
- *
- * vsprintf uses this to divide a 64-bit integer N by a small integer BASE.
- * This is incredibly hard on IA-64...
- */
-
-#define do_div(n,base) \
-({ \
- int _res; \
- _res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- _res; \
-})
-
-#endif /* _ASM_IA64_DIV64_H */
Index: include/asm-mips64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsjVZJpB Thu Feb 14 16:48:21 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,19 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License. See the file "COPYING" in the main directory of this archive
- * for more details.
- */
-#ifndef _ASM_DIV64_H
-#define _ASM_DIV64_H
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* _ASM_DIV64_H */
Index: include/asm-parisc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsvkng5v Thu Feb 14 16:48:21 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,54 +0,0 @@
-#ifndef __ASM_PARISC_DIV64
-#define __ASM_PARISC_DIV64
-
-#ifdef __LP64__
-
-/*
- * Copyright (C) 1999 Hewlett-Packard Co
- * Copyright (C) 1999 David Mosberger-Tang <[email protected]>
- *
- * vsprintf uses this to divide a 64-bit integer N by a small integer BASE.
- * This is incredibly hard on IA-64 and HPPA
- */
-
-#define do_div(n,base) \
-({ \
- int _res; \
- _res = ((unsigned long) (n)) % (unsigned) (base); \
- (n) = ((unsigned long) (n)) / (unsigned) (base); \
- _res; \
-})
-
-#else
-/*
- * unsigned long long division. Yuck Yuck! What is Linux coming to?
- * This is 100% disgusting
- */
-#define do_div(n,base) \
-({ \
- unsigned long __low, __low2, __high, __rem; \
- __low = (n) & 0xffffffff; \
- __high = (n) >> 32; \
- if (__high) { \
- __rem = __high % (unsigned long)base; \
- __high = __high / (unsigned long)base; \
- __low2 = __low >> 16; \
- __low2 += __rem << 16; \
- __rem = __low2 % (unsigned long)base; \
- __low2 = __low2 / (unsigned long)base; \
- __low = __low & 0xffff; \
- __low += __rem << 16; \
- __rem = __low % (unsigned long)base; \
- __low = __low / (unsigned long)base; \
- n = __low + ((long long)__low2 << 16) + \
- ((long long) __high << 32); \
- } else { \
- __rem = __low % (unsigned long)base; \
- n = (__low / (unsigned long)base); \
- } \
- __rem; \
-})
-#endif
-
-#endif
-
Index: include/asm-ppc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsl8SXkz Thu Feb 14 16:48:21 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,13 +0,0 @@
-/*
- * BK Id: SCCS/s.div64.h 1.5 05/17/01 18:14:24 cort
- */
-#ifndef __PPC_DIV64
-#define __PPC_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif
Index: include/asm-s390x/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvs0sUH4K Thu Feb 14 16:48:22 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,10 +0,0 @@
-#ifndef __S390_DIV64
-#define __S390_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif
Index: include/asm-sh/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsvYhpX1 Thu Feb 14 16:48:22 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,10 +0,0 @@
-#ifndef __ASM_SH_DIV64
-#define __ASM_SH_DIV64
-
-#define do_div(n,base) ({ \
-int __res; \
-__res = ((unsigned long) n) % (unsigned) base; \
-n = ((unsigned long) n) / (unsigned) base; \
-__res; })
-
-#endif /* __ASM_SH_DIV64 */
Index: include/asm-sparc/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsgmV0Up Thu Feb 14 16:48:22 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,11 +0,0 @@
-#ifndef __SPARC_DIV64
-#define __SPARC_DIV64
-
-/* We're not 64-bit, but... */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* __SPARC_DIV64 */
Index: include/asm-sparc64/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /tmp/cvsRaRtBV Thu Feb 14 16:48:22 2002
+++ /dev/null Tue May 5 13:32:27 1998
@@ -1,14 +0,0 @@
-#ifndef __SPARC64_DIV64
-#define __SPARC64_DIV64
-
-/*
- * Hey, we're already 64-bit, no
- * need to play games..
- */
-#define do_div(n,base) ({ \
- int __res; \
- __res = ((unsigned long) n) % (unsigned) base; \
- n = ((unsigned long) n) / (unsigned) base; \
- __res; })
-
-#endif /* __SPARC64_DIV64 */
Index: include/linux/div64.h
===================================================================
RCS file: div64.h
diff -N div64.h
--- /dev/null Tue May 5 13:32:27 1998
+++ div64.h Thu Feb 14 16:47:33 2002
@@ -0,0 +1,89 @@
+/*
+ * include/linux/div64.h
+ *
+ * Primarily used by vsprintf to divide a 64 bit int N by a small integer base
+ * and provide both quotient and remainder.
+ *
+ * We really do NOT want to encourage people to do slow 64 bit divides in
+ * the kernel, so the 'default' version of this function prints a warning
+ * try and divide a 64 bit number by anything other than 8 or 16.
+ *
+ * If you really *really* need this, #define USE_SLOW_64BIT_DIVIDES
+ * before including this file.
+ *
+ *
+ */
+#ifndef __DIV64
+#define __DIV64
+
+#include <linux/config.h>
+#include <asm/processor.h>
+
+/* configurable, define this if you want the original ASM */
+#undef __USE_ASM
+
+
+#ifdef __USE_ASM
+/* yeah, this is a mess, and leaves out m68k.... */
+# if defined(CONFIG_X86) || defined(CONFIG_ARCH_S390) || defined(CONFIG_MIPS) || defined(__mc68000__)
+# define __USE_ASM__
+# endif
+#endif
+
+#ifdef __USE_ASM__
+#include <asm/div64.h>
+#else /* __USE_ASM__ */
+static inline int do_div(unsigned long long * n, uint16_t base)
+{
+ int res = 0;
+ unsigned long long t = *n;
+ /* this should handle 64 bit platforms, as well as
+ * the case of n < 32 bits (vsprintf does this a lot) */
+ if ((BITS_PER_LONG == 64) || (t == (unsigned long)t)){
+ res = ((unsigned long) t) % base;
+ t = ((unsigned long) t) / base;
+ } else {
+#ifndef USE_SLOW_64BIT_DIVIDES
+ switch (base) {
+ case 8:
+ res = ((unsigned long) t & 0x7);
+ t = t >> 3;
+ break;
+ case 16:
+ res = ((unsigned long) t & 0xf);
+ t = t >> 4;
+ break;
+ default:
+ printk(KERN_WARNING "do_div called at 0x%x\n with unsupported base %d", current_text_addr(), base);
+ t = 0;
+ res = 0;
+ }
+#else /* USE_SLOW_64BIT_DIVIDES */
+ /*
+ * generic C 64 bit divide for a 'small' base ( < 65536)
+ */
+ unsigned long low, low2, high;
+ low = (t) & 0xffffffff;
+ high = (t) >> 32;
+ res = high % base;
+ high = high / base;
+ low2 = low >> 16;
+ low2 += res << 16;
+ res = low2 % base;
+ low2 = low2 / base;
+ low = low & 0xffff;
+ low += res << 16;
+ res = low % base;
+ low = low / base;
+ t = low + ((long long)low2 << 16) +
+ ((long long) high << 32);
+#endif /* USE_SLOW_64BIT_DIVIDES */
+ }
+ *n = t;
+ return res;
+}
+
+
+#endif /* __USE_ASM__ */
+
+#endif
Index: lib/vsprintf.c
===================================================================
RCS file: /cvsdev/hhl-2.4.17/linux/lib/vsprintf.c,v
retrieving revision 1.1
diff -u -r1.1 vsprintf.c
--- lib/vsprintf.c 2001/11/30 22:28:59 1.1
+++ lib/vsprintf.c 2002/02/15 00:47:56
@@ -19,9 +19,9 @@
#include <linux/string.h>
#include <linux/ctype.h>
#include <linux/kernel.h>
+/* #define USE_SLOW_64BIT_DIVIDE */
+#include <linux/div64.h>

-#include <asm/div64.h>
-
/**
* simple_strtoul - convert a string to an unsigned long
* @cp: The start of the string
@@ -165,7 +165,7 @@
if (num == 0)
tmp[i++]='0';
else while (num != 0)
- tmp[i++] = digits[do_div(num,base)];
+ tmp[i++] = digits[do_div(&num,base)];
if (i > precision)
precision = i;
size -= precision;
@@ -426,22 +426,31 @@
}
continue;
}
- if (qualifier == 'L')
+
+ switch (qualifier) {
+ case 'L':
num = va_arg(args, long long);
- else if (qualifier == 'l') {
- num = va_arg(args, unsigned long);
+ break;
+ case 'l':
if (flags & SIGN)
- num = (signed long) num;
- } else if (qualifier == 'Z') {
+ num = (signed long long) va_arg(args, long);
+ else
+ num = va_arg(args, unsigned long);
+ break;
+ case 'Z':
num = va_arg(args, size_t);
- } else if (qualifier == 'h') {
- num = (unsigned short) va_arg(args, int);
+ break;
+ case 'h':
if (flags & SIGN)
- num = (signed short) num;
- } else {
- num = va_arg(args, unsigned int);
+ num = (signed long long) va_arg(args, int);
+ else
+ num = va_arg(args, unsigned int);
+ break;
+ default:
if (flags & SIGN)
- num = (signed int) num;
+ num = (signed long long) va_arg(args, int);
+ else
+ num = va_arg(args, unsigned int);
}
str = number(str, end, num, base,
field_width, precision, flags);

2002-02-22 15:01:57

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Anton Altaparmakov <[email protected]> writes:

> At 17:57 08/02/02, Troy Benjegerdes wrote:
> >Well, there's a reason I left out CONFIG_M68K deps.. Go tell me where
> >CONFIG_M68K is defined.. ;)
>
> Appologies, it's in Configure.help but that is not a too useful places to
> have it. However the kernel seems to be using:
>
> #if defined(__mc68000__) so just use that instead. Any m68k people reading
> this care to comment?

__mc68000__ is the correct define, I don't know who put in CONFIG_M68K
but it doesn't belong there.

Jes

2002-02-22 15:17:49

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Jes Sorensen wrote:
>
> Anton Altaparmakov <[email protected]> writes:
>
> > At 17:57 08/02/02, Troy Benjegerdes wrote:
> > >Well, there's a reason I left out CONFIG_M68K deps.. Go tell me where
> > >CONFIG_M68K is defined.. ;)
> >
> > Appologies, it's in Configure.help but that is not a too useful places to
> > have it. However the kernel seems to be using:
> >
> > #if defined(__mc68000__) so just use that instead. Any m68k people reading
> > this care to comment?
>
> __mc68000__ is the correct define, I don't know who put in CONFIG_M68K
> but it doesn't belong there.

I disagree -- look at arch/*/config.in.

Each arch needs to define a CONFIG_$ARCH.

It looks like cris and mk68 are one of the few arches that fail to do
this...
> # Identify this as a Sparc64 build
> define_bool CONFIG_SPARC64 y

Sigh, this should be in an 'arch' specification :)

Jeff


--
Jeff Garzik | "UNIX enhancements aren't."
Building 1024 | -- says /usr/games/fortune
MandrakeSoft |

2002-02-22 15:20:29

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

>>>>> "Jeff" == Jeff Garzik <[email protected]> writes:

Jeff> Jes Sorensen wrote:
>> __mc68000__ is the correct define, I don't know who put in
>> CONFIG_M68K but it doesn't belong there.

Jeff> I disagree -- look at arch/*/config.in.

Jeff> Each arch needs to define a CONFIG_$ARCH.

Why? CONFIG_$ARCH only makes sense if you can enable two architectures
in the same build. What does CONFIG_M68K give you that __mc68000__
doesn't provide?

Jes

2002-02-22 15:26:39

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Jes Sorensen wrote:
>
> >>>>> "Jeff" == Jeff Garzik <[email protected]> writes:
>
> Jeff> Jes Sorensen wrote:
> >> __mc68000__ is the correct define, I don't know who put in
> >> CONFIG_M68K but it doesn't belong there.
>
> Jeff> I disagree -- look at arch/*/config.in.
>
> Jeff> Each arch needs to define a CONFIG_$ARCH.
>
> Why? CONFIG_$ARCH only makes sense if you can enable two architectures
> in the same build. What does CONFIG_M68K give you that __mc68000__
> doesn't provide?

1) it is a Linux kernel standard. all arches save two define
CONFIG_$arch.

2) you have two tests, "ARCH==m68k" in config.in and "__mc68000__" in C
code. CONFIG_M68K means you only test one symbol, the same symbol, in
all code.

3) as this thread shows, due to #1, users -expect- that CONFIG_M68K will
exist

--
Jeff Garzik | "UNIX enhancements aren't."
Building 1024 | -- says /usr/games/fortune
MandrakeSoft |

2002-02-22 15:34:39

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

>>>>> "Jeff" == Jeff Garzik <[email protected]> writes:

Jeff> Jes Sorensen wrote:
>> Why? CONFIG_$ARCH only makes sense if you can enable two
>> architectures in the same build. What does CONFIG_M68K give you that
>> __mc68000__ doesn't provide?

Jeff> 1) it is a Linux kernel standard. all arches save two define
Jeff> CONFIG_$arch.

Ehm, what standard? the standard way has always been ARCH==, CONFIG_PPC
used to be the only place using this and all it did was to make things
uglier and inconsistent.

Jeff> 2) you have two tests, "ARCH==m68k" in config.in and "__mc68000__"
Jeff> in C code. CONFIG_M68K means you only test one symbol, the same
Jeff> symbol, in all code.

If you want to do that, then one should use CONFIG_<ARCH> in the
Makefiles as well.

Jeff> 3) as this thread shows, due to #1, users -expect- that
Jeff> CONFIG_M68K will exist

Ehm, most kernel developers will expect ARCH== in Config.in as thats
how it's always been.

Jes

2002-02-22 15:56:19

by Andreas Schwab

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Jeff Garzik <[email protected]> writes:

|> Jes Sorensen wrote:
|> >
|> > >>>>> "Jeff" == Jeff Garzik <[email protected]> writes:
|> >
|> > Jeff> Jes Sorensen wrote:
|> > >> __mc68000__ is the correct define, I don't know who put in
|> > >> CONFIG_M68K but it doesn't belong there.
|> >
|> > Jeff> I disagree -- look at arch/*/config.in.
|> >
|> > Jeff> Each arch needs to define a CONFIG_$ARCH.
|> >
|> > Why? CONFIG_$ARCH only makes sense if you can enable two architectures
|> > in the same build. What does CONFIG_M68K give you that __mc68000__
|> > doesn't provide?
|>
|> 1) it is a Linux kernel standard. all arches save two define
|> CONFIG_$arch.
|>
|> 2) you have two tests, "ARCH==m68k" in config.in and "__mc68000__" in C
|> code. CONFIG_M68K means you only test one symbol, the same symbol, in
|> all code.
|>
|> 3) as this thread shows, due to #1, users -expect- that CONFIG_M68K will
|> exist

If consistency is your goal then CONFIG_I386 should be defined.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2002-02-22 16:01:09

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Andreas Schwab wrote:
> If consistency is your goal then CONFIG_I386 should be defined.

I was referring to consistency of having one symbol that can be tested
in most (note: not all) makefiles, C code, and config.in code.

The i386 arch already has CONFIG_X86. I agree that if someone cared to
make define always equal CONFIG_upcase($arch), you are correct.

Jeff



--
Jeff Garzik | "UNIX enhancements aren't."
Building 1024 | -- says /usr/games/fortune
MandrakeSoft |

2002-02-22 16:06:19

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

Jes Sorensen wrote:
>
> >>>>> "Jeff" == Jeff Garzik <[email protected]> writes:
>
> Jeff> Jes Sorensen wrote:
> >> Why? CONFIG_$ARCH only makes sense if you can enable two
> >> architectures in the same build. What does CONFIG_M68K give you that
> >> __mc68000__ doesn't provide?
>
> Jeff> 1) it is a Linux kernel standard. all arches save two define
> Jeff> CONFIG_$arch.
>
> Ehm, what standard? the standard way has always been ARCH==, CONFIG_PPC
> used to be the only place using this and all it did was to make things
> uglier and inconsistent.

*shrug* I've known about this for years. And every arch but m68k and
cris follows the standard.


> Jeff> 2) you have two tests, "ARCH==m68k" in config.in and "__mc68000__"
> Jeff> in C code. CONFIG_M68K means you only test one symbol, the same
> Jeff> symbol, in all code.
>
> If you want to do that, then one should use CONFIG_<ARCH> in the
> Makefiles as well.

I'm betting a few special makefiles require $ARCH information when
.config is info is not available. Otherwise, agreed.


> Jeff> 3) as this thread shows, due to #1, users -expect- that
> Jeff> CONFIG_M68K will exist
>
> Ehm, most kernel developers will expect ARCH== in Config.in as thats
> how it's always been.

and I forgot about this flat-out bug:

4) you cannot test ARCH==m68k as a component of dep_xxx declarations.
And yes, this is a requirement, this is used currently in */config.in
all over the kernel tree.

Jeff


--
Jeff Garzik | "UNIX enhancements aren't."
Building 1024 | -- says /usr/games/fortune
MandrakeSoft |

2002-02-22 16:09:09

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH] bring sanity to div64.h and do_div usage

On Fri, 22 Feb 2002, Jes Sorensen wrote:

> >>>>> "Jeff" == Jeff Garzik <[email protected]> writes:
>
> Jeff> Jes Sorensen wrote:
> >> __mc68000__ is the correct define, I don't know who put in
> >> CONFIG_M68K but it doesn't belong there.
>
> Jeff> I disagree -- look at arch/*/config.in.
>
> Jeff> Each arch needs to define a CONFIG_$ARCH.
>
> Why? CONFIG_$ARCH only makes sense if you can enable two architectures
> in the same build. What does CONFIG_M68K give you that __mc68000__
> doesn't provide?

Uniformity.


Nicolas