[PATCH 0/7] x86, microcode, AMD: Some fixes

[PATCH 0/7] x86, microcode, AMD: Some fixes

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Hi,

this is a small patchset that takes care of various smallish issues with
the AMD microcode driver, see the individual commit messages for more
info.

Suggestions and comments are welcome, as always.

 arch/x86/kernel/microcode_amd.c  |  142 ++++++++++++++++++--------------------
 arch/x86/kernel/microcode_core.c |    6 +-
 drivers/base/sys.c               |   65 ++++++++++++-----
 3 files changed, 118 insertions(+), 95 deletions(-)

Thanks,
Boris.

[PATCH 1/7] sysdev: Do not register with sysdev when erroring on add

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

When encountering an error while executing the driver's ->add method, we
should cancel registration and unwind what we've regged so far. The low
level ->add methods do return proper error codes but those aren't looked
at in sysdev_driver_register(). Fix that by sharing the unregistering
code.

Also, fixup warning messages formatting while at it.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 drivers/base/sys.c |   65 ++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/base/sys.c b/drivers/base/sys.c
index 1667aaf..f6fb547 100644
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -166,6 +166,36 @@ EXPORT_SYMBOL_GPL(sysdev_class_unregister);
 
 static DEFINE_MUTEX(sysdev_drivers_lock);
 
+/*
+ * @dev != NULL means that we're unwinding because some drv->add()
+ * failed for some reason. You need to grab sysdev_drivers_lock before
+ * calling this.
+ */
+static void __sysdev_driver_remove(struct sysdev_class *cls,
+				   struct sysdev_driver *drv,
+				   struct sys_device *from_dev)
+{
+	struct sys_device *dev = from_dev;
+
+	list_del_init(&drv->entry);
+	if (!cls)
+		return;
+
+	if (!drv->remove)
+		goto kset_put;
+
+	if (dev)
+		list_for_each_entry_continue_reverse(dev, &cls->kset.list,
+						     kobj.entry)
+			drv->remove(dev);
+	else
+		list_for_each_entry(dev, &cls->kset.list, kobj.entry)
+			drv->remove(dev);
+
+kset_put:
+	kset_put(&cls->kset);
+}
+
 /**
  *	sysdev_driver_register - Register auxillary driver
  *	@cls:	Device class driver belongs to.
@@ -175,14 +205,14 @@ static DEFINE_MUTEX(sysdev_drivers_lock);
  *	called on each operation on devices of that class. The refcount
  *	of @cls is incremented.
  */
-
 int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
 {
+	struct sys_device *dev = NULL;
 	int err = 0;
 
 	if (!cls) {
-		WARN(1, KERN_WARNING "sysdev: invalid class passed to "
-			"sysdev_driver_register!\n");
+		WARN(1, KERN_WARNING "sysdev: invalid class passed to %s!\n",
+			__func__);
 		return -EINVAL;
 	}
 
@@ -198,19 +228,27 @@ int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
 
 		/* If devices of this class already exist, tell the driver */
 		if (drv->add) {
-			struct sys_device *dev;
-			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
-				drv->add(dev);
+			list_for_each_entry(dev, &cls->kset.list, kobj.entry) {
+				err = drv->add(dev);
+				if (err)
+					goto unwind;
+			}
 		}
 	} else {
 		err = -EINVAL;
 		WARN(1, KERN_ERR "%s: invalid device class\n", __func__);
 	}
+
+	goto unlock;
+
+unwind:
+	__sysdev_driver_remove(cls, drv, dev);
+
+unlock:
 	mutex_unlock(&sysdev_drivers_lock);
 	return err;
 }
 
-
 /**
  *	sysdev_driver_unregister - Remove an auxillary driver.
  *	@cls:	Class driver belongs to.
@@ -220,23 +258,12 @@ void sysdev_driver_unregister(struct sysdev_class *cls,
 			      struct sysdev_driver *drv)
 {
 	mutex_lock(&sysdev_drivers_lock);
-	list_del_init(&drv->entry);
-	if (cls) {
-		if (drv->remove) {
-			struct sys_device *dev;
-			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
-				drv->remove(dev);
-		}
-		kset_put(&cls->kset);
-	}
+	__sysdev_driver_remove(cls, drv, NULL);
 	mutex_unlock(&sysdev_drivers_lock);
 }
-
 EXPORT_SYMBOL_GPL(sysdev_driver_register);
 EXPORT_SYMBOL_GPL(sysdev_driver_unregister);
 
-
-
 /**
  *	sysdev_register - add a system device to the tree
  *	@sysdev:	device in question
-- 
1.7.4.rc2

[PATCH 2/7] x86, microcode: Correct sysdev_add error path

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

When we encounter an error while initting the microcode driver on a CPU,
we must undo the previously added sysfs group.

Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 1cca374..87af68e 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -417,8 +417,10 @@ static int mc_sysdev_add(struct sys_device *sys_dev)
 	if (err)
 		return err;
 
-	if (microcode_init_cpu(cpu) == UCODE_ERROR)
-		err = -EINVAL;
+	if (microcode_init_cpu(cpu) == UCODE_ERROR) {
+		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
+		return -EINVAL;
+	}
 
 	return err;
 }
-- 
1.7.4.rc2

[PATCH 3/7] x86, microcode, AMD: Release firmware on error

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

When the ucode magic is wrong, for whatever reason, we don't release the
loaded firmware binary and its related resources. Make sure we do. Also,
fix function naming to fit this driver's convention and shorten variable
names.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 0fe6d1a..ef91df0 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -278,27 +278,29 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	return state;
 }
 
-static enum ucode_state request_microcode_fw(int cpu, struct device *device)
+static enum ucode_state request_microcode_amd(int cpu, struct device *device)
 {
 	const char *fw_name = "amd-ucode/microcode_amd.bin";
-	const struct firmware *firmware;
-	enum ucode_state ret;
+	const struct firmware *fw;
+	enum ucode_state ret = UCODE_NFOUND;
 
-	if (request_firmware(&firmware, fw_name, device)) {
+	if (request_firmware(&fw, fw_name, device)) {
 		printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
-		return UCODE_NFOUND;
+		goto out;
 	}
 
-	if (*(u32 *)firmware->data != UCODE_MAGIC) {
-		pr_err("invalid UCODE_MAGIC (0x%08x)\n",
-		       *(u32 *)firmware->data);
-		return UCODE_ERROR;
+	ret = UCODE_ERROR;
+	if (*(u32 *)fw->data != UCODE_MAGIC) {
+		pr_err("Invalid UCODE_MAGIC (0x%08x)\n", *(u32 *)fw->data);
+		goto fw_release;
 	}
 
-	ret = generic_load_microcode(cpu, firmware->data, firmware->size);
+	ret = generic_load_microcode(cpu, fw->data, fw->size);
 
-	release_firmware(firmware);
+fw_release:
+	release_firmware(fw);
 
+out:
 	return ret;
 }
 
@@ -319,7 +321,7 @@ static void microcode_fini_cpu_amd(int cpu)
 
 static struct microcode_ops microcode_amd_ops = {
 	.request_microcode_user           = request_microcode_user,
-	.request_microcode_fw             = request_microcode_fw,
+	.request_microcode_fw             = request_microcode_amd,
 	.collect_cpu_info                 = collect_cpu_info_amd,
 	.apply_microcode                  = apply_microcode_amd,
 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
-- 
1.7.4.rc2

[PATCH 4/7] x86, microcode, AMD: Simplify install_equiv_cpu_table

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

There's no need to memcpy the ucode header in order to look at it only
in this function - use the original buffer instead. Also, fix return
type semantics by returning a negative value on error and a positive
otherwise.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c |   21 ++++++++-------------
 1 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index ef91df0..9a451d7 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -188,27 +188,22 @@ get_next_ucode(const u8 *buf, unsigned int size, unsigned int *mc_size)
 
 static int install_equiv_cpu_table(const u8 *buf)
 {
-	u8 *container_hdr[UCODE_CONTAINER_HEADER_SIZE];
-	unsigned int *buf_pos = (unsigned int *)container_hdr;
-	unsigned long size;
+	unsigned int *ibuf = (unsigned int *)buf;
+	unsigned int type = ibuf[1];
+	unsigned int size = ibuf[2];
 
-	get_ucode_data(&container_hdr, buf, UCODE_CONTAINER_HEADER_SIZE);
-
-	size = buf_pos[2];
-
-	if (buf_pos[1] != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
+	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
 		pr_err("error: invalid type field in container file section header\n");
-		return 0;
+		return -EINVAL;
 	}
 
 	equiv_cpu_table = vmalloc(size);
 	if (!equiv_cpu_table) {
 		pr_err("failed to allocate equivalent CPU table\n");
-		return 0;
+		return -ENOMEM;
 	}
 
-	buf += UCODE_CONTAINER_HEADER_SIZE;
-	get_ucode_data(equiv_cpu_table, buf, size);
+	get_ucode_data(equiv_cpu_table, buf + UCODE_CONTAINER_HEADER_SIZE, size);
 
 	return size + UCODE_CONTAINER_HEADER_SIZE; /* add header length */
 }
@@ -232,7 +227,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	enum ucode_state state = UCODE_OK;
 
 	offset = install_equiv_cpu_table(ucode_ptr);
-	if (!offset) {
+	if (offset < 0) {
 		pr_err("failed to create equivalent cpu table\n");
 		return UCODE_ERROR;
 	}
-- 
1.7.4.rc2

[PATCH 5/7] x86, microcode, AMD: Simplify get_next_ucode

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Do not copy the section header but look at it directly through the
pointer. Also, make it return a ptr to a ucode header directly
thus dropping a bunch of unneeded casts. Finally, simplify
generic_load_microcode(), while at it.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c |   68 ++++++++++++++++++--------------------
 1 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 9a451d7..8b58fc1 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -88,9 +88,9 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	return 0;
 }
 
-static int get_matching_microcode(int cpu, void *mc, int rev)
+static int get_matching_microcode(int cpu, struct microcode_header_amd *mc_hdr,
+				  int rev)
 {
-	struct microcode_header_amd *mc_header = mc;
 	unsigned int current_cpu_id;
 	u16 equiv_cpu_id = 0;
 	unsigned int i = 0;
@@ -109,17 +109,17 @@ static int get_matching_microcode(int cpu, void *mc, int rev)
 	if (!equiv_cpu_id)
 		return 0;
 
-	if (mc_header->processor_rev_id != equiv_cpu_id)
+	if (mc_hdr->processor_rev_id != equiv_cpu_id)
 		return 0;
 
 	/* ucode might be chipset specific -- currently we don't support this */
-	if (mc_header->nb_dev_id || mc_header->sb_dev_id) {
+	if (mc_hdr->nb_dev_id || mc_hdr->sb_dev_id) {
 		pr_err("CPU%d: loading of chipset specific code not yet supported\n",
 		       cpu);
 		return 0;
 	}
 
-	if (mc_header->patch_id <= rev)
+	if (mc_hdr->patch_id <= rev)
 		return 0;
 
 	return 1;
@@ -155,21 +155,18 @@ static int apply_microcode_amd(int cpu)
 	return 0;
 }
 
-static void *
+static struct microcode_header_amd *
 get_next_ucode(const u8 *buf, unsigned int size, unsigned int *mc_size)
 {
+	struct microcode_header_amd *mc;
 	unsigned int total_size;
-	u8 section_hdr[UCODE_CONTAINER_SECTION_HDR];
-	void *mc;
 
-	get_ucode_data(section_hdr, buf, UCODE_CONTAINER_SECTION_HDR);
-
-	if (section_hdr[0] != UCODE_UCODE_TYPE) {
+	if (buf[0] != UCODE_UCODE_TYPE) {
 		pr_err("error: invalid type field in container file section header\n");
 		return NULL;
 	}
 
-	total_size = (unsigned long) (section_hdr[4] + (section_hdr[5] << 8));
+	total_size = buf[4] + (buf[5] << 8);
 
 	if (total_size > size || total_size > UCODE_MAX_SIZE) {
 		pr_err("error: size mismatch\n");
@@ -218,12 +215,12 @@ static enum ucode_state
 generic_load_microcode(int cpu, const u8 *data, size_t size)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct microcode_header_amd *mc_hdr = NULL;
+	unsigned int mc_size, leftover;
+	unsigned long offset;
 	const u8 *ucode_ptr = data;
 	void *new_mc = NULL;
-	void *mc;
 	int new_rev = uci->cpu_sig.rev;
-	unsigned int leftover;
-	unsigned long offset;
 	enum ucode_state state = UCODE_OK;
 
 	offset = install_equiv_cpu_table(ucode_ptr);
@@ -236,38 +233,37 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	leftover = size - offset;
 
 	while (leftover) {
-		unsigned int uninitialized_var(mc_size);
-		struct microcode_header_amd *mc_header;
-
-		mc = get_next_ucode(ucode_ptr, leftover, &mc_size);
-		if (!mc)
+		mc_hdr = get_next_ucode(ucode_ptr, leftover, &mc_size);
+		if (!mc_hdr)
 			break;
 
-		mc_header = (struct microcode_header_amd *)mc;
-		if (get_matching_microcode(cpu, mc, new_rev)) {
+		if (get_matching_microcode(cpu, mc_hdr, new_rev)) {
 			vfree(new_mc);
-			new_rev = mc_header->patch_id;
-			new_mc  = mc;
+			new_rev = mc_hdr->patch_id;
+			new_mc  = mc_hdr;
 		} else
-			vfree(mc);
+			vfree(mc_hdr);
 
 		ucode_ptr += mc_size;
 		leftover  -= mc_size;
 	}
 
-	if (new_mc) {
-		if (!leftover) {
-			vfree(uci->mc);
-			uci->mc = new_mc;
-			pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
-				 cpu, new_rev, uci->cpu_sig.rev);
-		} else {
-			vfree(new_mc);
-			state = UCODE_ERROR;
-		}
-	} else
+	if (!new_mc) {
 		state = UCODE_NFOUND;
+		goto free_table;
+	}
+
+	if (!leftover) {
+		vfree(uci->mc);
+		uci->mc = new_mc;
+		pr_debug("CPU%d update ucode to version 0x%x (from 0x%x)\n",
+			 cpu, new_rev, uci->cpu_sig.rev);
+	} else {
+		vfree(new_mc);
+		state = UCODE_ERROR;
+	}
 
+free_table:
 	free_equiv_cpu_table();
 
 	return state;
-- 
1.7.4.rc2

[PATCH 6/7] x86, microcode, AMD: Remove unneeded memset call

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

collect_cpu_info_amd() clears its csig arg but this is done in the
microcode_core's collect_cpu_info() by clearing the embedding struct
ucode_cpu_info. Drop it.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 8b58fc1..274fa40 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -77,7 +77,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	u32 dummy;
 
-	memset(csig, 0, sizeof(*csig));
 	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
 		pr_warning("microcode: CPU%d: AMD CPU family 0x%x not "
 			   "supported\n", cpu, c->x86);
-- 
1.7.4.rc2

[PATCH 7/7] x86, microcode, AMD: Cleanup dmesg output

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Unify pr_* to use pr_fmt, shorten messages, correct type formatting.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Acked-by: Andreas Herrmann <Andreas.Herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 274fa40..adbcef4 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -78,12 +78,13 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	u32 dummy;
 
 	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
-		pr_warning("microcode: CPU%d: AMD CPU family 0x%x not "
-			   "supported\n", cpu, c->x86);
+		pr_warning("CPU%d: family %d not supported\n", cpu, c->x86);
 		return -1;
 	}
+
 	rdmsr(MSR_AMD64_PATCH_LEVEL, csig->rev, dummy);
-	pr_info("CPU%d: patch_level=0x%x\n", cpu, csig->rev);
+	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
+
 	return 0;
 }
 
@@ -113,7 +114,7 @@ static int get_matching_microcode(int cpu, struct microcode_header_amd *mc_hdr,
 
 	/* ucode might be chipset specific -- currently we don't support this */
 	if (mc_hdr->nb_dev_id || mc_hdr->sb_dev_id) {
-		pr_err("CPU%d: loading of chipset specific code not yet supported\n",
+		pr_err("CPU%d: chipset specific code not yet supported\n",
 		       cpu);
 		return 0;
 	}
@@ -143,12 +144,12 @@ static int apply_microcode_amd(int cpu)
 
 	/* check current patch id and patch's id for match */
 	if (rev != mc_amd->hdr.patch_id) {
-		pr_err("CPU%d: update failed (for patch_level=0x%x)\n",
+		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
 		       cpu, mc_amd->hdr.patch_id);
 		return -1;
 	}
 
-	pr_info("CPU%d: updated (new patch_level=0x%x)\n", cpu, rev);
+	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
 	uci->cpu_sig.rev = rev;
 
 	return 0;
@@ -161,14 +162,14 @@ get_next_ucode(const u8 *buf, unsigned int size, unsigned int *mc_size)
 	unsigned int total_size;
 
 	if (buf[0] != UCODE_UCODE_TYPE) {
-		pr_err("error: invalid type field in container file section header\n");
+		pr_err("invalid type field in container file section header\n");
 		return NULL;
 	}
 
 	total_size = buf[4] + (buf[5] << 8);
 
 	if (total_size > size || total_size > UCODE_MAX_SIZE) {
-		pr_err("error: size mismatch\n");
+		pr_err("section size mismatch\n");
 		return NULL;
 	}
 
@@ -189,7 +190,8 @@ static int install_equiv_cpu_table(const u8 *buf)
 	unsigned int size = ibuf[2];
 
 	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
-		pr_err("error: invalid type field in container file section header\n");
+		pr_err("empty section/"
+		       "invalid type field in container file section header\n");
 		return -EINVAL;
 	}
 
@@ -219,7 +221,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	unsigned long offset;
 	const u8 *ucode_ptr = data;
 	void *new_mc = NULL;
-	int new_rev = uci->cpu_sig.rev;
+	unsigned int new_rev = uci->cpu_sig.rev;
 	enum ucode_state state = UCODE_OK;
 
 	offset = install_equiv_cpu_table(ucode_ptr);
@@ -255,8 +257,8 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	if (!leftover) {
 		vfree(uci->mc);
 		uci->mc = new_mc;
-		pr_debug("CPU%d update ucode to version 0x%x (from 0x%x)\n",
-			 cpu, new_rev, uci->cpu_sig.rev);
+		pr_debug("CPU%d update ucode (0x%08x -> 0x%08x)\n",
+			 cpu, uci->cpu_sig.rev, new_rev);
 	} else {
 		vfree(new_mc);
 		state = UCODE_ERROR;
@@ -275,13 +277,13 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device)
 	enum ucode_state ret = UCODE_NFOUND;
 
 	if (request_firmware(&fw, fw_name, device)) {
-		printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
+		pr_err("failed to load file %s\n", fw_name);
 		goto out;
 	}
 
 	ret = UCODE_ERROR;
 	if (*(u32 *)fw->data != UCODE_MAGIC) {
-		pr_err("Invalid UCODE_MAGIC (0x%08x)\n", *(u32 *)fw->data);
+		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
 		goto fw_release;
 	}
 
-- 
1.7.4.rc2

Re: [PATCH 1/7] sysdev: Do not register with sysdev when erroring on add

[Borislav Petkov]

On Mon, Jan 24, 2011 at 10:28:57AM -0500, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> When encountering an error while executing the driver's ->add method, we
> should cancel registration and unwind what we've regged so far. The low
> level ->add methods do return proper error codes but those aren't looked
> at in sysdev_driver_register(). Fix that by sharing the unregistering
> code.
> 
> Also, fixup warning messages formatting while at it.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>

Greg, can I get an ACK/NACK please :) ?

Also, whether it is ok to go through -tip?

Thanks.

> ---
>  drivers/base/sys.c |   65 ++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/sys.c b/drivers/base/sys.c
> index 1667aaf..f6fb547 100644
> --- a/drivers/base/sys.c
> +++ b/drivers/base/sys.c
> @@ -166,6 +166,36 @@ EXPORT_SYMBOL_GPL(sysdev_class_unregister);
>  
>  static DEFINE_MUTEX(sysdev_drivers_lock);
>  
> +/*
> + * @dev != NULL means that we're unwinding because some drv->add()
> + * failed for some reason. You need to grab sysdev_drivers_lock before
> + * calling this.
> + */
> +static void __sysdev_driver_remove(struct sysdev_class *cls,
> +				   struct sysdev_driver *drv,
> +				   struct sys_device *from_dev)
> +{
> +	struct sys_device *dev = from_dev;
> +
> +	list_del_init(&drv->entry);
> +	if (!cls)
> +		return;
> +
> +	if (!drv->remove)
> +		goto kset_put;
> +
> +	if (dev)
> +		list_for_each_entry_continue_reverse(dev, &cls->kset.list,
> +						     kobj.entry)
> +			drv->remove(dev);
> +	else
> +		list_for_each_entry(dev, &cls->kset.list, kobj.entry)
> +			drv->remove(dev);
> +
> +kset_put:
> +	kset_put(&cls->kset);
> +}
> +
>  /**
>   *	sysdev_driver_register - Register auxillary driver
>   *	@cls:	Device class driver belongs to.
> @@ -175,14 +205,14 @@ static DEFINE_MUTEX(sysdev_drivers_lock);
>   *	called on each operation on devices of that class. The refcount
>   *	of @cls is incremented.
>   */
> -
>  int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
>  {
> +	struct sys_device *dev = NULL;
>  	int err = 0;
>  
>  	if (!cls) {
> -		WARN(1, KERN_WARNING "sysdev: invalid class passed to "
> -			"sysdev_driver_register!\n");
> +		WARN(1, KERN_WARNING "sysdev: invalid class passed to %s!\n",
> +			__func__);
>  		return -EINVAL;
>  	}
>  
> @@ -198,19 +228,27 @@ int sysdev_driver_register(struct sysdev_class *cls, struct sysdev_driver *drv)
>  
>  		/* If devices of this class already exist, tell the driver */
>  		if (drv->add) {
> -			struct sys_device *dev;
> -			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
> -				drv->add(dev);
> +			list_for_each_entry(dev, &cls->kset.list, kobj.entry) {
> +				err = drv->add(dev);
> +				if (err)
> +					goto unwind;
> +			}
>  		}
>  	} else {
>  		err = -EINVAL;
>  		WARN(1, KERN_ERR "%s: invalid device class\n", __func__);
>  	}
> +
> +	goto unlock;
> +
> +unwind:
> +	__sysdev_driver_remove(cls, drv, dev);
> +
> +unlock:
>  	mutex_unlock(&sysdev_drivers_lock);
>  	return err;
>  }
>  
> -
>  /**
>   *	sysdev_driver_unregister - Remove an auxillary driver.
>   *	@cls:	Class driver belongs to.
> @@ -220,23 +258,12 @@ void sysdev_driver_unregister(struct sysdev_class *cls,
>  			      struct sysdev_driver *drv)
>  {
>  	mutex_lock(&sysdev_drivers_lock);
> -	list_del_init(&drv->entry);
> -	if (cls) {
> -		if (drv->remove) {
> -			struct sys_device *dev;
> -			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
> -				drv->remove(dev);
> -		}
> -		kset_put(&cls->kset);
> -	}
> +	__sysdev_driver_remove(cls, drv, NULL);
>  	mutex_unlock(&sysdev_drivers_lock);
>  }
> -
>  EXPORT_SYMBOL_GPL(sysdev_driver_register);
>  EXPORT_SYMBOL_GPL(sysdev_driver_unregister);
>  
> -
> -
>  /**
>   *	sysdev_register - add a system device to the tree
>   *	@sysdev:	device in question
> -- 
> 1.7.4.rc2
> 
> 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 1/7] sysdev: Do not register with sysdev when erroring on add

[Greg KH]

On Mon, Jan 24, 2011 at 04:28:57PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> When encountering an error while executing the driver's ->add method, we
> should cancel registration and unwind what we've regged so far. The low
> level ->add methods do return proper error codes but those aren't looked
> at in sysdev_driver_register(). Fix that by sharing the unregistering
> code.

Have you actually hit this before?  If so, where?

> Also, fixup warning messages formatting while at it.

Please, no, one patch per thing.  Please break this up into two
different patches.

And, in the future, let me know that you need this for other work.
Actually, why did you send this to Ingo in your pull request?  What does
your microcode patches need from this patch?  It should go through my
tree, especially as there is other sysdev work happening at the moment.

so, no, please don't send this on to anyone else right now.

thanks,

greg k-h

Re: [PATCH 1/7] sysdev: Do not register with sysdev when erroring on add

[Borislav Petkov]

On Mon, Jan 31, 2011 at 05:16:59PM -0500, Greg KH wrote:
> On Mon, Jan 24, 2011 at 04:28:57PM +0100, Borislav Petkov wrote:
> > From: Borislav Petkov <borislav.petkov@amd.com>
> > 
> > When encountering an error while executing the driver's ->add method, we
> > should cancel registration and unwind what we've regged so far. The low
> > level ->add methods do return proper error codes but those aren't looked
> > at in sysdev_driver_register(). Fix that by sharing the unregistering
> > code.
> 
> Have you actually hit this before?  If so, where?

Sorta. I want to be paranoid when loading the microcode image file and
in patch 3/7 (http://marc.info/?l=linux-kernel&m=129588287409770) I
propagate the error in case loading the microcode fails. If we don't
cleanup here, the driver remains loaded and takes up resources. And if
you load it again, you get a sysfs warning about duplicated entries.

> > Also, fixup warning messages formatting while at it.
> 
> Please, no, one patch per thing.  Please break this up into two
> different patches.

Yes, will do.

> And, in the future, let me know that you need this for other work.

Will do.

> Actually, why did you send this to Ingo in your pull request?  What does
> your microcode patches need from this patch?  It should go through my
> tree, especially as there is other sysdev work happening at the moment.

Sorry about that, I did not mean to bypass/offend you in any way.

> so, no, please don't send this on to anyone else right now.

Do you need the patches rediffed against a some tree of yours?

@Ingo: please disregard my pull request from today:
http://marc.info/?l=linux-kernel&m=129648061017908

Guys, sorry again if I've caused any trouble.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 1/7] sysdev: Do not register with sysdev when erroring on add

[Greg KH]

On Mon, Jan 31, 2011 at 11:33:19PM +0100, Borislav Petkov wrote:
> On Mon, Jan 31, 2011 at 05:16:59PM -0500, Greg KH wrote:
> > On Mon, Jan 24, 2011 at 04:28:57PM +0100, Borislav Petkov wrote:
> > > From: Borislav Petkov <borislav.petkov@amd.com>
> > > 
> > > When encountering an error while executing the driver's ->add method, we
> > > should cancel registration and unwind what we've regged so far. The low
> > > level ->add methods do return proper error codes but those aren't looked
> > > at in sysdev_driver_register(). Fix that by sharing the unregistering
> > > code.
> > 
> > Have you actually hit this before?  If so, where?
> 
> Sorta. I want to be paranoid when loading the microcode image file and
> in patch 3/7 (http://marc.info/?l=linux-kernel&m=129588287409770) I
> propagate the error in case loading the microcode fails. If we don't
> cleanup here, the driver remains loaded and takes up resources. And if
> you load it again, you get a sysfs warning about duplicated entries.

Ok, it's good to clean up properly, but I was wondering if this was a
real problem that people saw "in the wild" or not.

> > > Also, fixup warning messages formatting while at it.
> > 
> > Please, no, one patch per thing.  Please break this up into two
> > different patches.
> 
> Yes, will do.
> 
> > And, in the future, let me know that you need this for other work.
> 
> Will do.
> 
> > Actually, why did you send this to Ingo in your pull request?  What does
> > your microcode patches need from this patch?  It should go through my
> > tree, especially as there is other sysdev work happening at the moment.
> 
> Sorry about that, I did not mean to bypass/offend you in any way.
> 
> > so, no, please don't send this on to anyone else right now.
> 
> Do you need the patches rediffed against a some tree of yours?

Just resend the two patches to me against linux-next and all should be
good.

thanks,

greg k-h