[patch] PCI device matching fix

[patch] PCI device matching fix

[Andrew Morton]

The new pci_device_probe() is always passing the zeroeth
entry in the id_table to the device's probe method.  It
needs to scan that table for the correct ID first.

This fixes the recent 3c59x strangenesses.


--- 2.5.20/drivers/pci/pci-driver.c~pci-scan	Mon Jun  3 17:37:59 2002
+++ 2.5.20-akpm/drivers/pci/pci-driver.c	Mon Jun  3 17:38:03 2002
@@ -38,12 +38,19 @@ pci_match_device(const struct pci_device
 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)
-		error = drv->probe(pci_dev,drv->id_table);
+	if (drv->probe) {
+		const struct pci_device_id *id;
+
+		id = pci_match_device(drv->id_table, pci_dev);
+		if (id)
+			error = drv->probe(pci_dev, id);
+	}
 	return error > 0 ? 0 : -ENODEV;
 }
 

-

Re: [patch] PCI device matching fix

[Anton Altaparmakov]

At 01:41 04/06/02, Andrew Morton wrote:
>The new pci_device_probe() is always passing the zeroeth
>entry in the id_table to the device's probe method.  It
>needs to scan that table for the correct ID first.
>
>This fixes the recent 3c59x strangenesses.

Andrew,

Just as a heads up, this patch does indeed fix my 3c905 misdetection problem.

Thanks!!!

         Anton



>--- 2.5.20/drivers/pci/pci-driver.c~pci-scan    Mon Jun  3 17:37:59 2002
>+++ 2.5.20-akpm/drivers/pci/pci-driver.c        Mon Jun  3 17:38:03 2002
>@@ -38,12 +38,19 @@ pci_match_device(const struct pci_device
>  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)
>-               error = drv->probe(pci_dev,drv->id_table);
>+       if (drv->probe) {
>+               const struct pci_device_id *id;
>+
>+               id = pci_match_device(drv->id_table, pci_dev);
>+               if (id)
>+                       error = drv->probe(pci_dev, id);
>+       }
>         return error > 0 ? 0 : -ENODEV;
>  }
>
>
>-
>-
>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/

-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [patch] PCI device matching fix

[Patrick Mochel]

On Mon, 3 Jun 2002, Andrew Morton wrote:

> The new pci_device_probe() is always passing the zeroeth
> entry in the id_table to the device's probe method.  It
> needs to scan that table for the correct ID first.

Arg. Ok. I've applied the patch, though I wonder if there's a better way. 
The proper ID is already determined, though it happens when dealing with 
generic struct devices and not PCI devices:

driver_register calls driver_bind, which calls the bus's bind() callback. 
That compares the bus-specific IDs that the driver supports to the 
device's ID. The format and semantics for comparing IDs is inherently 
bus-specific, so I made sure to keep them in the bus-specific structs. So, 
which ID the device really is, is lost when we return back to the core. 

We could keep it as a void *, or a char *, but I'd rather not lose the 
type information. Or, a union, but those get ugly and difficult to manage. 
Does anyone have any other preference? 

Along with your patch and the other fixes below (in which yours is
included), all the oddities should be resolved, including the OOPSes on
module unload. A patch is appended, after the changelog. You can also soon
pull from

	bk://ldm.bkbits.net/linux-2.5

Could people could please test it, and let me know if anything is still 
broken?

	-pat

ChangeSet@1.416, 2002-06-03 19:53:50-07:00, mochel@osdl.org
  PCI driver mgmt:
  - Make sure proper pci id is passed to probe()
  - make sure pci_dev->driver is set and reset on driver registration/unregistration
  - call remove_driver to force unload of driver on unregistration

 drivers/pci/pci-driver.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions, 8 deletions


ChangeSet@1.415, 2002-06-03 19:51:12-07:00, mochel@osdl.org
  Do manual traversing of drivers' devices list when unbinding the driver.
  
  driver_unbind was called when drv->refcount == 0.
  It would call driver_for_each_dev to do the unbinding
  The first thing that would do was get_device, which...
  BUG()'d if drv->refcount == 0. 
  Duh. 

 drivers/base/core.c |   39 ++++++++++++++++++++++++++++++++-------
 1 files changed, 32 insertions, 7 deletions


ChangeSet@1.414, 2002-06-03 19:48:44-07:00, mochel@osdl.org
  device model udpate:
  - make sure drv->devices is initialized on registration (from Peter Osterlund)
  - add remove_driver for forcing removal of driver
  
  There was a potential race with the module unload code. When a pci driver was unloaded, it would call pci_unregister_driver, which would simply call put_driver.
  If the driver's refcount wasn't 0, it wouldn't unbind it from devices, but the module unload would still continue. 
  If something tried to access the driver later (since everyone thinks its still there), Bad Things would happen. 
  This fixes it until there can be tighter integration between the device model and module unload code.

 drivers/base/driver.c  |   35 ++++++++++++++++++++++++++---------
 include/linux/device.h |    1 +
 2 files changed, 27 insertions, 9 deletions


diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Mon Jun  3 20:11:55 2002
+++ b/drivers/base/core.c	Mon Jun  3 20:11:55 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/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	Mon Jun  3 20:11:55 2002
+++ b/drivers/pci/pci-driver.c	Mon Jun  3 20:11:55 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 = {

Re: [patch] PCI device matching fix

[Greg KH]

On Mon, Jun 03, 2002 at 08:14:25PM -0700, Patrick Mochel wrote:
> 
> 	bk://ldm.bkbits.net/linux-2.5
> 
> Could people could please test it, and let me know if anything is still 
> broken?

These changesets seem to solve the USB controller problems I have been
seeing since 2.4.19, good job.

It looks like you didn't include all of the patch in your message.  I've
attached it here for those without bk to test if they want to.

Now let's talk about why you took away pci_announce_device_to_drivers()...

thanks,

greg k-h


diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Mon Jun  3 22:27:50 2002
+++ b/drivers/base/core.c	Mon Jun  3 22:27:50 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	Mon Jun  3 22:27:50 2002
+++ b/drivers/base/driver.c	Mon Jun  3 22:27:50 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	Mon Jun  3 22:27:50 2002
+++ b/drivers/pci/pci-driver.c	Mon Jun  3 22:27:50 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	Mon Jun  3 22:27:50 2002
+++ b/include/linux/device.h	Mon Jun  3 22:27:50 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: [patch] PCI device matching fix

[Kai Germaschewski]

> @@ -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);
> +}

> @@ -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 = {

Now does that mean you have given up on getting the ref counting right? 
Forcing the ref count to zero is obviously not a solution, 
pci_unregister_driver is usually called at module unload time, which means 
that struct pci_driver will go away immediately after the call returns. If 
you know that there are no other users at this time (which I doubt), you 
don't need to do the whole refcounting thing at all (since at all times 
before the ref count is surely >= 1). If not, you're racy.

I think I brought up that issue before, though nobody seemed interested. 
AFAICS there are two possible solutions:

o provide a destructor callback which is called from put_driver when we're 
  about to throw away the last reference which would let whoever registered 
  the thing in the beginning know that it's all his again now (If it was
  kmalloced, that would be the time to kfree it). In this case it's 
  rather that the callback would tell us we can now finish 
  module_exit().
o Use a completion or something and do the above inside 
  pci_unregister_driver (or remove_driver). I.e. have it sleep until
  all references are gone. That would fix the race for the typical

void my_module_exit(void) { pci_unregister_driver(&my_driver); }

  case.

(The latter would be my preference, as I see no point in having driver
 authors deal with the complication of waiting for completion of 
 pci_unregister_driver() themselves)

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

[cc list trimmed]

> Now does that mean you have given up on getting the ref counting right? 
> Forcing the ref count to zero is obviously not a solution, 
> pci_unregister_driver is usually called at module unload time, which means 
> that struct pci_driver will go away immediately after the call returns. If 
> you know that there are no other users at this time (which I doubt), you 
> don't need to do the whole refcounting thing at all (since at all times 
> before the ref count is surely >= 1). If not, you're racy.

No, I haven't given up on it, and I admit it's fishy. However, I did it to 
ensure that no one can access the driver since it is going away as soon as 
we return to pci_unregister_driver. Though, I'll also admit that making 
the next potential user hit the BUG() in get_driver() is not the nicest 
thing to do...

> I think I brought up that issue before, though nobody seemed interested. 
> AFAICS there are two possible solutions:
> 
> o provide a destructor callback which is called from put_driver when we're 
>   about to throw away the last reference which would let whoever registered 
>   the thing in the beginning know that it's all his again now (If it was
>   kmalloced, that would be the time to kfree it). In this case it's 
>   rather that the callback would tell us we can now finish 
>   module_exit().
> o Use a completion or something and do the above inside 
>   pci_unregister_driver (or remove_driver). I.e. have it sleep until
>   all references are gone. That would fix the race for the typical
> 
> void my_module_exit(void) { pci_unregister_driver(&my_driver); }
> 
>   case.
> 
> (The latter would be my preference, as I see no point in having driver
>  authors deal with the complication of waiting for completion of 
>  pci_unregister_driver() themselves)

There is a destructor: struct device_driver::release(). It's the last 
thing called from __remove_driver(). 

No matter what, the module will get unloaded immediately. What would be 
nice is if we could delay it until we said it was ok (from the driver's 
release callback). We could even define a common release callback for all 
drivers:

void driver_release(struct device_driver * drv)
{
	struct module * module = drv->module;

	/* do unload of module */
}

We wouldn't need a completion; we would just have to wait until the 
refcount hit 0 naturally. But, I'm just making this stuff up. Is this at 
all possible?

	-pat

Re: [patch] PCI device matching fix

[Kai Germaschewski]

On Wed, 5 Jun 2002, Patrick Mochel wrote:

> No, I haven't given up on it, and I admit it's fishy. However, I did it
> to ensure that no one can access the driver since it is going away as
> soon as we return to pci_unregister_driver. Though, I'll also admit that
> making the next potential user hit the BUG() in get_driver() is not the
> nicest thing to do...

There's two phases:
o remove_driver():
  Make sure that now one else can find (and inc the refcount) your struct
  anymore. That can and should be done immediately. Drop your reference.
o At some point later, everybody else will have dropped their references
  and the destructor can be called.

Alternatively:
o remove_driver():
  Make sure that now one else can find (and inc the refcount) your struct
  anymore. That can and should be done immediately. Drop your reference.
  Wait for everybody else to drop their reference. Then return to the 
  caller
o At some point later, everybody else will have dropped their references
  and the remove_driver() should be woken up and you get to the return.

I claim the latter is more appropriate, since driver_register() and 
driver_remove() (btw, I'd rather have the latter called 
driver_unregister()) will always be called by the same user, normally
the driver module. That's different from some object which isn't
explicitly unregistered somewhere, but will be cleaned up e.g. due to
memory pressure some time later. You can always achieve the second
if you have the first, but I see no point in having driver authors deal
with it.

> There is a destructor: struct device_driver::release(). It's the last 
> thing called from __remove_driver(). 
> 
> No matter what, the module will get unloaded immediately. What would be 
> nice is if we could delay it until we said it was ok (from the driver's 
> release callback). We could even define a common release callback for all 
> drivers:

That's not true. module_exit() is called from rmmod(8) and thus in process
context. It's perfectly fine to sleep in module_exit() until we're safe to 
be deleted.

> void driver_release(struct device_driver * drv)
> {
> 	struct module * module = drv->module;
> 
> 	/* do unload of module */
> }
> 
> We wouldn't need a completion; we would just have to wait until the 
> refcount hit 0 naturally. But, I'm just making this stuff up. Is this at 
> all possible?

Now this is bad, what if the last reference went away from irq context? 
Unloading a module from irq context doesn't sound like a good idea to me.

I'd suggest something like

pci_driver_release(pdrv)
{
	complete(&pdrv->complete);
}

pci_register_driver(pdrv)
{
	...
	pdrv->driver.release = pci_driver_release;
	pdrv->complete = NULL;
}

pci_unregister_driver(pdrv)
{
	DECLARE_COMPLETION(complete);

	pdrv->complete = &complete;
	put_driver(&drv->driver);
	wait_for_completion(&complete);
}

And make clear that pci_unregister_driver() can be called from process 
context only.

Actually, as said above, I'd rather do the same thing in remove_driver(),
i.e. make that sleep as well - unless you can come up with an example
where anybody would want to call remove_driver() and cannot sleep. 
Just replace ->release with the struct completion *.

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

On Wed, 5 Jun 2002, Kai Germaschewski wrote:

> On Wed, 5 Jun 2002, Patrick Mochel wrote:
> 
> > No, I haven't given up on it, and I admit it's fishy. However, I did it
> > to ensure that no one can access the driver since it is going away as
> > soon as we return to pci_unregister_driver. Though, I'll also admit that
> > making the next potential user hit the BUG() in get_driver() is not the
> > nicest thing to do...
> 
> There's two phases:
> o remove_driver():
>   Make sure that now one else can find (and inc the refcount) your struct
>   anymore. That can and should be done immediately. Drop your reference.
> o At some point later, everybody else will have dropped their references
>   and the destructor can be called.
> 
> Alternatively:
> o remove_driver():
>   Make sure that now one else can find (and inc the refcount) your struct
>   anymore. That can and should be done immediately. Drop your reference.
>   Wait for everybody else to drop their reference. Then return to the 
>   caller
> o At some point later, everybody else will have dropped their references
>   and the remove_driver() should be woken up and you get to the return.

Alright, I can deal. I expanded on what you had, and moved it into the 
core. It's not tested, but lemme know what you think. 

	-pat

===== drivers/base/driver.c 1.7 vs edited =====
--- 1.7/drivers/base/driver.c	Wed Jun  5 15:59:31 2002
+++ edited/drivers/base/driver.c	Wed Jun  5 17:29:55 2002
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/errno.h>
+#include <linux/completion.h>
 #include "base.h"
 
 
@@ -70,6 +71,8 @@
 	atomic_set(&drv->refcount,2);
 	rwlock_init(&drv->lock);
 	INIT_LIST_HEAD(&drv->devices);
+	init_completion(&drv->complete);
+
 	write_lock(&drv->bus->lock);
 	list_add(&drv->bus_list,&drv->bus->drivers);
 	write_unlock(&drv->bus->lock);
@@ -84,18 +87,21 @@
 	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
 	driver_detach(drv);
 	driverfs_remove_dir(&drv->dir);
-	if (drv->release)
-		drv->release(drv);
 	put_bus(drv->bus);
 }
 
-void remove_driver(struct device_driver * drv)
+void driver_unregister(struct device_driver * drv)
 {
+	/* make sure no one can get to it via the bus any more */
 	write_lock(&drv->bus->lock);
-	atomic_set(&drv->refcount,0);
 	list_del_init(&drv->bus_list);
 	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+
+	/* dec the refcount */
+	put_driver(drv);
+
+	/* if we didn't get rid of the driver, wait for it to be done */
+	wait_for_completion(&drv->complete);
 }
 
 /**
@@ -112,9 +118,10 @@
 	list_del_init(&drv->bus_list);
 	write_unlock(&drv->bus->lock);
 	__remove_driver(drv);
+	complete(&drv->complete);
 }
 
 EXPORT_SYMBOL(driver_for_each_dev);
 EXPORT_SYMBOL(driver_register);
+EXPORT_SYMBOL(driver_unregister);
 EXPORT_SYMBOL(put_driver);
-EXPORT_SYMBOL(remove_driver);
===== include/linux/device.h 1.20 vs edited =====
--- 1.20/include/linux/device.h	Wed Jun  5 14:43:23 2002
+++ edited/include/linux/device.h	Wed Jun  5 17:33:21 2002
@@ -28,6 +28,7 @@
 #include <linux/ioport.h>
 #include <linux/list.h>
 #include <linux/sched.h>
+#include <linux/completion.h>
 #include <linux/driverfs_fs.h>
 
 #define DEVICE_NAME_SIZE	80
@@ -91,6 +92,7 @@
 
 	rwlock_t		lock;
 	atomic_t		refcount;
+	struct completion	complete;
 
 	list_t			bus_list;
 	list_t			devices;
@@ -102,13 +104,12 @@
 
 	int	(*suspend)	(struct device * dev, u32 state, u32 level);
 	int	(*resume)	(struct device * dev, u32 level);
-
-	void	(*release)	(struct device_driver * drv);
 };
 
 
 
 extern int driver_register(struct device_driver * drv);
+extern void driver_unregister(struct device_driver * drv);
 
 static inline struct device_driver * get_driver(struct device_driver * drv)
 {
@@ -118,7 +119,6 @@
 }
 
 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: [patch] PCI device matching fix

[Kai Germaschewski]

On Wed, 5 Jun 2002, Patrick Mochel wrote:

> Alright, I can deal. I expanded on what you had, and moved it into the 
> core. It's not tested, but lemme know what you think. 

Looks fine to me, though I didn't test it either ;-)

However, it's possible to put the struct completion onto the stack
as I suggested, it saves a couple of bytes in struct driver, and, more
importantly, it'll blow up if the refcount goes to zero before 
driver_unregister() has been called, so it provides a bug trap for
messed-up refcounting.

Anyway, that's a question of taste, I suppose.

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

On Wed, 5 Jun 2002, Kai Germaschewski wrote:

> On Wed, 5 Jun 2002, Patrick Mochel wrote:
> 
> > Alright, I can deal. I expanded on what you had, and moved it into the 
> > core. It's not tested, but lemme know what you think. 
> 
> Looks fine to me, though I didn't test it either ;-)
> 
> However, it's possible to put the struct completion onto the stack
> as I suggested, it saves a couple of bytes in struct driver, and, more
> importantly, it'll blow up if the refcount goes to zero before 
> driver_unregister() has been called, so it provides a bug trap for
> messed-up refcounting.

Actually, I don't think the completion is needed at all. For one, it
didn't work, and I found another bug. When we find a driver to a device,
we inc the refcount. But, when we detach the driver via driver_detach(),
we weren't decrementing it. The patch below does this. When you call
driver_unregister(), it detaches the driver, then decs the refcount once,
and Lo! it works.

Second, Greg brought up an interesting point: if something is using a 
module, it should have already inc'd the refcount, and rmmod(8) won't let 
you remove it. If we can remove a module, but something is still using it, 
it's a bug. And, exposing those will get those fixed sooner...

That said, I'm an offender of that rule with driverfs. It's the topic of
another thread, and I haven't quite figured out the best solution. When we
open a file belonging to a driver, we want to inc the module refcount.
But, we don't currently have anyway to get at the module to do this. 

The f_ops of the file has an owner, but the f_ops structure is shared by 
all files. We could add an owner to struct driver_file_entry and struct 
device_driver, but that would make the former twice as large. We could add 
it only to the latter, but it would require a differnt open for each type 
of structure (device, device_driver, bus_type) to deference and access the 
owner. 

I'm leading towards the last, and will see how clean it can be..

	-pat


===== drivers/base/core.c 1.28 vs edited =====
--- 1.28/drivers/base/core.c	Thu Jun  6 09:50:30 2002
+++ edited/drivers/base/core.c	Thu Jun  6 11:23:50 2002
@@ -144,6 +144,7 @@
 			drv->remove(dev);
 	} else
 		unlock_device(dev);
+	put_driver(drv);
 	return 0;
 }
 
===== drivers/base/driver.c 1.7 vs edited =====
--- 1.7/drivers/base/driver.c	Wed Jun  5 15:59:31 2002
+++ edited/drivers/base/driver.c	Thu Jun  6 12:07:34 2002
@@ -79,23 +79,18 @@
 	return 0;
 }
 
-static void __remove_driver(struct device_driver * drv)
-{
-	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
-	driver_detach(drv);
-	driverfs_remove_dir(&drv->dir);
-	if (drv->release)
-		drv->release(drv);
-	put_bus(drv->bus);
-}
-
-void remove_driver(struct device_driver * drv)
+void driver_unregister(struct device_driver * drv)
 {
+	/* make sure no one can get to it via the bus any more */
 	write_lock(&drv->bus->lock);
-	atomic_set(&drv->refcount,0);
 	list_del_init(&drv->bus_list);
 	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+
+	/* force removal from devices */
+	driver_detach(drv);
+
+	/* dec the refcount (this should clean it up) */
+	put_driver(drv);
 }
 
 /**
@@ -111,10 +106,14 @@
 	}
 	list_del_init(&drv->bus_list);
 	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+
+	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
+	driverfs_remove_dir(&drv->dir);
+
+	put_bus(drv->bus);
 }
 
 EXPORT_SYMBOL(driver_for_each_dev);
 EXPORT_SYMBOL(driver_register);
+EXPORT_SYMBOL(driver_unregister);
 EXPORT_SYMBOL(put_driver);
-EXPORT_SYMBOL(remove_driver);
===== include/linux/device.h 1.20 vs edited =====
--- 1.20/include/linux/device.h	Wed Jun  5 14:43:23 2002
+++ edited/include/linux/device.h	Thu Jun  6 11:46:07 2002
@@ -102,13 +102,12 @@
 
 	int	(*suspend)	(struct device * dev, u32 state, u32 level);
 	int	(*resume)	(struct device * dev, u32 level);
-
-	void	(*release)	(struct device_driver * drv);
 };
 
 
 
 extern int driver_register(struct device_driver * drv);
+extern void driver_unregister(struct device_driver * drv);
 
 static inline struct device_driver * get_driver(struct device_driver * drv)
 {
@@ -118,7 +117,6 @@
 }
 
 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: [patch] PCI device matching fix

[Kai Germaschewski]

On Thu, 6 Jun 2002, Patrick Mochel wrote:

> Actually, I don't think the completion is needed at all. For one, it
> didn't work, and I found another bug. When we find a driver to a device,
> we inc the refcount. But, when we detach the driver via driver_detach(),
> we weren't decrementing it. The patch below does this. When you call
> driver_unregister(), it detaches the driver, then decs the refcount once,
> and Lo! it works.

Well, of course, when the refcounting is messed up, it doesn't work, but 
at least it pointed out the error - I suppose after fixing that, it did 
work?

> Second, Greg brought up an interesting point: if something is using a 
> module, it should have already inc'd the refcount, and rmmod(8) won't let 
> you remove it. If we can remove a module, but something is still using it, 
> it's a bug. And, exposing those will get those fixed sooner...

That's not quite right. It's okay to be using a module while the module
refcount is zero, in fact it's very common. But you need to have a 
module_exit() function which makes sure that the module is not used 
anymore before it returns.

I think a good example are network drivers, since I think they get the 
refcounting right. If you load your network driver, it'll register_netdev(), 
but not inc the module use count. If someone ifconfig up's the interface,
only then the module use count is incremented and the ::open() method is
called. If you ifdown the interface, the module use count goes back to 
zero after ::close() has been called. However, the struct net_device
is still registered with the network layer.

When you rmmod the module, which is possible since its use count is zero,
module_exit() will call unregister_netdev(), which initiate unregistering 
and then sleep until noone else is using the struct net_device anymore.
At that point it's save to return to the module, which will kfree()
and exit.

(One thing is different there, though not really: struct net_device is
alloced dynamically, so the problem is to kfree() only after noone else
uses it, for struct driver it's most likely statically alloced and freed
after module_exit(), so that's why we have to wait for other users to go 
away)

Anyway, the completion/wait is needed, there may be someone else using
struct driver at this moment (say someone doing driver_for_each_dev() on
it). Otherwise, the refcount is completely useless, between
driver_register() and driver_unregister() it will always be >= 1, so
inc'ing / dec'ing doesn't make any difference, the only point where it is
needed is exactly at driver_unregister() time.

> That said, I'm an offender of that rule with driverfs. It's the topic of
> another thread, and I haven't quite figured out the best solution. When we
> open a file belonging to a driver, we want to inc the module refcount.
> But, we don't currently have anyway to get at the module to do this. 

Well, if you do the refcounting correctly, this should be doable, too.

I don't really have the time to go through drivers/base and fs/driverfs in 
detail, but the general idea would be to following:

At driver_unregister() time, you need to unregister the dir_entry from its 
parent, so that it won't be found anymore. However, the dir_entry still 
is referenced from the dentry, so you hopefully inc'ed the 
refcount for that. Eventually, the dentry will become unreferenced and
driverfs_d_delete_file() be called. You kfree() the dentry and put the
reference you had - if it was the last one, the usual mechanism
kicks in, your completion gets done and your module removed.

Alternatively, make it possible to dissociate the dentry and dir_entry 
before, so you can do that, drop the ref count there and someone keeping 
open a dir in /driversfs won't be able to delay the module unload 
indefinitely. (The latter problem you'd surely have when you go
to driver::owner and use the to inc the use count) 

You didn't select an easy problem to hack on, that's for sure. But I think 
it's possible to get it right, just consistently make sure your refcount
is always right, and don't forget about indirect references e.g. via 
list_entry in driversfs/inode.c.

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

> That's not quite right. It's okay to be using a module while the module
> refcount is zero, in fact it's very common. But you need to have a 
> module_exit() function which makes sure that the module is not used 
> anymore before it returns.

Is that correct behavior? Shouldn't any use increment the refcount? 

> I think a good example are network drivers, since I think they get the 
> refcounting right. If you load your network driver, it'll register_netdev(), 
> but not inc the module use count. If someone ifconfig up's the interface,
> only then the module use count is incremented and the ::open() method is
> called. If you ifdown the interface, the module use count goes back to 
> zero after ::close() has been called. However, the struct net_device
> is still registered with the network layer.
> 
> When you rmmod the module, which is possible since its use count is zero,
> module_exit() will call unregister_netdev(), which initiate unregistering 
> and then sleep until noone else is using the struct net_device anymore.
> At that point it's save to return to the module, which will kfree()
> and exit.
> 
> (One thing is different there, though not really: struct net_device is
> alloced dynamically, so the problem is to kfree() only after noone else
> uses it, for struct driver it's most likely statically alloced and freed
> after module_exit(), so that's why we have to wait for other users to go 
> away)

One minor, though important, distinction is that there is one struct 
net_device allocated for each device the driver is attached to. When 
module_exit() calls pci_unregister_driver(), the driver's remove callback 
is called for each of these devices, at which point the struct net_device 
is freed. 

The struct device_driver is statically allocated, since there is one per 
driver/module. 

> Anyway, the completion/wait is needed, there may be someone else using
> struct driver at this moment (say someone doing driver_for_each_dev() on
> it). Otherwise, the refcount is completely useless, between
> driver_register() and driver_unregister() it will always be >= 1, so
> inc'ing / dec'ing doesn't make any difference, the only point where it is
> needed is exactly at driver_unregister() time.

Yes, you're right. I think I owe you a beer for keeping me honest. 

However, taking a step back... The only time the driver refcount will hit
0 is if it's going away. The only time it goes away is if the module is
removed. If someone is using the driver, you don't want the module to 
unload. Instead of trying to keep the refcounts of the two objects 
synchronized against each other, can't we just use the same one? 

We can replace the refcount in struct device_driver with a struct module
pointer, which modular drivers will already have. get_driver and
put_driver can either go away, or simply become wrappers for touching the
module reference count.

> > That said, I'm an offender of that rule with driverfs. It's the topic of
> > another thread, and I haven't quite figured out the best solution. When we
> > open a file belonging to a driver, we want to inc the module refcount.
> > But, we don't currently have anyway to get at the module to do this. 
> 
> Well, if you do the refcounting correctly, this should be doable, too.
> 
> I don't really have the time to go through drivers/base and fs/driverfs in 
> detail, but the general idea would be to following:
...

I'm pretty sure the file/directory refcounting is ok. Famous last words, 
but I'm doing something like you described. My concern was instead over 
the validity of the callback pointers that a driver has registered for a 
file. 

A user opens a file. Before they read from it, the module is unloaded. 
They try and read from it, the show() callback is referenced, and we 
crash. 

Currently, the driverfs open call increments the reference count of the 
device that created it. There are currently no provisions for device 
drivers or bus drivers. So, no matter what, I have to modify that call to 
handle those types. If the refcounting is right, the module won't be 
unloaded, and the callbacks will be valid. 

	-pat

Re: [patch] PCI device matching fix

[Kai Germaschewski]

On Thu, 6 Jun 2002, Patrick Mochel wrote:

> > That's not quite right. It's okay to be using a module while the module
> > refcount is zero, in fact it's very common. But you need to have a
> > module_exit() function which makes sure that the module is not used
> > anymore before it returns.
> 
> Is that correct behavior? Shouldn't any use increment the refcount?

It's safe, as in not racy. It's the only way to do it if you want to be 
able to use "rmmod" to unregister your driver, see below. Well, there was
some talk about going to a two stage module unload in 2.5. I don't 
remember how exactly that worked, but it didn't happen yet, anyway.

> > (One thing is different there, though not really: struct net_device is
> > alloced dynamically, so the problem is to kfree() only after noone else
> > uses it, for struct driver it's most likely statically alloced and freed
> > after module_exit(), so that's why we have to wait for other users to go 
> > away)
> 
> One minor, though important, distinction is that there is one struct 
> net_device allocated for each device the driver is attached to. When 
> module_exit() calls pci_unregister_driver(), the driver's remove callback 
> is called for each of these devices, at which point the struct net_device 
> is freed. 

That's what I meant above, the way the memory is deallocated is different, 
kfree() vs. module_unload(), but the way to handle it is the same - wait 
until it's all ours, and the free it - explicitly with kfree() vs 
implicitly by finishing module_exit()

> However, taking a step back... The only time the driver refcount will hit
> 0 is if it's going away. The only time it goes away is if the module is
> removed. If someone is using the driver, you don't want the module to 
> unload. Instead of trying to keep the refcounts of the two objects 
> synchronized against each other, can't we just use the same one? 

Yes, you're basically right, whether you use the refcount in struct driver 
or the module use count doesn't make any difference. Except for one thing:
You cannot call module_exit() when the module count is > 0. Since only 
then unregister_driver() is called, there's no way to get the use count to 
zero and thus unload the module. One could of course change the policy to 
call unregister_driver() not from module_exit(), but e.g. by
"echo remove > /driversfs/../my_driver", but it's surely not that I'm 
suggesting this.

> We can replace the refcount in struct device_driver with a struct module
> pointer, which modular drivers will already have. get_driver and
> put_driver can either go away, or simply become wrappers for touching the
> module reference count.

They would still need to be wrappers for touching the module count 
instead. So it doesn't buy anything.

> I'm pretty sure the file/directory refcounting is ok. Famous last words, 
> but I'm doing something like you described. My concern was instead over 
> the validity of the callback pointers that a driver has registered for a 
> file. 
> 
> A user opens a file. Before they read from it, the module is unloaded. 
> They try and read from it, the show() callback is referenced, and we 
> crash. 
> 
> Currently, the driverfs open call increments the reference count of the 
> device that created it. There are currently no provisions for device 
> drivers or bus drivers. So, no matter what, I have to modify that call to 
> handle those types. If the refcounting is right, the module won't be 
> unloaded, and the callbacks will be valid. 

Yeah, that seems right. Maybe it's better to not install callbacks 
directly, but a reference to the struct driver, where the struct driver 
contains the callbacks. This way it's explicit you still reference the 
struct driver (and need to get/put it). But I didn't even look how you do 
it currently, so maybe you're doing it that way already.

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

> Yes, you're basically right, whether you use the refcount in struct driver 
> or the module use count doesn't make any difference. Except for one thing:
> You cannot call module_exit() when the module count is > 0. Since only 
> then unregister_driver() is called, there's no way to get the use count to 
> zero and thus unload the module. One could of course change the policy to 
> call unregister_driver() not from module_exit(), but e.g. by
> "echo remove > /driversfs/../my_driver", but it's surely not that I'm 
> suggesting this.

We can keep the same policy as modules now - keep the usage count at 0 
when not in use. That way the module can unload at any time. module_exit() 
calls driver_unregister(), which, down the line, calls the drivers' 
remove() for each device attached to it. 

That seems a lot cleaner than anything so far; we leverage the existing
module infrastructure as much as possible. I'll see about codifying the
concept in the next day or so and sending it out..

	-pat

Re: [patch] PCI device matching fix

[Patrick Mochel]

> That seems a lot cleaner than anything so far; we leverage the existing
> module infrastructure as much as possible. I'll see about codifying the
> concept in the next day or so and sending it out..

Actually, it appears to be as easy as the patch below. Or, I just could 
just not know what the hell I'm talking about. 

Things appear to be working (with the eepro100 as a module)....

	-pat

===== drivers/base/driver.c 1.7 vs edited =====
--- 1.7/drivers/base/driver.c	Wed Jun  5 15:59:31 2002
+++ edited/drivers/base/driver.c	Thu Jun  6 16:13:35 2002
@@ -67,9 +67,9 @@
 	pr_debug("Registering driver '%s' with bus '%s'\n",drv->name,drv->bus->name);
 
 	get_bus(drv->bus);
-	atomic_set(&drv->refcount,2);
 	rwlock_init(&drv->lock);
 	INIT_LIST_HEAD(&drv->devices);
+	SET_MODULE_OWNER(drv);
 	write_lock(&drv->bus->lock);
 	list_add(&drv->bus_list,&drv->bus->drivers);
 	write_unlock(&drv->bus->lock);
@@ -79,42 +79,41 @@
 	return 0;
 }
 
-static void __remove_driver(struct device_driver * drv)
+void driver_unregister(struct device_driver * drv)
 {
-	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
+	struct bus_type * bus;
+
+	write_lock(&drv->lock);
+	bus = drv->bus;
+	drv->bus = NULL;
+	write_unlock(&drv->lock);
+
+	/* make sure no one can get to it via the bus any more */
+	write_lock(&bus->lock);
+	list_del_init(&drv->bus_list);
+	write_unlock(&bus->lock);
+
+	/* force removal from devices */
 	driver_detach(drv);
+
+	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
 	driverfs_remove_dir(&drv->dir);
-	if (drv->release)
-		drv->release(drv);
-	put_bus(drv->bus);
+
+	put_bus(bus);
 }
 
-void remove_driver(struct device_driver * drv)
+struct device_driver * get_driver(struct device_driver * drv)
 {
-	write_lock(&drv->bus->lock);
-	atomic_set(&drv->refcount,0);
-	list_del_init(&drv->bus_list);
-	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+	__MOD_INC_USE_COUNT(drv->owner);
+	return drv;
 }
 
-/**
- * put_driver - decrement driver's refcount and clean up if necessary
- * @drv:	driver in question
- */
 void put_driver(struct device_driver * drv)
 {
-	write_lock(&drv->bus->lock);
-	if (!atomic_dec_and_test(&drv->refcount)) {
-		write_unlock(&drv->bus->lock);
-		return;
-	}
-	list_del_init(&drv->bus_list);
-	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+	__MOD_DEC_USE_COUNT(drv->owner);
 }
 
 EXPORT_SYMBOL(driver_for_each_dev);
 EXPORT_SYMBOL(driver_register);
+EXPORT_SYMBOL(driver_unregister);
 EXPORT_SYMBOL(put_driver);
-EXPORT_SYMBOL(remove_driver);
===== include/linux/device.h 1.20 vs edited =====
--- 1.20/include/linux/device.h	Wed Jun  5 14:43:23 2002
+++ edited/include/linux/device.h	Thu Jun  6 16:13:19 2002
@@ -90,7 +90,7 @@
 	struct bus_type		* bus;
 
 	rwlock_t		lock;
-	atomic_t		refcount;
+	struct module		* owner;
 
 	list_t			bus_list;
 	list_t			devices;
@@ -102,23 +102,15 @@
 
 	int	(*suspend)	(struct device * dev, u32 state, u32 level);
 	int	(*resume)	(struct device * dev, u32 level);
-
-	void	(*release)	(struct device_driver * drv);
 };
 
 
 
 extern int driver_register(struct device_driver * drv);
+extern void driver_unregister(struct device_driver * drv);
 
-static inline struct device_driver * get_driver(struct device_driver * drv)
-{
-	BUG_ON(!atomic_read(&drv->refcount));
-	atomic_inc(&drv->refcount);
-	return drv;
-}
-
+extern struct device_driver * get_driver(struct device_driver * drv);
 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: [patch] PCI device matching fix

[Kai Germaschewski]

On Thu, 6 Jun 2002, Patrick Mochel wrote:

> Actually, it appears to be as easy as the patch below. Or, I just could 
> just not know what the hell I'm talking about. 
> 
> Things appear to be working (with the eepro100 as a module)....
> 
> 	-pat
> 
> ===== drivers/base/driver.c 1.7 vs edited =====
> --- 1.7/drivers/base/driver.c	Wed Jun  5 15:59:31 2002
> +++ edited/drivers/base/driver.c	Thu Jun  6 16:13:35 2002
> @@ -67,9 +67,9 @@
>  	pr_debug("Registering driver '%s' with bus '%s'\n",drv->name,drv->bus->name);
>  
>  	get_bus(drv->bus);
> -	atomic_set(&drv->refcount,2);
>  	rwlock_init(&drv->lock);
>  	INIT_LIST_HEAD(&drv->devices);
> +	SET_MODULE_OWNER(drv);
>  	write_lock(&drv->bus->lock);
>  	list_add(&drv->bus_list,&drv->bus->drivers);
>  	write_unlock(&drv->bus->lock);

drivers/base is always compiled in. So SET_MODULE_OWNER will set the owner 
to NULL.

> -void remove_driver(struct device_driver * drv)
> +struct device_driver * get_driver(struct device_driver * drv)
>  {
> -	write_lock(&drv->bus->lock);
> -	atomic_set(&drv->refcount,0);
> -	list_del_init(&drv->bus_list);
> -	write_unlock(&drv->bus->lock);
> -	__remove_driver(drv);
> +	__MOD_INC_USE_COUNT(drv->owner);
> +	return drv;
>  }
>  
> -/**
> - * put_driver - decrement driver's refcount and clean up if necessary
> - * @drv:	driver in question
> - */
>  void put_driver(struct device_driver * drv)
>  {
> -	write_lock(&drv->bus->lock);
> -	if (!atomic_dec_and_test(&drv->refcount)) {
> -		write_unlock(&drv->bus->lock);
> -		return;
> -	}
> -	list_del_init(&drv->bus_list);
> -	write_unlock(&drv->bus->lock);
> -	__remove_driver(drv);
> +	__MOD_DEC_USE_COUNT(drv->owner);
>  }

So the __MOD_{INC,DEC}_USE_COUNT() should oops right here.
If you tested it and it doesn't oops, I don't understand why.

So, for one, if you want to go that road, you should use fops_get()/put()
(you can use just these, or rename them appropriately), they'll do the
right thing if a thing is modular as opposed to built-in (owner == NULL).

And, you need to set the owner from the module which you want to protect.

So in the your driver:

struct pci_driver my_drv {
	probe: ...
	driver : {
		owner: THIS_MODULE,
	}
}

which certainly is ugly since owner is in a sub-struct. But it's not
really a possibility anyway, since in this case pci_register_driver will 
do a MOD_INC_USE_COUNT and you never have a chance to call 
pci_unregister_driver() to decrement it again, since you need to have it
decremented before you can call pci_unregister_driver() (because
that's called from your module_exit() function).

--Kai

		

Re: [patch] PCI device matching fix

[Patrick Mochel]

Hi. Sorry about the long delay in replying, I've been out of town, and am 
actually about to leave again for a week+ in a few hours...

> So the __MOD_{INC,DEC}_USE_COUNT() should oops right here.
> If you tested it and it doesn't oops, I don't understand why.
> 
> So, for one, if you want to go that road, you should use fops_get()/put()
> (you can use just these, or rename them appropriately), they'll do the
> right thing if a thing is modular as opposed to built-in (owner == NULL).

Ah yes; patch appended which does just that. 

> And, you need to set the owner from the module which you want to protect.
> 
> So in the your driver:
> 
> struct pci_driver my_drv {
> 	probe: ...
> 	driver : {
> 		owner: THIS_MODULE,
> 	}
> }

This is certainly not the prettiest, and I've run into it when setting 
other generic driver fields. Is there a cleaner way to do this?

> which certainly is ugly since owner is in a sub-struct. But it's not
> really a possibility anyway, since in this case pci_register_driver will 
> do a MOD_INC_USE_COUNT and you never have a chance to call 
> pci_unregister_driver() to decrement it again, since you need to have it
> decremented before you can call pci_unregister_driver() (because
> that's called from your module_exit() function).

pci_register_driver() doesn't inc the refcount. The refcount is held at 1 
by the module loading code, IIUC, until module_init() is done. After that, 
the refcount stays at 0 until someone uses it.

	-pat

===== drivers/base/driver.c 1.7 vs edited =====
--- 1.7/drivers/base/driver.c	Wed Jun  5 15:59:31 2002
+++ edited/drivers/base/driver.c	Mon Jun 10 06:54:00 2002
@@ -67,54 +67,55 @@
 	pr_debug("Registering driver '%s' with bus '%s'\n",drv->name,drv->bus->name);
 
 	get_bus(drv->bus);
-	atomic_set(&drv->refcount,2);
 	rwlock_init(&drv->lock);
 	INIT_LIST_HEAD(&drv->devices);
+	SET_MODULE_OWNER(drv);
 	write_lock(&drv->bus->lock);
 	list_add(&drv->bus_list,&drv->bus->drivers);
 	write_unlock(&drv->bus->lock);
 	driver_make_dir(drv);
 	driver_attach(drv);
-	put_driver(drv);
 	return 0;
 }
 
-static void __remove_driver(struct device_driver * drv)
+void driver_unregister(struct device_driver * drv)
 {
-	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
+	struct bus_type * bus;
+
+	write_lock(&drv->lock);
+	bus = drv->bus;
+	drv->bus = NULL;
+	write_unlock(&drv->lock);
+
+	/* make sure no one can get to it via the bus any more */
+	write_lock(&bus->lock);
+	list_del_init(&drv->bus_list);
+	write_unlock(&bus->lock);
+
+	/* force removal from devices */
 	driver_detach(drv);
+
+	pr_debug("Unregistering driver '%s' from bus '%s'\n",drv->name,drv->bus->name);
 	driverfs_remove_dir(&drv->dir);
-	if (drv->release)
-		drv->release(drv);
-	put_bus(drv->bus);
+
+	put_bus(bus);
 }
 
-void remove_driver(struct device_driver * drv)
+struct device_driver * get_driver(struct device_driver * drv)
 {
-	write_lock(&drv->bus->lock);
-	atomic_set(&drv->refcount,0);
-	list_del_init(&drv->bus_list);
-	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+	if (drv && drv->owner)
+		if (!try_inc_mod_count(drv->owner))
+			return NULL;
+	return drv;
 }
 
-/**
- * put_driver - decrement driver's refcount and clean up if necessary
- * @drv:	driver in question
- */
 void put_driver(struct device_driver * drv)
 {
-	write_lock(&drv->bus->lock);
-	if (!atomic_dec_and_test(&drv->refcount)) {
-		write_unlock(&drv->bus->lock);
-		return;
-	}
-	list_del_init(&drv->bus_list);
-	write_unlock(&drv->bus->lock);
-	__remove_driver(drv);
+	if (drv && drv->owner)
+		__MOD_DEC_USE_COUNT(drv->owner);
 }
 
 EXPORT_SYMBOL(driver_for_each_dev);
 EXPORT_SYMBOL(driver_register);
+EXPORT_SYMBOL(driver_unregister);
 EXPORT_SYMBOL(put_driver);
-EXPORT_SYMBOL(remove_driver);
===== include/linux/device.h 1.20 vs edited =====
--- 1.20/include/linux/device.h	Wed Jun  5 14:43:23 2002
+++ edited/include/linux/device.h	Thu Jun  6 16:13:19 2002
@@ -90,7 +90,7 @@
 	struct bus_type		* bus;
 
 	rwlock_t		lock;
-	atomic_t		refcount;
+	struct module		* owner;
 
 	list_t			bus_list;
 	list_t			devices;
@@ -102,23 +102,15 @@
 
 	int	(*suspend)	(struct device * dev, u32 state, u32 level);
 	int	(*resume)	(struct device * dev, u32 level);
-
-	void	(*release)	(struct device_driver * drv);
 };
 
 
 
 extern int driver_register(struct device_driver * drv);
+extern void driver_unregister(struct device_driver * drv);
 
-static inline struct device_driver * get_driver(struct device_driver * drv)
-{
-	BUG_ON(!atomic_read(&drv->refcount));
-	atomic_inc(&drv->refcount);
-	return drv;
-}
-
+extern struct device_driver * get_driver(struct device_driver * drv);
 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: [patch] PCI device matching fix

[Kai Germaschewski]

On Mon, 10 Jun 2002, Patrick Mochel wrote:

> > And, you need to set the owner from the module which you want to protect.
> > 
> > So in the your driver:
> > 
> > struct pci_driver my_drv {
> > 	probe: ...
> > 	driver : {
> > 		owner: THIS_MODULE,
> > 	}
> > }
> 
> This is certainly not the prettiest, and I've run into it when setting 
> other generic driver fields. Is there a cleaner way to do this?

Depends on your definition of clean ;) The only thing I can think of is
adding some new macro which hides it. Which is still ugly. Or, of course,
unnamed struct members, which however I think is not supported by gcc < 3.
It may be something to think about.

> pci_register_driver() doesn't inc the refcount. The refcount is held at 1 
> by the module loading code, IIUC, until module_init() is done. After that, 
> the refcount stays at 0 until someone uses it.

Sounds okay. Well, I'm generally lost in too many patches, if you put
the whole thing onto bkbits.net or so, I'll try to give it a look.

--Kai

Re: [patch] PCI device matching fix

[Patrick Mochel]

> Sounds okay. Well, I'm generally lost in too many patches, if you put
> the whole thing onto bkbits.net or so, I'll try to give it a look.

Heh, sure. You can pull from bk://ldm.bkbits.net/linux-2.5-module.

	-pat

Re: [patch] PCI device matching fix

[Sebastian Droege]

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

On Mon, 03 Jun 2002 17:41:40 -0700
Andrew Morton <akpm@zip.com.au> wrote:

> The new pci_device_probe() is always passing the zeroeth
> entry in the id_table to the device's probe method.  It
> needs to scan that table for the correct ID first.
> 
> This fixes the recent 3c59x strangenesses.

This fixes the yamaha ymfpci misdetection, too... Thanks :)

Bye

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]