[PATCH] drivers/base: error handling fixes

[PATCH] drivers/base: error handling fixes

[Jeff Garzik]

drivers/base/class:
- class_device_rename(): function basically shat itself if anything
  failed, leaving things in an indeterminant state.  If kmalloc() ever
  failed, it would dereference ERR_PTR(-ENOMEM).  Fix a bunch of bugs,
  over and above sysfs_create_link() error handling.

- class_device_add(): add missing sysfs_remove_link() [fix leak] to error path
- class_device_add(): handle sysfs_create_link() failure

drivers/base/dmapool:
- kmalloc() takes a GFP_xxx argument
- handle device_create_file() failure

drivers/base/platform:
- properly handle errors (fix leak-on-err) in platform_bus_init()

drivers/base/topology:
- return sysfs error via NOTIFY_BAD

Signed-off-by: Jeff Garzik <jeff@garzik.org>

---

 drivers/base/class.c    |   56 ++++++++++++++++++++++++++++++++++++++++++------
 drivers/base/dmapool.c  |    8 +++++-
 drivers/base/platform.c |   10 ++++++--
 drivers/base/topology.c |   11 ++++-----
 4 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index b32b77f..673db14 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -562,7 +562,9 @@ int class_device_add(struct class_device
 		goto out2;
 
 	/* add the needed attributes to this device */
-	sysfs_create_link(&class_dev->kobj, &parent_class->subsys.kset.kobj, "subsystem");
+	error = sysfs_create_link(&class_dev->kobj, &parent_class->subsys.kset.kobj, "subsystem");
+	if (error)
+		goto out2_5;
 	class_dev->uevent_attr.attr.name = "uevent";
 	class_dev->uevent_attr.attr.mode = S_IWUSR;
 	class_dev->uevent_attr.attr.owner = parent_class->owner;
@@ -639,6 +641,8 @@ int class_device_add(struct class_device
  out4:
 	class_device_remove_file(class_dev, &class_dev->uevent_attr);
  out3:
+	sysfs_remove_link(&class_dev->kobj, "subsystem");
+ out2_5:
 	kobject_del(&class_dev->kobj);
  out2:
 	if(parent_class_dev)
@@ -791,36 +795,76 @@ void class_device_destroy(struct class *
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
-	int error = 0;
+	int error = 0, err2;
 	char *old_class_name = NULL, *new_class_name = NULL;
+	char *old_name;
 
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;
 
-	pr_debug("CLASS: renaming '%s' to '%s'\n", class_dev->class_id,
+	old_name = kstrdup(class_dev->class_id, GFP_KERNEL);
+	if (!old_name) {
+		error = -ENOMEM;
+		goto err_out;
+	}
+
+	pr_debug("CLASS: renaming '%s' to '%s'\n", old_name,
 		 new_name);
 
-	if (class_dev->dev)
+	if (class_dev->dev) {
 		old_class_name = make_class_name(class_dev->class->name,
 						 &class_dev->kobj);
+		if (IS_ERR(old_class_name)) {
+			error = PTR_ERR(old_class_name);
+			goto err_out_oname;
+		}
+	}
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
 	error = kobject_rename(&class_dev->kobj, new_name);
+	if (error)
+		goto err_out_nameset;
 
 	if (class_dev->dev) {
 		new_class_name = make_class_name(class_dev->class->name,
 						 &class_dev->kobj);
-		sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
-				  new_class_name);
+		if (IS_ERR(new_class_name)) {
+			error = PTR_ERR(new_class_name);
+			goto err_out_rename;
+		}
+
+		error = sysfs_create_link(&class_dev->dev->kobj,
+					  &class_dev->kobj, new_class_name);
+		if (error)
+			goto err_out_new_class;
+
 		sysfs_remove_link(&class_dev->dev->kobj, old_class_name);
 	}
+
 	class_device_put(class_dev);
 
+	kfree(old_name);
 	kfree(old_class_name);
 	kfree(new_class_name);
 
+	return 0;
+
+err_out_new_class:
+	kfree(new_class_name);
+err_out_rename:
+	err2 = kobject_rename(&class_dev->kobj, old_name);
+	if (err2)
+		printk(KERN_ERR "Error %d while undoing kobject_rename('%s') failure... ruh roh, raggy\n",
+			err2, new_name);
+err_out_nameset:
+	strlcpy(class_dev->class_id, old_name, KOBJ_NAME_LEN);
+	kfree(old_class_name);
+err_out_oname:
+	kfree(old_name);
+err_out:
+	class_device_put(class_dev);
 	return error;
 }
 
diff --git a/drivers/base/dmapool.c b/drivers/base/dmapool.c
index 33c5cce..b418b6b 100644
--- a/drivers/base/dmapool.c
+++ b/drivers/base/dmapool.c
@@ -126,7 +126,7 @@ dma_pool_create (const char *name, struc
 	} else if (allocation < size)
 		return NULL;
 
-	if (!(retval = kmalloc (sizeof *retval, SLAB_KERNEL)))
+	if (!(retval = kmalloc (sizeof *retval, GFP_KERNEL)))
 		return retval;
 
 	strlcpy (retval->name, name, sizeof retval->name);
@@ -143,7 +143,11 @@ dma_pool_create (const char *name, struc
 	if (dev) {
 		down (&pools_lock);
 		if (list_empty (&dev->dma_pools))
-			device_create_file (dev, &dev_attr_pools);
+			if (device_create_file (dev, &dev_attr_pools)) {
+				up (&pools_lock);
+				kfree(retval);
+				return NULL;
+			}
 		/* note:  not currently insisting "name" be unique */
 		list_add (&retval->pools, &dev->dma_pools);
 		up (&pools_lock);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 940ce41..3b6b758 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -563,8 +563,14 @@ EXPORT_SYMBOL_GPL(platform_bus_type);
 
 int __init platform_bus_init(void)
 {
-	device_register(&platform_bus);
-	return bus_register(&platform_bus_type);
+	int err = device_register(&platform_bus);
+	if (err)
+		return err;
+
+	err = bus_register(&platform_bus_type);
+	if (err)
+		device_unregister(&platform_bus);
+	return err;
 }
 
 #ifndef ARCH_HAS_DMA_GET_REQUIRED_MASK
diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 3ef9d51..2d0c965 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -97,14 +97,12 @@ static struct attribute_group topology_a
 /* Add/Remove cpu_topology interface for CPU device */
 static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
 {
-	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
-	return 0;
+	return sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
 }
 
-static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
+static void __cpuinit topology_remove_dev(struct sys_device * sys_dev)
 {
 	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
-	return 0;
 }
 
 static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
@@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
 {
 	unsigned int cpu = (unsigned long)hcpu;
 	struct sys_device *sys_dev;
+	int rc = 0;
 
 	sys_dev = get_cpu_sysdev(cpu);
 	switch (action) {
 	case CPU_ONLINE:
-		topology_add_dev(sys_dev);
+		rc = topology_add_dev(sys_dev);
 		break;
 	case CPU_DEAD:
 		topology_remove_dev(sys_dev);
 		break;
 	}
-	return NOTIFY_OK;
+	return rc ? NOTIFY_BAD : NOTIFY_OK;
 }
 
 static struct notifier_block __cpuinitdata topology_cpu_notifier =

Re: [PATCH] drivers/base: error handling fixes

[Cornelia Huck]

On Wed, 4 Oct 2006 09:05:54 -0400,
Jeff Garzik <jeff@garzik.org> wrote:

> drivers/base/class:
> - class_device_rename(): function basically shat itself if anything
>   failed, leaving things in an indeterminant state.  If kmalloc() ever
>   failed, it would dereference ERR_PTR(-ENOMEM).  Fix a bunch of bugs,
>   over and above sysfs_create_link() error handling.
> 
> - class_device_add(): add missing sysfs_remove_link() [fix leak] to error path
> - class_device_add(): handle sysfs_create_link() failure
> 
> drivers/base/dmapool:
> - kmalloc() takes a GFP_xxx argument
> - handle device_create_file() failure
> 
> drivers/base/platform:
> - properly handle errors (fix leak-on-err) in platform_bus_init()
> 
> drivers/base/topology:
> - return sysfs error via NOTIFY_BAD

Hm, I already did fixes for some of these which are in -mm / in Greg's
tree. It would perhaps make sense if you rediffed against one of those
trees.

Cornelia

Re: [PATCH] drivers/base: error handling fixes

[Cornelia Huck]

On Wed, 4 Oct 2006 09:05:54 -0400,
Jeff Garzik <jeff@garzik.org> wrote:

>  static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
> @@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
>  {
>  	unsigned int cpu = (unsigned long)hcpu;
>  	struct sys_device *sys_dev;
> +	int rc = 0;
>  
>  	sys_dev = get_cpu_sysdev(cpu);
>  	switch (action) {
>  	case CPU_ONLINE:
> -		topology_add_dev(sys_dev);
> +		rc = topology_add_dev(sys_dev);
>  		break;
>  	case CPU_DEAD:
>  		topology_remove_dev(sys_dev);
>  		break;
>  	}
> -	return NOTIFY_OK;
> +	return rc ? NOTIFY_BAD : NOTIFY_OK;
>  }

Wouldn't that also require that _cpu_up checked the return code when
doing CPU_ONLINE notification (and clean up on error)?

Re: [PATCH] drivers/base: error handling fixes

[Heiko Carstens]

On Wed, Oct 04, 2006 at 05:24:34PM +0200, Cornelia Huck wrote:
> On Wed, 4 Oct 2006 09:05:54 -0400,
> Jeff Garzik <jeff@garzik.org> wrote:
> 
> >  static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
> > @@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
> >  {
> >  	unsigned int cpu = (unsigned long)hcpu;
> >  	struct sys_device *sys_dev;
> > +	int rc = 0;
> >  
> >  	sys_dev = get_cpu_sysdev(cpu);
> >  	switch (action) {
> >  	case CPU_ONLINE:
> > -		topology_add_dev(sys_dev);
> > +		rc = topology_add_dev(sys_dev);
> >  		break;
> >  	case CPU_DEAD:
> >  		topology_remove_dev(sys_dev);
> >  		break;
> >  	}
> > -	return NOTIFY_OK;
> > +	return rc ? NOTIFY_BAD : NOTIFY_OK;
> >  }
> 
> Wouldn't that also require that _cpu_up checked the return code when
> doing CPU_ONLINE notification (and clean up on error)?

After all code that gets a CPU_ONLINE notification is not supposed to fail.
For allocating resources while bringing up a cpu CPU_UP_PREPARE is supposed
to be used. That one is allowed to fail.

Re: [PATCH] drivers/base: error handling fixes

[Jeff Garzik]

Heiko Carstens wrote:
> On Wed, Oct 04, 2006 at 05:24:34PM +0200, Cornelia Huck wrote:
>> On Wed, 4 Oct 2006 09:05:54 -0400,
>> Jeff Garzik <jeff@garzik.org> wrote:
>>
>>>  static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
>>> @@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
>>>  {
>>>  	unsigned int cpu = (unsigned long)hcpu;
>>>  	struct sys_device *sys_dev;
>>> +	int rc = 0;
>>>  
>>>  	sys_dev = get_cpu_sysdev(cpu);
>>>  	switch (action) {
>>>  	case CPU_ONLINE:
>>> -		topology_add_dev(sys_dev);
>>> +		rc = topology_add_dev(sys_dev);
>>>  		break;
>>>  	case CPU_DEAD:
>>>  		topology_remove_dev(sys_dev);
>>>  		break;
>>>  	}
>>> -	return NOTIFY_OK;
>>> +	return rc ? NOTIFY_BAD : NOTIFY_OK;
>>>  }
>> Wouldn't that also require that _cpu_up checked the return code when
>> doing CPU_ONLINE notification (and clean up on error)?
> 
> After all code that gets a CPU_ONLINE notification is not supposed to fail.
> For allocating resources while bringing up a cpu CPU_UP_PREPARE is supposed
> to be used. That one is allowed to fail.

It's a bug no matter how you look at it... I just lessen the impact.  :)

If someone wants to provide a better fix, let's see the patch...

	Jeff

Re: [PATCH] drivers/base: error handling fixes

[Heiko Carstens]

> >>> static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
> >>>@@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
> >>> {
> >>> 	unsigned int cpu = (unsigned long)hcpu;
> >>> 	struct sys_device *sys_dev;
> >>>+	int rc = 0;
> >>>  	sys_dev = get_cpu_sysdev(cpu);
> >>> 	switch (action) {
> >>> 	case CPU_ONLINE:
> >>>-		topology_add_dev(sys_dev);
> >>>+		rc = topology_add_dev(sys_dev);
> >>> 		break;
> >>> 	case CPU_DEAD:
> >>> 		topology_remove_dev(sys_dev);
> >>> 		break;
> >>> 	}
> >>>-	return NOTIFY_OK;
> >>>+	return rc ? NOTIFY_BAD : NOTIFY_OK;
> >>> }
> >>Wouldn't that also require that _cpu_up checked the return code when
> >>doing CPU_ONLINE notification (and clean up on error)?
> >After all code that gets a CPU_ONLINE notification is not supposed to fail.
> >For allocating resources while bringing up a cpu CPU_UP_PREPARE is supposed
> >to be used. That one is allowed to fail.
> 
> It's a bug no matter how you look at it... I just lessen the impact.  :)
> 
> If someone wants to provide a better fix, let's see the patch...

If sysfs_remove_group() would also work for non-created (-existent) groups
then the patch below would work. Unfortunately that is not the case. So one
would have to remember if sysfs_create_group() was done and succeeded before
calling sysfs_remove_group()...
There must be an easier way.

diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 3ef9d51..d0056c3 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -97,8 +97,7 @@ static struct attribute_group topology_a
 /* Add/Remove cpu_topology interface for CPU device */
 static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
 {
-	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
-	return 0;
+	return sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
 }
 
 static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
@@ -112,12 +111,16 @@ static int __cpuinit topology_cpu_callba
 {
 	unsigned int cpu = (unsigned long)hcpu;
 	struct sys_device *sys_dev;
+	int rc;
 
 	sys_dev = get_cpu_sysdev(cpu);
 	switch (action) {
-	case CPU_ONLINE:
-		topology_add_dev(sys_dev);
+	case CPU_UP_PREPARE:
+		rc = topology_add_dev(sys_dev);
+		if (rc)
+			return NOTIFY_BAD;
 		break;
+	case CPU_UP_CANCELED:
 	case CPU_DEAD:
 		topology_remove_dev(sys_dev);
 		break;
@@ -132,11 +135,15 @@ static struct notifier_block __cpuinitda
 
 static int __cpuinit topology_sysfs_init(void)
 {
-	int i;
-
-	for_each_online_cpu(i) {
-		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
-				(void *)(long)i);
+	struct sys_device *sys_dev;
+	int cpu;
+	int rc;
+
+	for_each_online_cpu(cpu) {
+		sys_dev = get_cpu_sysdev(cpu);
+		rc = topology_add_dev(sys_dev);
+		if (rc)
+			return rc;
 	}
 
 	register_hotcpu_notifier(&topology_cpu_notifier);

Re: [PATCH] drivers/base: error handling fixes

[Jeff Garzik]

Heiko Carstens wrote:
>>>>> static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
>>>>> @@ -112,17 +110,18 @@ static int __cpuinit topology_cpu_callba
>>>>> {
>>>>> 	unsigned int cpu = (unsigned long)hcpu;
>>>>> 	struct sys_device *sys_dev;
>>>>> +	int rc = 0;
>>>>>  	sys_dev = get_cpu_sysdev(cpu);
>>>>> 	switch (action) {
>>>>> 	case CPU_ONLINE:
>>>>> -		topology_add_dev(sys_dev);
>>>>> +		rc = topology_add_dev(sys_dev);
>>>>> 		break;
>>>>> 	case CPU_DEAD:
>>>>> 		topology_remove_dev(sys_dev);
>>>>> 		break;
>>>>> 	}
>>>>> -	return NOTIFY_OK;
>>>>> +	return rc ? NOTIFY_BAD : NOTIFY_OK;
>>>>> }
>>>> Wouldn't that also require that _cpu_up checked the return code when
>>>> doing CPU_ONLINE notification (and clean up on error)?
>>> After all code that gets a CPU_ONLINE notification is not supposed to fail.
>>> For allocating resources while bringing up a cpu CPU_UP_PREPARE is supposed
>>> to be used. That one is allowed to fail.
>> It's a bug no matter how you look at it... I just lessen the impact.  :)
>>
>> If someone wants to provide a better fix, let's see the patch...
> 
> If sysfs_remove_group() would also work for non-created (-existent) groups
> then the patch below would work. Unfortunately that is not the case. So one
> would have to remember if sysfs_create_group() was done and succeeded before
> calling sysfs_remove_group()...
> There must be an easier way.
> 
> diff --git a/drivers/base/topology.c b/drivers/base/topology.c
> index 3ef9d51..d0056c3 100644
> --- a/drivers/base/topology.c
> +++ b/drivers/base/topology.c

ACK

Re: [PATCH] drivers/base: error handling fixes

[Cornelia Huck]

On Thu, 5 Oct 2006 14:48:48 +0200,
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> If sysfs_remove_group() would also work for non-created (-existent) groups
> then the patch below would work. Unfortunately that is not the case. So one
> would have to remember if sysfs_create_group() was done and succeeded before
> calling sysfs_remove_group()...
> There must be an easier way.

<snip>

> @@ -132,11 +135,15 @@ static struct notifier_block __cpuinitda
>  
>  static int __cpuinit topology_sysfs_init(void)
>  {
> -	int i;
> -
> -	for_each_online_cpu(i) {
> -		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
> -				(void *)(long)i);
> +	struct sys_device *sys_dev;
> +	int cpu;
> +	int rc;
> +
> +	for_each_online_cpu(cpu) {
> +		sys_dev = get_cpu_sysdev(cpu);
> +		rc = topology_add_dev(sys_dev);
> +		if (rc)
> +			return rc;
>  	}
>  
>  	register_hotcpu_notifier(&topology_cpu_notifier);

Shouldn't the added attribute groups be removed again in the failure
case?

Also, it might be a bit overkill to fail the whole initialization
because of one "bad" cpu. (And the "bad" cpu wouldn't matter if we
could safely remove non-existent groups :)

Re: [PATCH] drivers/base: error handling fixes

[Heiko Carstens]

> >>If someone wants to provide a better fix, let's see the patch...
> >If sysfs_remove_group() would also work for non-created (-existent) groups
> >then the patch below would work. Unfortunately that is not the case. So one
> >would have to remember if sysfs_create_group() was done and succeeded before
> >calling sysfs_remove_group()...
> >There must be an easier way.
> >diff --git a/drivers/base/topology.c b/drivers/base/topology.c
> >index 3ef9d51..d0056c3 100644
> >--- a/drivers/base/topology.c
> >+++ b/drivers/base/topology.c
> 
> ACK

Ah, you probably got me wrong. The patch doesn't work, because of the
sysfs_remove_group() stuff. That's why there is no Signed-off-by:...

What _could_ happen with this patch applied: some code fails on
CPU_UP_PREPARE and then all notifiers get called with CPU_UP_CANCELLED.

That would cause the call of topology_remove_dev(sys_dev) and therefore
sysfs_remove_group(&sys_dev->kobj, &topology_attr_group) which will crash,
because sysfs_create_group() wasn't even called.
To fix this one has to remember if sysfs_create_group() succeeded or was
even called. That would be a per-cpu array. Now, I think it's just
overkill to introduce a per-cpu array for error-checking.

IMHO it would make sense to change sysfs_remove_group() so it will survive
if being asked to remove groups that don't exist.

Re: [PATCH] drivers/base: error handling fixes

[Heiko Carstens]

> > +	for_each_online_cpu(cpu) {
> > +		sys_dev = get_cpu_sysdev(cpu);
> > +		rc = topology_add_dev(sys_dev);
> > +		if (rc)
> > +			return rc;
> >  	}
> >  
> >  	register_hotcpu_notifier(&topology_cpu_notifier);
> 
> Shouldn't the added attribute groups be removed again in the failure
> case?
> 
> Also, it might be a bit overkill to fail the whole initialization
> because of one "bad" cpu. (And the "bad" cpu wouldn't matter if we
> could safely remove non-existent groups :)

This is initcall stuff. The only sane reason why this would fail, would
be an out of memory situation . If we are that early short on memory, we
are in serious trouble anyway. So I doubt it's worth the extra code.

Re: [PATCH] drivers/base: error handling fixes

[Cornelia Huck]

On Thu, 5 Oct 2006 15:20:06 +0200,
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> This is initcall stuff. The only sane reason why this would fail, would
> be an out of memory situation . If we are that early short on memory, we
> are in serious trouble anyway. So I doubt it's worth the extra code.

OK, at this stage -ENOMEM sounds like the only possiblilty. Agreed.

[patch 1/2] sysfs: allow removal of nonexistent sysfs groups.

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

This patch makes it safe to call sysfs_remove_group() with a name group
that doesn't exist. Needed to make fix cpu hotplug stuff in topology code.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/sysfs/group.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/sysfs/group.c
===================================================================
--- linux-2.6.orig/fs/sysfs/group.c	2006-10-09 09:15:25.000000000 +0200
+++ linux-2.6/fs/sysfs/group.c	2006-10-09 09:25:23.000000000 +0200
@@ -68,9 +68,12 @@
 {
 	struct dentry * dir;
 
-	if (grp->name)
+	if (grp->name) {
+		if (!sysfs_dirent_exist(kobj->dentry->d_fsdata, grp->name))
+			return;
 		dir = lookup_one_len(grp->name, kobj->dentry,
 				strlen(grp->name));
+	}
 	else
 		dir = dget(kobj->dentry);
 

[patch 2/2] cpu topology: various fixes/cleanups

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

- take return valie sysfs_create_group into account.
- don't call code that might fail on CPU_ONLINE notification. Instead call
  it on CPU_UP_PREPARE and check it's return value.
- since CPU_UP_PREPARE might fail add a CPU_UP_CANCELED handling as well.
- use hotcpu_notifier instead of register_hotcpu_notifier
- #ifdef code that isn't needed in the !CONFIG_HOTPLUG_CPU case.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

Could be split up into a bunch of several patches if really needed.

 drivers/base/topology.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/base/topology.c
===================================================================
--- linux-2.6.orig/drivers/base/topology.c	2006-10-09 09:15:27.000000000 +0200
+++ linux-2.6/drivers/base/topology.c	2006-10-09 09:25:05.000000000 +0200
@@ -97,10 +97,10 @@
 /* Add/Remove cpu_topology interface for CPU device */
 static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
 {
-	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
-	return 0;
+	return sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
 }
 
+#ifdef CONFIG_HOTPLUG_CPU
 static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
 {
 	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
@@ -112,34 +112,35 @@
 {
 	unsigned int cpu = (unsigned long)hcpu;
 	struct sys_device *sys_dev;
+	int rc = 0;
 
 	sys_dev = get_cpu_sysdev(cpu);
 	switch (action) {
-	case CPU_ONLINE:
-		topology_add_dev(sys_dev);
+	case CPU_UP_PREPARE:
+		rc = topology_add_dev(sys_dev);
 		break;
+	case CPU_UP_CANCELED:
 	case CPU_DEAD:
 		topology_remove_dev(sys_dev);
 		break;
 	}
-	return NOTIFY_OK;
+	return rc ? NOTIFY_BAD : NOTIFY_OK;
 }
-
-static struct notifier_block __cpuinitdata topology_cpu_notifier =
-{
-	.notifier_call = topology_cpu_callback,
-};
+#endif
 
 static int __cpuinit topology_sysfs_init(void)
 {
-	int i;
+	struct sys_device *sys_dev;
+	int cpu;
+	int rc;
 
-	for_each_online_cpu(i) {
-		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
-				(void *)(long)i);
+	for_each_online_cpu(cpu) {
+		sys_dev = get_cpu_sysdev(cpu);
+		rc = topology_add_dev(sys_dev);
+		if (rc)
+			return rc;
 	}
-
-	register_hotcpu_notifier(&topology_cpu_notifier);
+	hotcpu_notifier(topology_cpu_callback, 0);
 
 	return 0;
 }

Re: [patch 1/2] sysfs: allow removal of nonexistent sysfs groups.

[Andrew Morton]

On Mon, 9 Oct 2006 09:29:20 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> This patch makes it safe to call sysfs_remove_group() with a name group
> that doesn't exist. Needed to make fix cpu hotplug stuff in topology code.
> 

Surely an attempt to remove a non-existent entry is a bug, and this
(racy-looking) patch just covers that up?

> ---
>  fs/sysfs/group.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/fs/sysfs/group.c
> ===================================================================
> --- linux-2.6.orig/fs/sysfs/group.c	2006-10-09 09:15:25.000000000 +0200
> +++ linux-2.6/fs/sysfs/group.c	2006-10-09 09:25:23.000000000 +0200
> @@ -68,9 +68,12 @@
>  {
>  	struct dentry * dir;
>  
> -	if (grp->name)
> +	if (grp->name) {
> +		if (!sysfs_dirent_exist(kobj->dentry->d_fsdata, grp->name))
> +			return;
>  		dir = lookup_one_len(grp->name, kobj->dentry,
>  				strlen(grp->name));
> +	}
>  	else
>  		dir = dget(kobj->dentry);
>  

Re: [patch 1/2] sysfs: allow removal of nonexistent sysfs groups.

[Heiko Carstens]

On Mon, Oct 09, 2006 at 12:40:14AM -0700, Andrew Morton wrote:
> On Mon, 9 Oct 2006 09:29:20 +0200
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > This patch makes it safe to call sysfs_remove_group() with a name group
> > that doesn't exist. Needed to make fix cpu hotplug stuff in topology code.
> > 
> 
> Surely an attempt to remove a non-existent entry is a bug, and this
> (racy-looking) patch just covers that up?

It just tries to keep cpu hotplug code in drivers/base/topology.c simple. Since
otherwise one would have to remember if sysfs_create_group() succeeded or not.
Hmm.. thinking again, this patch looks indeed racy.

> > Index: linux-2.6/fs/sysfs/group.c
> > ===================================================================
> > --- linux-2.6.orig/fs/sysfs/group.c	2006-10-09 09:15:25.000000000 +0200
> > +++ linux-2.6/fs/sysfs/group.c	2006-10-09 09:25:23.000000000 +0200
> > @@ -68,9 +68,12 @@
> >  {
> >  	struct dentry * dir;
> >  
> > -	if (grp->name)
> > +	if (grp->name) {
> > +		if (!sysfs_dirent_exist(kobj->dentry->d_fsdata, grp->name))
> > +			return;
> >  		dir = lookup_one_len(grp->name, kobj->dentry,
> >  				strlen(grp->name));
> > +	}
> >  	else
> >  		dir = dget(kobj->dentry);
> >  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/