Problem with new driver model?

Problem with new driver model?

[Paul Fulghum]

When testing the drivers I maintain on 2.5.20, I hit the
BUG_ON in include/linux/devices.txt:115.

This is in get_driver() which is called in a chain from:
driver_for_each_dev()
driver_unbind()
do_driver_unbind()
free_module()
sys_delete_module()

This occurs when unloading the modules (rmmod).
This was not the case on 2.5.14 (the last kernel I tested).

The machine is an SMP dual PentiumII-400.

A reading of Documentation/driver-model.txt indicates that
changes were made to the global driver model that should not require
changes to individual device specific drivers.

Is the documentation correct and this is a bug with the
new driver model code, or are there changes required for
all the device specific drivers.

Thanks,

Paul Fulghum
paulkf@microgate.com

Re: Problem with new driver model?

[Greg KH]

On Wed, Jun 05, 2002 at 03:25:08PM -0500, Paul Fulghum wrote:
> When testing the drivers I maintain on 2.5.20, I hit the
> BUG_ON in include/linux/devices.txt:115.

See:
	http://marc.theaimsgroup.com/?l=linux-kernel&m=102316813812289&w=2

for a fix for this problem.

thanks,

greg k-h

Re: Problem with new driver model?

[Patrick Mochel]

On 5 Jun 2002, Paul Fulghum wrote:

> When testing the drivers I maintain on 2.5.20, I hit the
> BUG_ON in include/linux/devices.txt:115.

There is a patch that should fix this in Linus's tree. 

If you're using bitkeeper, you can pull it from 
linux.bkbits.net/linux-2.5. 

If not, it's appended here.

	-pat

diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Wed Jun  5 13:43:20 2002
+++ b/drivers/base/core.c	Wed Jun  5 13:43:20 2002
@@ -5,6 +5,8 @@
  *		 2002 Open Source Development Lab
  */
 
+#define DEBUG 0
+
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -118,9 +120,8 @@
 	return bus_for_each_dev(drv->bus,drv,do_driver_bind);
 }
 
-static int do_driver_unbind(struct device * dev, void * data)
+static int do_driver_unbind(struct device * dev, struct device_driver * drv)
 {
-	struct device_driver * drv = (struct device_driver *)data;
 	lock_device(dev);
 	if (dev->driver == drv) {
 		dev->driver = NULL;
@@ -134,7 +135,31 @@
 
 void driver_unbind(struct device_driver * drv)
 {
-	driver_for_each_dev(drv,drv,do_driver_unbind);
+	struct device * next;
+	struct device * dev = NULL;
+	struct list_head * node;
+	int error = 0;
+
+	read_lock(&drv->lock);
+	node = drv->devices.next;
+	while (node != &drv->devices) {
+		next = list_entry(node,struct device,driver_list);
+		get_device(next);
+		read_unlock(&drv->lock);
+
+		if (dev)
+			put_device(dev);
+		dev = next;
+		if ((error = do_driver_unbind(dev,drv))) {
+			put_device(dev);
+			break;
+		}
+		read_lock(&drv->lock);
+		node = dev->driver_list.next;
+	}
+	read_unlock(&drv->lock);
+	if (dev)
+		put_device(dev);
 }
 
 /**
@@ -178,8 +203,8 @@
 	}
 	spin_unlock(&device_lock);
 
-	DBG("DEV: registering device: ID = '%s', name = %s\n",
-	    dev->bus_id, dev->name);
+	pr_debug("DEV: registering device: ID = '%s', name = %s\n",
+		 dev->bus_id, dev->name);
 
 	if ((error = device_make_dir(dev)))
 		goto register_done;
@@ -212,8 +237,8 @@
 	list_del_init(&dev->g_list);
 	spin_unlock(&device_lock);
 
-	DBG("DEV: Unregistering device. ID = '%s', name = '%s'\n",
-	    dev->bus_id,dev->name);
+	pr_debug("DEV: Unregistering device. ID = '%s', name = '%s'\n",
+		 dev->bus_id,dev->name);
 
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
diff -Nru a/drivers/base/driver.c b/drivers/base/driver.c
--- a/drivers/base/driver.c	Wed Jun  5 13:43:20 2002
+++ b/drivers/base/driver.c	Wed Jun  5 13:43:20 2002
@@ -3,6 +3,8 @@
  *
  */
 
+#define DEBUG 0
+
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/errno.h>
@@ -67,6 +69,7 @@
 	get_bus(drv->bus);
 	atomic_set(&drv->refcount,2);
 	rwlock_init(&drv->lock);
+	INIT_LIST_HEAD(&drv->devices);
 	write_lock(&drv->bus->lock);
 	list_add(&drv->bus_list,&drv->bus->drivers);
 	write_unlock(&drv->bus->lock);
@@ -76,16 +79,8 @@
 	return 0;
 }
 
-/**
- * put_driver - decrement driver's refcount and clean up if necessary
- * @drv:	driver in question
- */
-void put_driver(struct device_driver * drv)
+static void __remove_driver(struct device_driver * drv)
 {
-	if (!atomic_dec_and_lock(&drv->refcount,&device_lock))
-		return;
-	spin_unlock(&device_lock);
-
 	if (drv->bus) {
 		pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
 
@@ -101,6 +96,28 @@
 		drv->release(drv);
 }
 
+void remove_driver(struct device_driver * drv)
+{
+	spin_lock(&device_lock);
+	atomic_set(&drv->refcount,0);
+	spin_unlock(&device_lock);
+	__remove_driver(drv);
+}
+
+/**
+ * put_driver - decrement driver's refcount and clean up if necessary
+ * @drv:	driver in question
+ */
+void put_driver(struct device_driver * drv)
+{
+	if (!atomic_dec_and_lock(&drv->refcount,&device_lock))
+		return;
+	spin_unlock(&device_lock);
+
+	__remove_driver(drv);
+}
+
 EXPORT_SYMBOL(driver_for_each_dev);
 EXPORT_SYMBOL(driver_register);
 EXPORT_SYMBOL(put_driver);
+EXPORT_SYMBOL(remove_driver);
diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	Wed Jun  5 13:43:20 2002
+++ b/drivers/pci/pci-driver.c	Wed Jun  5 13:43:20 2002
@@ -38,23 +38,35 @@
 static int pci_device_probe(struct device * dev)
 {
 	int error = 0;
+	struct pci_driver *drv;
+	struct pci_dev *pci_dev;
 
-	struct pci_driver * drv = list_entry(dev->driver,struct pci_driver,driver);
-	struct pci_dev * pci_dev = list_entry(dev,struct pci_dev,dev);
+	drv = list_entry(dev->driver, struct pci_driver, driver);
+	pci_dev = list_entry(dev, struct pci_dev, dev);
+
+	if (drv->probe) {
+		const struct pci_device_id *id;
 
-	if (drv->probe)
-		error = drv->probe(pci_dev,drv->id_table);
-	return error > 0 ? 0 : -ENODEV;
+		id = pci_match_device(drv->id_table, pci_dev);
+		if (id)
+			error = drv->probe(pci_dev, id);
+		if (error >= 0) {
+			pci_dev->driver = drv;
+			error = 0;
+		}
+	}
+	return error;
 }
 
 static int pci_device_remove(struct device * dev)
 {
 	struct pci_dev * pci_dev = list_entry(dev,struct pci_dev,dev);
+	struct pci_driver * drv = pci_dev->driver;
 
-	if (dev->driver) {
-		struct pci_driver * drv = list_entry(dev->driver,struct pci_driver,driver);
+	if (drv) {
 		if (drv->remove)
 			drv->remove(pci_dev);
+		pci_dev->driver = NULL;
 	}
 	return 0;
 }
@@ -124,7 +136,7 @@
 void
 pci_unregister_driver(struct pci_driver *drv)
 {
-	put_driver(&drv->driver);
+	remove_driver(&drv->driver);
 }
 
 static struct pci_driver pci_compat_driver = {
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Wed Jun  5 13:43:20 2002
+++ b/include/linux/device.h	Wed Jun  5 13:43:20 2002
@@ -118,6 +118,7 @@
 }
 
 extern void put_driver(struct device_driver * drv);
+extern void remove_driver(struct device_driver * drv);
 
 extern int driver_for_each_dev(struct device_driver * drv, void * data, 
 			       int (*callback)(struct device * dev, void * data));

Re: Problem with new driver model?

[Brian Gerst]

Patrick Mochel wrote:
> 
> On 5 Jun 2002, Paul Fulghum wrote:
> 
> > When testing the drivers I maintain on 2.5.20, I hit the
> > BUG_ON in include/linux/devices.txt:115.
> 
> There is a patch that should fix this in Linus's tree.
> 
> If you're using bitkeeper, you can pull it from
> linux.bkbits.net/linux-2.5.
> 
> If not, it's appended here.
> 
>         -pat
> 
> +void remove_driver(struct device_driver * drv)
> +{
> +       spin_lock(&device_lock);
> +       atomic_set(&drv->refcount,0);
> +       spin_unlock(&device_lock);
> +       __remove_driver(drv);
> +}
> +
> +/**
> + * put_driver - decrement driver's refcount and clean up if necessary
> + * @drv:       driver in question
> + */
> +void put_driver(struct device_driver * drv)
> +{
> +       if (!atomic_dec_and_lock(&drv->refcount,&device_lock))
> +               return;
> +       spin_unlock(&device_lock);
> +
> +       __remove_driver(drv);
> +}

Shouldn't the calls to __remove_driver be done inside the device_lock?

--

				Brian Gerst

Re: Problem with new driver model?

[Patrick Mochel]

> Shouldn't the calls to __remove_driver be done inside the device_lock?

No. If the driver needs to lock, it is free to use it. But, what the 
driver needs to do is up to the driver, and we don't want to force it not 
to sleep. 

But, the driver needs to be removed from the bus's list inside the lock, 
so if a device the driver supports gets inserted, we won't try and bind 
the two...Patch coming shortly.

	-pat

Re: Problem with new driver model?

[Paul Fulghum]

On Wed, 2002-06-05 at 15:43, Patrick Mochel wrote:
> There is a patch that should fix this in Linus's tree. 

That did the trick. Thanks to all who answered.

Paul Fulghum
paulkf@microgate.com