Skip to contentSkip to conversation

device_initialize()

RFC In review Greg KH 3 participants 4 messages Oct 14, 2002 – Oct 15, 2002

Subsystems (7)
USB PCI Driver core USB ehci driver +3 more subsystems
lore
Share & tools
lore

Greg KH

author
#0 lore raw

RFC 1/1 device_initialize()

Hi,

In working to clean up the usb device reference counting logic, I moved all of the calls to usb_get_dev() and usb_put_dev() to get_device() and put_device() as it does not make sense to have two reference counts in the same structure. But when doing that, I realized that the USB core accesses devices before it calls device_register(). And calling get_device() and put_device() before device_register() is called is not a good thing :)

So here's a patch that adds a device_initialize() call, that can be done when the device structure is created. At that point, get_device() and put_device() can be successfully called, as the USB core needs, and at a later time, device_register() can be called. It also replaces the present flag with a device state, allowing 3 valid states for a device (initialized, registered, and gone).

I know Al needs much the same thing for his gendisk cleanup, and has implemented almost the same patch as I have, but I think mine solves his problem of having to manipulate the device refcount variable himself.

Anyway, here's a patch against 2.5.42 that adds device_initialize() and fixes up the USB core to use it. It works for me, but I haven't stress tested it much (with lots of device removals.) Any comments would be appreciated.

thanks,

greg k-h

===== drivers/base/core.c 1.40 vs edited =====

drivers/base/core.c +32 -14
@@ -150,6 +150,27 @@
 }
 
 /**
+ * device_initialize - initialize a device
+ * @dev:	pointer to the device structure
+ */
+int device_initialize (struct device *dev)
+{
+	if (!dev)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&dev->node);
+	INIT_LIST_HEAD(&dev->children);
+	INIT_LIST_HEAD(&dev->g_list);
+	INIT_LIST_HEAD(&dev->driver_list);
+	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->intf_list);
+	spin_lock_init(&dev->lock);
+	atomic_set(&dev->refcount,1);
+	dev->state = DEVICE_INITIALIZED;
+	return 0;
+}
+
+/**
  * device_register - register a device
  * @dev:	pointer to the device structure
  *
@@ -167,15 +188,10 @@
 	if (!dev || !strlen(dev->bus_id))
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&dev->node);
-	INIT_LIST_HEAD(&dev->children);
-	INIT_LIST_HEAD(&dev->g_list);
-	INIT_LIST_HEAD(&dev->driver_list);
-	INIT_LIST_HEAD(&dev->bus_list);
-	INIT_LIST_HEAD(&dev->intf_list);
-	spin_lock_init(&dev->lock);
-	atomic_set(&dev->refcount,2);
-	dev->present = 1;
+	if (dev->state != DEVICE_INITIALIZED)
+		return -EINVAL;
+
+	get_device(dev);
 	spin_lock(&device_lock);
 	if (dev->parent) {
 		get_device_locked(dev->parent);
@@ -212,6 +228,7 @@
 		if (dev->parent)
 			put_device(dev->parent);
 	}
+	dev->state = DEVICE_INITIALIZED;
 	put_device(dev);
 	return error;
 }
@@ -219,7 +236,9 @@
 struct device * get_device_locked(struct device * dev)
 {
 	struct device * ret = dev;
-	if (dev && dev->present && atomic_read(&dev->refcount) > 0)
+	if (dev && 
+	    dev->state != DEVICE_GONE && 
+	    atomic_read(&dev->refcount) > 0)
 		atomic_inc(&dev->refcount);
 	else
 		ret = NULL;
@@ -248,8 +267,6 @@
 	dev->parent = NULL;
 	spin_unlock(&device_lock);
 
-	BUG_ON(dev->present);
-
 	if (dev->release)
 		dev->release(dev);
 
@@ -263,13 +280,13 @@
  *
  * The device has been removed from the system, so we disavow knowledge
  * of it. It might not be the final reference to the device, so we mark
- * it as !present, so no more references to it can be acquired.
+ * it as gone, so no more references to it can be acquired.
  * In the end, we decrement the final reference count for it.
  */
 void device_unregister(struct device * dev)
 {
 	spin_lock(&device_lock);
-	dev->present = 0;
+	dev->state = DEVICE_GONE;
 	list_del_init(&dev->node);
 	list_del_init(&dev->g_list);
 	list_del_init(&dev->bus_list);
@@ -309,6 +326,7 @@
 
 core_initcall(device_init);
 
+EXPORT_SYMBOL(device_initialize);
 EXPORT_SYMBOL(device_register);
 EXPORT_SYMBOL(device_unregister);
 EXPORT_SYMBOL(get_device);

===== drivers/base/platform.c 1.2 vs edited =====

drivers/base/platform.c +2 -0
@@ -33,6 +33,7 @@
 
 	pr_debug("Registering platform device '%s'. Parent at %s\n",
 		 pdev->dev.bus_id,pdev->dev.parent->bus_id);
+	device_initialize(&pdev->dev);
 	return device_register(&pdev->dev);
 }
 
@@ -54,6 +55,7 @@
 
 static int __init platform_bus_init(void)
 {
+	device_initialize(&legacy_bus);
 	device_register(&legacy_bus);
 	return bus_register(&platform_bus_type);
 }

===== drivers/base/sys.c 1.4 vs edited =====

drivers/base/sys.c +4 -0
@@ -55,11 +55,13 @@
 
 	pr_debug("Registering system board %d\n",root->id);
 
+	device_initialize(&root->dev);
 	error = device_register(&root->dev);
 	if (!error) {
 		strncpy(root->sysdev.bus_id,"sys",BUS_ID_SIZE);
 		strncpy(root->sysdev.name,"System Bus",DEVICE_NAME_SIZE);
 		root->sysdev.parent = &root->dev;
+		device_initialize(&root->sysdev);
 		error = device_register(&root->sysdev);
 	};
 
@@ -119,6 +121,7 @@
 
 	pr_debug("Registering system device %s\n", sysdev->dev.bus_id);
 
+	device_initialize(&sysdev->dev);
 	return device_register(&sysdev->dev);
 }
 
@@ -135,6 +138,7 @@
 static int sys_bus_init(void)
 {
 	bus_register(&system_bus_type);
+	device_initialize(&system_bus);
 	return device_register(&system_bus);
 }
 

===== drivers/ide/ide-probe.c 1.10 vs edited =====

drivers/ide/ide-probe.c +4 -1
@@ -583,6 +583,7 @@
 		hwif->gendev.parent = &hwif->pci_dev->dev;
 	else
 		hwif->gendev.parent = NULL; /* Would like to do = &device_legacy */
+	device_initialize(&hwif->gendev);
 	device_register(&hwif->gendev);
 
 	if (hwif->mmio == 2)
@@ -1021,8 +1022,10 @@
 			 "%s","IDE Drive");
 		drive->gendev.parent = &hwif->gendev;
 		drive->gendev.bus = &ide_bus_type;
-		if (drive->present)
+		if (drive->present) {
+			device_initialize(&drive->gendev);
 			device_register(&drive->gendev);
+		}
 	}
 
 	return;

===== drivers/pci/probe.c 1.10 vs edited =====

drivers/pci/probe.c +2 -0
@@ -450,6 +450,7 @@
 	strcpy(dev->dev.name,dev->name);
 	strcpy(dev->dev.bus_id,dev->slot_name);
 
+	device_initialize(&dev->dev);
 	device_register(&dev->dev);
 	return dev;
 }
@@ -568,6 +569,7 @@
 	memset(b->dev,0,sizeof(*(b->dev)));
 	sprintf(b->dev->bus_id,"pci%d",bus);
 	strcpy(b->dev->name,"Host/PCI Bridge");
+	device_initialize(b->dev);
 	device_register(b->dev);
 
 	b->number = b->secondary = bus;

===== drivers/usb/core/config.c 1.8 vs edited =====

drivers/usb/core/config.c +1 -0
@@ -99,6 +99,7 @@
 	interface->act_altsetting = 0;
 	interface->num_altsetting = 0;
 	interface->max_altsetting = USB_ALTSETTINGALLOC;
+	device_initialize (&interface->dev);
 
 	interface->altsetting = kmalloc(sizeof(struct usb_interface_descriptor) * interface->max_altsetting, GFP_KERNEL);
 	

===== drivers/usb/core/hcd.c 1.66 vs edited =====

drivers/usb/core/hcd.c +2 -2
@@ -968,7 +968,7 @@
 	list_del_init (&urb->urb_list);
 	dev = urb->dev;
 	urb->dev = NULL;
-	usb_put_dev (dev);
+	put_device (&dev->dev);
 	spin_unlock_irqrestore (&hcd_data_lock, flags);
 }
 
@@ -1007,7 +1007,7 @@
 
 	spin_lock_irqsave (&hcd_data_lock, flags);
 	if (HCD_IS_RUNNING (hcd->state) && hcd->state != USB_STATE_QUIESCING) {
-		usb_get_dev (urb->dev);
+		get_device (&urb->dev->dev);
 		list_add (&urb->urb_list, &dev->urb_list);
 		status = 0;
 	} else {

===== drivers/usb/core/hub.c 1.65 vs edited =====

drivers/usb/core/hub.c +2 -2
@@ -848,7 +848,7 @@
 
 		/* Reset the device, and detect its speed */
 		if (usb_hub_port_reset(hub, port, dev, delay)) {
-			usb_free_dev(dev);
+			put_device(&dev->dev);
 			break;
 		}
 
@@ -898,7 +898,7 @@
 			goto done;
 
 		/* Free the configuration if there was an error */
-		usb_free_dev(dev);
+		put_device(&dev->dev);
 
 		/* Switch to a long reset time */
 		delay = HUB_LONG_RESET_TIME;

===== drivers/usb/core/usb.c 1.163 vs edited =====

drivers/usb/core/usb.c +25 -44

114 lines not on this page · raw message

===== drivers/usb/host/ehci-hcd.c 1.50 vs edited =====

drivers/usb/host/ehci-hcd.c +1 -1
@@ -456,7 +456,7 @@
 			ehci_ready (ehci);
 		ehci_reset (ehci);
 		bus->root_hub = 0;
-		usb_free_dev (udev); 
+		put_device (&udev->dev); 
 		retval = -ENODEV;
 		goto done2;
 	}

===== drivers/usb/host/hc_sl811_rh.c 1.3 vs edited =====

drivers/usb/host/hc_sl811_rh.c +1 -1
@@ -566,7 +566,7 @@
 	hci->bus->root_hub = usb_dev;
 	usb_connect (usb_dev);
 	if (usb_new_device (usb_dev) != 0) {
-		usb_free_dev (usb_dev);
+		put_device (&usb_dev->dev);
 		return -ENODEV;
 	}
 

===== drivers/usb/host/ohci-hcd.c 1.45 vs edited =====

drivers/usb/host/ohci-hcd.c +1 -1
@@ -509,7 +509,7 @@
 	usb_connect (udev);
 	udev->speed = USB_SPEED_FULL;
 	if (usb_register_root_hub (udev, ohci->parent_dev) != 0) {
-		usb_free_dev (udev);
+		put_device (&udev->dev);
 		ohci->hcd.self.root_hub = NULL;
 		disable (ohci);
 		ohci->hc_control &= ~OHCI_CTRL_HCFS;

===== drivers/usb/host/uhci-hcd.c 1.22 vs edited =====

drivers/usb/host/uhci-hcd.c +5 -5
@@ -161,7 +161,7 @@
 	INIT_LIST_HEAD(&td->list);
 	INIT_LIST_HEAD(&td->fl_list);
 
-	usb_get_dev(dev);
+	get_device(&dev->dev);
 
 	return td;
 }
@@ -315,7 +315,7 @@
 		dbg("td %p is still in fl_list!", td);
 
 	if (td->dev)
-		usb_put_dev(td->dev);
+		put_device(&td->dev->dev);
 
 	pci_pool_free(uhci->td_pool, td, td->dma_handle);
 }
@@ -340,7 +340,7 @@
 	INIT_LIST_HEAD(&qh->list);
 	INIT_LIST_HEAD(&qh->remove_list);
 
-	usb_get_dev(dev);
+	get_device(&dev->dev);
 
 	return qh;
 }
@@ -353,7 +353,7 @@
 		dbg("qh %p still in remove_list!", qh);
 
 	if (qh->dev)
-		usb_put_dev(qh->dev);
+		put_device(&qh->dev->dev);
 
 	pci_pool_free(uhci->qh_pool, qh, qh->dma_handle);
 }
@@ -2328,7 +2328,7 @@
 		}
 
 err_alloc_skeltd:
-	usb_free_dev(udev);
+	put_device(&udev->dev);
 	hcd->self.root_hub = NULL;
 
 err_alloc_root_hub:

===== drivers/usb/media/usbvideo.c 1.34 vs edited =====

drivers/usb/media/usbvideo.c +2 -2
@@ -991,7 +991,7 @@
 	for (i=0; i < USBVIDEO_NUMSBUF; i++)
 		usb_free_urb(uvd->sbuf[i].urb);
 
-	usb_put_dev(uvd->dev);
+	put_device(&uvd->dev->dev);
 	uvd->dev = NULL;    	    /* USB device is no more */
 
 	video_unregister_device(&uvd->vdev);
@@ -1195,7 +1195,7 @@
 	}
 #endif
 
-	usb_get_dev(uvd->dev);
+	get_device(&uvd->dev->dev);
 	return 0;
 }
 

===== drivers/usb/net/cdc-ether.c 1.24 vs edited =====

drivers/usb/net/cdc-ether.c +2 -2
@@ -1257,7 +1257,7 @@
 	                            ether_dev );
 
 	// Does this REALLY do anything???
-	usb_get_dev( usb );
+	get_device(&usb->dev);
 
 	// TODO - last minute HACK
 	ether_dev->comm_ep_in = 5;
@@ -1316,7 +1316,7 @@
 	ether_dev->net = NULL;
 
 	// I ask again, does this do anything???
-	usb_put_dev( usb );
+	put_device(&usb->dev);
 
 	// We are done with this interface
 	usb_driver_release_interface( &CDCEther_driver, 

===== drivers/usb/net/pegasus.c 1.48 vs edited =====

drivers/usb/net/pegasus.c +2 -2
@@ -1062,7 +1062,7 @@
 		return -ENOMEM;
 	}
 
-	usb_get_dev(dev);
+	get_device(&dev->dev);
 	memset(pegasus, 0, sizeof(struct pegasus));
 	pegasus->dev_index = dev_index;
 	init_waitqueue_head(&pegasus->ctrl_wait);
@@ -1142,7 +1142,7 @@
 
 	pegasus->flags |= PEGASUS_UNPLUG;
 	unregister_netdev(pegasus->net);
-	usb_put_dev(interface_to_usbdev(intf));
+	put_device(&interface_to_usbdev(intf)->dev);
 	unlink_all_urbs(pegasus);
 	free_all_urbs(pegasus);
 	free_skb_pool(pegasus);

===== drivers/usb/net/usbnet.c 1.49 vs edited =====

drivers/usb/net/usbnet.c +2 -2
@@ -1968,7 +1968,7 @@
 	flush_scheduled_work ();
 
 	kfree (dev);
-	usb_put_dev (xdev);
+	put_device (&xdev->dev);
 }
 
 
@@ -2006,7 +2006,7 @@
 	memset (dev, 0, sizeof *dev);
 
 	init_MUTEX_LOCKED (&dev->mutex);
-	usb_get_dev (xdev);
+	get_device (&xdev->dev);
 	dev->udev = xdev;
 	dev->driver_info = info;
 	dev->msg_level = msg_level;

===== drivers/usb/storage/usb.c 1.48 vs edited =====

drivers/usb/storage/usb.c +3 -3
@@ -624,7 +624,7 @@
 
 	/* mark the device as gone */
 	ss->flags &= ~ US_FL_DEV_ATTACHED;
-	usb_put_dev(ss->pusb_dev);
+	put_device(&ss->pusb_dev->dev);
 	ss->pusb_dev = NULL;
 }
 
@@ -747,7 +747,7 @@
 	}
 
 	/* At this point, we've decided to try to use the device */
-	usb_get_dev(dev);
+	get_device(&dev->dev);
 
 	/* clear the GUID and fetch the strings */
 	GUID_CLEAR(guid);
@@ -821,7 +821,7 @@
 		if ((ss = (struct us_data *)kmalloc(sizeof(struct us_data), 
 						    GFP_KERNEL)) == NULL) {
 			printk(KERN_WARNING USB_STORAGE "Out of memory\n");
-			usb_put_dev(dev);
+			put_device(&dev->dev);
 			return -ENOMEM;
 		}
 		memset(ss, 0, sizeof(struct us_data));

===== fs/partitions/check.c 1.29 vs edited =====

fs/partitions/check.c +3 -0
@@ -168,6 +168,7 @@
 	dev->parent = parent;
 	if (parent)
 		dev->bus = parent->bus;
+	device_initialize(dev);
 	device_register(dev);
 	device_create_file(dev, &dev_attr_type);
 	device_create_file(dev, &dev_attr_kdev);
@@ -179,6 +180,7 @@
 			dev->bus = parent->bus;
 		if (!dev->driver_data)
 			continue;
+		device_initialize(dev);
 		device_register(dev);
 		device_create_file(dev, &dev_attr_type);
 		device_create_file(dev, &dev_attr_kdev);
@@ -441,6 +443,7 @@
 		return;
 	dev->driver_data =
 		(void *)(long)__mkdev(disk->major, disk->first_minor+part);
+	device_initialize(dev);
 	device_register(dev);
 	device_create_file(dev, &dev_attr_type);
 	device_create_file(dev, &dev_attr_kdev);

===== include/linux/device.h 1.34 vs edited =====

include/linux/device.h +7 -2
@@ -256,7 +256,11 @@
 
 extern int interface_add_data(struct intf_data *);
 
-
+enum device_state {
+	DEVICE_INITIALIZED =	1,
+	DEVICE_REGISTERED =	2,
+	DEVICE_GONE =		3,
+};
 
 struct device {
 	struct list_head g_list;        /* node in depth-first order list */
@@ -289,7 +293,7 @@
 	void		*platform_data;	/* Platform specific data (e.g. ACPI,
 					   BIOS data relevant to device) */
 
-	u32		present;
+	enum device_state state;
 	u32		current_state;  /* Current operating state. In
 					   ACPI-speak, this is D0-D3, D0
 					   being fully functional, and D3
@@ -328,6 +332,7 @@
  * High level routines for use by the bus drivers
  */
 extern int device_register(struct device * dev);
+extern int device_initialize(struct device * dev);
 extern void device_unregister(struct device * dev);
 
 /* driverfs interface for exporting device attributes */

===== include/linux/usb.h 1.101 vs edited =====

include/linux/usb.h +3 -3
@@ -426,9 +426,9 @@
 #define	to_usb_device(d) container_of(d, struct usb_device, dev)
 
 extern struct usb_device *usb_alloc_dev(struct usb_device *parent, struct usb_bus *);
-extern struct usb_device *usb_get_dev(struct usb_device *dev);
-extern void usb_free_dev(struct usb_device *);
-#define usb_put_dev usb_free_dev
+//extern struct usb_device *usb_get_dev(struct usb_device *dev);
+//extern void usb_free_dev(struct usb_device *);
+//#define usb_put_dev usb_free_dev
 
 /* mostly for devices emulating SCSI over USB */
 extern int usb_reset_device(struct usb_device *dev);
author
#1 lore raw
Quoted 25 lines · Greg KH
drivers/base/core.c +5 -9
+/**
  * device_register - register a device
  * @dev:	pointer to the device structure
  *
@@ -167,15 +188,10 @@
 	if (!dev || !strlen(dev->bus_id))
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&dev->node);
-	INIT_LIST_HEAD(&dev->children);
-	INIT_LIST_HEAD(&dev->g_list);
-	INIT_LIST_HEAD(&dev->driver_list);
-	INIT_LIST_HEAD(&dev->bus_list);
-	INIT_LIST_HEAD(&dev->intf_list);
-	spin_lock_init(&dev->lock);
-	atomic_set(&dev->refcount,2);
-	dev->present = 1;
+	if (dev->state != DEVICE_INITIALIZED)
+		return -EINVAL;
+
+	get_device(dev);
 	spin_lock(&device_lock);
 	if (dev->parent) {
 		get_device_locked(dev->parent);

drivers/base/core.c line 231

Quoted 4 lines
drivers/base/core.c +1 -0
@@ -212,6 +228,7 @@
 		if (dev->parent)
 			put_device(dev->parent);
 	}
+	dev->state = DEVICE_INITIALIZED;

As someone just kindly pointed out to me, this should be DEVICE_REGISTERED. Sorry about that.

thanks,

greg k-h

#2 lore raw
In reply to Greg KH

Hey there.

Quoted 64 lines

===== drivers/base/core.c 1.40 vs edited =====

drivers/base/core.c +26 -9
@@ -150,6 +150,27 @@
 }
 
 /**
+ * device_initialize - initialize a device
+ * @dev:	pointer to the device structure
+ */
+int device_initialize (struct device *dev)
+{
+	if (!dev)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&dev->node);
+	INIT_LIST_HEAD(&dev->children);
+	INIT_LIST_HEAD(&dev->g_list);
+	INIT_LIST_HEAD(&dev->driver_list);
+	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->intf_list);
+	spin_lock_init(&dev->lock);
+	atomic_set(&dev->refcount,1);
+	dev->state = DEVICE_INITIALIZED;
+	return 0;
+}
+
+/**
  * device_register - register a device
  * @dev:	pointer to the device structure
  *
@@ -167,15 +188,10 @@
 	if (!dev || !strlen(dev->bus_id))
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&dev->node);
-	INIT_LIST_HEAD(&dev->children);
-	INIT_LIST_HEAD(&dev->g_list);
-	INIT_LIST_HEAD(&dev->driver_list);
-	INIT_LIST_HEAD(&dev->bus_list);
-	INIT_LIST_HEAD(&dev->intf_list);
-	spin_lock_init(&dev->lock);
-	atomic_set(&dev->refcount,2);
-	dev->present = 1;
+	if (dev->state != DEVICE_INITIALIZED)
+		return -EINVAL;
+
+	get_device(dev);
 	spin_lock(&device_lock);
 	if (dev->parent) {
 		get_device_locked(dev->parent);
@@ -212,6 +228,7 @@
 		if (dev->parent)
 			put_device(dev->parent);
 	}
+	dev->state = DEVICE_INITIALIZED;
 	put_device(dev);
 	return error;
 }

===== include/linux/device.h 1.34 vs edited =====

include/linux/device.h line 263

Quoted 9 lines
include/linux/device.h +5 -1
@@ -256,7 +256,11 @@
 
 extern int interface_add_data(struct intf_data *);
 
-
+enum device_state {
+	DEVICE_INITIALIZED =	1,
+	DEVICE_REGISTERED =	2,
+	DEVICE_GONE =		3,
+};

Overall, I agree with the concept of the patch. I wonder though if we could do it without burdening all the callers of device_register() to first call device_initialize(). Well, tastefully at least.

What about:

enum device_state {

	DEVICE_UNINITIALIZED =  0,
	DEVICE_INITIALIZED =    1,
	DEVICE_REGISTERED =     2,   
	DEVICE_GONE =           3,

};

int device_register(struct device * dev)

{
	...
	if (dev->state == DEVICE_UNINITIALIZED)
		device_initialize(dev);
	...
}

This assumes that the device structure is initialized to 0 before device_register() is called (which it should be anyway to prevent other unpredictable behavior).

Is that too ugly?

-pat

#3 lore raw
Quoted 3 lines · Patrick Mochel

Overall, I agree with the concept of the patch. I wonder though if we could do it without burdening all the callers of device_register() to first call device_initialize(). Well, tastefully at least.

I'd rather add device_add() and make device_register() call device_initialize() followed by device_add()...

BTW, my variant of the same beast is on
ftp.math.psu.edu/pub/viro/O/O142-device_add-C42
with uses in O14[3-6]* in the same place...