[RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

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: greg@kroah.com
 Subject: driver core: allow userspace to unbind drivers from devices.

 Signed-off-by: Tejun Heo <tj@home-tj.org>
 Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
 Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-11-09 14:26:35 -08:00
+++ b/drivers/base/bus.c	2004-11-09 14:26:35 -08:00
@@ -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)

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

Hi Greg,

On Tue, 9 Nov 2004 14:37:29 -0800, Greg KH <greg@kroah.com> wrote:
> 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.

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.

> 
> 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.)
> 

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Tue, Nov 09, 2004 at 06:36:52PM -0500, Dmitry Torokhov wrote:
> Hi Greg,
> 
> On Tue, 9 Nov 2004 14:37:29 -0800, Greg KH <greg@kroah.com> wrote:
> > 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.
> 
> 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.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Tuesday 09 November 2004 07:33 pm, Greg KH wrote:
> 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.
> 
...
> 
> So, off to rework this mess...
> 

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> On Tuesday 09 November 2004 07:33 pm, Greg KH wrote:
> > 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.
> > 
> ...
> > 
> > So, off to rework this mess...
> > 
> 
> 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...

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

thanks,

greg k-h

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

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

On Mon, 15 Nov 2004 21:54:40 -0800, Greg KH <greg@kroah.com> wrote:
> On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> > 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...
> 
> Care to resend it?  I can't find it in my archives.
> 

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

[-- Attachment #2: bind_mode-attr.patch --]
[-- Type: application/octet-stream, Size: 10551 bytes --]

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

ChangeSet@1.1963, 2004-11-16 02:07:15-05:00, dtor_core@ameritech.net
  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
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 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
--- a/drivers/base/bus.c	2004-11-16 02:20:53 -0500
+++ b/drivers/base/bus.c	2004-11-16 02:20:53 -0500
@@ -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/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c	2004-11-16 02:20:53 -0500
+++ b/drivers/base/interface.c	2004-11-16 02:20:53 -0500
@@ -1,6 +1,6 @@
 /*
  * drivers/base/interface.c - common driverfs interface that's exported to
- * 	the world for all devices.
+ * 	the world for all devices and drivers.
  *
  * Copyright (c) 2002-3 Patrick Mochel
  * Copyright (c) 2002-3 Open Source Development Labs
@@ -14,6 +14,40 @@
 #include <linux/stat.h>
 #include <linux/string.h>
 
+
+static int parse_bind_mode(const char * buf, size_t count, unsigned int * manual_bind)
+{
+	int retval = count;
+
+	if (!strncmp(buf, "manual", count)) {
+		*manual_bind = 1;
+	} else if (!strncmp(buf, "auto", count)) {
+		*manual_bind = 0;
+	} else {
+		retval = -EINVAL;
+	}
+	return retval;
+}
+
+/**
+ *	bind_mode - controls the binding mode for the device.
+ *
+ *	When set to "auto" driver core will try to automatically bind the
+ *	device once appropriate driver becomes available. When bind mode
+ *	is "manual" user intervention is required.
+ */
+
+static ssize_t dev_bind_mode_show(struct device * dev, char * buf)
+{
+	return sprintf(buf, "%s\n", dev->manual_bind ? "manual" : "auto");
+}
+
+static ssize_t dev_bind_mode_store(struct device * dev, const char * buf, size_t count)
+{
+	return parse_bind_mode(buf, count, &dev->manual_bind);
+}
+
+
 /**
  *	detach_state - control the default power state for the device.
  *
@@ -27,12 +61,12 @@
  *	driver's suspend method.
  */
 
-static ssize_t detach_show(struct device * dev, char * buf)
+static ssize_t dev_detach_show(struct device * dev, char * buf)
 {
 	return sprintf(buf, "%u\n", dev->detach_state);
 }
 
-static ssize_t detach_store(struct device * dev, const char * buf, size_t n)
+static ssize_t dev_detach_store(struct device * dev, const char * buf, size_t n)
 {
 	u32 state;
 	state = simple_strtoul(buf, NULL, 10);
@@ -42,10 +76,36 @@
 	return n;
 }
 
-static DEVICE_ATTR(detach_state, 0644, detach_show, detach_store);
-
+static DEVICE_ATTR(bind_mode, 0644, dev_bind_mode_show, dev_bind_mode_store);
+static DEVICE_ATTR(detach_state, 0644, dev_detach_show, dev_detach_store);
 
 struct attribute * dev_default_attrs[] = {
+	&dev_attr_bind_mode.attr,
 	&dev_attr_detach_state.attr,
 	NULL,
 };
+
+/**
+ *	bind_mode - controls the binding mode for the driver.
+ *
+ *	When set to "auto" driver core will try to automatically bind the
+ *	driver once appropriate device becomes available. When bind mode
+ *	is "manual" user intervention is required.
+ */
+
+static ssize_t drv_bind_mode_show(struct device_driver * drv, char * buf)
+{
+	return sprintf(buf, "%s\n", drv->manual_bind ? "manual" : "auto");
+}
+
+static ssize_t drv_bind_mode_store(struct device_driver * drv, const char * buf, size_t count)
+{
+	return parse_bind_mode(buf, count, &drv->manual_bind);
+}
+
+static DRIVER_ATTR(bind_mode, 0644, drv_bind_mode_show, drv_bind_mode_store);
+
+struct attribute * drv_default_attrs[] = {
+	&driver_attr_bind_mode.attr,
+	NULL,
+};
diff -urN a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
--- a/drivers/input/serio/serio.c	2004-11-16 02:20:53 -0500
+++ b/drivers/input/serio/serio.c	2004-11-16 02:20:53 -0500
@@ -92,7 +92,7 @@
 	struct serio_driver *drv;
 
 	list_for_each_entry(drv, &serio_driver_list, node)
-		if (!drv->manual_bind)
+		if (!drv->driver.manual_bind)
 			if (serio_bind_driver(serio, drv))
 				break;
 }
@@ -277,33 +277,9 @@
 	return retval;
 }
 
-static ssize_t serio_show_bind_mode(struct device *dev, char *buf)
-{
-	struct serio *serio = to_serio_port(dev);
-	return sprintf(buf, "%s\n", serio->manual_bind ? "manual" : "auto");
-}
-
-static ssize_t serio_set_bind_mode(struct device *dev, const char *buf, size_t count)
-{
-	struct serio *serio = to_serio_port(dev);
-	int retval;
-
-	retval = count;
-	if (!strncmp(buf, "manual", count)) {
-		serio->manual_bind = 1;
-	} else if (!strncmp(buf, "auto", count)) {
-		serio->manual_bind = 0;
-	} else {
-		retval = -EINVAL;
-	}
-
-	return retval;
-}
-
 static struct device_attribute serio_device_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_show_description, NULL),
 	__ATTR(drvctl, S_IWUSR, NULL, serio_rebind_driver),
-	__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
 	__ATTR_NULL
 };
 
@@ -371,7 +347,7 @@
 
 	if (drv)
 		serio_bind_driver(serio, drv);
-	else if (!serio->manual_bind)
+	else if (!serio->dev.manual_bind)
 		serio_find_driver(serio);
 
 	/* Ok, now bind children, if any */
@@ -383,7 +359,7 @@
 
 		serio_create_port(serio);
 
-		if (!serio->manual_bind) {
+		if (!serio->dev.manual_bind) {
 			/*
 			 * With children we just _prefer_ passed in driver,
 			 * but we will try other options in case preferred
@@ -505,34 +481,8 @@
 	return sprintf(buf, "%s\n", driver->description ? driver->description : "(none)");
 }
 
-static ssize_t serio_driver_show_bind_mode(struct device_driver *drv, char *buf)
-{
-	struct serio_driver *serio_drv = to_serio_driver(drv);
-	return sprintf(buf, "%s\n", serio_drv->manual_bind ? "manual" : "auto");
-}
-
-static ssize_t serio_driver_set_bind_mode(struct device_driver *drv, const char *buf, size_t count)
-{
-	struct serio_driver *serio_drv = to_serio_driver(drv);
-	int retval;
-
-	retval = count;
-	if (!strncmp(buf, "manual", count)) {
-		serio_drv->manual_bind = 1;
-	} else if (!strncmp(buf, "auto", count)) {
-		serio_drv->manual_bind = 0;
-	} else {
-		retval = -EINVAL;
-	}
-
-	return retval;
-}
-
-
 static struct driver_attribute serio_driver_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_driver_show_description, NULL),
-	__ATTR(bind_mode, S_IWUSR | S_IRUGO,
-		serio_driver_show_bind_mode, serio_driver_set_bind_mode),
 	__ATTR_NULL
 };
 
@@ -547,7 +497,7 @@
 	drv->driver.bus = &serio_bus;
 	driver_register(&drv->driver);
 
-	if (drv->manual_bind)
+	if (drv->driver.manual_bind)
 		goto out;
 
 start_over:
diff -urN a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
--- a/drivers/input/serio/serio_raw.c	2004-11-16 02:20:53 -0500
+++ b/drivers/input/serio/serio_raw.c	2004-11-16 02:20:53 -0500
@@ -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
--- a/include/linux/device.h	2004-11-16 02:20:53 -0500
+++ b/include/linux/device.h	2004-11-16 02:20:53 -0500
@@ -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
--- a/include/linux/serio.h	2004-11-16 02:20:53 -0500
+++ b/include/linux/serio.h	2004-11-16 02:20:53 -0500
@@ -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 *);

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Tue, Nov 16, 2004 at 03:43:21PM -0500, Dmitry Torokhov wrote:
> On Mon, 15 Nov 2004 21:54:40 -0800, Greg KH <greg@kroah.com> wrote:
> > On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> > > 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...
> > 
> > Care to resend it?  I can't find it in my archives.
> > 
> 
> 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.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Tuesday 16 November 2004 06:08 pm, Greg KH wrote:
> On Tue, Nov 16, 2004 at 03:43:21PM -0500, Dmitry Torokhov wrote:
> > On Mon, 15 Nov 2004 21:54:40 -0800, Greg KH <greg@kroah.com> wrote:
> > > On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> > > > 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...
> > > 
> > > Care to resend it?  I can't find it in my archives.
> > > 
> > 
> > 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.
> 
> 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.
> 

*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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Wed, Nov 17, 2004 at 02:00:32AM -0500, Dmitry Torokhov wrote:
> On Tuesday 16 November 2004 06:08 pm, Greg KH wrote:
> > On Tue, Nov 16, 2004 at 03:43:21PM -0500, Dmitry Torokhov wrote:
> > > On Mon, 15 Nov 2004 21:54:40 -0800, Greg KH <greg@kroah.com> wrote:
> > > > On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> > > > > 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...
> > > > 
> > > > Care to resend it?  I can't find it in my archives.
> > > > 
> > > 
> > > 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.
> > 
> > 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.
> > 
> 
> *confused* This patch is completely independent from any locking issues in
> driver core...

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

> It is just 2 flags in device and driver structures that are checked in
> device_attach and driver_attach.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Adam Belay]

On Tue, Nov 09, 2004 at 10:49:43PM -0500, Dmitry Torokhov wrote:
> On Tuesday 09 November 2004 07:33 pm, Greg KH wrote:
> > 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.
> > 
> ...
> > 
> > So, off to rework this mess...
> > 
> 
> 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...
> 

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Tuesday 16 November 2004 01:13 am, Adam Belay wrote: 
> 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'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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Adam Belay]

On Tue, Nov 16, 2004 at 01:37:57AM -0500, Dmitry Torokhov wrote:
> On Tuesday 16 November 2004 01:13 am, Adam Belay wrote: 
> > 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'd like having one node as well. Right now serio bus uses "drvctl" and supports

Great!  I'm glad we agree.

> 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>"
> 

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

>From this point the device would be completely usable.

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?


> 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.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Tue, Nov 16, 2004 at 02:04:13AM -0500, Adam Belay wrote:
> 
> 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.

You mean like the power management people want?  :)

> 	int	(*start)	(struct device * dev); <-----
> 	int	(*stop)		(struct device * dev); <-----

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.

> "*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.

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...)

> 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
> 
> From this point the device would be completely usable.
> 
> 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?

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Tue, 16 Nov 2004 02:04:13 -0500, Adam Belay <ambx1@neo.rr.com> wrote:
> 
> "*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?
> 

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?

> 
> > 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.
> 
> 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.
> 

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Tue, Nov 16, 2004 at 01:13:15AM -0500, Adam Belay wrote:
> 
> 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.

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.

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

Why?

> 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.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Tuesday 16 November 2004 03:17 pm, Greg KH wrote:
> > 2.) I don't like having an "unbind" file.
> 
> Why?

I do not like interfaces accepting and encouraging writing garbage data. What
value sould be written into "unbind"? Yes, any junk.
 
> 
> > 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.
> 
> 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.)

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.

> 
> 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.

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Greg KH]

On Wed, Nov 17, 2004 at 02:07:14AM -0500, Dmitry Torokhov wrote:
> On Tuesday 16 November 2004 03:17 pm, Greg KH wrote:
> > > 2.) I don't like having an "unbind" file.
> > 
> > Why?
> 
> I do not like interfaces accepting and encouraging writing garbage data. What
> value sould be written into "unbind"? Yes, any junk.

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

> > 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.
> 
> Does that imply that I can not rebind device while it is bound to a driver?

Yes.  You must unbind it first.

> ("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?

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

Re: [RFC] [PATCH] driver core: allow userspace to unbind drivers from devices.

[Dmitry Torokhov]

On Wed, 17 Nov 2004 09:53:59 -0800, Greg KH <greg@kroah.com> wrote:
> On Wed, Nov 17, 2004 at 02:07:14AM -0500, Dmitry Torokhov wrote:
> > On Tuesday 16 November 2004 03:17 pm, Greg KH wrote:
> > > > 2.) I don't like having an "unbind" file.
> > >
> > > Why?
> >
> > I do not like interfaces accepting and encouraging writing garbage data. What
> > value sould be written into "unbind"? Yes, any junk.
> 
> Ok, we restrict it to working only if you write a "1" into it.  That was
> an easy fix :)
> 
> > > 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.
> >
> > Does that imply that I can not rebind device while it is bound to a driver?
> 
> Yes.  You must unbind it first.
> 
> > ("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?
> 
> rescan is a bus specific thing, not a driver or device thing.

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.

> reconnect would be the same as "unbind" + "bind" and you can do that
> with the scheme I posted.

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