Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] Don't use a klist for drivers' set-of-devices

5 views
Skip to first unread message

Alan Stern

unread,
Aug 10, 2005, 5:00:15 PM8/10/05
to
Greg and Pat:

This patch (as536) simplifies the driver-model core by replacing the klist
used to store the set of devices bound to a driver with a regular list
protected by a mutex. It turns out that even with a klist, there are too
many opportunities for races for the list to be used safely by more than
one thread at a time. And given that only one thread uses the list at any
moment, there's no need to add all the extra overhead of making it a
klist.

This version of the patch addresses the concerns raised earlier by Pat:
the list and mutex have been given more succinct names, and the obscure
special-case code in device_attach has been replaced with a FIXME comment.
Note that the new iterators in driver.c could easily be changed to use
list_for_each_entry_safe and list_for_each_entry, if the functions didn't
offer the feature of starting in the middle of the list. If no callers
use that feature, it should be removed.

I still think the APIs for device_bind_driver and device_release_driver
need to be changed, because they each rely on dev->drv not changing at a
time when no protecting locks are held. They will have to be fixed up in
a later patch.

Alan Stern

Signed-off-by: Alan Stern <st...@rowland.harvard.edu>

Index: usb-2.6/drivers/base/dd.c
===================================================================
--- usb-2.6.orig/drivers/base/dd.c
+++ usb-2.6/drivers/base/dd.c
@@ -24,28 +24,37 @@
#define to_drv(node) container_of(node, struct device_driver, kobj.entry)


+/*
+ * The caller must hold both @dev->drv->devlist_mutex and
+ * @dev->sem (acquired in that order).
+ */
+static void __device_bind_driver(struct device * dev)
+{
+ pr_debug("bound device '%s' to driver '%s'\n",
+ dev->bus_id, dev->driver->name);
+ list_add_tail(&dev->node_driver, &dev->driver->devlist);
+ sysfs_create_link(&dev->driver->kobj, &dev->kobj,
+ kobject_name(&dev->kobj));
+ sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");
+}
+
/**
* device_bind_driver - bind a driver to one device.
* @dev: device.
*
* Allow manual attachment of a driver to a device.
* Caller must have already set @dev->driver.
- *
- * Note that this does not modify the bus reference count
- * nor take the bus's rwsem. Please verify those are accounted
- * for before calling this. (It is ok to call with no other effort
- * from a driver's probe() method.)
- *
- * This function must be called with @dev->sem held.
*/
void device_bind_driver(struct device * dev)
{
- pr_debug("bound device '%s' to driver '%s'\n",
- dev->bus_id, dev->driver->name);
- klist_add_tail(&dev->driver->klist_devices, &dev->knode_driver);
- sysfs_create_link(&dev->driver->kobj, &dev->kobj,
- kobject_name(&dev->kobj));
- sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");
+ struct device_driver *drv = dev->driver;
+
+ down(&drv->devlist_mutex);
+ down(&dev->sem);
+ if (dev->driver == drv)
+ __device_bind_driver(dev);
+ up(&dev->sem);
+ up(&drv->devlist_mutex);
}

/**
@@ -59,16 +68,21 @@ void device_bind_driver(struct device *
* because we don't know the format of the ID structures, nor what
* is to be considered a match and what is not.
*
- *
* This function returns 1 if a match is found, an error if one
* occurs (that is not -ENODEV or -ENXIO), and 0 otherwise.
*
- * This function must be called with @dev->sem held.
+ * The caller must hold @drv->devlist_mutex.
*/
int driver_probe_device(struct device_driver * drv, struct device * dev)
{
int ret = 0;

+ down(&dev->sem);
+ if (dev->driver) {
+ ret = -EBUSY;
+ goto Done;
+ }
+
if (drv->bus->match && !drv->bus->match(dev, drv))
goto Done;

@@ -82,7 +96,7 @@ int driver_probe_device(struct device_dr
goto ProbeFailed;
}
}
- device_bind_driver(dev);
+ __device_bind_driver(dev);
ret = 1;
pr_debug("%s: Bound Device %s to Driver %s\n",
drv->bus->name, dev->bus_id, drv->name);
@@ -102,13 +116,20 @@ int driver_probe_device(struct device_dr
drv->name, dev->bus_id, ret);
}
Done:
+ up(&dev->sem);
return ret;
}

static int __device_attach(struct device_driver * drv, void * data)
{
struct device * dev = data;
- return driver_probe_device(drv, dev);
+ int ret;
+
+ down(&drv->devlist_mutex);
+ ret = driver_probe_device(drv, dev);
+ up(&drv->devlist_mutex);
+
+ return ret;
}

/**
@@ -124,15 +145,21 @@ static int __device_attach(struct device
*/
int device_attach(struct device * dev)
{
- int ret = 0;
+ int ret = 1;
+ struct device_driver * drv;

- down(&dev->sem);
- if (dev->driver) {
- device_bind_driver(dev);
- ret = 1;
+ drv = dev->driver;
+ if (drv) {
+
+ /* FIXME: What if drv is rmmod'ed right now? */
+
+ down(&drv->devlist_mutex);
+ down(&dev->sem);
+ __device_bind_driver(dev);
+ up(&dev->sem);
+ up(&drv->devlist_mutex);
} else
ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
- up(&dev->sem);
return ret;
}

@@ -141,7 +168,7 @@ static int __driver_attach(struct device
struct device_driver * drv = data;

/*
- * Lock device and try to bind to it. We drop the error
+ * Try to bind drv to dev. We drop the error
* here and always return 0, because we need to keep trying
* to bind to devices and some drivers will return an error
* simply if it didn't support the device.
@@ -150,11 +177,8 @@ static int __driver_attach(struct device
* is an error.
*/

- down(&dev->sem);
if (!dev->driver)
driver_probe_device(drv, dev);
- up(&dev->sem);
-

return 0;
}
@@ -170,46 +194,55 @@ static int __driver_attach(struct device
*/
void driver_attach(struct device_driver * drv)
{
+ down(&drv->devlist_mutex);
bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
+ up(&drv->devlist_mutex);
}

-/**
- * device_release_driver - manually detach device from driver.
- * @dev: device.
- *
- * Manually detach device from driver.
- *
- * __device_release_driver() must be called with @dev->sem held.
- */

-static void __device_release_driver(struct device * dev)
+/*
+ * The caller must hold @driver->devlist_mutex.
+ */
+static int __device_release_driver(struct device * dev, void * driver)
{
- struct device_driver * drv;
+ struct device_driver * drv = driver;

- drv = dev->driver;
- if (drv) {
- get_driver(drv);
+ down(&dev->sem);
+ if (dev->driver == drv) {
sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
sysfs_remove_link(&dev->kobj, "driver");
- klist_remove(&dev->knode_driver);
+ list_del(&dev->node_driver);

if (drv->remove)
drv->remove(dev);
dev->driver = NULL;
- put_driver(drv);
}
+ up(&dev->sem);
+ return 0;
}

+/**
+ * device_release_driver - manually detach device from driver.
+ * @dev: device.
+ *
+ * Manually detach device from driver.
+ */
void device_release_driver(struct device * dev)
{
+ struct device_driver * drv;
+
+ drv = dev->driver;
+ if (!drv)
+ return;
+
/*
* If anyone calls device_release_driver() recursively from
* within their ->remove callback for the same device, they
* will deadlock right here.
*/
- down(&dev->sem);
- __device_release_driver(dev);
- up(&dev->sem);
+ down(&drv->devlist_mutex);
+ __device_release_driver(dev, drv);
+ up(&drv->devlist_mutex);
}


@@ -219,25 +252,7 @@ void device_release_driver(struct device
*/
void driver_detach(struct device_driver * drv)
{
- struct device * dev;
-
- for (;;) {
- spin_lock_irq(&drv->klist_devices.k_lock);
- if (list_empty(&drv->klist_devices.k_list)) {
- spin_unlock_irq(&drv->klist_devices.k_lock);
- break;
- }
- dev = list_entry(drv->klist_devices.k_list.prev,
- struct device, knode_driver.n_node);
- get_device(dev);
- spin_unlock_irq(&drv->klist_devices.k_lock);
-
- down(&dev->sem);
- if (dev->driver == drv)
- __device_release_driver(dev);
- up(&dev->sem);
- put_device(dev);
- }
+ driver_for_each_device(drv, NULL, drv, __device_release_driver);
}


Index: usb-2.6/drivers/base/driver.c
===================================================================
--- usb-2.6.orig/drivers/base/driver.c
+++ usb-2.6/drivers/base/driver.c
@@ -15,16 +15,10 @@
#include <linux/string.h>
#include "base.h"

-#define to_dev(node) container_of(node, struct device, driver_list)
+#define to_dev(node) container_of(node, struct device, node_driver)
#define to_drv(obj) container_of(obj, struct device_driver, kobj)


-static struct device * next_device(struct klist_iter * i)
-{
- struct klist_node * n = klist_next(i);
- return n ? container_of(n, struct device, knode_driver) : NULL;
-}
-
/**
* driver_for_each_device - Iterator for devices bound to a driver.
* @drv: Driver we're iterating.
@@ -37,18 +31,22 @@ static struct device * next_device(struc
int driver_for_each_device(struct device_driver * drv, struct device * start,
void * data, int (*fn)(struct device *, void *))
{
- struct klist_iter i;
- struct device * dev;
+ struct list_head * ptr, * tmp;
int error = 0;

if (!drv)
return -EINVAL;

- klist_iter_init_node(&drv->klist_devices, &i,
- start ? &start->knode_driver : NULL);
- while ((dev = next_device(&i)) && !error)
- error = fn(dev, data);
- klist_iter_exit(&i);
+ down(&drv->devlist_mutex);
+ ptr = (start ? &start->node_driver : drv->devlist.next);
+ while (ptr != &drv->devlist) {
+ tmp = ptr->next;
+ error = fn(to_dev(ptr), data);
+ if (error)
+ break;
+ ptr = tmp;
+ }
+ up(&drv->devlist_mutex);
return error;
}

@@ -74,18 +72,22 @@ struct device * driver_find_device(struc
struct device * start, void * data,
int (*match)(struct device *, void *))
{
- struct klist_iter i;
- struct device *dev;
+ struct list_head * ptr;
+ struct device *dev = NULL;

if (!drv)
return NULL;

- klist_iter_init_node(&drv->klist_devices, &i,
- (start ? &start->knode_driver : NULL));
- while ((dev = next_device(&i)))
+ down(&drv->devlist_mutex);
+ ptr = (start ? &start->node_driver : drv->devlist.next);
+ for (; ptr != &drv->devlist; ptr = ptr->next) {
+ dev = to_dev(ptr);
if (match(dev, data) && get_device(dev))
break;
- klist_iter_exit(&i);
+ }
+ if (ptr == &drv->devlist)
+ dev = NULL;
+ up(&drv->devlist_mutex);
return dev;
}
EXPORT_SYMBOL_GPL(driver_find_device);
@@ -157,7 +159,8 @@ void put_driver(struct device_driver * d
*/
int driver_register(struct device_driver * drv)
{
- klist_init(&drv->klist_devices);
+ INIT_LIST_HEAD(&drv->devlist);
+ init_MUTEX(&drv->devlist_mutex);
init_completion(&drv->unloaded);
return bus_add_driver(drv);
}
Index: usb-2.6/include/linux/device.h
===================================================================
--- usb-2.6.orig/include/linux/device.h
+++ usb-2.6/include/linux/device.h
@@ -107,7 +107,8 @@ struct device_driver {

struct completion unloaded;
struct kobject kobj;
- struct klist klist_devices;
+ struct list_head devlist;
+ struct semaphore devlist_mutex;
struct klist_node knode_bus;

struct module * owner;
@@ -270,7 +271,7 @@ extern void class_device_destroy(struct
struct device {
struct klist klist_children;
struct klist_node knode_parent; /* node in sibling list */
- struct klist_node knode_driver;
+ struct list_head node_driver;
struct klist_node knode_bus;
struct device * parent;

Index: usb-2.6/drivers/base/bus.c
===================================================================
--- usb-2.6.orig/drivers/base/bus.c
+++ usb-2.6/drivers/base/bus.c
@@ -133,7 +133,7 @@ static struct kobj_type ktype_bus = {
decl_subsys(bus, &ktype_bus, NULL);


-/* Manually detach a device from it's associated driver. */
+/* Manually detach a device from its associated driver. */
static int driver_helper(struct device *dev, void *data)
{
const char *name = data;
@@ -175,9 +175,9 @@ static ssize_t driver_bind(struct device
dev = bus_find_device(bus, NULL, (void *)buf, driver_helper);
if ((dev) &&
(dev->driver == NULL)) {
- down(&dev->sem);
+ down(&drv->devlist_mutex);
err = driver_probe_device(drv, dev);
- up(&dev->sem);
+ up(&drv->devlist_mutex);
put_device(dev);
}
return err;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH

unread,
Aug 11, 2005, 2:30:12 PM8/11/05
to
On Wed, Aug 10, 2005 at 04:56:08PM -0400, Alan Stern wrote:
> Greg and Pat:
>
> This patch (as536) simplifies the driver-model core by replacing the klist
> used to store the set of devices bound to a driver with a regular list
> protected by a mutex. It turns out that even with a klist, there are too
> many opportunities for races for the list to be used safely by more than
> one thread at a time. And given that only one thread uses the list at any
> moment, there's no need to add all the extra overhead of making it a
> klist.

Hm, but that was the whole reason to go to a klist in the first place.

> This version of the patch addresses the concerns raised earlier by Pat:
> the list and mutex have been given more succinct names, and the obscure
> special-case code in device_attach has been replaced with a FIXME comment.
> Note that the new iterators in driver.c could easily be changed to use
> list_for_each_entry_safe and list_for_each_entry, if the functions didn't
> offer the feature of starting in the middle of the list. If no callers
> use that feature, it should be removed.

Pat, any opinions on this patch?

thanks,

greg k-h

Christoph Hellwig

unread,
Aug 11, 2005, 3:00:19 PM8/11/05
to
On Thu, Aug 11, 2005 at 11:24:23AM -0700, Greg KH wrote:
> > This patch (as536) simplifies the driver-model core by replacing the klist
> > used to store the set of devices bound to a driver with a regular list
> > protected by a mutex. It turns out that even with a klist, there are too
> > many opportunities for races for the list to be used safely by more than
> > one thread at a time. And given that only one thread uses the list at any
> > moment, there's no need to add all the extra overhead of making it a
> > klist.
>
> Hm, but that was the whole reason to go to a klist in the first place.

And shows once more that the klist approach was totally misguided.

Alan Stern

unread,
Aug 11, 2005, 3:50:25 PM8/11/05
to
On Thu, 11 Aug 2005, Christoph Hellwig wrote:

> On Thu, Aug 11, 2005 at 11:24:23AM -0700, Greg KH wrote:
> > > This patch (as536) simplifies the driver-model core by replacing the klist
> > > used to store the set of devices bound to a driver with a regular list
> > > protected by a mutex. It turns out that even with a klist, there are too
> > > many opportunities for races for the list to be used safely by more than
> > > one thread at a time. And given that only one thread uses the list at any
> > > moment, there's no need to add all the extra overhead of making it a
> > > klist.
> >
> > Hm, but that was the whole reason to go to a klist in the first place.
>
> And shows once more that the klist approach was totally misguided.

I'll let Pat answer Christoph's comment.

Do note that the bus's list of devices and the bus's list of registered
drivers are still klists. Only the driver's list of bound devices gets
reverted to a normal list.

Alan Stern

Dmitry Torokhov

unread,
Aug 11, 2005, 4:40:09 PM8/11/05
to
On 8/11/05, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 11 Aug 2005, Christoph Hellwig wrote:
>
> > On Thu, Aug 11, 2005 at 11:24:23AM -0700, Greg KH wrote:
> > > > This patch (as536) simplifies the driver-model core by replacing the klist
> > > > used to store the set of devices bound to a driver with a regular list
> > > > protected by a mutex. It turns out that even with a klist, there are too
> > > > many opportunities for races for the list to be used safely by more than
> > > > one thread at a time. And given that only one thread uses the list at any
> > > > moment, there's no need to add all the extra overhead of making it a
> > > > klist.
> > >
> > > Hm, but that was the whole reason to go to a klist in the first place.
> >
> > And shows once more that the klist approach was totally misguided.
>
> I'll let Pat answer Christoph's comment.
>
> Do note that the bus's list of devices and the bus's list of registered
> drivers are still klists. Only the driver's list of bound devices gets
> reverted to a normal list.
>

Hmm, so what do I do in the following scenario - I have a serio port
(AUX) that has a synaptics touchpad bound to it which is driven by
psmouse driver. psmouse driver registers a child port (synaptics
pass-through) during probe call. The child port is also driven by
psmouse module - but it looks like it will deadlock when binding.

Am I missing something here?

--
Dmitry

Alan Stern

unread,
Aug 11, 2005, 5:00:14 PM8/11/05
to
On Thu, 11 Aug 2005, Dmitry Torokhov wrote:

> Hmm, so what do I do in the following scenario - I have a serio port
> (AUX) that has a synaptics touchpad bound to it which is driven by
> psmouse driver. psmouse driver registers a child port (synaptics
> pass-through) during probe call. The child port is also driven by
> psmouse module - but it looks like it will deadlock when binding.
>
> Am I missing something here?

I hate to say this, but you are right. Can you suggest a way around this
problem? Perhaps arranging things so that the devlist_mutex is held only
during the actual __device_bind_driver call and not during probe... But
there are so many tricky interactions and possible races that this
requires a lot of thought. I'll get back to you.

Alan Stern

Alan Stern

unread,
Aug 12, 2005, 9:50:10 AM8/12/05
to
On Thu, 11 Aug 2005, Dmitry Torokhov wrote:

> Hmm, so what do I do in the following scenario - I have a serio port
> (AUX) that has a synaptics touchpad bound to it which is driven by
> psmouse driver. psmouse driver registers a child port (synaptics
> pass-through) during probe call. The child port is also driven by
> psmouse module - but it looks like it will deadlock when binding.

Dmitry's point is well founded. Greg, I want to retract that klist patch
(as536). I'll send in a revised version before long.

It looks like the best approach will simply be to eliminate the driver's
list of bound devices altogether. That should make Christoph happy -- no
klist, no list, no nothing! :-) The list hardly ever gets used, mainly
when the driver is unloaded. We can already get the same effect by
iterating over the bus's list of devices and skipping everything not bound
to the driver.

This will eliminate a whole set of races and make the code more
transparent (I hope).

Alan Stern

Alan Stern

unread,
Aug 15, 2005, 5:30:11 PM8/15/05
to
Dmitry, Pat, Greg, and everyone else:

This is a revised version of the patch I sent in last week -- not yet a
submission, more of an RFC. It turns out that removing the driver's list
of bound devices isn't practical, because there are times when a device
not yet on the bus's overall klist will be bound (this happens whenever a
new device is registered, for instance).

Anyway, in this new version the locking is greatly simplified and the
requirements are made much more explicit in the comments. This code will
not be subject to deadlock when a driver's probe routine registers a child
device that uses the same driver, or when a remove routine unregisters
a child device from the same driver. I think this version is okay, but
please look it over and see if anything looks amiss.

The next step will be to clean up the races inherent in device_bind_driver
and device_release_driver. There are two obvious approaches:

Pass the driver as an argument rather than trusting the value
stored in dev->driver.

Require the caller to lock dev->sem.

On the face of it, neither is particularly more attractive than the other.
However, reading through the various places that call these routines (for
example, drivers/input/serio/serio.c or drivers/pnp/card.c) revealed a
pattern. In most cases, the callers of device_bind_driver _really_ want
something that functions more like driver_probe_device but without the
matching step. The callers are invoking the probe methods by hand rather
than relying on the driver core to do so. There's maybe only one place
where a caller actually does want the device bound to the driver without
doing a probe.

This suggests it might be a good idea to split driver_probe_device into
two pieces, one for matching and one for probing & binding. The second
piece could then be used instead of device_bind_driver.

Another thing became apparent as well. All of the places that use these
routines currently rely on the bus subsystem's rwsem for synchronization.
Obviously this is bad, because that rwsem is on its way out. I don't know
enough about all those different drivers to be able to tell whether they
lock the rwsem merely because the old API required it or because they
really do need some mutual exclusion. Still, this suggests that it might
be best to require the callers to lock dev->sem -- i.e., use dev->sem sort
of as a replacement for the bus rwsem.

Comments?

Alan Stern

Index: usb-2.6/drivers/base/dd.c
===================================================================
--- usb-2.6.orig/drivers/base/dd.c
+++ usb-2.6/drivers/base/dd.c

@@ -21,31 +21,61 @@
#include "base.h"
#include "power/power.h"

-#define to_drv(node) container_of(node, struct device_driver, kobj.entry)

+/*
+ * @dev must be guaranteed not to be unregistered, in the process
+ * of registration, or else pinned by its knode_bus.
+ * The caller must hold @dev->sem.
+ */
+static int __device_bind_driver(struct device * dev)
+{
+ struct device_driver * drv = dev->driver;


+ int ret;
+
+ down(&drv->devlist_mutex);
+

+ /* Make sure that drv is registered */
+ if (klist_node_attached(&drv->knode_bus)) {


+ pr_debug("bound device '%s' to driver '%s'\n",
+ dev->bus_id, dev->driver->name);
+ list_add_tail(&dev->node_driver, &dev->driver->devlist);
+ sysfs_create_link(&dev->driver->kobj, &dev->kobj,
+ kobject_name(&dev->kobj));
+ sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");

+ ret = 0;
+ } else
+ ret = -EIDRM;
+
+ up(&drv->devlist_mutex);
+ return ret;


+}

/**
* device_bind_driver - bind a driver to one device.
* @dev: device.
*
* Allow manual attachment of a driver to a device.

- * Caller must have already set @dev->driver.


*
- * Note that this does not modify the bus reference count
- * nor take the bus's rwsem. Please verify those are accounted
- * for before calling this. (It is ok to call with no other effort
- * from a driver's probe() method.)
- *
- * This function must be called with @dev->sem held.

- */
-void device_bind_driver(struct device * dev)
-{


- pr_debug("bound device '%s' to driver '%s'\n",
- dev->bus_id, dev->driver->name);
- klist_add_tail(&dev->driver->klist_devices, &dev->knode_driver);
- sysfs_create_link(&dev->driver->kobj, &dev->kobj,
- kobject_name(&dev->kobj));
- sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");

+ * Caller must have already set @dev->driver.
+ * @dev must be guaranteed not to be unregistered, in the process
+ * of registration, or else pinned by its knode_bus.
+ */
+int device_bind_driver(struct device * dev)
+{


+ struct device_driver *drv = dev->driver;

+ int ret;
+
+ /* Make sure dev hasn't been unregistered or bound to
+ * a different driver */
+ down(&dev->sem);
+ if (!klist_node_attached(&dev->knode_bus))
+ ret = -ENODEV;
+ else if (dev->driver != drv)
+ ret = -EBUSY;
+ else
+ ret = __device_bind_driver(dev);
+ up(&dev->sem);
+ return ret;
}

/**
@@ -59,16 +89,22 @@ void device_bind_driver(struct device *

* because we don't know the format of the ID structures, nor what
* is to be considered a match and what is not.
*
- *
* This function returns 1 if a match is found, an error if one
* occurs (that is not -ENODEV or -ENXIO), and 0 otherwise.
*
- * This function must be called with @dev->sem held.

+ * @dev must be guaranteed not to be unregistered, in the process
+ * of registration, or else pinned by its knode_bus.


*/
int driver_probe_device(struct device_driver * drv, struct device * dev)
{
int ret = 0;

+ down(&dev->sem);
+ if (dev->driver) {
+ ret = -EBUSY;
+ goto Done;
+ }
+
if (drv->bus->match && !drv->bus->match(dev, drv))
goto Done;

@@ -79,35 +115,42 @@ int driver_probe_device(struct device_dr
ret = drv->probe(dev);
if (ret) {
dev->driver = NULL;
- goto ProbeFailed;
+ if (ret == -ENODEV || ret == -ENXIO) {
+ /* Driver matched, but didn't support device
+ * or device not found.
+ * Not an error; keep going.
+ */
+ ret = 0;
+ } else {
+ /* driver matched but the probe failed */
+ printk(KERN_WARNING "%s: probe of %s failed "
+ "with error %d\n",
+ drv->name, dev->bus_id, ret);
+ }
+ goto Done;
}
}
- device_bind_driver(dev);
+
+ if (__device_bind_driver(dev) != 0) {
+ /* Driver was unregistered; keep going */
+ if (drv->remove)
+ drv->remove(dev);
+ dev->driver = NULL;
+ goto Done;
+ }


ret = 1;
pr_debug("%s: Bound Device %s to Driver %s\n",
drv->bus->name, dev->bus_id, drv->name);

- goto Done;

- ProbeFailed:
- if (ret == -ENODEV || ret == -ENXIO) {
- /* Driver matched, but didn't support device
- * or device not found.
- * Not an error; keep going.
- */
- ret = 0;
- } else {
- /* driver matched but the probe failed */
- printk(KERN_WARNING
- "%s: probe of %s failed with error %d\n",
- drv->name, dev->bus_id, ret);
- }


Done:
+ up(&dev->sem);
return ret;
}

static int __device_attach(struct device_driver * drv, void * data)
{
struct device * dev = data;

+
return driver_probe_device(drv, dev);
}

@@ -121,18 +164,28 @@ static int __device_attach(struct device
*
* Returns 1 if the device was bound to a driver;
* 0 if no matching device was found; error code otherwise.
+ *
+ * @dev must be guaranteed not to be unregistered, in the process
+ * of registration, or else pinned by its knode_bus.


*/
int device_attach(struct device * dev)
{
- int ret = 0;
+ int ret = 1;

+ struct device_driver * drv = dev->driver;



- down(&dev->sem);
- if (dev->driver) {
- device_bind_driver(dev);
- ret = 1;

- } else
+ if (drv) {
+ down(&dev->sem);
+
+ /* Make sure dev hasn't been bound to a different driver */
+ if (dev->driver != drv)
+ ret = -EBUSY;
+ else if (__device_bind_driver(dev) != 0)
+ drv = dev->driver = NULL;
+ up(&dev->sem);
+ }
+
+ if (!drv)


ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
- up(&dev->sem);
return ret;
}

@@ -141,7 +194,7 @@ static int __driver_attach(struct device


struct device_driver * drv = data;

/*
- * Lock device and try to bind to it. We drop the error
+ * Try to bind drv to dev. We drop the error
* here and always return 0, because we need to keep trying
* to bind to devices and some drivers will return an error
* simply if it didn't support the device.

@@ -150,12 +203,8 @@ static int __driver_attach(struct device


* is an error.
*/

- down(&dev->sem);
if (!dev->driver)
driver_probe_device(drv, dev);
- up(&dev->sem);
-

-
return 0;
}

@@ -173,43 +222,48 @@ void driver_attach(struct device_driver

bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
}

-/**
- * device_release_driver - manually detach device from driver.
- * @dev: device.
- *
- * Manually detach device from driver.
- *
- * __device_release_driver() must be called with @dev->sem held.
- */

-static void __device_release_driver(struct device * dev)
+/*

+ * It's okay for @dev or @driver (or both) to be unregistered.
+ */
+static void __device_release_driver(struct device * dev, void * driver)


{
- struct device_driver * drv;
+ struct device_driver * drv = driver;

- drv = dev->driver;
- if (drv) {
- get_driver(drv);

+ /*
+ * If anyone calls device_release_driver() recursively from
+ * within their ->remove callback for the same device, they
+ * will deadlock right here.
+ */
+ down(&dev->sem);
+
+ /* Make sure dev hasn't already been unbound from drv */


+ if (dev->driver == drv) {
sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
sysfs_remove_link(&dev->kobj, "driver");
- klist_remove(&dev->knode_driver);

+ down(&drv->devlist_mutex);
+ list_del(&dev->node_driver);
+ up(&drv->devlist_mutex);



if (drv->remove)
drv->remove(dev);
dev->driver = NULL;
- put_driver(drv);
}
+ up(&dev->sem);
}

+/**
+ * device_release_driver - manually detach device from driver.
+ * @dev: device.
+ *
+ * Manually detach device from driver.
+ */
void device_release_driver(struct device * dev)
{

- /*
- * If anyone calls device_release_driver() recursively from
- * within their ->remove callback for the same device, they
- * will deadlock right here.
- */


- down(&dev->sem);
- __device_release_driver(dev);
- up(&dev->sem);

+ struct device_driver * drv = dev->driver;
+
+ if (drv)
+ __device_release_driver(dev, drv);
}


@@ -221,21 +275,22 @@ void driver_detach(struct device_driver
{
struct device * dev;

+ /* Note: we can't use driver_for_each_device here, because clients
+ * of that routine are forbidden to unbind the device. */


for (;;) {
- spin_lock_irq(&drv->klist_devices.k_lock);
- if (list_empty(&drv->klist_devices.k_list)) {
- spin_unlock_irq(&drv->klist_devices.k_lock);
- break;

+ down(&drv->devlist_mutex);
+ if (list_empty(&drv->devlist))
+ dev = NULL;
+ else {
+ dev = list_entry(drv->devlist.prev,
+ struct device, node_driver);
+ get_device(dev);


}
- dev = list_entry(drv->klist_devices.k_list.prev,
- struct device, knode_driver.n_node);
- get_device(dev);
- spin_unlock_irq(&drv->klist_devices.k_lock);

+ up(&drv->devlist_mutex);



- down(&dev->sem);
- if (dev->driver == drv)
- __device_release_driver(dev);
- up(&dev->sem);

+ if (!dev)
+ break;
+ __device_release_driver(dev, drv);
put_device(dev);


}
}
Index: usb-2.6/drivers/base/driver.c
===================================================================
--- usb-2.6.orig/drivers/base/driver.c
+++ usb-2.6/drivers/base/driver.c
@@ -15,16 +15,10 @@
#include <linux/string.h>
#include "base.h"

-#define to_dev(node) container_of(node, struct device, driver_list)
+#define to_dev(node) container_of(node, struct device, node_driver)
#define to_drv(obj) container_of(obj, struct device_driver, kobj)


-static struct device * next_device(struct klist_iter * i)
-{
- struct klist_node * n = klist_next(i);
- return n ? container_of(n, struct device, knode_driver) : NULL;
-}
-
/**
* driver_for_each_device - Iterator for devices bound to a driver.
* @drv: Driver we're iterating.

@@ -32,23 +26,31 @@ static struct device * next_device(struc
* @fn: Function to call for each device.
*
* Iterate over the @drv's list of devices calling @fn for each one.
+ *
+ * @fn must not attempt to unbind the device it gets -- the attempt
+ * would deadlock because we will be holding @drv->devlist_mutex.
+ * @fn also must not attempt to lock the device; this would deadlock
+ * if another thread was trying to unbind it.
*/



int driver_for_each_device(struct device_driver * drv, struct device * start,
void * data, int (*fn)(struct device *, void *))
{
- struct klist_iter i;
- struct device * dev;

+ struct list_head * ptr;


int error = 0;

if (!drv)
return -EINVAL;

- klist_iter_init_node(&drv->klist_devices, &i,
- start ? &start->knode_driver : NULL);
- while ((dev = next_device(&i)) && !error)
- error = fn(dev, data);
- klist_iter_exit(&i);
+ down(&drv->devlist_mutex);

+ for (ptr = (start ? &start->node_driver : drv->devlist.next);
+ ptr != &drv->devlist;
+ ptr = ptr->next) {


+ error = fn(to_dev(ptr), data);
+ if (error)
+ break;
+ }

+ up(&drv->devlist_mutex);
return error;
}

@@ -74,18 +76,23 @@ struct device * driver_find_device(struc


struct device * start, void * data,
int (*match)(struct device *, void *))
{
- struct klist_iter i;
- struct device *dev;
+ struct list_head * ptr;
+ struct device *dev = NULL;

if (!drv)
return NULL;

- klist_iter_init_node(&drv->klist_devices, &i,
- (start ? &start->knode_driver : NULL));
- while ((dev = next_device(&i)))
+ down(&drv->devlist_mutex);

+ for (ptr = (start ? &start->node_driver : drv->devlist.next);
+ ptr != &drv->devlist;
+ ptr = ptr->next) {


+ dev = to_dev(ptr);
if (match(dev, data) && get_device(dev))
break;
- klist_iter_exit(&i);
+ }
+ if (ptr == &drv->devlist)
+ dev = NULL;
+ up(&drv->devlist_mutex);
return dev;
}
EXPORT_SYMBOL_GPL(driver_find_device);

@@ -157,7 +164,8 @@ void put_driver(struct device_driver * d

@@ -278,8 +279,7 @@ struct device {
char bus_id[BUS_ID_SIZE]; /* position on parent bus */

struct semaphore sem; /* semaphore to synchronize calls to
- * its driver.
- */
+ * its driver. */

struct bus_type * bus; /* type of bus device is on */
struct device_driver *driver; /* which driver has allocated this
@@ -333,7 +333,7 @@ extern int device_for_each_child(struct
* Manual binding of a device to driver. See drivers/base/bus.c
* for information on use.
*/
-extern void device_bind_driver(struct device * dev);
+extern int device_bind_driver(struct device * dev);
extern void device_release_driver(struct device * dev);
extern int device_attach(struct device * dev);
extern void driver_attach(struct device_driver * drv);


Index: usb-2.6/drivers/base/bus.c
===================================================================
--- usb-2.6.orig/drivers/base/bus.c
+++ usb-2.6/drivers/base/bus.c
@@ -133,7 +133,7 @@ static struct kobj_type ktype_bus = {
decl_subsys(bus, &ktype_bus, NULL);


-/* Manually detach a device from it's associated driver. */
+/* Manually detach a device from its associated driver. */
static int driver_helper(struct device *dev, void *data)
{
const char *name = data;

@@ -173,11 +173,12 @@ static ssize_t driver_bind(struct device
int err = -ENODEV;



dev = bus_find_device(bus, NULL, (void *)buf, driver_helper);

- if ((dev) &&
- (dev->driver == NULL)) {
- down(&dev->sem);
- err = driver_probe_device(drv, dev);
- up(&dev->sem);
+ if (dev) {
+ /* Make sure dev doesn't get unregistered */
+ if (klist_get(&dev->knode_bus)) {
+ err = driver_probe_device(drv, dev);
+ klist_del(&dev->knode_bus);
+ }
put_device(dev);
}
return err;
@@ -443,8 +444,8 @@ int bus_add_driver(struct device_driver
return error;
}

- driver_attach(drv);
klist_add_tail(&bus->klist_drivers, &drv->knode_bus);
+ driver_attach(drv);
module_add_driver(drv->owner, drv);

driver_add_attrs(bus, drv);
Index: usb-2.6/lib/klist.c
===================================================================
--- usb-2.6.orig/lib/klist.c
+++ usb-2.6/lib/klist.c
@@ -122,6 +122,31 @@ static int klist_dec_and_del(struct klis


/**
+ * klist_get - Increment the reference count of node
+ * @n: node we're pinning.
+ *
+ * Returns NULL if @n is not on a klist, @n otherwise.
+ */
+
+struct klist_node * klist_get(struct klist_node * n)
+{
+ struct klist * k = n->n_klist;
+
+ if (!k)
+ return NULL;
+ spin_lock(&k->k_lock);
+ if (n->n_klist == k)
+ kref_get(&n->n_ref);
+ else
+ n = NULL;
+ spin_unlock(&k->k_lock);
+ return n;
+}
+
+EXPORT_SYMBOL_GPL(klist_get);
+
+
+/**
* klist_del - Decrement the reference count of node and try to remove.
* @n: node we're deleting.
*/
Index: usb-2.6/include/linux/klist.h
===================================================================
--- usb-2.6.orig/include/linux/klist.h
+++ usb-2.6/include/linux/klist.h
@@ -34,6 +34,7 @@ struct klist_node {
extern void klist_add_tail(struct klist * k, struct klist_node * n);
extern void klist_add_head(struct klist * k, struct klist_node * n);

+extern struct klist_node * klist_get(struct klist_node * n);
extern void klist_del(struct klist_node * n);
extern void klist_remove(struct klist_node * n);

Dmitry Torokhov

unread,
Aug 16, 2005, 6:40:09 PM8/16/05
to
On 8/15/05, Alan Stern <st...@rowland.harvard.edu> wrote:

> On the face of it, neither is particularly more attractive than the other.
> However, reading through the various places that call these routines (for
> example, drivers/input/serio/serio.c or drivers/pnp/card.c) revealed a
> pattern. In most cases, the callers of device_bind_driver _really_ want
> something that functions more like driver_probe_device but without the
> matching step. The callers are invoking the probe methods by hand rather
> than relying on the driver core to do so. There's maybe only one place
> where a caller actually does want the device bound to the driver without
> doing a probe.
>

Alan,

I am sorry I don't have time to properly review the patch at
themoment, just a couple of comments about serio - I would not look at
serio for examples of typical use as it was trying very hard to work
around the original driver model limitation that prevented drivers to
register childern on the same bus from their probe function. I think
now that that limitation is lifted serio implemenation can be
simplified.

--
Dmitry

Alan Stern

unread,
Aug 17, 2005, 10:00:20 AM8/17/05
to
On Tue, 16 Aug 2005, Dmitry Torokhov wrote:

> Alan,
>
> I am sorry I don't have time to properly review the patch at
> themoment, just a couple of comments about serio - I would not look at
> serio for examples of typical use as it was trying very hard to work
> around the original driver model limitation that prevented drivers to
> register childern on the same bus from their probe function. I think
> now that that limitation is lifted serio implemenation can be
> simplified.

Okay. The same comments may apply to the other users of
device_bind_driver. Apart from a couple in the USB stack that I can
handle already, those users are:

drivers/pnp/card.c
drivers/input/serio/serio.c
drivers/input/gameport/gameport.c

Presumably the gameport code is very similar to the serio code.
Interestingly, callers of device_release_driver include:

drivers/pnp/card.c
drivers/input/serio/serio.c
drivers/input/gameport/gameport.c
drivers/ide/ide-proc.c
drivers/ieee1394/nodemgr.c

It's not obvious (to me) why ide-proc.c and nodemgr.c call one but not the
other.

Anyway, I appreciate you taking the time to comment on this work. When
you've had a chance to read through that patch, let me know what you
think.

Alan Stern

Bartlomiej Zolnierkiewicz

unread,
Aug 17, 2005, 10:40:14 AM8/17/05
to
Hi,

ide-proc.c calls device_attach() instead of device_bind_driver()

"driver" interface in ide-proc.c will be scheduled for removal
since proper bind/unbind sysfs interface is available now.

Bartlomiej

0 new messages