[PATCH] Cleanups & fixes for the MTRR driver

[PATCH] Cleanups & fixes for the MTRR driver

[Andi Kleen]

To fix some bugs and cut down code duplication between i386 and x86-64
I decided to reuse (using symlinks) the i386 MTRR driver for x86-64.
To do that I did some cleanups and a few bug fixes in the i386 mtrr driver.

- Convert it to seq_file. This makes it a lot cleaner and gets rid
of some cruft.
- Some long -> int required on 64bit (nop on 32bit) 
- Add some RED-PENs for unfixed bugs (e.g. it doesn't support
physical addresses > 4GB on 32bit hosts and the SMP locking is not
very strong) 
- Remove bogus // in mtrr_write that made the function a nop
(= only ioctl was working for changing) 
- Unify duplicated mtrr_strings define.
- Some other minor fixes and cleanups

Patch for 2.5.49.

Please consider applying, 

-Andi



diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/generic.c linux/arch/i386/kernel/cpu/mtrr/generic.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/generic.c	2002-11-13 12:07:27.000000000 +0100
+++ linux/arch/i386/kernel/cpu/mtrr/generic.c	2002-11-27 12:13:18.000000000 +0100
@@ -1,3 +1,5 @@
+/* This only handles 32bit MTRR on 32bit hosts. This is strictly wrong
+   because MTRRs can span upto 40 bits (36bits on most modern x86) */ 
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/mm.h>
@@ -30,7 +32,7 @@ get_mtrr_var_range(unsigned int index, s
 static void __init
 get_fixed_ranges(mtrr_type * frs)
 {
-	unsigned long *p = (unsigned long *) frs;
+	unsigned int *p = (unsigned int *) frs;
 	int i;
 
 	rdmsr(MTRRfix64K_00000_MSR, p[0], p[1]);
@@ -46,7 +48,7 @@ void get_mtrr_state(void)
 {
 	unsigned int i;
 	struct mtrr_var_range *vrs;
-	unsigned long lo, dummy;
+	unsigned lo, dummy;
 
 	if (!mtrr_state.var_ranges) {
 		mtrr_state.var_ranges = kmalloc(num_var_ranges * sizeof (struct mtrr_var_range), 
@@ -113,11 +115,10 @@ int generic_get_free_region(unsigned lon
 	return -ENOSPC;
 }
 
-
 void generic_get_mtrr(unsigned int reg, unsigned long *base,
 		      unsigned long *size, mtrr_type * type)
 {
-	unsigned long mask_lo, mask_hi, base_lo, base_hi;
+	unsigned int mask_lo, mask_hi, base_lo, base_hi;
 
 	rdmsr(MTRRphysMask_MSR(reg), mask_lo, mask_hi);
 	if ((mask_lo & 0x800) == 0) {
@@ -143,10 +144,10 @@ void generic_get_mtrr(unsigned int reg, 
 
 static int __init set_fixed_ranges(mtrr_type * frs)
 {
-	unsigned long *p = (unsigned long *) frs;
+	unsigned int *p = (unsigned int *) frs;
 	int changed = FALSE;
 	int i;
-	unsigned long lo, hi;
+	unsigned int lo, hi;
 
 	rdmsr(MTRRfix64K_00000_MSR, lo, hi);
 	if (p[0] != lo || p[1] != hi) {
@@ -228,13 +229,13 @@ static unsigned long set_mtrr_state(u32 
 }
 
 
-static u32 cr4 = 0;
+static unsigned long cr4 = 0;
 static u32 deftype_lo, deftype_hi;
 static spinlock_t set_atomicity_lock = SPIN_LOCK_UNLOCKED;
 
 static void prepare_set(void)
 {
-	u32 cr0;
+	unsigned long cr0;
 
 	/*  Note that this is not ideal, since the cache is only flushed/disabled
 	   for this CPU while the MTRRs are changed, but changing this requires
diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/if.c linux/arch/i386/kernel/cpu/mtrr/if.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/if.c	2002-10-19 06:01:18.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/if.c	2002-11-27 12:33:08.000000000 +0100
@@ -3,27 +3,27 @@
 #include <linux/devfs_fs_kernel.h>
 #include <linux/ctype.h>
 #include <linux/module.h>
+#include <linux/seq_file.h>
 #include <asm/uaccess.h>
 
-/* What kind of fucking hack is this? */
-#define MTRR_NEED_STRINGS
+#define LINE_SIZE 80
 
 #include <asm/mtrr.h>
 #include "mtrr.h"
 
-static char *ascii_buffer;
-static unsigned int ascii_buf_bytes;
-
+/* RED-PEN: this is accessed without any locking */
 extern unsigned int *usage_table;
 
-#define LINE_SIZE      80
+static int mtrr_seq_show(struct seq_file *seq, void *offset);
+
+#define FILE_FCOUNT(f) (((struct seq_file *)((f)->private_data))->private)
 
 static int
 mtrr_file_add(unsigned long base, unsigned long size,
 	      unsigned int type, char increment, struct file *file, int page)
 {
 	int reg, max;
-	unsigned int *fcount = file->private_data;
+	unsigned int *fcount = FILE_FCOUNT(file); 
 
 	max = num_var_ranges;
 	if (fcount == NULL) {
@@ -33,7 +33,7 @@ mtrr_file_add(unsigned long base, unsign
 			return -ENOMEM;
 		}
 		memset(fcount, 0, max * sizeof *fcount);
-		file->private_data = fcount;
+		FILE_FCOUNT(file) = fcount;
 	}
 	if (!page) {
 		if ((base & (PAGE_SIZE - 1)) || (size & (PAGE_SIZE - 1))) {
@@ -79,19 +79,7 @@ mtrr_file_del(unsigned long base, unsign
 	return reg;
 }
 
-static ssize_t
-mtrr_read(struct file *file, char *buf, size_t len, loff_t * ppos)
-{
-	if (*ppos >= ascii_buf_bytes)
-		return 0;
-	if (*ppos + len > ascii_buf_bytes)
-		len = ascii_buf_bytes - *ppos;
-	if (copy_to_user(buf, ascii_buffer + *ppos, len))
-		return -EFAULT;
-	*ppos += len;
-	return len;
-}
-
+/* RED-PEN: seq_file can seek now. this is ignored. */
 static ssize_t
 mtrr_write(struct file *file, const char *buf, size_t len, loff_t * ppos)
 /*  Format of control line:
@@ -149,7 +137,7 @@ mtrr_write(struct file *file, const char
 	ptr += 5;
 	for (; isspace(*ptr); ++ptr) ;
 	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
-//		if (strcmp(ptr, mtrr_strings[i]))
+		if (strcmp(ptr, mtrr_strings[i]))
 			continue;
 		base >>= PAGE_SHIFT;
 		size >>= PAGE_SHIFT;
@@ -173,6 +161,8 @@ mtrr_ioctl(struct inode *inode, struct f
 	struct mtrr_sentry sentry;
 	struct mtrr_gentry gentry;
 
+	printk("mtrr_ioctl %d\n", cmd);
+
 	switch (cmd) {
 	default:
 		return -ENOIOCTLCMD;
@@ -291,10 +281,9 @@ static int
 mtrr_close(struct inode *ino, struct file *file)
 {
 	int i, max;
-	unsigned int *fcount = file->private_data;
+	unsigned int *fcount = FILE_FCOUNT(file);
 
-	if (fcount == NULL)
-		return 0;
+	if (fcount != NULL) {
 	max = num_var_ranges;
 	for (i = 0; i < max; ++i) {
 		while (fcount[i] > 0) {
@@ -304,13 +293,26 @@ mtrr_close(struct inode *ino, struct fil
 		}
 	}
 	kfree(fcount);
-	file->private_data = NULL;
-	return 0;
+		FILE_FCOUNT(file) = NULL;
+	}
+	return single_release(ino, file);
+}
+
+static int mtrr_open(struct inode *inode, struct file *file)
+{
+	if (!mtrr_if) 
+		return -EIO;
+	if (!mtrr_if->get) 
+		return -ENXIO; 
+	return single_open(file, mtrr_seq_show, NULL);
 }
 
 static struct file_operations mtrr_fops = {
 	.owner   = THIS_MODULE,
-	.read    = mtrr_read,
+	.open	 = mtrr_open, 
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
 	.write   = mtrr_write,
 	.ioctl   = mtrr_ioctl,
 	.release = mtrr_close,
@@ -329,14 +331,14 @@ char * attrib_to_str(int x)
 	return (x <= 6) ? mtrr_strings[x] : "?";
 }
 
-void compute_ascii(void)
+static int mtrr_seq_show(struct seq_file *seq, void *offset)
 {
 	char factor;
-	int i, max;
+	int i, max, len;
 	mtrr_type type;
 	unsigned long base, size;
 
-	ascii_buf_bytes = 0;
+	len = 0;
 	max = num_var_ranges;
 	for (i = 0; i < max; i++) {
 		mtrr_if->get(i, &base, &size, &type);
@@ -351,32 +353,19 @@ void compute_ascii(void)
 				factor = 'M';
 				size >>= 20 - PAGE_SHIFT;
 			}
-			sprintf
-			    (ascii_buffer + ascii_buf_bytes,
+			/* RED-PEN: base can be > 32bit */ 
+			len += seq_printf(seq, 
 			     "reg%02i: base=0x%05lx000 (%4liMB), size=%4li%cB: %s, count=%d\n",
 			     i, base, base >> (20 - PAGE_SHIFT), size, factor,
 			     attrib_to_str(type), usage_table[i]);
-			ascii_buf_bytes +=
-			    strlen(ascii_buffer + ascii_buf_bytes);
 		}
 	}
-	devfs_set_file_size(devfs_handle, ascii_buf_bytes);
-#  ifdef CONFIG_PROC_FS
-	if (proc_root_mtrr)
-		proc_root_mtrr->size = ascii_buf_bytes;
-#  endif			/*  CONFIG_PROC_FS  */
+	devfs_set_file_size(devfs_handle, len);	
+	return 0;
 }
 
 static int __init mtrr_if_init(void)
 {
-	int max = num_var_ranges;
-
-	if ((ascii_buffer = kmalloc(max * LINE_SIZE, GFP_KERNEL)) == NULL) {
-		printk("mtrr: could not allocate\n");
-		return -ENOMEM;
-	}
-	ascii_buf_bytes = 0;
-	compute_ascii();
 #ifdef CONFIG_PROC_FS
 	proc_root_mtrr =
 	    create_proc_entry("mtrr", S_IWUSR | S_IRUGO, &proc_root);
diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/main.c linux/arch/i386/kernel/cpu/mtrr/main.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/main.c	2002-10-19 06:01:09.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/main.c	2002-11-27 11:43:26.000000000 +0100
@@ -36,7 +36,6 @@
 #include <linux/pci.h>
 #include <linux/smp.h>
 
-#define MTRR_NEED_STRINGS
 #include <asm/mtrr.h>
 
 #include <asm/uaccess.h>
@@ -54,6 +53,7 @@ static DECLARE_MUTEX(main_lock);
 u32 size_or_mask, size_and_mask;
 
 static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
+
 struct mtrr_ops * mtrr_if = NULL;
 
 __initdata char *mtrr_if_name[] = {
@@ -125,14 +125,6 @@ static void init_table(void)
 	}
 	for (i = 0; i < max; i++)
 		usage_table[i] = 1;
-#ifdef USERSPACE_INTERFACE
-	if ((ascii_buffer = kmalloc(max * LINE_SIZE, GFP_KERNEL)) == NULL) {
-		printk("mtrr: could not allocate\n");
-		return;
-	}
-	ascii_buf_bytes = 0;
-	compute_ascii();
-#endif
 }
 
 struct set_mtrr_data {
@@ -163,7 +155,7 @@ static void ipi_handler(void *info)
 	}
 
 	/*  The master has cleared me to execute  */
-	if (data->smp_reg != ~0UL) 
+	if (data->smp_reg != ~0U) 
 		mtrr_if->set(data->smp_reg, data->smp_base, 
 			     data->smp_size, data->smp_type);
 	else
@@ -253,7 +245,7 @@ static void set_mtrr(unsigned int reg, u
 	 * to replicate across all the APs. 
 	 * If we're doing that @reg is set to something special...
 	 */
-	if (reg != ~0UL) 
+	if (reg != ~0U) 
 		mtrr_if->set(reg,base,size,type);
 
 	/* wait for the others */
@@ -361,7 +353,6 @@ int mtrr_add_page(unsigned long base, un
 		}
 		if (increment)
 			++usage_table[i];
-		compute_ascii();
 		error = i;
 		goto out;
 	}
@@ -370,7 +361,6 @@ int mtrr_add_page(unsigned long base, un
 	if (i >= 0) {
 		set_mtrr(i, base, size, type);
 		usage_table[i] = 1;
-		compute_ascii();
 	} else
 		printk("mtrr: no more MTRRs available\n");
 	error = i;
@@ -491,7 +481,6 @@ int mtrr_del_page(int reg, unsigned long
 	}
 	if (--usage_table[reg] < 1)
 		set_mtrr(reg, 0, 0, 0);
-	compute_ascii();
 	error = reg;
  out:
 	up(&main_lock);
@@ -547,7 +536,7 @@ static void init_other_cpus(void)
 		get_mtrr_state();
 
 	/* bring up the other processors */
-	set_mtrr(~0UL,0,0,0);
+	set_mtrr(~0U,0,0,0);
 
 	if (use_intel()) {
 		finalize_mtrr_state();
@@ -583,7 +572,7 @@ static int __init mtrr_init(void)
 			   query the width (in bits) of the physical
 			   addressable memory on the Hammer family.
 			 */
-			if (boot_cpu_data.x86 == 7
+			if (boot_cpu_data.x86 >= 7 
 			    && (cpuid_eax(0x80000000) >= 0x80000008)) {
 				u32 phys_addr;
 				phys_addr = cpuid_eax(0x80000008) & 0xff;
@@ -643,5 +632,16 @@ static int __init mtrr_init(void)
 	return mtrr_if ? -ENXIO : 0;
 }
 
+char *mtrr_strings[MTRR_NUM_TYPES] =
+{
+    "uncachable",               /* 0 */
+    "write-combining",          /* 1 */
+    "?",                        /* 2 */
+    "?",                        /* 3 */
+    "write-through",            /* 4 */
+    "write-protect",            /* 5 */
+    "write-back",               /* 6 */
+};
+
 core_initcall(mtrr_init);
 
diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/mtrr.h linux/arch/i386/kernel/cpu/mtrr/mtrr.h
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/mtrr.h	2002-10-19 06:01:18.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/mtrr.h	2002-11-27 12:03:45.000000000 +0100
@@ -85,9 +85,6 @@ void get_mtrr_state(void);
 
 extern void set_mtrr_ops(struct mtrr_ops * ops);
 
-/* Don't even ask... */
-extern void compute_ascii(void);
-
 extern u32 size_or_mask, size_and_mask;
 extern struct mtrr_ops * mtrr_if;
 
diff -burpN -X ../KDIFX linux-vanilla/include/asm-i386/mtrr.h linux/include/asm-i386/mtrr.h
--- linux-vanilla/include/asm-i386/mtrr.h	2002-10-19 06:01:52.000000000 +0200
+++ linux/include/asm-i386/mtrr.h	2002-11-27 12:41:20.000000000 +0100
@@ -65,21 +65,10 @@ struct mtrr_gentry
 #define MTRR_TYPE_WRBACK     6
 #define MTRR_NUM_TYPES       7
 
-#ifdef MTRR_NEED_STRINGS
-static char *mtrr_strings[MTRR_NUM_TYPES] =
-{
-    "uncachable",               /* 0 */
-    "write-combining",          /* 1 */
-    "?",                        /* 2 */
-    "?",                        /* 3 */
-    "write-through",            /* 4 */
-    "write-protect",            /* 5 */
-    "write-back",               /* 6 */
-};
-#endif
-
 #ifdef __KERNEL__
 
+extern char *mtrr_strings[]; 
+
 /*  The following functions are for use by other drivers  */
 # ifdef CONFIG_MTRR
 extern int mtrr_add (unsigned long base, unsigned long size,