Re: [PATCH] Don't use a klist for drivers' set-of-devices
Hi, On 8/17/05, Alan Stern <[EMAIL PROTECTED]> wrote: > 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. 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
On 8/15/05, Alan Stern <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 u
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
On 8/11/05, Alan Stern <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't use a klist for drivers' set-of-devices
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Don't use a klist for drivers' set-of-devices
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 <[EMAIL PROTECTED]>
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
