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

[PATCH 1/2] 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.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---

Patch against next-20110201.

 drivers/base/sys.c |   61 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/base/sys.c b/drivers/base/sys.c
index 1667aaf..a1eeff0 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,9 +205,9 @@ 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) {
@@ -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

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

[Brian Gerst]

On Tue, Feb 1, 2011 at 11:19 AM, Borislav Petkov <bp@amd64.org> 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.

This patch is causing boot failures on a virtual machine running
Fedora 15 32-bit.  With this patch, the microcode driver repeatedly
spews:
microcode: CPU0: AMD CPU family 0x6 not supported
modprobe: FATAL: Error inserting microcode: Invalid argument.
until eventually causing an oops.  Reverting this patch allows the
boot to proceed.

/proc/cpuinfo:
processor	: 0
vendor_id	: AuthenticAMD
cpu family	: 6
model		: 2
model name	: QEMU Virtual CPU version 0.13.0
stepping	: 3
cpu MHz		: 2809.416
cache size	: 512 KB
fdiv_bug	: no
hlt_bug		: no
f00f_bug	: no
coma_bug	: no
fpu		: yes
fpu_exception	: yes
cpuid level	: 4
wp		: yes
flags		: fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat
pse36 clflush mmx fxsr sse sse2 syscall nx lm pni cx16 popcnt
hypervisor lahf_lm abm sse4a
bogomips	: 5618.83
clflush size	: 64
cache_alignment	: 64
address sizes	: 40 bits physical, 48 bits virtual
power management:

--
Brian Gerst

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

[Borislav Petkov]

On Sun, Jun 05, 2011 at 07:07:34AM -0400, Brian Gerst wrote:
> On Tue, Feb 1, 2011 at 11:19 AM, Borislav Petkov <bp@amd64.org> 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.
> 
> This patch is causing boot failures on a virtual machine running
> Fedora 15 32-bit.  With this patch, the microcode driver repeatedly
> spews:
> microcode: CPU0: AMD CPU family 0x6 not supported
> modprobe: FATAL: Error inserting microcode: Invalid argument.
> until eventually causing an oops.  Reverting this patch allows the
> boot to proceed.

Yeah, we had a similar issue with a K8 machine on Fedora:
https://bugzilla.kernel.org/show_bug.cgi?id=35522

Another fix would be if you remove the *microcode.rules in
/etc/udev/rules.d/ which tries to load the microcode driver
continuously. The funny thing is, this happens only on Fedora - all the
other distros simply get the -EINVAL from the driver failing to load
and that's it. I guess Fedora's udev doesn't accept the error value and
continues to try to load the module.

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