Updated cpu module autoprobing patchkit

Updated cpu module autoprobing patchkit

[Andi Kleen]

Due to popular demand a new version of the cpu module autoprobing
patchkit. This exports x86 CPUID information (features, vendors, models etc.)
in a way that udev can autoprobe modules, similar to how
USB or PCI devices are autoloaded. This allows to easily
add CPU specific modules without requiring distributions
to have special case init scripts.

The biggest difference compared to the old version is that it 
hangs off the cpuid devices now, now the cpu devices. cpu devices
were sysdevs which don't have all the infrastructure needed 
for udev. cpuid has all this so it works much better.
This requires the cpuid driver to be loaded or builtin for 
the autoprobing to work.

Any testing appreciated

-Andi

[PATCH 01/10] Add driver auto probing for x86 features

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Old patchkit, resurrect due to popular demand.

There's a growing number of drivers that support a specific x86 feature
or CPU.  Currently loading these drivers currently on a generic
distribution requires various driver specific hacks and it often
doesn't work.

This patch adds auto probing for drivers based on the x86 cpuid
information, in particular based on vendor/family/model number
and also based on CPUID feature bits.

For example a common issue is not loading the SSE 4.2 accelerated
CRC module: this can significantly lower the performance of BTRFS
which relies on fast CRC.

Another issue is loading the right CPUFREQ driver for the current CPU.
Currently distributions often try all all possible driver until
one sticks, which is not really a good way to do this.

It works with existing udev without any changes. The code
exports the x86 information as a generic string in sysfs
that can be matched by udev's pattern matching.

This scheme does not support numeric ranges, so if you want to
handle e.g. ranges of model numbers they have to be encoded
in ASCII or simply all models or families listed. Fixing
that would require changing udev.

Another issue is that udev will happily load all drivers that match,
there is currently no nice way to stop a specific driver from
being loaded if it's not needed (e.g. if you don't need fast CRC)
But there are not that many cpu specific drivers around and they're
all not that bloated, so this isn't a particularly serious issue.

Originally this patch added the modalias to the normal cpu
sysdevs. However sysdevs don't have all the infrastructure
needed for udev, so it couldn't really autoload drivers.
This patch instead adds the CPU modaliases to the cpuid devices,
which are real devices with full support for udev. This implies
that the cpuid driver has to be loaded to use this.

This patch just adds infrastructure, some driver conversions
in followups.

Thanks to Kay for helping with some sysfs magic.

Cc: davej@redhat.com
Cc: trenn@suse.de
Cc: kay.sievers@vrfy.org
Cc: axboe@kernel.dk
Cc: hpa@zytor.com
Cc: herbert@gondor.apana.org.au
Cc: ying.huang@intel.com
Cc: lenb@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/cpu_device_id.h |   13 +++++++
 arch/x86/kernel/cpu/Makefile         |    1 +
 arch/x86/kernel/cpu/match.c          |   48 ++++++++++++++++++++++++++++
 arch/x86/kernel/cpuid.c              |   58 +++++++++++++++++++++++++++++++++-
 include/linux/mod_devicetable.h      |   20 ++++++++++++
 scripts/mod/file2alias.c             |   27 ++++++++++++++++
 6 files changed, 166 insertions(+), 1 deletions(-)
 create mode 100755 arch/x86/include/asm/cpu_device_id.h
 create mode 100644 arch/x86/kernel/cpu/match.c

diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
new file mode 100755
index 0000000..2f7d127
--- /dev/null
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -0,0 +1,13 @@
+#ifndef _CPU_DEVICE_ID
+#define _CPU_DEVICE_ID 1
+
+/*
+ * Declare drivers belonging to specific x86 CPUs
+ * Similar in spirit to pci_device_id and related PCI functions
+ */
+
+#include <linux/mod_devicetable.h>
+
+extern struct x86_cpu_id *x86_match_cpu(struct x86_cpu_id *match);
+
+#endif
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 25f24dc..6ab6aa2 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -16,6 +16,7 @@ obj-y			:= intel_cacheinfo.o scattered.o topology.o
 obj-y			+= proc.o capflags.o powerflags.o common.o
 obj-y			+= vmware.o hypervisor.o sched.o mshyperv.o
 obj-y			+= rdrand.o
+obj-y			+= match.o
 
 obj-$(CONFIG_X86_32)	+= bugs.o
 obj-$(CONFIG_X86_64)	+= bugs_64.o
diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
new file mode 100644
index 0000000..2c23457
--- /dev/null
+++ b/arch/x86/kernel/cpu/match.c
@@ -0,0 +1,48 @@
+#include <asm/cpu_device_id.h>
+#include <asm/processor.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+
+/**
+ * x86_match_cpu - match current CPU again an array of x86_cpu_ids
+ * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
+ *         X86_MODEL_END.
+ *
+ * Return the entry if the current CPU matches the entries in the
+ * passed x86_cpu_id match table. Otherwise NULL.  The match table
+ * contains vendor (X86_VENDOR_*), family, model and feature bits or
+ * respective wildcard entries.
+ *
+ * A typical table entry would be to match a specific CPU
+ * { X86_VENDOR_INTEL, 6, 0x12 }
+ * or to match a specific CPU feature
+ * { X86_FEATURE_MATCH(X86_FEATURE_FOOBAR) }
+ *
+ * Felds can be wildcarded with %X86_VENDOR_ANY, %X86_FAMILY_ANY,
+ * %X86_MODEL_ANY, %X86_FEATURE_ANY.
+ *
+ * Arrays used to match for this should also be declared using 
+ * MODULE_DEVICE_TABLE(x86_cpu, ...)
+ *
+ * This always matches against the boot cpu, assuming models and features are 
+ * consistent over all CPUs.
+ */
+struct x86_cpu_id *x86_match_cpu(struct x86_cpu_id *match)
+{
+	struct x86_cpu_id *m;
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	for (m = match; m->vendor | m->family | m->model | m->feature; m++) {
+		if (m->vendor != X86_VENDOR_ANY && c->x86_vendor != m->vendor)
+			continue;
+		if (m->family != X86_FAMILY_ANY && c->x86 != m->family)
+			continue;
+		if (m->model != X86_MODEL_ANY && c->x86_model != m->model)
+			continue;
+		if (m->feature != X86_FEATURE_ANY && !cpu_has(c, m->feature))
+			continue;
+		return m;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(x86_match_cpu);
diff --git a/arch/x86/kernel/cpuid.c b/arch/x86/kernel/cpuid.c
index 212a6a4..6bb24d3 100644
--- a/arch/x86/kernel/cpuid.c
+++ b/arch/x86/kernel/cpuid.c
@@ -40,6 +40,7 @@
 #include <linux/notifier.h>
 #include <linux/uaccess.h>
 #include <linux/gfp.h>
+#include <linux/slab.h>
 
 #include <asm/processor.h>
 #include <asm/msr.h>
@@ -138,13 +139,56 @@ static const struct file_operations cpuid_fops = {
 	.open = cpuid_open,
 };
 
+static ssize_t print_cpu_modalias(struct device *dev,
+				  struct device_attribute *attr,
+				  char *bufptr)
+{
+	int size = PAGE_SIZE;
+	int i, n;
+	char *buf = bufptr;
+
+	n = snprintf(buf, size, "x86cpu:vendor:%04x:family:%04x:model:%04x:feature:",
+		boot_cpu_data.x86_vendor,
+		boot_cpu_data.x86,
+		boot_cpu_data.x86_model);
+	size -= n;
+	buf += n;
+	size -= 2;
+	for (i = 0; i < NCAPINTS*32; i++) {
+		if (boot_cpu_has(i)) {
+			n = snprintf(buf, size, ",%04x", i);
+			if (n < 0) {
+				WARN(1, "x86 features overflow page\n");
+				break;
+			}
+			size -= n;
+			buf += n;
+		}
+	}
+	*buf++ = ',';
+	*buf++ = '\n';
+	return buf - bufptr;
+}
+
+static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL);
+
 static __cpuinit int cpuid_device_create(int cpu)
 {
 	struct device *dev;
+	int err;
 
 	dev = device_create(cpuid_class, NULL, MKDEV(CPUID_MAJOR, cpu), NULL,
 			    "cpu%d", cpu);
-	return IS_ERR(dev) ? PTR_ERR(dev) : 0;
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	err = device_create_file(dev, &dev_attr_modalias);
+	if (err) {
+		/* keep device around on error. attribute is optional. */
+		return err;
+	}
+
+	return 0;
 }
 
 static void cpuid_device_destroy(int cpu)
@@ -182,6 +226,17 @@ static char *cpuid_devnode(struct device *dev, mode_t *mode)
 	return kasprintf(GFP_KERNEL, "cpu/%u/cpuid", MINOR(dev->devt));
 }
 
+static int cpuid_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (buf) {
+		print_cpu_modalias(NULL, NULL, buf);
+		add_uevent_var(env, "MODALIAS=%s", buf);
+		kfree(buf);
+	}
+	return 0;
+}
+
 static int __init cpuid_init(void)
 {
 	int i, err = 0;
@@ -200,6 +255,7 @@ static int __init cpuid_init(void)
 		goto out_chrdev;
 	}
 	cpuid_class->devnode = cpuid_devnode;
+	cpuid_class->dev_uevent = cpuid_dev_uevent;
 	for_each_online_cpu(i) {
 		err = cpuid_device_create(i);
 		if (err != 0)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 468819c..8971fec 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -542,4 +542,24 @@ struct isapnp_device_id {
 	kernel_ulong_t driver_data;	/* data private to the driver */
 };
 
+/*
+ * Match x86 CPUs for CPU specific drivers.
+ * See documentation of "x86_match_cpu" for details.
+ */
+
+struct x86_cpu_id {
+	__u16 vendor;
+	__u16 family;
+	__u16 model;
+ 	__u16 feature;	/* bit index */
+	kernel_ulong_t driver_data;
+};
+
+#define X86_FEATURE_MATCH(x) { X86_VENDOR_ANY, X86_FAMILY_ANY, X86_MODEL_ANY, x }
+
+#define X86_VENDOR_ANY 0xffff
+#define X86_FAMILY_ANY 0
+#define X86_MODEL_ANY  0
+#define X86_FEATURE_ANY 0	/* Same as FPU, you can't test for that */
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index f936d1f..e5dd089 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -880,6 +880,29 @@ static int do_isapnp_entry(const char *filename,
 	return 1;
 }
 
+/* LOOKS like x86cpu:vendor:VVVV:family:FFFF:model:MMMM:feature:*,FEAT,*
+ * All fields are numbers. It would be nicer to use strings for vendor
+ * and feature, but getting those out of the build system here is too
+ * complicated.
+ */
+
+static int do_x86cpu_entry(const char *filename, struct x86_cpu_id *id, 
+			   char *alias)
+{
+	id->feature = TO_NATIVE(id->feature);
+	id->family = TO_NATIVE(id->family);
+	id->model = TO_NATIVE(id->model);
+	id->vendor = TO_NATIVE(id->vendor);
+
+	strcpy(alias, "x86cpu:");
+	ADD(alias, "vendor:",  id->vendor != X86_VENDOR_ANY, id->vendor);
+	ADD(alias, ":family:", id->family != X86_FAMILY_ANY, id->family);
+	ADD(alias, ":model:",  id->model  != X86_MODEL_ANY,  id->model);
+	ADD(alias, ":feature:*,", id->feature != X86_FEATURE_ANY, id->feature);
+	strcat(alias, ",*");
+	return 1;
+}
+
 /* Ignore any prefix, eg. some architectures prepend _ */
 static inline int sym_is(const char *symbol, const char *name)
 {
@@ -1047,6 +1070,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 		do_table(symval, sym->st_size,
 			sizeof(struct isapnp_device_id), "isa",
 			do_isapnp_entry, mod);
+	else if (sym_is(symname, "__mod_x86cpu_device_table"))
+		do_table(symval, sym->st_size,
+			 sizeof(struct x86_cpu_id), "x86cpu",
+			 do_x86cpu_entry, mod);
 	free(zeros);
 }
 
-- 
1.7.4.4

[PATCH 02/10] crypto: Add support for x86 cpuid auto loading for x86 crypto drivers

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add support for auto-loading of crypto drivers based on cpuid features.
This enables auto-loading of the VIA and Intel specific drivers
for AES, hashing and CRCs.

Requires the earlier infrastructure patch to add x86 modinfo.
I kept it all in a single patch for now.

I dropped the printks when the driver cpuid doesn't match (imho
drivers never should print anything in such a case)

One drawback is that udev doesn't know if the drivers are used or not,
so they will be unconditionally loaded at boot up. That's better
than not loading them at all, like it often happens.

Cc: davej@redhat.com
Cc: trenn@suse.de
Cc: kay.sievers@vrfy.org
Cc: axboe@kernel.dk
Cc: hpa@zytor.com
Cc: herbert@gondor.apana.org.au
Cc: ying.huang@intel.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/crypto/aesni-intel_glue.c         |   12 +++++++++---
 arch/x86/crypto/crc32c-intel.c             |   11 ++++++++---
 arch/x86/crypto/ghash-clmulni-intel_glue.c |   12 ++++++++----
 drivers/crypto/padlock-aes.c               |    9 ++++++++-
 drivers/crypto/padlock-sha.c               |   16 ++++++++--------
 5 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 545d0ce..4a34a77 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -28,6 +28,7 @@
 #include <crypto/aes.h>
 #include <crypto/cryptd.h>
 #include <crypto/ctr.h>
+#include <asm/cpu_device_id.h>
 #include <asm/i387.h>
 #include <asm/aes.h>
 #include <crypto/scatterwalk.h>
@@ -1253,14 +1254,19 @@ static struct crypto_alg __rfc4106_alg = {
 };
 #endif
 
+
+static struct x86_cpu_id aesni_cpu_id[] = {
+	X86_FEATURE_MATCH(X86_FEATURE_AES),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, aesni_cpu_id);
+
 static int __init aesni_init(void)
 {
 	int err;
 
-	if (!cpu_has_aes) {
-		printk(KERN_INFO "Intel AES-NI instructions are not detected.\n");
+	if (!x86_match_cpu(aesni_cpu_id))
 		return -ENODEV;
-	}
 
 	if ((err = crypto_fpu_init()))
 		goto fpu_err;
diff --git a/arch/x86/crypto/crc32c-intel.c b/arch/x86/crypto/crc32c-intel.c
index b9d0026..0fe11a9 100644
--- a/arch/x86/crypto/crc32c-intel.c
+++ b/arch/x86/crypto/crc32c-intel.c
@@ -31,6 +31,7 @@
 #include <crypto/internal/hash.h>
 
 #include <asm/cpufeature.h>
+#include <asm/cpu_device_id.h>
 
 #define CHKSUM_BLOCK_SIZE	1
 #define CHKSUM_DIGEST_SIZE	4
@@ -173,13 +174,17 @@ static struct shash_alg alg = {
 	}
 };
 
+static struct x86_cpu_id crc32c_cpu_id[] = {
+	X86_FEATURE_MATCH(X86_FEATURE_XMM4_2),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, crc32c_cpu_id);
 
 static int __init crc32c_intel_mod_init(void)
 {
-	if (cpu_has_xmm4_2)
-		return crypto_register_shash(&alg);
-	else
+	if (!x86_match_cpu(crc32c_cpu_id))
 		return -ENODEV;
+	return crypto_register_shash(&alg);
 }
 
 static void __exit crc32c_intel_mod_fini(void)
diff --git a/arch/x86/crypto/ghash-clmulni-intel_glue.c b/arch/x86/crypto/ghash-clmulni-intel_glue.c
index 976aa64..4d5b128 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_glue.c
+++ b/arch/x86/crypto/ghash-clmulni-intel_glue.c
@@ -20,6 +20,7 @@
 #include <crypto/gf128mul.h>
 #include <crypto/internal/hash.h>
 #include <asm/i387.h>
+#include <asm/cpu_device_id.h>
 
 #define GHASH_BLOCK_SIZE	16
 #define GHASH_DIGEST_SIZE	16
@@ -294,15 +295,18 @@ static struct ahash_alg ghash_async_alg = {
 	},
 };
 
+static struct x86_cpu_id pcmul_cpu_id[] = {
+	X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), /* Pickle-Mickle-Duck */
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, pcmul_cpu_id);
+
 static int __init ghash_pclmulqdqni_mod_init(void)
 {
 	int err;
 
-	if (!cpu_has_pclmulqdq) {
-		printk(KERN_INFO "Intel PCLMULQDQ-NI instructions are not"
-		       " detected.\n");
+	if (!x86_match_cpu(pcmul_cpu_id))
 		return -ENODEV;
-	}
 
 	err = crypto_register_shash(&ghash_alg);
 	if (err)
diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 29b9469..37b2e94 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -19,6 +19,7 @@
 #include <linux/percpu.h>
 #include <linux/smp.h>
 #include <linux/slab.h>
+#include <asm/cpu_device_id.h>
 #include <asm/byteorder.h>
 #include <asm/processor.h>
 #include <asm/i387.h>
@@ -503,12 +504,18 @@ static struct crypto_alg cbc_aes_alg = {
 	}
 };
 
+static struct x86_cpu_id padlock_cpu_id[] = {
+	X86_FEATURE_MATCH(X86_FEATURE_XCRYPT),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, padlock_cpu_id);
+
 static int __init padlock_init(void)
 {
 	int ret;
 	struct cpuinfo_x86 *c = &cpu_data(0);
 
-	if (!cpu_has_xcrypt)
+	if (!x86_match_cpu(padlock_cpu_id))
 		return -ENODEV;
 
 	if (!cpu_has_xcrypt_enabled) {
diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c
index 06bdb4b..9266c0e 100644
--- a/drivers/crypto/padlock-sha.c
+++ b/drivers/crypto/padlock-sha.c
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/scatterlist.h>
+#include <asm/cpu_device_id.h>
 #include <asm/i387.h>
 
 struct padlock_sha_desc {
@@ -526,6 +527,12 @@ static struct shash_alg sha256_alg_nano = {
 	}
 };
 
+static struct x86_cpu_id padlock_sha_ids[] = {
+	X86_FEATURE_MATCH(X86_FEATURE_PHE),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, padlock_sha_ids);
+
 static int __init padlock_init(void)
 {
 	int rc = -ENODEV;
@@ -533,15 +540,8 @@ static int __init padlock_init(void)
 	struct shash_alg *sha1;
 	struct shash_alg *sha256;
 
-	if (!cpu_has_phe) {
-		printk(KERN_NOTICE PFX "VIA PadLock Hash Engine not detected.\n");
-		return -ENODEV;
-	}
-
-	if (!cpu_has_phe_enabled) {
-		printk(KERN_NOTICE PFX "VIA PadLock detected, but not enabled. Hmm, strange...\n");
+	if (!x86_match_cpu(padlock_sha_ids) || !cpu_has_phe_enabled)
 		return -ENODEV;
-	}
 
 	/* Register the newly added algorithm module if on *
 	* VIA Nano processor, or else just do as before */
-- 
1.7.4.4

[PATCH 03/10] intel-idle: convert to x86_cpu_id auto probing

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

With this it should be automatically loaded on suitable systems by
udev.

The old switch () is replaced with a table based approach, this
also cleans up the code.

Cc: lenb@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/idle/intel_idle.c |  116 +++++++++++++++++++++++++-------------------
 1 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 5d2f8e1..a2f3619 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -62,6 +62,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
 
@@ -81,6 +82,17 @@ static unsigned int mwait_substates;
 /* Reliable LAPIC Timer States, bit 1 for C1 etc.  */
 static unsigned int lapic_timer_reliable_states = (1 << 1);	 /* Default to only C1 */
 
+struct idle_cpu {
+	struct cpuidle_state *state_table;
+
+	/*
+	 * Hardware C-state auto-demotion may not always be optimal.
+	 * Indicate which enable bits to clear here.
+	 */
+	unsigned long auto_demotion_disable_flags;
+};
+
+static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
@@ -88,12 +100,6 @@ static int intel_idle(struct cpuidle_device *dev,
 static struct cpuidle_state *cpuidle_state_table;
 
 /*
- * Hardware C-state auto-demotion may not always be optimal.
- * Indicate which enable bits to clear here.
- */
-static unsigned long long auto_demotion_disable_flags;
-
-/*
  * Set this flag for states where the HW flushes the TLB for us
  * and so we don't need cross-calls to keep it consistent.
  * If this flag is set, SW flushes the TLB, so even if the
@@ -320,27 +326,72 @@ static void auto_demotion_disable(void *dummy)
 	unsigned long long msr_bits;
 
 	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
-	msr_bits &= ~auto_demotion_disable_flags;
+	msr_bits &= ~(icpu->auto_demotion_disable_flags);
 	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
 }
 
+static const struct idle_cpu idle_cpu_nehalem = {
+	.state_table = nehalem_cstates,
+};
+
+static const struct idle_cpu idle_cpu_westmere = {
+	.state_table = nehalem_cstates,
+	.auto_demotion_disable_flags = NHM_C1_AUTO_DEMOTE | NHM_C3_AUTO_DEMOTE,
+};
+
+static const struct idle_cpu idle_cpu_atom = {
+	.state_table = atom_cstates,
+};
+
+static const struct idle_cpu idle_cpu_lincroft = {
+	.state_table = atom_cstates,
+	.auto_demotion_disable_flags = ATM_LNC_C6_AUTO_DEMOTE,
+};
+
+static const struct idle_cpu idle_cpu_snb = {
+	.state_table = snb_cstates,
+};
+
+#define ICPU(model, cpu) \
+	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT, (unsigned long)&cpu }
+
+static struct x86_cpu_id intel_idle_ids[] = {
+	ICPU(0x1a, idle_cpu_nehalem),
+	ICPU(0x1e, idle_cpu_nehalem),
+	ICPU(0x1f, idle_cpu_nehalem),
+	ICPU(0x25, idle_cpu_westmere),
+	ICPU(0x2c, idle_cpu_westmere),
+	ICPU(0x2f, idle_cpu_westmere),
+	ICPU(0x1c, idle_cpu_atom),
+	ICPU(0x26, idle_cpu_lincroft),
+	ICPU(0x2f, idle_cpu_westmere),
+	ICPU(0x2a, idle_cpu_snb),
+	ICPU(0x2d, idle_cpu_snb),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids);
+
 /*
  * intel_idle_probe()
  */
 static int intel_idle_probe(void)
 {
 	unsigned int eax, ebx, ecx;
+	struct x86_cpu_id *id;
 
 	if (max_cstate == 0) {
 		pr_debug(PREFIX "disabled\n");
 		return -EPERM;
 	}
 
-	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
-		return -ENODEV;
-
-	if (!boot_cpu_has(X86_FEATURE_MWAIT))
+	id = x86_match_cpu(intel_idle_ids);
+	if (!id) {
+		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+		    boot_cpu_data.x86 == 6)
+			pr_debug(PREFIX "does not run on family %d model %d\n",
+				boot_cpu_data.x86, boot_cpu_data.x86_model);
 		return -ENODEV;
+	}
 
 	if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF)
 		return -ENODEV;
@@ -353,44 +404,9 @@ static int intel_idle_probe(void)
 
 	pr_debug(PREFIX "MWAIT substates: 0x%x\n", mwait_substates);
 
-
-	if (boot_cpu_data.x86 != 6)	/* family 6 */
-		return -ENODEV;
-
-	switch (boot_cpu_data.x86_model) {
-
-	case 0x1A:	/* Core i7, Xeon 5500 series */
-	case 0x1E:	/* Core i7 and i5 Processor - Lynnfield Jasper Forest */
-	case 0x1F:	/* Core i7 and i5 Processor - Nehalem */
-	case 0x2E:	/* Nehalem-EX Xeon */
-	case 0x2F:	/* Westmere-EX Xeon */
-	case 0x25:	/* Westmere */
-	case 0x2C:	/* Westmere */
-		cpuidle_state_table = nehalem_cstates;
-		auto_demotion_disable_flags =
-			(NHM_C1_AUTO_DEMOTE | NHM_C3_AUTO_DEMOTE);
-		break;
-
-	case 0x1C:	/* 28 - Atom Processor */
-		cpuidle_state_table = atom_cstates;
-		break;
-
-	case 0x26:	/* 38 - Lincroft Atom Processor */
-		cpuidle_state_table = atom_cstates;
-		auto_demotion_disable_flags = ATM_LNC_C6_AUTO_DEMOTE;
-		break;
-
-	case 0x2A:	/* SNB */
-	case 0x2D:	/* SNB Xeon */
-		cpuidle_state_table = snb_cstates;
-		break;
-
-	default:
-		pr_debug(PREFIX "does not run on family %d model %d\n",
-			boot_cpu_data.x86, boot_cpu_data.x86_model);
-		return -ENODEV;
-	}
-
+	icpu = (const struct idle_cpu *)id->driver_data;
+	cpuidle_state_table = icpu->state_table;
+	
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
 	else {
@@ -470,7 +486,7 @@ static int intel_idle_cpuidle_driver_init(void)
 		drv->state_count += 1;
 	}
 
-	if (auto_demotion_disable_flags)
+	if (icpu->auto_demotion_disable_flags)
 		smp_call_function(auto_demotion_disable, NULL, 1);
 
 	return 0;
-- 
1.7.4.4

[PATCH 04/10] ACPI: Load acpi-cpufreq from processor driver automatically

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The only left over hole in automatic cpufreq driver loading was the loading
of ACPI cpufreq. This driver should be loaded when ACPI supports a _PDC
method and the CPU vendor wants to use acpi cpufreq.

Simply add a request module call to the acpi processor core driver
when this is true. This seems like the simplest solution for this.

Cc: lenb@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/acpi/processor_driver.c  |    1 +
 drivers/acpi/processor_perflib.c |   22 ++++++++++++++++++++++
 include/acpi/processor.h         |    1 +
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 9d7bc9f..9e933ed 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -499,6 +499,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 
 #ifdef CONFIG_CPU_FREQ
 	acpi_processor_ppc_has_changed(pr, 0);
+	acpi_processor_load_module(pr);
 #endif
 	acpi_processor_get_throttling_info(pr);
 	acpi_processor_get_limit_info(pr);
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 85b3237..a533512 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -240,6 +240,28 @@ void acpi_processor_ppc_exit(void)
 	acpi_processor_ppc_status &= ~PPC_REGISTERED;
 }
 
+/* 
+ * Do a quick check if the systems looks like it should use ACPI 
+ * cpufreq. We look at a _PCT method being available, but don't
+ * do a whole lot of sanity checks. 
+ */
+void acpi_processor_load_module(struct acpi_processor *pr)
+{
+	static int requested;
+	acpi_status status = 0;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	if (!arch_has_acpi_pdc() || requested)
+		return;
+	status = acpi_evaluate_object(pr->handle, "_PCT", NULL, &buffer);
+	if (!ACPI_FAILURE(status)) {
+		printk(KERN_INFO PREFIX "Requesting acpi_cpufreq\n");
+		request_module_nowait("acpi_cpufreq");
+		requested = 1;
+	}
+	kfree(buffer.pointer);
+}
+
 static int acpi_processor_get_performance_control(struct acpi_processor *pr)
 {
 	int result = 0;
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 610f6fb..da57fdc 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -224,6 +224,7 @@ struct acpi_processor_errata {
 	} piix4;
 };
 
+extern void acpi_processor_load_module(struct acpi_processor *pr);
 extern int acpi_processor_preregister_performance(struct
 						  acpi_processor_performance
 						  __percpu *performance);
-- 
1.7.4.4

[PATCH 05/10] HWMON: Convert via-cputemp to x86 cpuid autoprobing

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use the new x86 cpuid autoprobe interface.

Cc: khali@linux-fr.org
Cc: guenter.roeck@ericsson.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/hwmon/via-cputemp.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
index 8eac67d..36ddb83 100644
--- a/drivers/hwmon/via-cputemp.c
+++ b/drivers/hwmon/via-cputemp.c
@@ -37,6 +37,7 @@
 #include <linux/cpu.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
+#include <asm/cpu_device_id.h>
 
 #define DRVNAME	"via_cputemp"
 
@@ -308,34 +309,26 @@ static struct notifier_block via_cputemp_cpu_notifier __refdata = {
 	.notifier_call = via_cputemp_cpu_callback,
 };
 
+static struct x86_cpu_id cputemp_ids[] = {
+	{ X86_VENDOR_CENTAUR, 6, 0xa, }, /* C7a */
+	{ X86_VENDOR_CENTAUR, 6, 0xd, }, /* C7d */
+	{ X86_VENDOR_CENTAUR, 6, 0xf, }, /* Nano */
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, cputemp_ids);
+
 static int __init via_cputemp_init(void)
 {
 	int i, err;
 
-	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
-		printk(KERN_DEBUG DRVNAME ": Not a VIA CPU\n");
-		err = -ENODEV;
-		goto exit;
-	}
+	if (!x86_match_cpu(cputemp_ids))
+		return -ENODEV;
 
 	err = platform_driver_register(&via_cputemp_driver);
 	if (err)
 		goto exit;
 
 	for_each_online_cpu(i) {
-		struct cpuinfo_x86 *c = &cpu_data(i);
-
-		if (c->x86 != 6)
-			continue;
-
-		if (c->x86_model < 0x0a)
-			continue;
-
-		if (c->x86_model > 0x0f) {
-			pr_warn("Unknown CPU model 0x%x\n", c->x86_model);
-			continue;
-		}
-
 		via_cputemp_device_add(i);
 	}
 
-- 
1.7.4.4

[PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use the new x86 cpuid autoprobe interface for the Intel coretemp
driver.

Cc: fenghua.yu@intel.com
Cc: khali@linux-fr.org
Cc: guenter.roeck@ericsson.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/hwmon/coretemp.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 104b376..5de1579 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -39,6 +39,7 @@
 #include <linux/moduleparam.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
+#include <asm/cpu_device_id.h>
 
 #define DRVNAME	"coretemp"
 
@@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
 	.notifier_call = coretemp_cpu_callback,
 };
 
+static struct x86_cpu_id coretemp_ids[] = {
+	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTS },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
+
 static int __init coretemp_init(void)
 {
 	int i, err = -ENODEV;
 
-	/* quick check if we run Intel */
-	if (cpu_data(0).x86_vendor != X86_VENDOR_INTEL)
-		goto exit;
+	/*
+	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
+	 * sensors. We check this bit only, all the early CPUs
+	 * without thermal sensors will be filtered out.
+	 */
+	if (!x86_match_cpu(coretemp_ids))
+		return -ENODEV;
 
 	err = platform_driver_register(&coretemp_driver);
 	if (err)
-- 
1.7.4.4

[PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

This marks all the x86 cpuinfo tables to the CPU specific device drivers,
to allow auto loading by udev. This should simplify the distribution
startup scripts for this greatly.

I didn't add MODULE_DEVICE_IDs to the centrino and p4-clockmod drivers,
because those probably shouldn't be auto loaded and the acpi driver
be used instead (not fully sure on that, would appreciate feedback)

The old nforce drivers autoload based on the PCI ID.

ACPI cpufreq is autoloaded in another patch.

Cc: davej@redhat.com
Cc: trenn@suse.de
Cc: kay.sievers@vrfy.org
Cc: hpa@zytor.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/cpufreq/cpufreq-nforce2.c    |    8 ++++++++
 drivers/cpufreq/e_powersaver.c       |   20 +++++++++++---------
 drivers/cpufreq/elanfreq.c           |   14 +++++++-------
 drivers/cpufreq/gx-suspmod.c         |   15 ++++++++++-----
 drivers/cpufreq/longhaul.c           |    8 +++++++-
 drivers/cpufreq/longrun.c            |   13 ++++++++-----
 drivers/cpufreq/p4-clockmod.c        |   17 +++++++++++------
 drivers/cpufreq/powernow-k6.c        |   12 ++++++++----
 drivers/cpufreq/powernow-k7.c        |   14 ++++++++------
 drivers/cpufreq/powernow-k8.c        |   24 ++++++++++++++++++------
 drivers/cpufreq/sc520_freq.c         |   14 ++++++++------
 drivers/cpufreq/speedstep-centrino.c |   24 ++++++++++++++++++++----
 drivers/cpufreq/speedstep-ich.c      |   15 +++++++++++++++
 drivers/cpufreq/speedstep-lib.c      |    1 +
 drivers/cpufreq/speedstep-smi.c      |   15 +++++++++++++++
 15 files changed, 155 insertions(+), 59 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-nforce2.c b/drivers/cpufreq/cpufreq-nforce2.c
index 7bac808..661251b 100644
--- a/drivers/cpufreq/cpufreq-nforce2.c
+++ b/drivers/cpufreq/cpufreq-nforce2.c
@@ -385,6 +385,14 @@ static struct cpufreq_driver nforce2_driver = {
 	.owner = THIS_MODULE,
 };
 
+#ifdef MODULE
+static struct pci_device_id nforce2_ids[] = {
+	{ PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE2 },
+	{}
+};
+MODULE_DEVICE_TABLE(pci, nforce2_ids);
+#endif
+
 /**
  * nforce2_detect_chipset - detect the Southbridge which contains FSB PLL logic
  *
diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
index 4bd6815..4604951 100644
--- a/drivers/cpufreq/e_powersaver.c
+++ b/drivers/cpufreq/e_powersaver.c
@@ -16,6 +16,7 @@
 #include <linux/io.h>
 #include <linux/delay.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/msr.h>
 #include <asm/tsc.h>
 
@@ -437,18 +438,19 @@ static struct cpufreq_driver eps_driver = {
 	.attr		= eps_attr,
 };
 
+
+/* This driver will work only on Centaur C7 processors with
+ * Enhanced SpeedStep/PowerSaver registers */
+static struct x86_cpu_id eps_cpu_id[] = {
+	{ X86_VENDOR_CENTAUR, 6, X86_MODEL_ANY, X86_FEATURE_EST },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, eps_cpu_id);
+
 static int __init eps_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	/* This driver will work only on Centaur C7 processors with
-	 * Enhanced SpeedStep/PowerSaver registers */
-	if (c->x86_vendor != X86_VENDOR_CENTAUR
-	    || c->x86 != 6 || c->x86_model < 10)
-		return -ENODEV;
-	if (!cpu_has(c, X86_FEATURE_EST))
+	if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
 		return -ENODEV;
-
 	if (cpufreq_register_driver(&eps_driver))
 		return -EINVAL;
 	return 0;
diff --git a/drivers/cpufreq/elanfreq.c b/drivers/cpufreq/elanfreq.c
index c587db4..7a124b1 100644
--- a/drivers/cpufreq/elanfreq.c
+++ b/drivers/cpufreq/elanfreq.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/cpufreq.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/msr.h>
 #include <linux/timex.h>
 #include <linux/io.h>
@@ -277,17 +278,16 @@ static struct cpufreq_driver elanfreq_driver = {
 	.attr		= elanfreq_attr,
 };
 
+static struct x86_cpu_id elan_id[] = {
+	{ X86_VENDOR_AMD, 4, 10, },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, elan_id);
 
 static int __init elanfreq_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	/* Test if we have the right hardware */
-	if ((c->x86_vendor != X86_VENDOR_AMD) ||
-		(c->x86 != 4) || (c->x86_model != 10)) {
-		printk(KERN_INFO "elanfreq: error: no Elan processor found!\n");
+	if (!x86_match_cpu(elan_id))
 		return -ENODEV;
-	}
 	return cpufreq_register_driver(&elanfreq_driver);
 }
 
diff --git a/drivers/cpufreq/gx-suspmod.c b/drivers/cpufreq/gx-suspmod.c
index ffe1f2c..99ae003 100644
--- a/drivers/cpufreq/gx-suspmod.c
+++ b/drivers/cpufreq/gx-suspmod.c
@@ -82,6 +82,7 @@
 #include <linux/errno.h>
 #include <linux/slab.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/processor-cyrix.h>
 
 /* PCI config registers, all at F0 */
@@ -171,12 +172,20 @@ static struct pci_device_id gx_chipset_tbl[] __initdata = {
 	{ PCI_VDEVICE(CYRIX, PCI_DEVICE_ID_CYRIX_5510), },
 	{ 0, },
 };
+/* MODULE_DEVICE_TABLE here too? But the CPU should be enough. */
 
 static void gx_write_byte(int reg, int value)
 {
 	pci_write_config_byte(gx_params->cs55x0, reg, value);
 }
 
+static struct x86_cpu_id gx_ids[] = {
+	{ X86_VENDOR_NSC, },
+	{ X86_VENDOR_CYRIX },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, gx_ids);
+
 /**
  * gx_detect_chipset:
  *
@@ -185,12 +194,8 @@ static __init struct pci_dev *gx_detect_chipset(void)
 {
 	struct pci_dev *gx_pci = NULL;
 
-	/* check if CPU is a MediaGX or a Geode. */
-	if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) &&
-	    (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) {
-		pr_debug("error: no MediaGX/Geode processor found!\n");
+	if (!x86_match_cpu(gx_ids))
 		return NULL;
-	}
 
 	/* detect which companion chip is used */
 	for_each_pci_dev(gx_pci) {
diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
index f47d26e..b4263ce 100644
--- a/drivers/cpufreq/longhaul.c
+++ b/drivers/cpufreq/longhaul.c
@@ -35,6 +35,7 @@
 #include <linux/acpi.h>
 
 #include <asm/msr.h>
+#include <asm/cpu_device_id.h>
 #include <acpi/processor.h>
 
 #include "longhaul.h"
@@ -951,12 +952,17 @@ static struct cpufreq_driver longhaul_driver = {
 	.attr	= longhaul_attr,
 };
 
+static struct x86_cpu_id longhaul_id[] = {
+	{ X86_VENDOR_CENTAUR, 6 },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, longhaul_id);
 
 static int __init longhaul_init(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
 
-	if (c->x86_vendor != X86_VENDOR_CENTAUR || c->x86 != 6)
+	if (!x86_match_cpu(longhaul_id))
 		return -ENODEV;
 
 #ifdef CONFIG_SMP
diff --git a/drivers/cpufreq/longrun.c b/drivers/cpufreq/longrun.c
index 34ea359..a2579b6 100644
--- a/drivers/cpufreq/longrun.c
+++ b/drivers/cpufreq/longrun.c
@@ -14,6 +14,7 @@
 
 #include <asm/msr.h>
 #include <asm/processor.h>
+#include <asm/cpu_device_id.h>
 
 static struct cpufreq_driver	longrun_driver;
 
@@ -288,6 +289,12 @@ static struct cpufreq_driver longrun_driver = {
 	.owner		= THIS_MODULE,
 };
 
+static struct x86_cpu_id longrun_ids[] = {
+	{ X86_VENDOR_TRANSMETA, X86_FAMILY_ANY, X86_MODEL_ANY,
+	  X86_FEATURE_LONGRUN },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, longrun_ids);
 
 /**
  * longrun_init - initializes the Transmeta Crusoe LongRun CPUFreq driver
@@ -296,12 +303,8 @@ static struct cpufreq_driver longrun_driver = {
  */
 static int __init longrun_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	if (c->x86_vendor != X86_VENDOR_TRANSMETA ||
-	    !cpu_has(c, X86_FEATURE_LONGRUN))
+	if (!x86_match_cpu(longrun_ids))
 		return -ENODEV;
-
 	return cpufreq_register_driver(&longrun_driver);
 }
 
diff --git a/drivers/cpufreq/p4-clockmod.c b/drivers/cpufreq/p4-clockmod.c
index 6be3e07..f652f48 100644
--- a/drivers/cpufreq/p4-clockmod.c
+++ b/drivers/cpufreq/p4-clockmod.c
@@ -31,6 +31,7 @@
 #include <asm/processor.h>
 #include <asm/msr.h>
 #include <asm/timer.h>
+#include <asm/cpu_device_id.h>
 
 #include "speedstep-lib.h"
 
@@ -289,21 +290,25 @@ static struct cpufreq_driver p4clockmod_driver = {
 	.attr		= p4clockmod_attr,
 };
 
+static struct x86_cpu_id cpufreq_p4_id[] = {
+	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_ACC },
+	{}
+};
+
+/*
+ * Intentionally no MODULE_DEVICE_TABLE here: this driver should not
+ * be auto loaded.  Please don't add one.
+ */
 
 static int __init cpufreq_p4_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
 	int ret;
 
 	/*
 	 * THERM_CONTROL is architectural for IA32 now, so
 	 * we can rely on the capability checks
 	 */
-	if (c->x86_vendor != X86_VENDOR_INTEL)
-		return -ENODEV;
-
-	if (!test_cpu_cap(c, X86_FEATURE_ACPI) ||
-				!test_cpu_cap(c, X86_FEATURE_ACC))
+	if (!x86_match_cpu(cpufreq_p4_id) || !boot_cpu_has(X86_FEATURE_ACPI))
 		return -ENODEV;
 
 	ret = cpufreq_register_driver(&p4clockmod_driver);
diff --git a/drivers/cpufreq/powernow-k6.c b/drivers/cpufreq/powernow-k6.c
index b3379d6..6293ad1 100644
--- a/drivers/cpufreq/powernow-k6.c
+++ b/drivers/cpufreq/powernow-k6.c
@@ -16,6 +16,7 @@
 #include <linux/timex.h>
 #include <linux/io.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/msr.h>
 
 #define POWERNOW_IOPORT 0xfff0          /* it doesn't matter where, as long
@@ -210,6 +211,12 @@ static struct cpufreq_driver powernow_k6_driver = {
 	.attr		= powernow_k6_attr,
 };
 
+static struct x86_cpu_id powernow_k6_ids[] = {
+	{ X86_VENDOR_AMD, 5, 12 },
+	{ X86_VENDOR_AMD, 5, 13 },
+	{}
+};
+
 
 /**
  * powernow_k6_init - initializes the k6 PowerNow! CPUFreq driver
@@ -220,10 +227,7 @@ static struct cpufreq_driver powernow_k6_driver = {
  */
 static int __init powernow_k6_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	if ((c->x86_vendor != X86_VENDOR_AMD) || (c->x86 != 5) ||
-		((c->x86_model != 12) && (c->x86_model != 13)))
+	if (!x86_match_cpu(powernow_k6_ids))
 		return -ENODEV;
 
 	if (!request_region(POWERNOW_IOPORT, 16, "PowerNow!")) {
diff --git a/drivers/cpufreq/powernow-k7.c b/drivers/cpufreq/powernow-k7.c
index d71d9f3..6bc5e6f 100644
--- a/drivers/cpufreq/powernow-k7.c
+++ b/drivers/cpufreq/powernow-k7.c
@@ -28,6 +28,7 @@
 #include <asm/timer.h>		/* Needed for recalibrate_cpu_khz() */
 #include <asm/msr.h>
 #include <asm/system.h>
+#include <asm/cpu_device_id.h>
 
 #ifdef CONFIG_X86_POWERNOW_K7_ACPI
 #include <linux/acpi.h>
@@ -110,18 +111,19 @@ static int check_fsb(unsigned int fsbspeed)
 	return delta < 5;
 }
 
+static struct x86_cpu_id powernow_k7_cpuids[] = {
+	{ X86_VENDOR_AMD, 7, },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, powernow_k7_cpuids);
+
 static int check_powernow(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
 	unsigned int maxei, eax, ebx, ecx, edx;
 
-	if ((c->x86_vendor != X86_VENDOR_AMD) || (c->x86 != 6)) {
-#ifdef MODULE
-		printk(KERN_INFO PFX "This module only works with "
-				"AMD K7 CPUs\n");
-#endif
+	if (!x86_match_cpu(powernow_k7_cpuids))
 		return 0;
-	}
 
 	/* Get maximum capabilities */
 	maxei = cpuid_eax(0x80000000);
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index bce576d..069fa86 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -37,6 +37,7 @@
 #include <linux/delay.h>
 
 #include <asm/msr.h>
+#include <asm/cpu_device_id.h>
 
 #include <linux/acpi.h>
 #include <linux/mutex.h>
@@ -514,6 +515,20 @@ static int core_voltage_post_transition(struct powernow_k8_data *data,
 	return 0;
 }
 
+static struct x86_cpu_id powernow_k8_ids[] = {
+	{ X86_VENDOR_AMD, 0xf, },
+	/* RED-PEN If HW PSTATE was a normal feature bit it could be matched here
+	 * instead of a (limited) model list.
+	 */
+	{ X86_VENDOR_AMD, 0x10, },
+	{ X86_VENDOR_AMD, 0x11, },
+	{ X86_VENDOR_AMD, 0x12, },
+	{ X86_VENDOR_AMD, 0x13, },
+	{ X86_VENDOR_AMD, 0x14, },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, powernow_k8_ids);
+
 static void check_supported_cpu(void *_rc)
 {
 	u32 eax, ebx, ecx, edx;
@@ -521,13 +536,7 @@ static void check_supported_cpu(void *_rc)
 
 	*rc = -ENODEV;
 
-	if (__this_cpu_read(cpu_info.x86_vendor) != X86_VENDOR_AMD)
-		return;
-
 	eax = cpuid_eax(CPUID_PROCESSOR_SIGNATURE);
-	if (((eax & CPUID_XFAM) != CPUID_XFAM_K8) &&
-	    ((eax & CPUID_XFAM) < CPUID_XFAM_10H))
-		return;
 
 	if ((eax & CPUID_XFAM) == CPUID_XFAM_K8) {
 		if (((eax & CPUID_USE_XFAM_XMOD) != CPUID_USE_XFAM_XMOD) ||
@@ -1542,6 +1551,9 @@ static int __cpuinit powernowk8_init(void)
 	unsigned int i, supported_cpus = 0, cpu;
 	int rv;
 
+	if (!x86_match_cpu(powernow_k8_ids))
+		return -ENODEV;
+
 	for_each_online_cpu(i) {
 		int rc;
 		smp_call_function_single(i, check_supported_cpu, &rc, 1);
diff --git a/drivers/cpufreq/sc520_freq.c b/drivers/cpufreq/sc520_freq.c
index 1e205e6..3491578 100644
--- a/drivers/cpufreq/sc520_freq.c
+++ b/drivers/cpufreq/sc520_freq.c
@@ -22,6 +22,7 @@
 #include <linux/timex.h>
 #include <linux/io.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/msr.h>
 
 #define MMCR_BASE	0xfffef000	/* The default base address */
@@ -150,18 +151,19 @@ static struct cpufreq_driver sc520_freq_driver = {
 	.attr	= sc520_freq_attr,
 };
 
+static struct x86_cpu_id sc520_ids[] = {
+	{ X86_VENDOR_AMD, 4, 9 },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, sc520_ids);
 
 static int __init sc520_freq_init(void)
 {
-	struct cpuinfo_x86 *c = &cpu_data(0);
 	int err;
 
-	/* Test if we have the right hardware */
-	if (c->x86_vendor != X86_VENDOR_AMD ||
-	    c->x86 != 4 || c->x86_model != 9) {
-		pr_debug("no Elan SC520 processor found!\n");
+	if (!x86_match_cpu(sc520_ids))
 		return -ENODEV;
-	}
+
 	cpuctl = ioremap((unsigned long)(MMCR_BASE + OFFS_CPUCTL), 1);
 	if (!cpuctl) {
 		printk(KERN_ERR "sc520_freq: error: failed to remap memory\n");
diff --git a/drivers/cpufreq/speedstep-centrino.c b/drivers/cpufreq/speedstep-centrino.c
index 6ea3455..93213c0 100644
--- a/drivers/cpufreq/speedstep-centrino.c
+++ b/drivers/cpufreq/speedstep-centrino.c
@@ -25,6 +25,7 @@
 #include <asm/msr.h>
 #include <asm/processor.h>
 #include <asm/cpufeature.h>
+#include <asm/cpu_device_id.h>
 
 #define PFX		"speedstep-centrino: "
 #define MAINTAINER	"cpufreq@vger.kernel.org"
@@ -595,6 +596,24 @@ static struct cpufreq_driver centrino_driver = {
 	.owner		= THIS_MODULE,
 };
 
+/*
+ * This doesn't replace the detailed checks above because
+ * the generic CPU IDs don't have a way to match for steppings
+ * or ASCII model IDs.
+ */
+static struct x86_cpu_id centrino_ids[] = {
+	{ X86_VENDOR_INTEL, 6, 9, X86_FEATURE_EST },
+	{ X86_VENDOR_INTEL, 6, 13, X86_FEATURE_EST },
+	{ X86_VENDOR_INTEL, 6, 13, X86_FEATURE_EST },
+	{ X86_VENDOR_INTEL, 6, 13, X86_FEATURE_EST },
+	{ X86_VENDOR_INTEL, 15, 3, X86_FEATURE_EST },
+	{ X86_VENDOR_INTEL, 15, 4, X86_FEATURE_EST },
+	{}
+};
+#if 0
+/* Autoload or not? Do not for now. */
+MODULE_DEVICE_TABLE(x86cpu, centrino_ids);
+#endif
 
 /**
  * centrino_init - initializes the Enhanced SpeedStep CPUFreq driver
@@ -612,11 +631,8 @@ static struct cpufreq_driver centrino_driver = {
  */
 static int __init centrino_init(void)
 {
-	struct cpuinfo_x86 *cpu = &cpu_data(0);
-
-	if (!cpu_has(cpu, X86_FEATURE_EST))
+	if (!x86_match_cpu(centrino_ids))
 		return -ENODEV;
-
 	return cpufreq_register_driver(&centrino_driver);
 }
 
diff --git a/drivers/cpufreq/speedstep-ich.c b/drivers/cpufreq/speedstep-ich.c
index a748ce7..acd5578 100644
--- a/drivers/cpufreq/speedstep-ich.c
+++ b/drivers/cpufreq/speedstep-ich.c
@@ -25,6 +25,8 @@
 #include <linux/pci.h>
 #include <linux/sched.h>
 
+#include <asm/cpu_device_id.h>
+
 #include "speedstep-lib.h"
 
 
@@ -388,6 +390,16 @@ static struct cpufreq_driver speedstep_driver = {
 	.attr	= speedstep_attr,
 };
 
+static struct x86_cpu_id ss_smi_ids[] = {
+	{ X86_VENDOR_INTEL, 6, 0xb, },
+	{ X86_VENDOR_INTEL, 6, 0x8, },
+	{ X86_VENDOR_INTEL, 15, 2 },
+	{}
+};
+#if 0
+/* Autoload or not? Do not for now. */
+MODULE_DEVICE_TABLE(x86cpu, ss_smi_ids);
+#endif
 
 /**
  * speedstep_init - initializes the SpeedStep CPUFreq driver
@@ -398,6 +410,9 @@ static struct cpufreq_driver speedstep_driver = {
  */
 static int __init speedstep_init(void)
 {
+	if (!x86_match_cpu(ss_smi_ids))
+		return -ENODEV;
+
 	/* detect processor */
 	speedstep_processor = speedstep_detect_processor();
 	if (!speedstep_processor) {
diff --git a/drivers/cpufreq/speedstep-lib.c b/drivers/cpufreq/speedstep-lib.c
index 8af2d2f..7047821 100644
--- a/drivers/cpufreq/speedstep-lib.c
+++ b/drivers/cpufreq/speedstep-lib.c
@@ -249,6 +249,7 @@ EXPORT_SYMBOL_GPL(speedstep_get_frequency);
  *                 DETECT SPEEDSTEP-CAPABLE PROCESSOR                *
  *********************************************************************/
 
+/* Keep in sync with the x86_cpu_id tables in the different modules */
 unsigned int speedstep_detect_processor(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
diff --git a/drivers/cpufreq/speedstep-smi.c b/drivers/cpufreq/speedstep-smi.c
index c76ead3..8cb62df 100644
--- a/drivers/cpufreq/speedstep-smi.c
+++ b/drivers/cpufreq/speedstep-smi.c
@@ -20,6 +20,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <asm/ist.h>
+#include <asm/cpu_device_id.h>
 
 #include "speedstep-lib.h"
 
@@ -379,6 +380,17 @@ static struct cpufreq_driver speedstep_driver = {
 	.attr		= speedstep_attr,
 };
 
+static struct x86_cpu_id ss_smi_ids[] = {
+	{ X86_VENDOR_INTEL, 6, 0xb, },
+	{ X86_VENDOR_INTEL, 6, 0x8, },
+	{ X86_VENDOR_INTEL, 15, 2 },
+	{}
+};
+#if 0
+/* Not auto loaded currently */
+MODULE_DEVICE_TABLE(x86cpu, ss_smi_ids);
+#endif
+
 /**
  * speedstep_init - initializes the SpeedStep CPUFreq driver
  *
@@ -388,6 +400,9 @@ static struct cpufreq_driver speedstep_driver = {
  */
 static int __init speedstep_init(void)
 {
+	if (!x86_match_cpu(ss_smi_ids))
+		return -ENODEV;
+
 	speedstep_processor = speedstep_detect_processor();
 
 	switch (speedstep_processor) {
-- 
1.7.4.4

[PATCH 08/10] x86: autoload microcode driver on Intel and AMD systems

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Don't try to describe the actual models for now.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/microcode_core.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 9d46f5e..51016ad 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -86,6 +86,7 @@
 
 #include <asm/microcode.h>
 #include <asm/processor.h>
+#include <asm/cpu_device_id.h>
 
 MODULE_DESCRIPTION("Microcode Update Driver");
 MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
@@ -502,6 +503,20 @@ static struct notifier_block __refdata mc_cpu_notifier = {
 	.notifier_call	= mc_cpu_callback,
 };
 
+#ifdef MODULE
+/* Autoload on Intel and AMD systems */
+static struct x86_cpu_id microcode_id[] = {
+#ifdef CONFIG_MICROCODE_INTEL
+       { X86_VENDOR_INTEL, X86_VENDOR_ANY, X86_MODEL_ANY, },
+#endif
+#ifdef CONFIG_MICROCODE_AMD
+       { X86_VENDOR_AMD, X86_VENDOR_ANY, X86_MODEL_ANY, },
+#endif
+       {}
+};
+MODULE_DEVICE_TABLE(x86cpu, microcode_id);
+#endif
+
 static int __init microcode_init(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
-- 
1.7.4.4

[PATCH 09/10] x86: Add a test module for cpu loading

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Not for merging, but useful for testing.

Not-Signed-off: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/Makefile        |    2 ++
 arch/x86/kernel/cpu/test-cpu-load.c |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/test-cpu-load.c

diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 6ab6aa2..1af7e7b 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -48,3 +48,5 @@ cpufeature = $(src)/../../include/asm/cpufeature.h
 targets += capflags.c
 $(obj)/capflags.c: $(cpufeature) $(src)/mkcapflags.pl FORCE
 	$(call if_changed,mkcapflags)
+
+obj-m					+= test-cpu-load.o
diff --git a/arch/x86/kernel/cpu/test-cpu-load.c b/arch/x86/kernel/cpu/test-cpu-load.c
new file mode 100644
index 0000000..497a764
--- /dev/null
+++ b/arch/x86/kernel/cpu/test-cpu-load.c
@@ -0,0 +1,19 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <asm/cpu_device_id.h>
+
+static struct x86_cpu_id microcode_id[] = {
+        { X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, microcode_id);
+
+static int start(void)
+{
+	printk("test-cpu-load loaded\n");
+	return 0;
+}
+
+module_init(start);
+
+
-- 
1.7.4.4

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Dave Jones]

On Wed, Dec 07, 2011 at 04:41:20PM -0800, Andi Kleen wrote:
 
I think this bit isn't right..

 > diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
 > index 4bd6815..4604951 100644
 > --- a/drivers/cpufreq/e_powersaver.c
 > +++ b/drivers/cpufreq/e_powersaver.c
 
 > +static struct x86_cpu_id eps_cpu_id[] = {
 > +	{ X86_VENDOR_CENTAUR, 6, X86_MODEL_ANY, X86_FEATURE_EST },
 > +	{}
 > +};
 ...

 > +MODULE_DEVICE_TABLE(x86cpu, eps_cpu_id);
 > +
 >  static int __init eps_init(void)
 >  {
 > -	struct cpuinfo_x86 *c = &cpu_data(0);
 > -
 > -	/* This driver will work only on Centaur C7 processors with
 > -	 * Enhanced SpeedStep/PowerSaver registers */
 > -	if (c->x86_vendor != X86_VENDOR_CENTAUR
 > -	    || c->x86 != 6 || c->x86_model < 10)
 > -		return -ENODEV;

This makes e_powersaver bind to every family 6 VIA cpu.
But the old logic only bound to certain models.
Won't this will clash with this other driver if both are built ?

 > diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
 > index f47d26e..b4263ce 100644
 > --- a/drivers/cpufreq/longhaul.c
 > +++ b/drivers/cpufreq/longhaul.c
 > 
 > +static struct x86_cpu_id longhaul_id[] = {
 > +	{ X86_VENDOR_CENTAUR, 6 },
 > +	{}
 > +};
 > +MODULE_DEVICE_TABLE(x86cpu, longhaul_id);

iirc, the intention here was longhaul on cpus that don't have EST,
and e_powersaver on those that do. Maybe an additional check for the
absense of EST in longhaul's init code would do the trick.
(sidenote: I don't recall why we even have e-powersaver, instead of them
 just using acpi-cpufreq).

 > +static struct x86_cpu_id powernow_k8_ids[] = {
 > +	{ X86_VENDOR_AMD, 0xf, },
 > +	/* RED-PEN If HW PSTATE was a normal feature bit it could be matched here
 > +	 * instead of a (limited) model list.
 > +	 */

You mean make fake one in the identify code ? Do-able I guess.
Though that would just be moving this list from this driver to a different place,
and as this is the only place that cares..

Other stuff looks ok on a quick eyeball. I wouldn't be surprised though if
this series throws some surprises, especially on some of the older hardware.

Seems like a step in the right direction though.

	Dave

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Andi Kleen]

On Wed, Dec 07, 2011 at 08:07:31PM -0500, Dave Jones wrote:
>  > +MODULE_DEVICE_TABLE(x86cpu, eps_cpu_id);
>  > +
>  >  static int __init eps_init(void)
>  >  {
>  > -	struct cpuinfo_x86 *c = &cpu_data(0);
>  > -
>  > -	/* This driver will work only on Centaur C7 processors with
>  > -	 * Enhanced SpeedStep/PowerSaver registers */
>  > -	if (c->x86_vendor != X86_VENDOR_CENTAUR
>  > -	    || c->x86 != 6 || c->x86_model < 10)
>  > -		return -ENODEV;
> 
> This makes e_powersaver bind to every family 6 VIA cpu.
> But the old logic only bound to certain models.
> Won't this will clash with this other driver if both are built ?

The code does

static int __init eps_init(void)
{
        if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
                return -ENODEV;

So modprobe will load it, but if the CPU is too old it will just error out
again. I think that's reasonable.  There's no direct way current
to express a >= in the matches because modprobe uses fnmatch()

Also most likely the old CPUs won't have the EST bit anyways, then
it won't even be loaded.

> 
>  > diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
>  > index f47d26e..b4263ce 100644
>  > --- a/drivers/cpufreq/longhaul.c
>  > +++ b/drivers/cpufreq/longhaul.c
>  > 
>  > +static struct x86_cpu_id longhaul_id[] = {
>  > +	{ X86_VENDOR_CENTAUR, 6 },
>  > +	{}
>  > +};
>  > +MODULE_DEVICE_TABLE(x86cpu, longhaul_id);
> 
> iirc, the intention here was longhaul on cpus that don't have EST,
> and e_powersaver on those that do. Maybe an additional check for the
> absense of EST in longhaul's init code would do the trick.
> (sidenote: I don't recall why we even have e-powersaver, instead of them
>  just using acpi-cpufreq).

It's not done today, but I could add it. 

But I tried to keep the existing behaviour. AFAIK distros just load them
all right? This matches this. I have no way to test these CPUs so I would 
prefer to be as compatible as possible.
> 
>  > +static struct x86_cpu_id powernow_k8_ids[] = {
>  > +	{ X86_VENDOR_AMD, 0xf, },
>  > +	/* RED-PEN If HW PSTATE was a normal feature bit it could be matched here
>  > +	 * instead of a (limited) model list.
>  > +	 */
> 
> You mean make fake one in the identify code ? Do-able I guess.
> Though that would just be moving this list from this driver to a different place,
> and as this is the only place that cares..

Ok. 

Thanks for the review. I'll remove the comment.

-Andi

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Dave Jones]

On Thu, Dec 08, 2011 at 02:13:56AM +0100, Andi Kleen wrote:

 > > This makes e_powersaver bind to every family 6 VIA cpu.
 > > But the old logic only bound to certain models.
 > > Won't this will clash with this other driver if both are built ?
 > 
 > The code does
 > 
 > static int __init eps_init(void)
 > {
 >         if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
 >                 return -ENODEV;
 > 
 > So modprobe will load it, but if the CPU is too old it will just error out
 > again. I think that's reasonable.  There's no direct way current
 > to express a >= in the matches because modprobe uses fnmatch()
 > 
 > Also most likely the old CPUs won't have the EST bit anyways, then
 > it won't even be loaded.

I don't have any VIA CPUs to check any more, and my memory is a little vague,
but I think you're right on this assumption.

 > > iirc, the intention here was longhaul on cpus that don't have EST,
 > > and e_powersaver on those that do. Maybe an additional check for the
 > > absense of EST in longhaul's init code would do the trick.
 > > (sidenote: I don't recall why we even have e-powersaver, instead of them
 > >  just using acpi-cpufreq).
 > 
 > It's not done today, but I could add it. 
 > 
 > But I tried to keep the existing behaviour. 
 > This matches this. I have no way to test these CPUs so I would 
 > prefer to be as compatible as possible.

That's fine. Don't sweat it for this, it's something that could be done later
if someone cares enough. We might have a hard time finding someone even
still using this driver.

 > AFAIK distros just load them all right?

In Fedora, we used to. In F16, we changed things so instead of a monstrous
init script with nested if's and a whole bunch of hairy logic, we changed
all the modules to be built-in's, and relied on the link-order in the cpufreq Makefile
to satisfy the 'first to bind wins'.

Yes, ugly, but we didn't have this patchset ;-) 

	Dave

Re: [PATCH 01/10] Add driver auto probing for x86 features

[H. Peter Anvin]

On 12/07/2011 04:41 PM, Andi Kleen wrote:
>  
> +/* LOOKS like x86cpu:vendor:VVVV:family:FFFF:model:MMMM:feature:*,FEAT,*
> + * All fields are numbers. It would be nicer to use strings for vendor
> + * and feature, but getting those out of the build system here is too
> + * complicated.
> + */

Hmm... how hard can that be, really?  Let me look at it a bit...

	-hpa

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Guenter Roeck]

Andi,

On Wed, Dec 07, 2011 at 07:41:19PM -0500, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Use the new x86 cpuid autoprobe interface for the Intel coretemp
> driver.
> 
> Cc: fenghua.yu@intel.com
> Cc: khali@linux-fr.org
> Cc: guenter.roeck@ericsson.com
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/hwmon/coretemp.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 104b376..5de1579 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -39,6 +39,7 @@
>  #include <linux/moduleparam.h>
>  #include <asm/msr.h>
>  #include <asm/processor.h>
> +#include <asm/cpu_device_id.h>
>  
>  #define DRVNAME	"coretemp"
>  
> @@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
>  	.notifier_call = coretemp_cpu_callback,
>  };
>  
> +static struct x86_cpu_id coretemp_ids[] = {
> +	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTS },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
> +
>  static int __init coretemp_init(void)
>  {
>  	int i, err = -ENODEV;
>  
> -	/* quick check if we run Intel */
> -	if (cpu_data(0).x86_vendor != X86_VENDOR_INTEL)
> -		goto exit;
> +	/*
> +	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> +	 * sensors. We check this bit only, all the early CPUs
> +	 * without thermal sensors will be filtered out.
> +	 */
> +	if (!x86_match_cpu(coretemp_ids))
> +		return -ENODEV;
>  

X86_FEATURE_DTS is checked elsewhere in the driver, in function get_core_online().
Can that check be removed ?

Guenter

>  	err = platform_driver_register(&coretemp_driver);
>  	if (err)
> -- 
> 1.7.4.4
> 

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Andi Kleen]

On Wed, Dec 07, 2011 at 08:24:29PM -0500, Dave Jones wrote:
>  > AFAIK distros just load them all right?
> 
> In Fedora, we used to. In F16, we changed things so instead of a monstrous
> init script with nested if's and a whole bunch of hairy logic, we changed
> all the modules to be built-in's, and relied on the link-order in the cpufreq Makefile
> to satisfy the 'first to bind wins'.

This should continue working -- udev/modprobe will load them in link order

I'll keep it like it is now.

> Yes, ugly, but we didn't have this patchset ;-) 

I should have posted it earlier.

Just don't forget to change them back to =m once it goes in.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Jean Delvare]

Hi Andi,

Thanks a lot for working on this. I wanted to give it a try long ago but
could never find the time.

On Wed, 7 Dec 2011 18:40:01 -0800, Guenter Roeck wrote:
> Andi,
> 
> On Wed, Dec 07, 2011 at 07:41:19PM -0500, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Use the new x86 cpuid autoprobe interface for the Intel coretemp
> > driver.
> > 
> > Cc: fenghua.yu@intel.com
> > Cc: khali@linux-fr.org
> > Cc: guenter.roeck@ericsson.com
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >  drivers/hwmon/coretemp.c |   17 ++++++++++++++---
> >  1 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > index 104b376..5de1579 100644
> > --- a/drivers/hwmon/coretemp.c
> > +++ b/drivers/hwmon/coretemp.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/moduleparam.h>
> >  #include <asm/msr.h>
> >  #include <asm/processor.h>
> > +#include <asm/cpu_device_id.h>
> >  
> >  #define DRVNAME	"coretemp"
> >  
> > @@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
> >  	.notifier_call = coretemp_cpu_callback,
> >  };
> >  
> > +static struct x86_cpu_id coretemp_ids[] = {

Can't this be made const?

> > +	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTS },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
> > +
> >  static int __init coretemp_init(void)
> >  {
> >  	int i, err = -ENODEV;
> >  
> > -	/* quick check if we run Intel */
> > -	if (cpu_data(0).x86_vendor != X86_VENDOR_INTEL)
> > -		goto exit;
> > +	/*
> > +	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> > +	 * sensors. We check this bit only, all the early CPUs
> > +	 * without thermal sensors will be filtered out.
> > +	 */
> > +	if (!x86_match_cpu(coretemp_ids))
> > +		return -ENODEV;
>
> X86_FEATURE_DTS is checked elsewhere in the driver, in function get_core_online().
> Can that check be removed ?

Is it not possible in theory to mix a CPU with DTS and one without DTS
on the same system? There may be a reason why the DTS flag was checked
in get_core_online() (i.e. on a per-core basis) rather than in
coretemp_init(). I seem to recall that someone explicitly asked for it.

To avoid any risk of regression, I wouldn't touch this part of the code.

-- 
Jean Delvare

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Borislav Petkov]

On Wed, Dec 07, 2011 at 04:41:20PM -0800, Andi Kleen wrote:
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index bce576d..069fa86 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -37,6 +37,7 @@
>  #include <linux/delay.h>
>  
>  #include <asm/msr.h>
> +#include <asm/cpu_device_id.h>
>  
>  #include <linux/acpi.h>
>  #include <linux/mutex.h>
> @@ -514,6 +515,20 @@ static int core_voltage_post_transition(struct powernow_k8_data *data,
>  	return 0;
>  }
>  
> +static struct x86_cpu_id powernow_k8_ids[] = {
> +	{ X86_VENDOR_AMD, 0xf, },
> +	/* RED-PEN If HW PSTATE was a normal feature bit it could be matched here
> +	 * instead of a (limited) model list.
> +	 */
> +	{ X86_VENDOR_AMD, 0x10, },
> +	{ X86_VENDOR_AMD, 0x11, },
> +	{ X86_VENDOR_AMD, 0x12, },
> +	{ X86_VENDOR_AMD, 0x13, },
> +	{ X86_VENDOR_AMD, 0x14, },
> +	{}

Well, there is CPUID_8000_0007_EDX[7] which denotes the presence
of the P-state registers. It probably could be added to
init_scattered_cpuid_features() but there's still special handling
needed for K8.

Can you do both a family and a feature bit test with the autoprobing
code?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 01/10] Add driver auto probing for x86 features

[Jean Delvare]

Hi Andi,

On Wed,  7 Dec 2011 16:41:14 -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Old patchkit, resurrect due to popular demand.

Oh yes!

> There's a growing number of drivers that support a specific x86 feature
> or CPU.  Currently loading these drivers currently on a generic
> distribution requires various driver specific hacks and it often
> doesn't work.
> 
> This patch adds auto probing for drivers based on the x86 cpuid
> information, in particular based on vendor/family/model number
> and also based on CPUID feature bits.
> 
> For example a common issue is not loading the SSE 4.2 accelerated
> CRC module: this can significantly lower the performance of BTRFS
> which relies on fast CRC.
> 
> Another issue is loading the right CPUFREQ driver for the current CPU.
> Currently distributions often try all all possible driver until
> one sticks, which is not really a good way to do this.
> 
> It works with existing udev without any changes. The code
> exports the x86 information as a generic string in sysfs
> that can be matched by udev's pattern matching.
> 
> This scheme does not support numeric ranges, so if you want to
> handle e.g. ranges of model numbers they have to be encoded
> in ASCII or simply all models or families listed. Fixing
> that would require changing udev.
> 
> Another issue is that udev will happily load all drivers that match,
> there is currently no nice way to stop a specific driver from
> being loaded if it's not needed (e.g. if you don't need fast CRC)
> But there are not that many cpu specific drivers around and they're
> all not that bloated, so this isn't a particularly serious issue.
> 
> Originally this patch added the modalias to the normal cpu
> sysdevs. However sysdevs don't have all the infrastructure
> needed for udev, so it couldn't really autoload drivers.
> This patch instead adds the CPU modaliases to the cpuid devices,
> which are real devices with full support for udev. This implies
> that the cpuid driver has to be loaded to use this.
> 
> This patch just adds infrastructure, some driver conversions
> in followups.
> 
> Thanks to Kay for helping with some sysfs magic.
> (...)

Partial review:

> diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
> new file mode 100644
> index 0000000..2c23457
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/match.c
> @@ -0,0 +1,48 @@
> +#include <asm/cpu_device_id.h>
> +#include <asm/processor.h>
> +#include <linux/cpu.h>
> +#include <linux/module.h>
> +
> +/**
> + * x86_match_cpu - match current CPU again an array of x86_cpu_ids

The code is actually matching the boot cpu, which isn't necessarily the
current CPU. I think it would be better to let the caller pass a
specific cpu as a parameter. At least the hwmon drivers would benefit
from that. Then you can add an helper function x86_match_boot_cpu()
calling x86_match_cpu() on &boot_cpu_data if you want.

> + * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
> + *         X86_MODEL_END.

I see no such X86_MODEL_END, your loop below is instead treating entries
with all fields set to 0 as the terminating entry (which seems
reasonable.)

> + *
> + * Return the entry if the current CPU matches the entries in the
> + * passed x86_cpu_id match table. Otherwise NULL.  The match table
> + * contains vendor (X86_VENDOR_*), family, model and feature bits or
> + * respective wildcard entries.
> + *
> + * A typical table entry would be to match a specific CPU
> + * { X86_VENDOR_INTEL, 6, 0x12 }
> + * or to match a specific CPU feature
> + * { X86_FEATURE_MATCH(X86_FEATURE_FOOBAR) }
> + *
> + * Felds can be wildcarded with %X86_VENDOR_ANY, %X86_FAMILY_ANY,

Spelling: Fields.

> + * %X86_MODEL_ANY, %X86_FEATURE_ANY.
> + *
> + * Arrays used to match for this should also be declared using 
> + * MODULE_DEVICE_TABLE(x86_cpu, ...)
> + *
> + * This always matches against the boot cpu, assuming models and features are 
> + * consistent over all CPUs.

Not necessarily a good assumption, as stated above.

> + */
> +struct x86_cpu_id *x86_match_cpu(struct x86_cpu_id *match)

match can be a const pointer. This would allow drivers to declare their
cpu tables as const too.

> +{
> +	struct x86_cpu_id *m;
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
> +
> +	for (m = match; m->vendor | m->family | m->model | m->feature; m++) {
> +		if (m->vendor != X86_VENDOR_ANY && c->x86_vendor != m->vendor)
> +			continue;
> +		if (m->family != X86_FAMILY_ANY && c->x86 != m->family)
> +			continue;
> +		if (m->model != X86_MODEL_ANY && c->x86_model != m->model)
> +			continue;
> +		if (m->feature != X86_FEATURE_ANY && !cpu_has(c, m->feature))
> +			continue;
> +		return m;
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL(x86_match_cpu);
> diff --git a/arch/x86/kernel/cpuid.c b/arch/x86/kernel/cpuid.c
> index 212a6a4..6bb24d3 100644
> --- a/arch/x86/kernel/cpuid.c
> +++ b/arch/x86/kernel/cpuid.c
> @@ -40,6 +40,7 @@
>  #include <linux/notifier.h>
>  #include <linux/uaccess.h>
>  #include <linux/gfp.h>
> +#include <linux/slab.h>
>  
>  #include <asm/processor.h>
>  #include <asm/msr.h>
> @@ -138,13 +139,56 @@ static const struct file_operations cpuid_fops = {
>  	.open = cpuid_open,
>  };
>  
> +static ssize_t print_cpu_modalias(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *bufptr)
> +{
> +	int size = PAGE_SIZE;
> +	int i, n;
> +	char *buf = bufptr;
> +
> +	n = snprintf(buf, size, "x86cpu:vendor:%04x:family:%04x:model:%04x:feature:",
> +		boot_cpu_data.x86_vendor,
> +		boot_cpu_data.x86,
> +		boot_cpu_data.x86_model);
> +	size -= n;
> +	buf += n;
> +	size -= 2;

You might as well initialize size to PAGE_SIZE - 2 directly.

> +	for (i = 0; i < NCAPINTS*32; i++) {
> +		if (boot_cpu_has(i)) {
> +			n = snprintf(buf, size, ",%04x", i);
> +			if (n < 0) {
> +				WARN(1, "x86 features overflow page\n");
> +				break;
> +			}
> +			size -= n;
> +			buf += n;
> +		}
> +	}
> +	*buf++ = ',';
> +	*buf++ = '\n';
> +	return buf - bufptr;
> +}
> +
> +static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL);
> +
>  static __cpuinit int cpuid_device_create(int cpu)
>  {
>  	struct device *dev;
> +	int err;
>  
>  	dev = device_create(cpuid_class, NULL, MKDEV(CPUID_MAJOR, cpu), NULL,
>  			    "cpu%d", cpu);
> -	return IS_ERR(dev) ? PTR_ERR(dev) : 0;
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	err = device_create_file(dev, &dev_attr_modalias);
> +	if (err) {
> +		/* keep device around on error. attribute is optional. */
> +		return err;
> +	}
> +
> +	return 0;
>  }
>  
>  static void cpuid_device_destroy(int cpu)
> @@ -182,6 +226,17 @@ static char *cpuid_devnode(struct device *dev, mode_t *mode)
>  	return kasprintf(GFP_KERNEL, "cpu/%u/cpuid", MINOR(dev->devt));
>  }
>  
> +static int cpuid_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (buf) {
> +		print_cpu_modalias(NULL, NULL, buf);
> +		add_uevent_var(env, "MODALIAS=%s", buf);
> +		kfree(buf);
> +	}
> +	return 0;
> +}
> +
>  static int __init cpuid_init(void)
>  {
>  	int i, err = 0;
> @@ -200,6 +255,7 @@ static int __init cpuid_init(void)
>  		goto out_chrdev;
>  	}
>  	cpuid_class->devnode = cpuid_devnode;
> +	cpuid_class->dev_uevent = cpuid_dev_uevent;
>  	for_each_online_cpu(i) {
>  		err = cpuid_device_create(i);
>  		if (err != 0)
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 468819c..8971fec 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -542,4 +542,24 @@ struct isapnp_device_id {
>  	kernel_ulong_t driver_data;	/* data private to the driver */
>  };
>  
> +/*
> + * Match x86 CPUs for CPU specific drivers.
> + * See documentation of "x86_match_cpu" for details.
> + */
> +
> +struct x86_cpu_id {
> +	__u16 vendor;
> +	__u16 family;
> +	__u16 model;
> + 	__u16 feature;	/* bit index */
> +	kernel_ulong_t driver_data;
> +};
> +
> +#define X86_FEATURE_MATCH(x) { X86_VENDOR_ANY, X86_FAMILY_ANY, X86_MODEL_ANY, x }
> +
> +#define X86_VENDOR_ANY 0xffff
> +#define X86_FAMILY_ANY 0
> +#define X86_MODEL_ANY  0

Are you sure family 0 or model 0 are never used, by any vendor? I
wouldn't take the risk. What's wrong with 0xffff?

> +#define X86_FEATURE_ANY 0	/* Same as FPU, you can't test for that */

Might be better to set X86_FEATURE_ANY to either 10 (unused feature
bit) or 0xffff then.

> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index f936d1f..e5dd089 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -880,6 +880,29 @@ static int do_isapnp_entry(const char *filename,
>  	return 1;
>  }
>  
> +/* LOOKS like x86cpu:vendor:VVVV:family:FFFF:model:MMMM:feature:*,FEAT,*
> + * All fields are numbers. It would be nicer to use strings for vendor
> + * and feature, but getting those out of the build system here is too
> + * complicated.
> + */
> +
> +static int do_x86cpu_entry(const char *filename, struct x86_cpu_id *id, 
> +			   char *alias)
> +{
> +	id->feature = TO_NATIVE(id->feature);
> +	id->family = TO_NATIVE(id->family);
> +	id->model = TO_NATIVE(id->model);
> +	id->vendor = TO_NATIVE(id->vendor);
> +
> +	strcpy(alias, "x86cpu:");
> +	ADD(alias, "vendor:",  id->vendor != X86_VENDOR_ANY, id->vendor);
> +	ADD(alias, ":family:", id->family != X86_FAMILY_ANY, id->family);
> +	ADD(alias, ":model:",  id->model  != X86_MODEL_ANY,  id->model);
> +	ADD(alias, ":feature:*,", id->feature != X86_FEATURE_ANY, id->feature);
> +	strcat(alias, ",*");
> +	return 1;
> +}
> +
>  /* Ignore any prefix, eg. some architectures prepend _ */
>  static inline int sym_is(const char *symbol, const char *name)
>  {
> @@ -1047,6 +1070,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
>  		do_table(symval, sym->st_size,
>  			sizeof(struct isapnp_device_id), "isa",
>  			do_isapnp_entry, mod);
> +	else if (sym_is(symname, "__mod_x86cpu_device_table"))
> +		do_table(symval, sym->st_size,
> +			 sizeof(struct x86_cpu_id), "x86cpu",
> +			 do_x86cpu_entry, mod);
>  	free(zeros);
>  }
>  


-- 
Jean Delvare

Re: [PATCH 05/10] HWMON: Convert via-cputemp to x86 cpuid autoprobing

[Jean Delvare]

On Wed,  7 Dec 2011 16:41:18 -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Use the new x86 cpuid autoprobe interface.
> 
> Cc: khali@linux-fr.org
> Cc: guenter.roeck@ericsson.com
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/hwmon/via-cputemp.c |   29 +++++++++++------------------
>  1 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
> index 8eac67d..36ddb83 100644
> --- a/drivers/hwmon/via-cputemp.c
> +++ b/drivers/hwmon/via-cputemp.c
> @@ -37,6 +37,7 @@
>  #include <linux/cpu.h>
>  #include <asm/msr.h>
>  #include <asm/processor.h>
> +#include <asm/cpu_device_id.h>
>  
>  #define DRVNAME	"via_cputemp"
>  
> @@ -308,34 +309,26 @@ static struct notifier_block via_cputemp_cpu_notifier __refdata = {
>  	.notifier_call = via_cputemp_cpu_callback,
>  };
>  
> +static struct x86_cpu_id cputemp_ids[] = {

Shall be const.

> +	{ X86_VENDOR_CENTAUR, 6, 0xa, }, /* C7a */
> +	{ X86_VENDOR_CENTAUR, 6, 0xd, }, /* C7d */

Better spelt "C7 A" and "C7 D".

> +	{ X86_VENDOR_CENTAUR, 6, 0xf, }, /* Nano */
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, cputemp_ids);
> +
>  static int __init via_cputemp_init(void)
>  {
>  	int i, err;
>  
> -	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
> -		printk(KERN_DEBUG DRVNAME ": Not a VIA CPU\n");
> -		err = -ENODEV;
> -		goto exit;
> -	}
> +	if (!x86_match_cpu(cputemp_ids))
> +		return -ENODEV;
>  
>  	err = platform_driver_register(&via_cputemp_driver);
>  	if (err)
>  		goto exit;
>  
>  	for_each_online_cpu(i) {
> -		struct cpuinfo_x86 *c = &cpu_data(i);
> -
> -		if (c->x86 != 6)
> -			continue;
> -
> -		if (c->x86_model < 0x0a)
> -			continue;
> -
> -		if (c->x86_model > 0x0f) {
> -			pr_warn("Unknown CPU model 0x%x\n", c->x86_model);
> -			continue;
> -		}

Here again, the per-cpu check was intended, and I would like to
preserve that (even though SMP systems based on VIA CPUs must be
rare...)

> -
>  		via_cputemp_device_add(i);
>  	}
>  


-- 
Jean Delvare

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Andi Kleen]

On Wed, Dec 07, 2011 at 06:40:01PM -0800, Guenter Roeck wrote:
> Andi,
> 
> On Wed, Dec 07, 2011 at 07:41:19PM -0500, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Use the new x86 cpuid autoprobe interface for the Intel coretemp
> > driver.
> > 
> > Cc: fenghua.yu@intel.com
> > Cc: khali@linux-fr.org
> > Cc: guenter.roeck@ericsson.com
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >  drivers/hwmon/coretemp.c |   17 ++++++++++++++---
> >  1 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > index 104b376..5de1579 100644
> > --- a/drivers/hwmon/coretemp.c
> > +++ b/drivers/hwmon/coretemp.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/moduleparam.h>
> >  #include <asm/msr.h>
> >  #include <asm/processor.h>
> > +#include <asm/cpu_device_id.h>
> >  
> >  #define DRVNAME	"coretemp"
> >  
> > @@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
> >  	.notifier_call = coretemp_cpu_callback,
> >  };
> >  
> > +static struct x86_cpu_id coretemp_ids[] = {
> > +	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTS },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
> > +
> >  static int __init coretemp_init(void)
> >  {
> >  	int i, err = -ENODEV;
> >  
> > -	/* quick check if we run Intel */
> > -	if (cpu_data(0).x86_vendor != X86_VENDOR_INTEL)
> > -		goto exit;
> > +	/*
> > +	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> > +	 * sensors. We check this bit only, all the early CPUs
> > +	 * without thermal sensors will be filtered out.
> > +	 */
> > +	if (!x86_match_cpu(coretemp_ids))
> > +		return -ENODEV;
> >  
> 
> X86_FEATURE_DTS is checked elsewhere in the driver, in function get_core_online().
> Can that check be removed ?

It's a bit fuzzy -- that function checks the target CPU,
while x86_match_cpu only checks the boot cpu. Now in practice 
we don't really support assymetric configurations for other reasons
anyways, but in theory it's still more correct and future proof to check 
each CPU individually. That's why I kept that other check.
So it's not really needed right now, but I would still keep it.

-Andi

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Andi Kleen]

> Well, there is CPUID_8000_0007_EDX[7] which denotes the presence
> of the P-state registers. It probably could be added to
> init_scattered_cpuid_features() but there's still special handling
> needed for K8.

Dave is probably right that it's not worth moving the list.

> Can you do both a family and a feature bit test with the autoprobing
> code?

Yes.
-Andi

Re: [PATCH 01/10] Add driver auto probing for x86 features

[Andi Kleen]

On Thu, Dec 08, 2011 at 10:35:40AM +0100, Jean Delvare wrote:
> > +/**
> > + * x86_match_cpu - match current CPU again an array of x86_cpu_ids
> 
> The code is actually matching the boot cpu, which isn't necessarily the
> current CPU. I think it would be better to let the caller pass a
> specific cpu as a parameter. At least the hwmon drivers would benefit
> from that. Then you can add an helper function x86_match_boot_cpu()
> calling x86_match_cpu() on &boot_cpu_data if you want.

I don't really see a point of this currently -- the udev/modprobe loading is 
global anyways.  If we really want to support asymmetric configs a lot 
more work all over the kernel is needed and this could be still
changed.
> 
> > + * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
> > + *         X86_MODEL_END.
> 
> I see no such X86_MODEL_END, your loop below is instead treating entries
> with all fields set to 0 as the terminating entry (which seems
> reasonable.)

Hmm yes I ended up with {}. I'll fix the comment.

> > +#define X86_VENDOR_ANY 0xffff
> > +#define X86_FAMILY_ANY 0
> > +#define X86_MODEL_ANY  0
> 
> Are you sure family 0 or model 0 are never used, by any vendor? I

They could be, but we never match for them.

> wouldn't take the risk. What's wrong with 0xffff?

That would not allow abbreviating the entries: the C compiler
doesn't know how to fill in 0xffff automatically.

> 
> > +#define X86_FEATURE_ANY 0	/* Same as FPU, you can't test for that */
> 
> Might be better to set X86_FEATURE_ANY to either 10 (unused feature
> bit) or 0xffff then.

I don't think this is a problem in practice.

Thanks for the review.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Guenter Roeck]

On Thu, 2011-12-08 at 02:24 -0500, Jean Delvare wrote:
> Hi Andi,
> 
> Thanks a lot for working on this. I wanted to give it a try long ago but
> could never find the time.
> 
> On Wed, 7 Dec 2011 18:40:01 -0800, Guenter Roeck wrote:
> > Andi,
> > 
> > On Wed, Dec 07, 2011 at 07:41:19PM -0500, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > Use the new x86 cpuid autoprobe interface for the Intel coretemp
> > > driver.
> > > 
> > > Cc: fenghua.yu@intel.com
> > > Cc: khali@linux-fr.org
> > > Cc: guenter.roeck@ericsson.com
> > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > ---
> > >  drivers/hwmon/coretemp.c |   17 ++++++++++++++---
> > >  1 files changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > > index 104b376..5de1579 100644
> > > --- a/drivers/hwmon/coretemp.c
> > > +++ b/drivers/hwmon/coretemp.c
> > > @@ -39,6 +39,7 @@
> > >  #include <linux/moduleparam.h>
> > >  #include <asm/msr.h>
> > >  #include <asm/processor.h>
> > > +#include <asm/cpu_device_id.h>
> > >  
> > >  #define DRVNAME	"coretemp"
> > >  
> > > @@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
> > >  	.notifier_call = coretemp_cpu_callback,
> > >  };
> > >  
> > > +static struct x86_cpu_id coretemp_ids[] = {
> 
> Can't this be made const?
> 
Unless I am missing something, the patchset would have to change to
accept a const as parameter to x86_match_cpu().

> > > +	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTS },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
> > > +
> > >  static int __init coretemp_init(void)
> > >  {
> > >  	int i, err = -ENODEV;
> > >  
> > > -	/* quick check if we run Intel */
> > > -	if (cpu_data(0).x86_vendor != X86_VENDOR_INTEL)
> > > -		goto exit;
> > > +	/*
> > > +	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> > > +	 * sensors. We check this bit only, all the early CPUs
> > > +	 * without thermal sensors will be filtered out.
> > > +	 */
> > > +	if (!x86_match_cpu(coretemp_ids))
> > > +		return -ENODEV;
> >
> > X86_FEATURE_DTS is checked elsewhere in the driver, in function get_core_online().
> > Can that check be removed ?
> 
> Is it not possible in theory to mix a CPU with DTS and one without DTS
> on the same system? There may be a reason why the DTS flag was checked
> in get_core_online() (i.e. on a per-core basis) rather than in
> coretemp_init(). I seem to recall that someone explicitly asked for it.
> 
> To avoid any risk of regression, I wouldn't touch this part of the code.
> 
Ok, makes sense.

Thanks,
Guenter

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Jean Delvare]

On Thu, 8 Dec 2011 08:09:29 -0800, Guenter Roeck wrote:
> On Thu, 2011-12-08 at 02:24 -0500, Jean Delvare wrote:
> > Hi Andi,
> > 
> > Thanks a lot for working on this. I wanted to give it a try long ago but
> > could never find the time.
> > 
> > On Wed, 7 Dec 2011 18:40:01 -0800, Guenter Roeck wrote:
> > > Andi,
> > > 
> > > On Wed, Dec 07, 2011 at 07:41:19PM -0500, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > Use the new x86 cpuid autoprobe interface for the Intel coretemp
> > > > driver.
> > > > 
> > > > Cc: fenghua.yu@intel.com
> > > > Cc: khali@linux-fr.org
> > > > Cc: guenter.roeck@ericsson.com
> > > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > > ---
> > > >  drivers/hwmon/coretemp.c |   17 ++++++++++++++---
> > > >  1 files changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > > > index 104b376..5de1579 100644
> > > > --- a/drivers/hwmon/coretemp.c
> > > > +++ b/drivers/hwmon/coretemp.c
> > > > @@ -39,6 +39,7 @@
> > > >  #include <linux/moduleparam.h>
> > > >  #include <asm/msr.h>
> > > >  #include <asm/processor.h>
> > > > +#include <asm/cpu_device_id.h>
> > > >  
> > > >  #define DRVNAME	"coretemp"
> > > >  
> > > > @@ -756,13 +757,23 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
> > > >  	.notifier_call = coretemp_cpu_callback,
> > > >  };
> > > >  
> > > > +static struct x86_cpu_id coretemp_ids[] = {
> > 
> > Can't this be made const?
> > 
> Unless I am missing something, the patchset would have to change to
> accept a const as parameter to x86_match_cpu().

Yes, I proposed that too in my review of the relevant patch.

-- 
Jean Delvare

Re: [PATCH 06/10] HWMON: Convert coretemp to x86 cpuid autoprobing

[Andi Kleen]

> > Unless I am missing something, the patchset would have to change to
> > accept a const as parameter to x86_match_cpu().
> 
> Yes, I proposed that too in my review of the relevant patch.

Done.
-Andi

Re: [PATCH 01/10] Add driver auto probing for x86 features

[Jean Delvare]

Hi Andi,

On Thu, 8 Dec 2011 15:45:16 +0100, Andi Kleen wrote:
> On Thu, Dec 08, 2011 at 10:35:40AM +0100, Jean Delvare wrote:
> > > +/**
> > > + * x86_match_cpu - match current CPU again an array of x86_cpu_ids
> > 
> > The code is actually matching the boot cpu, which isn't necessarily the
> > current CPU. I think it would be better to let the caller pass a
> > specific cpu as a parameter. At least the hwmon drivers would benefit
> > from that. Then you can add an helper function x86_match_boot_cpu()
> > calling x86_match_cpu() on &boot_cpu_data if you want.
> 
> I don't really see a point of this currently -- the udev/modprobe loading is 
> global anyways.

Irrelevant. You load a driver when any, not all, of the devices in the
system are supported by said driver. This obviously holds for PCI
devices and drivers (you don't refrain from loading the network card's
driver because that driver doesn't support your graphics card), the CPU
device case is no different (in theory at least.)

As a matter of fact, the sensors-detect script (which is in charge of
finding the right hwmon drivers to load on a given system when these
drivers do not support auto-loading via module aliases) does exactly
this for CPU thermal sensor detection. It loops over the complete list
of online CPUs, and if any is supported by coretemp or via-cputemp,
then it suggests loading the driver in question. The search is not
limited to CPU#0.

> If we really want to support asymmetric configs a lot 
> more work all over the kernel is needed and this could be still
> changed.

Can you guarantee that there is no asymmetric system already working
today? If you can't, then your approach could cause a regression. This
is why I am asking for an API change to let drivers pass a specific CPU
to x86_match_cpu(). There are at least two drivers who would take
benefit of this. Your patch set is supposed to only add driver
auto-loading, and while cleaning up the code in the process is nice, I
think you should avoid driver behavior changes, these are out of scope
for such a patch set.

Seriously, this API change is very small and unintrusive, so I just
can't think of a valid reason to not do it.

-- 
Jean Delvare

Re: [PATCH 01/10] Add driver auto probing for x86 features

[Andi Kleen]

> Can you guarantee that there is no asymmetric system already working
> today?

I used to have an asymmetric system myself, but we generally subset the 
CPU feature
flags to be the same everywhere and I didn't change any per CPU stepping 
checks
as they were there.

>   If you can't, then your approach could cause a regression. This
> is why I am asking for an API change to let drivers pass a specific CPU
> to x86_match_cpu(). There are at least two drivers who would take
> benefit of this. Your patch set is supposed to only add driver
> auto-loading, and while cleaning up the code in the process is nice, I
> think you should avoid driver behavior changes, these are out of scope
> for such a patch set.

I undoed that change to the coretemp driver and there's no other AFAIK.
Generally most drivers only check the boot cpu.

-Andi

Re: [PATCH 01/10] Add driver auto probing for x86 features

[Jean Delvare]

On Fri, 09 Dec 2011 12:24:47 -0800, Andi Kleen wrote:
> 
> > Can you guarantee that there is no asymmetric system already working
> > today?
> 
> I used to have an asymmetric system myself, but we generally subset the 
> CPU feature
> flags to be the same everywhere and I didn't change any per CPU stepping 
> checks
> as they were there.
> 
> >   If you can't, then your approach could cause a regression. This
> > is why I am asking for an API change to let drivers pass a specific CPU
> > to x86_match_cpu(). There are at least two drivers who would take
> > benefit of this. Your patch set is supposed to only add driver
> > auto-loading, and while cleaning up the code in the process is nice, I
> > think you should avoid driver behavior changes, these are out of scope
> > for such a patch set.
> 
> I undoed that change to the coretemp driver

Thanks.

> and there's no other AFAIK.

via-cputemp was affected as well.

> Generally most drivers only check the boot cpu.

-- 
Jean Delvare

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[H. Peter Anvin]

On 12/08/2011 06:37 AM, Andi Kleen wrote:
>> Well, there is CPUID_8000_0007_EDX[7] which denotes the presence
>> of the P-state registers. It probably could be added to
>> init_scattered_cpuid_features() but there's still special handling
>> needed for K8.
> 
> Dave is probably right that it's not worth moving the list.
> 
>> Can you do both a family and a feature bit test with the autoprobing
>> code?
> 

Another similar question: is it possible to key a load on more than one
CPU feature?  It's fairly common that you need more than one feature for
a specific piece of code to function, and it would be good to be able to
represent it.

	-hpa

Re: [PATCH 01/10] Add driver auto probing for x86 features

[H. Peter Anvin]

On 12/09/2011 12:16 PM, Jean Delvare wrote:
> 
> Irrelevant. You load a driver when any, not all, of the devices in the
> system are supported by said driver. This obviously holds for PCI
> devices and drivers (you don't refrain from loading the network card's
> driver because that driver doesn't support your graphics card), the CPU
> device case is no different (in theory at least.)
> 

Well, it is, actually.  We do a whole lot of tests using boot_cpu_has(),
which really isn't about the boot CPU at all but rather is the
intersection of all features supported by all the CPUs in the system.

There are, however, a handful of drivers which want to be loaded for
"any matching CPU" as opposed to "all matching CPUs" and I guess it
would be useful to have that capability as an alternative.

	-hpa

Re: [PATCH 07/10] cpufreq: Add support for x86 cpuinfo auto loading

[Andi Kleen]

On Thu, Dec 15, 2011 at 05:19:46PM -0800, H. Peter Anvin wrote:
> On 12/08/2011 06:37 AM, Andi Kleen wrote:
> >> Well, there is CPUID_8000_0007_EDX[7] which denotes the presence
> >> of the P-state registers. It probably could be added to
> >> init_scattered_cpuid_features() but there's still special handling
> >> needed for K8.
> > 
> > Dave is probably right that it's not worth moving the list.
> > 
> >> Can you do both a family and a feature bit test with the autoprobing
> >> code?
> > 
> 
> Another similar question: is it possible to key a load on more than one
> CPU feature?  It's fairly common that you need more than one feature for
> a specific piece of code to function, and it would be good to be able to
> represent it.

Right now it's not possible. The driver just errors out if it can't
find some additional capability.

I considered it originally, but all the cases I looked at were quite 
obscure (unlikely to happen) so I opted for this simpler setup.

-Andi