struct device::release issue

struct device::release issue

[Frank A. Uepping]

Hi,
if device_add fails (e.g. bus_add_device returns an error) then the release 
method will be called for the device. Is this a bug or a feature?

/FAU

Re: struct device::release issue

[Greg KH]

On Sat, Mar 06, 2004 at 12:47:24PM +0100, Frank A. Uepping wrote:
> Hi,
> if device_add fails (e.g. bus_add_device returns an error) then the release 
> method will be called for the device. Is this a bug or a feature?

Are you sure this will happen?  device_initialize() gets a reference
that is still present after device_add() fails, right?  So release()
will not get called.

thanks,

greg k-h

Re: struct device::release issue

[Frank A. Uepping]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Saturday 27 March 2004 02:14, Greg KH wrote:
> On Sat, Mar 06, 2004 at 12:47:24PM +0100, Frank A. Uepping wrote:
> > Hi,
> > if device_add fails (e.g. bus_add_device returns an error) then the release 
> > method will be called for the device. Is this a bug or a feature?
> 
> Are you sure this will happen?  device_initialize() gets a reference
> that is still present after device_add() fails, right?  So release()
> will not get called.
At the label PMError, kobject_unregister is called, which decrements the
recount by 2, which will result in calling release at label Done (put_device).

kobject_unregister should be superseded by kobject_del.
Here is a patch:

--- linux-2.6.4/drivers/base/core.c.orig        Sat Mar 13 10:33:40 2004
+++ linux-2.6.4/drivers/base/core.c     Sat Mar 27 15:04:27 2004
@@ -249,7 +249,7 @@
  BusError:
        device_pm_remove(dev);
  PMError:
-       kobject_unregister(&dev->kobj);
+       kobject_del(&dev->kobj);
  Error:
        if (parent)
                put_device(parent);


I have attached a simple module, which demonstrates the behavior.

/FAU

[-- Attachment #2: device_add_bug.c --]
[-- Type: text/x-csrc, Size: 1839 bytes --]

#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/device.h>
#include <../drivers/base/power/power.h>


#define DBG(format, arg...) do { printk(KERN_DEBUG "foo: %s(): " format "\n", __FUNCTION__ , ## arg); } while (0)


int device_add(struct device *dev)  // Defined in drivers/base/core.c
{
	struct device * parent;
	int error;

	DBG("1. refcount=%d", atomic_read(&dev->kobj.refcount));

	dev = get_device(dev);
	if (!dev || !strlen(dev->bus_id))
		return -EINVAL;

	DBG("2. refcount=%d", atomic_read(&dev->kobj.refcount));

	parent = get_device(dev->parent);

	pr_debug("DEV: registering device: ID = '%s'\n", dev->bus_id);

	/* first, register with generic layer. */
	kobject_set_name(&dev->kobj,dev->bus_id);
	if (parent)
		dev->kobj.parent = &parent->kobj;

	if ((error = kobject_add(&dev->kobj)))
		goto Error;

	DBG("3. refcount=%d", atomic_read(&dev->kobj.refcount));

	// e.g. device_pm_add failed
	error = -EBUSY;
	goto PMError;
/*
	if ((error = device_pm_add(dev)))
		goto PMError;
	if ((error = bus_add_device(dev)))
		goto BusError;
	down_write(&devices_subsys.rwsem);
	if (parent)
		list_add_tail(&dev->node,&parent->children);
	up_write(&devices_subsys.rwsem);

	if (platform_notify)
		platform_notify(dev);
*/
 Done:
	put_device(dev);
	DBG("5. refcount=%d", atomic_read(&dev->kobj.refcount));
	return error;
 BusError:
	device_pm_remove(dev);
 PMError:
	kobject_unregister(&dev->kobj);
	DBG("4. refcount=%d", atomic_read(&dev->kobj.refcount));
 Error:
	if (parent)
		put_device(parent);
	goto Done;
}


void foo_release(struct device* dev)
{
	DBG("XXX: called even if device_add fails?");
}


struct device foo_dev = {
	.bus_id		= "bus",
	.release	= foo_release
};


int __init foo_init(void)
{
	device_initialize(&foo_dev);
	return device_add(&foo_dev);
}


module_init(foo_init);

[-- Attachment #3: Makefile --]
[-- Type: text/x-makefile, Size: 146 bytes --]

obj-m += device_add_bug.o
KDIR := /lib/modules/$(shell uname -r)/build
PWD := $(shell pwd)

default:
	$(MAKE) -C $(KDIR) SUBDIRS="$(PWD)" modules

Re: struct device::release issue

[Greg KH]

On Sat, Mar 27, 2004 at 03:26:25PM +0100, Frank A. Uepping wrote:
> On Saturday 27 March 2004 02:14, Greg KH wrote:
> > On Sat, Mar 06, 2004 at 12:47:24PM +0100, Frank A. Uepping wrote:
> > > Hi,
> > > if device_add fails (e.g. bus_add_device returns an error) then the release 
> > > method will be called for the device. Is this a bug or a feature?
> > 
> > Are you sure this will happen?  device_initialize() gets a reference
> > that is still present after device_add() fails, right?  So release()
> > will not get called.
> At the label PMError, kobject_unregister is called, which decrements the
> recount by 2, which will result in calling release at label Done (put_device).
> 
> kobject_unregister should be superseded by kobject_del.
> Here is a patch:

Sorry for the long delay.  You are correct, I've applied this patch.  It
will show up in the next -mm release and will go to Linus after 2.6.7 is
out.

thanks,

greg k-h