driver core: allow userspace to unbind drivers from devices.

Ok, everone's been back and forth about the whole bind/unbind stuff lately, so let's just do this a step at a time.

How about the following patch. It adds a "unbind" file to any device that is bound to a driver. Writing any value to that file disconnects the device from the driver associated with it.

It's small, simple, and it works.

It also can cause bad things to happen if you aren't careful about what type of device you are unbinding (some i2c chip devices don't really unbind from the driver fully, but that's an i2c issue, and I'm working on it.)

Also, unbinding a device from a driver can cause the children devices to disappear, depending on the type of driver that is bound to the device.

As an example, a usb-storage device, that has a scsi-host, and scsi devices as children. If you unbind the usb-storage device, the scsi-host and devices are all removed from the system (as they should be.)

I put "Signed-off-by:" for both Tejun and Dmitry, as this patch is taken from both of their implementations that they have been posting to lkml recently. It's just that this one is smaller, and hence, more correct :)

Comments?

If we can agree on this, we can move on to the "bind from userspace" stuff next.

thanks,

greg k-h

------

From: [email protected]
Subject: driver core: allow userspace to unbind drivers from devices.

diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
@@ -243,6 +243,17 @@
 	return ret;
 }
 
+/* manually detach a device from it's associated driver. */
+/* Any write will cause it to happen. */
+static ssize_t device_unbind(struct device *dev, const char *buf, size_t count)
+{
+	down_write(&dev->bus->subsys.rwsem);
+	device_release_driver(dev);
+	up_write(&dev->bus->subsys.rwsem);
+	return count;
+}
+static DEVICE_ATTR(unbind, S_IWUSR, NULL, device_unbind);
+
 /**
  *	device_bind_driver - bind a driver to one device.
  *	@dev:	device.

@@ -264,6 +275,7 @@
 	sysfs_create_link(&dev->driver->kobj, &dev->kobj,
 			  kobject_name(&dev->kobj));
 	sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");
+	device_create_file(dev, &dev_attr_unbind);
 }
 
 

@@ -389,6 +401,7 @@
 	if (drv) {
 		sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
 		sysfs_remove_link(&dev->kobj, "driver");
+		device_remove_file(dev, &dev_attr_unbind);
 		list_del_init(&dev->driver_list);
 		device_detach_shutdown(dev);
 		if (drv->remove)

Hi Greg,

With the present implementation it is pretty much impossible to do since unbind grabs bus's rwsem. That means that any driver attempting to remove children will deadlock. Driver core is not aware of evry bus's topology issues that's why you need a bus method to do proper locking and children removal.

What about unbinding USB hub driver? It will hang because you can not remove children on the same bus. In serio the core takes care of removing any children before unbinding the driver, but again, this is bus-specific implementation. The bus knows how to handle this.

I also have issue with doing it in steps - it will cause every device have 3 or 4 method-attributes - unbind, bind, rescan, [reconnect]. They all implement very similar action - control link between device and driver. I do not see the reason for splitting them apart and it will be a waste of resources to have all of them as well.

Dmitry

Ok, in looking some more at this, you are right.

So, how to solve this. I don't want a bus callback for bind, unbind, re-bind, bind-every-other-day-depending-on-the-phase-of-the-moon, and so on. I really think we can do this with the current callbacks that we have today.

All we have to do is rework that rwsem lock. It's been staring at us since the beginning of the driver core work, and we knew it would have to be fixed eventually. So might as well do it now.

Unfortunatly it seems everyone likes to grab that lock, so it's not going to be a simple thing to fix (in their defense, we encouraged people to grab the lock, so it's our fault too.)

So, if we fix up the locking issues, then we can do the following, which I know people have been wanting to do:
- add new devices from within a probe() callback.
- remove other devices from within a probe() callback.
- remove other devices from the remove() callback.
- unbind devices from the driver core.

and other good stuff.

So, off to rework this mess...

thanks,

greg k-h

...

Do you have any ideas here? For me it seems that the semaphore has a dual role - protecting bus's lists and ensuring that probe/remove routines are serialized across bus...

In the meantime, can I please have bind_mode patch applied? I believe it is useful regardless of which bind/unbind solution will be adopted and having them will allow me clean up serio bus implementaion quite a bit.

Pretty please...

Dmitry

Care to resend it? I can't find it in my archives.

thanks,

greg k-h

Here it is, against 2.6.10-rc2. Apologies for sending it as an attachment but this interface will not let me put it inline without mangling.

Dmitry

===================================================================

[email protected], 2004-11-16 02:07:15-05:00, [email protected]
Driver core: add "bind_mode" default device and driver attribute.
Calls to device_attach() and driver_attach() will not bind device and driver if either one of them is in "manual" bind mode. Manual binding is expected to be handled by bus's drvctl()

echo -n "manual" > /sus/bus/serio/devices/serio2/bind_mode echo -n "auto" > /sys/bus/serio/drivers/serio_raw/bind_mode

 drivers/base/bus.c              |   20 +++++++----
 drivers/base/interface.c        |   70 +++++++++++++++++++++++++++++++++++++---
 drivers/input/serio/serio.c     |   58 ++-------------------------------
 drivers/input/serio/serio_raw.c |    4 +-
 include/linux/device.h          |    5 ++
 include/linux/serio.h           |    4 --
 6 files changed, 89 insertions(+), 72 deletions(-)

===================================================================

diff -urN a/drivers/base/bus.c b/drivers/base/bus.c
@@ -68,9 +68,12 @@
 	up(&drv->unload_sem);
 }
 
+extern struct attribute * drv_default_attrs[];
+
 static struct kobj_type ktype_driver = {
 	.sysfs_ops	= &driver_sysfs_ops,
 	.release	= driver_release,
+	.default_attrs	= drv_default_attrs,
 };
 
 

@@ -319,9 +322,12 @@
 		return 1;
 	}
 
-	if (bus->match) {
-		list_for_each(entry, &bus->drivers.list) {
-			struct device_driver * drv = to_drv(entry);
+	if (dev->manual_bind || !bus->match)
+		return 0;
+
+	list_for_each(entry, &bus->drivers.list) {
+		struct device_driver * drv = to_drv(entry);
+		if (!drv->manual_bind) {
 			error = driver_probe_device(drv, dev);
 			if (!error)
 				/* success, driver matched */

@@ -329,8 +335,8 @@
 			if (error != -ENODEV)
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING
-				    "%s: probe of %s failed with error %d\n",
-				    drv->name, dev->bus_id, error);
+					"%s: probe of %s failed with error %d\n",
+					drv->name, dev->bus_id, error);
 		}
 	}
 

@@ -356,12 +362,12 @@
 	struct list_head * entry;
 	int error;
 
-	if (!bus->match)
+	if (drv->manual_bind || !bus->match)
 		return;
 
 	list_for_each(entry, &bus->devices.list) {
 		struct device * dev = container_of(entry, struct device, bus_list);
-		if (!dev->driver) {
+		if (!dev->driver && !dev->manual_bind) {
 			error = driver_probe_device(drv, dev);
 			if (error && (error != -ENODEV))
 				/* driver matched but the probe failed */

diff -urN a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
@@ -365,14 +365,14 @@
 
 static struct serio_driver serio_raw_drv = {
 	.driver		= {
-		.name	= "serio_raw",
+		.name		= "serio_raw",
+		.manual_bind	= 1,
 	},
 	.description	= DRIVER_DESC,
 	.interrupt	= serio_raw_interrupt,
 	.connect	= serio_raw_connect,
 	.reconnect	= serio_raw_reconnect,
 	.disconnect	= serio_raw_disconnect,
-	.manual_bind	= 1,
 };
 
 int __init serio_raw_init(void)

diff -urN a/include/linux/device.h b/include/linux/device.h
@@ -102,6 +102,8 @@
 	char			* name;
 	struct bus_type		* bus;
 
+	unsigned int		manual_bind;
+
 	struct semaphore	unload_sem;
 	struct kobject		kobj;
 	struct list_head	devices;

@@ -264,6 +266,9 @@
 	struct bus_type	* bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	unsigned int manual_bind;	/* indicates whether the core will
+					   try to find a driver for the
+					   device automatically */
 	void		*driver_data;	/* data private to the driver */
 	void		*platform_data;	/* Platform specific data (e.g. ACPI,
 					   BIOS data relevant to device) */

diff -urN a/include/linux/serio.h b/include/linux/serio.h
@@ -27,8 +27,6 @@
 	char name[32];
 	char phys[32];
 
-	unsigned int manual_bind;
-
 	unsigned short idbus;
 	unsigned short idvendor;
 	unsigned short idproduct;

@@ -57,8 +55,6 @@
 	void *private;
 	char *description;
 
-	unsigned int manual_bind;
-
 	void (*write_wakeup)(struct serio *);
 	irqreturn_t (*interrupt)(struct serio *, unsigned char,
 			unsigned int, struct pt_regs *);

No, for now, if you want to do this, do it in the serio code only, let's clean up the locking first before we do this in the core.

thanks,

greg k-h

*confused* This patch is completely independent from any locking issues in driver core... It is just 2 flags in device and driver structures that are checked in device_attach and driver_attach.

Dmitry

I agree, it's a convient excuse to use to keep any other driver core changes from happening right now :)

But I'm not so sure we want to add this to the driver core yet. We can discuss this after the locking stuff is finished, ok?

thanks,

greg k-h

I'm not sure what your bind_mode patch includes, but I would like to start a general discussion on the bind/unbind issue.

I appreciate the effort, and I agree that this feature is important. However, I would like to discuss a few issues.

1.) I don't think we should merge a patch that supports driver "unbind" without also supporting driver "bind".

They're really very interelated, and we don't want to break userspace by changing everything around when we discover a cleaner solution.

2.) I don't like having an "unbind" file.

This implies that we will have at least three seperate files controlling driver binding when we really need only one or two at the most. Consider "bind", "unbind", and the link to the driver that is bound.

An Alternative Solution
=======================

Why not have a file named "bind". We can write the name of the driver we want bound to the device. When we want to unbind the driver we could do something like this:

# echo "" > bind
or
# echo 0 > bind

At least then we only have the link and the "bind" file to worry about. I've also been considering more inventive solutions (like deleting the symlink will cause the driver to unbind). But it could get complex very quickly. Really, we need to discuss this more.

I look forward to any comments.

Thanks,
Adam

I'd like having one node as well. Right now serio bus uses "drvctl" and supports the following operations:
- "none" to unbind;
- "rescan" to unbind if bound and then find appropriate driver;
- "reconnect" to reinitialize hardware without inbinding (so exesting input devices will be kept intact)
- <driver name> to unbind if bound and try to bind.

There was also ide of changing commands to form "CMD [DRIVER] [ARGS...]:
"detach", "rescan", "reconnect", "attach <driver_name>"

My bind mode patch is somewhat independent of "drvctl" as it just adds a new attribute - "bind_mode" to all devices and drivers. It can be either "auto" or "manual" and device/drivers that are set as manual mode will be ignored by driver core and will only be bound when user explicitely asks to do that. This is useful when you want "penalize" one driver over another, like psmouse/serio_raw.

Dmitry

Great! I'm glad we agree.

These additional features bring up another issue that we may want the driver model to handle. Basically, I think we should allow devices to be started and stopped.

So it would look something like this:

struct device_driver {
	char			* name;
	struct bus_type		* bus;

	struct semaphore	unload_sem;
	struct kobject		kobj;
	struct list_head	devices;

	int	(*probe)	(struct device * dev);
	int	(*start)	(struct device * dev); <-----
	int	(*stop)		(struct device * dev); <-----
	int 	(*remove)	(struct device * dev);
	void	(*shutdown)	(struct device * dev);
	int	(*suspend)	(struct device * dev, u32 state, u32 level);
	int	(*resume)	(struct device * dev, u32 level);
};

"*probe"
- determine if this driver is able to handle this device
- if so create data structures that can store information about the device
- bind the device to the driver and display additional config attributes.

At this point userspace can set up the configuration of this specific binding instance. The configuration options would primarily be things that cannot be modified while the device is in use. It can be loaded from a cache so that it is consistent between reboots, hotplugs etc. This would sort of replace module parameters.

Now that the user has specific his or her config preferences we can go to "*start"

"*start"
- parse resource information
- fill in device/driver data structures with information
- prepare the device to actually be used

Then at a later time, when the device...
- is no longer needed
- resources need to be rebalanced
- the user wants to remove the device in the near future

"*stop"
- safely stop the upper class layer
- free resources, and reset device specific data

And we're ready for the next step. (which may even include another *start)

This would easily allow for things like "reconnect", which would simply be a "*stop" follow by a "*start".

Comments?

That's actually a really interesting idea. In some cases we may not want the kernel automatically binding drivers. A question would be should this feature be disabled on a per device basis or globally? If it's globally then should it occur after init is done. And if that's the case, couldn't we free the device ID tables and handle everything from userspace. I'm sure there are some problems with this but I figured I'd mention it as well.

Thanks,
Adam

You mean like the power management people want? :)

Ick, no. Use the power interface for this. If you aren't already on the new linux-pm mailing list (hosted by osdl.org) you might want to be, as people are discussing this there.

But now that module paramaters are able to be passed as command line arguments, doesn't that solve the issue? :)

Anyway, I think people are working on making those kind of values persistant in a way, see the patches on lkml for something like this (from what I can tell, I haven't really been paying attention though...)

You are forcing the user to do too much work in order for their devices to start working :)

I understand the issue you are trying to solve, but what about something like the "persistant device tree" issue that we've all been talking about over the past few years? That would let systems where we can not probe for devices to be created all at once (like on most embedded systems) and I think would solve your resource issues too, right?

thanks,

greg k-h

Sounds interesting, although I do not think that freeing resources should be done at stop time, it is task for remove() IMHO. Do you have an idea how setting up process (between probe and start) will work? Will start called automatically or by request from userspace?

Well, it sure needs to be available on per-device/per-driver basis as while I am generally enjoying automatic binding without userspace involvement. For example I sometimes want to be able to disable my touchpad and not let it spring back to life (normally serio core will try to find proper driver for
an unbound port whenever there is a data coming from it).

Whether it should also be controlled on per bus/per system basis is another question. I am not quite ready to drop all device tables and rely solely on userspace handling, altthough if all tables are marked as __init and and the end of the boot process we walk all buses and set their ->match() methods to NULL we effectively switch to manual binding mode and can discard ID tables.

Dmitry

How would we break userspace, when you can't do either thing today? Just by adding one new feature, doesn't break anything :)

Anyway, I agree, but the core needs to have it's locking reworked in order for this to work properly. I'm working on this now.

Why?

No. Consider the fact that the "unbind" file will not be present if the device is not bound to anything. Once it is bound, the unbind file will be created, and the symlink will be created. The symlink matches other parts of sysfs. By trying to put the name of the driver in a file, that makes userspace work a lot harder to try to figure out exactly what driver is bound (consider the fact that I can have both a pci and a usb driver with the same name in sysfs, and that's legal.)

So, when a device is not bound to a driver, there will be no symlink, or a "unbind" file, only a "bind" file. Really there is only 1 "control" type file present at any single point in time.

Sound good?

thanks,

greg k-h

I do not like interfaces accepting and encouraging writing garbage data. What value sould be written into "unbind"? Yes, any junk.

But not 2 drivers with the same name on the same bus so I don't think this is a valid argument. Anyway, we already have this symlink.

Does that imply that I can not rebind device while it is bound to a driver? ("bind" would be missing it seems). And what about all other flavors of that operation - rescan, reconnect? Do we want to have separate attributes for them as well?

Dmitry

Ok, we restrict it to working only if you write a "1" into it. That was an easy fix :)

Yes. You must unbind it first.

rescan is a bus specific thing, not a driver or device thing. reconnect would be the same as "unbind" + "bind" and you can do that with the scheme I posted.

thanks,

greg k-h

Yes, it is - at least when I am talking about rescan I mean that I want to scan a bus for a specific driver for the particular device. I do not want any other device to be affected and therefore it is a device thing.

No quite - the point of reconnect is to be able to re-intialize the hardware while keeping everything else intact - if I do unbind and then bind on my touchpad while GPM and X are running it will jump from /dev/input/event0 to /dev/input/event4 and I will effectively lose it.

Dmitry