[GIT PATCH] Driver core patches for 2.6.13-rc1

[GIT PATCH] Driver core patches for 2.6.13-rc1

[Greg KH]

Here are some small patches for the driver core.  They fix a bug that
has caused some people to see deadlocks when some drivers are unloaded
(like ieee1394), and add the ability to bind and unbind drivers from
devices from userspace (something that people have been asking for for a
long time.)

Please pull from:
	rsync://rsync.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-2.6.git/
or if master.kernel.org hasn't synced up yet:
	master.kernel.org:/pub/scm/linux/kernel/git/gregkh/driver-2.6.git/

thanks,

greg k-h

 drivers/base/base.h    |    1 
 drivers/base/bus.c     |  117 +++++++++++++++++++++++++++++++++++++++++--------
 drivers/base/core.c    |    2 
 drivers/base/dd.c      |    2 
 drivers/base/driver.c  |   35 ++++++++++++++
 include/linux/device.h |    7 ++
 6 files changed, 143 insertions(+), 21 deletions(-)

--------------------

Cornelia Huck:
  driver core: add bus_find_device & driver_find_device functions

Greg Kroah-Hartman:
  driver core: Add the ability to bind drivers to devices from userspace
  driver core: change bus_rescan_devices to return void
  driver core: Add the ability to unbind drivers to devices from userspace

Patrick Mochel:
  Driver core: Use klist_del() instead of klist_remove().

[PATCH] driver core: add bus_find_device & driver_find_device functions

[Greg KH]

[PATCH] driver core: add bus_find_device & driver_find_device functions

Add bus_find_device() and driver_find_device() which allow searching for a
device in the bus's resp. the driver's klist and obtain a reference on it.

Signed-off-by: Cornelia Huck <cohuck@de.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit 0edb586049e57c56e625536476931117a57671e9
tree 9d92bb9821d134d199d62de1ff3096ff2b73fdc7
parent fd782a4a99d2d3e818b9465c427b10f7f027d7da
author Cornelia Huck <cohuck@de.ibm.com> Wed, 22 Jun 2005 16:59:51 +0200
committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:03 -0700

 drivers/base/bus.c     |   34 ++++++++++++++++++++++++++++++++++
 drivers/base/driver.c  |   35 +++++++++++++++++++++++++++++++++++
 include/linux/device.h |    5 +++++
 3 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -177,6 +177,39 @@ int bus_for_each_dev(struct bus_type * b
 	return error;
 }
 
+/**
+ * bus_find_device - device iterator for locating a particular device.
+ * @bus: bus type
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the bus_for_each_dev() function above, but it
+ * returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+ */
+struct device * bus_find_device(struct bus_type *bus,
+				struct device *start, void *data,
+				int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device *dev;
+
+	if (!bus)
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data) && get_device(dev))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
 
 
 static struct device_driver * next_driver(struct klist_iter * i)
@@ -557,6 +590,7 @@ int __init buses_init(void)
 
 
 EXPORT_SYMBOL_GPL(bus_for_each_dev);
+EXPORT_SYMBOL_GPL(bus_find_device);
 EXPORT_SYMBOL_GPL(bus_for_each_drv);
 
 EXPORT_SYMBOL_GPL(bus_add_device);
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -56,6 +56,41 @@ EXPORT_SYMBOL_GPL(driver_for_each_device
 
 
 /**
+ * driver_find_device - device iterator for locating a particular device.
+ * @driver: The device's driver
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the driver_for_each_device() function above, but
+ * it returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+ */
+struct device * driver_find_device(struct device_driver *drv,
+				   struct device * start, void * data,
+				   int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device *dev;
+
+	if (!drv)
+		return NULL;
+
+	klist_iter_init_node(&drv->klist_devices, &i,
+			     (start ? &start->knode_driver : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data) && get_device(dev))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
+EXPORT_SYMBOL_GPL(driver_find_device);
+
+/**
  *	driver_create_file - create sysfs file for driver.
  *	@drv:	driver.
  *	@attr:	driver attribute descriptor.
diff --git a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -80,6 +80,8 @@ extern struct bus_type * find_bus(char *
 
 int bus_for_each_dev(struct bus_type * bus, struct device * start, void * data,
 		     int (*fn)(struct device *, void *));
+struct device * bus_find_device(struct bus_type *bus, struct device *start,
+				void *data, int (*match)(struct device *, void *));
 
 int bus_for_each_drv(struct bus_type * bus, struct device_driver * start, 
 		     void * data, int (*fn)(struct device_driver *, void *));
@@ -142,6 +144,9 @@ extern void driver_remove_file(struct de
 
 extern int driver_for_each_device(struct device_driver * drv, struct device * start,
 				  void * data, int (*fn)(struct device *, void *));
+struct device * driver_find_device(struct device_driver *drv,
+				   struct device *start, void *data,
+				   int (*match)(struct device *, void *));
 
 
 /*

[PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

[PATCH] driver core: Add the ability to unbind drivers to devices from userspace

This adds a single file, "unbind", to the sysfs directory of every
device that is currently bound to a driver.  To unbind the driver from
the device, write anything to this file and they will be disconnected
from each other.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit 151ef38f7c0ec1b0420f04438b0316e3a30bf2e4
tree 3aa6504e12c08f70cacb7f9de6ef5858b45ee86d
parent 0edb586049e57c56e625536476931117a57671e9
author Greg Kroah-Hartman <gregkh@suse.de> Wed, 22 Jun 2005 16:09:05 -0700
committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:04 -0700

 drivers/base/bus.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -133,6 +133,34 @@ static struct kobj_type ktype_bus = {
 decl_subsys(bus, &ktype_bus, NULL);
 
 
+/* Manually detach a device from it's associated driver. */
+static int driver_helper(struct device *dev, void *data)
+{
+	const char *name = data;
+
+	if (strcmp(name, dev->bus_id) == 0)
+		return 1;
+	return 0;
+}
+
+static ssize_t driver_unbind(struct device_driver *drv,
+			     const char *buf, size_t count)
+{
+	struct bus_type *bus = get_bus(drv->bus);
+	struct device *dev;
+	int err = -ENODEV;
+
+	dev = bus_find_device(bus, NULL, (void *)buf, driver_helper);
+	if ((dev) &&
+	    (dev->driver == drv)) {
+		device_release_driver(dev);
+		err = count;
+	}
+	return err;
+}
+static DRIVER_ATTR(unbind, S_IWUSR, NULL, driver_unbind);
+
+
 static struct device * next_device(struct klist_iter * i)
 {
 	struct klist_node * n = klist_next(i);
@@ -396,6 +424,7 @@ int bus_add_driver(struct device_driver 
 		module_add_driver(drv->owner, drv);
 
 		driver_add_attrs(bus, drv);
+		driver_create_file(drv, &driver_attr_unbind);
 	}
 	return error;
 }
@@ -413,6 +442,7 @@ int bus_add_driver(struct device_driver 
 void bus_remove_driver(struct device_driver * drv)
 {
 	if (drv->bus) {
+		driver_remove_file(drv, &driver_attr_unbind);
 		driver_remove_attrs(drv->bus, drv);
 		klist_remove(&drv->knode_bus);
 		pr_debug("bus %s: remove driver %s\n", drv->bus->name, drv->name);

[PATCH] driver core: change bus_rescan_devices to return void

[Greg KH]

[PATCH] driver core: change bus_rescan_devices to return void

No one was looking at the return value of bus_rescan_devices, and it
really wasn't anything that anyone in the kernel would ever care about.
So change it which enabled some counting code to be removed also.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit 23d3d602cb96addd3c1158424fb01a49ea5e81b1
tree 2daa85579c964bfe3d1a91fe365d202b8f38422b
parent afdce75f1eaebcf358b7594ba7969aade105c3b0
author Greg Kroah-Hartman <gregkh@suse.de> Wed, 22 Jun 2005 16:09:05 -0700
committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:04 -0700

 drivers/base/bus.c     |   27 +++++++++------------------
 include/linux/device.h |    2 +-
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -483,31 +483,22 @@ void bus_remove_driver(struct device_dri
 /* Helper for bus_rescan_devices's iter */
 static int bus_rescan_devices_helper(struct device *dev, void *data)
 {
-	int *count = data;
-
-	if (!dev->driver && (device_attach(dev) > 0))
-		(*count)++;
-
+	if (!dev->driver)
+		device_attach(dev);
 	return 0;
 }
 
-
 /**
- *	bus_rescan_devices - rescan devices on the bus for possible drivers
- *	@bus:	the bus to scan.
+ * bus_rescan_devices - rescan devices on the bus for possible drivers
+ * @bus: the bus to scan.
  *
- *	This function will look for devices on the bus with no driver
- *	attached and rescan it against existing drivers to see if it
- *	matches any. Calls device_attach(). Returns the number of devices
- *	that were sucessfully bound to a driver.
+ * This function will look for devices on the bus with no driver
+ * attached and rescan it against existing drivers to see if it matches
+ * any by calling device_attach() for the unbound devices.
  */
-int bus_rescan_devices(struct bus_type * bus)
+void bus_rescan_devices(struct bus_type * bus)
 {
-	int count = 0;
-
-	bus_for_each_dev(bus, NULL, &count, bus_rescan_devices_helper);
-
-	return count;
+	bus_for_each_dev(bus, NULL, NULL, bus_rescan_devices_helper);
 }
 
 
diff --git a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -69,7 +69,7 @@ struct bus_type {
 extern int bus_register(struct bus_type * bus);
 extern void bus_unregister(struct bus_type * bus);
 
-extern int bus_rescan_devices(struct bus_type * bus);
+extern void bus_rescan_devices(struct bus_type * bus);
 
 extern struct bus_type * get_bus(struct bus_type * bus);
 extern void put_bus(struct bus_type * bus);

[PATCH] driver core: Add the ability to bind drivers to devices from userspace

[Greg KH]

[PATCH] driver core: Add the ability to bind drivers to devices from userspace

This adds a single file, "bind", to the sysfs directory of every driver
registered with the driver core.  To bind a device to a driver, write
the bus id of the device you wish to bind to that specific driver to the
"bind" file (remember to not add a trailing \n).  If that bus id matches
a device on that bus, and it does not currently have a driver bound to
it, the probe sequence will be initiated with that driver and device.

Note, this requires that the driver itself be willing and able to accept
that device (usually through a device id type table).  This patch does
not make it possible to override the driver's id table.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit afdce75f1eaebcf358b7594ba7969aade105c3b0
tree 5374a0e85e03c8706a1dd95478b9d0a3312917e0
parent 151ef38f7c0ec1b0420f04438b0316e3a30bf2e4
author Greg Kroah-Hartman <gregkh@suse.de> Wed, 22 Jun 2005 16:09:05 -0700
committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:04 -0700

 drivers/base/base.h |    1 +
 drivers/base/bus.c  |   26 ++++++++++++++++++++++++++
 drivers/base/dd.c   |    2 +-
 3 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -5,6 +5,7 @@ extern int bus_add_driver(struct device_
 extern void bus_remove_driver(struct device_driver *);
 
 extern void driver_detach(struct device_driver * drv);
+extern int driver_probe_device(struct device_driver *, struct device *);
 
 static inline struct class_device *to_class_dev(struct kobject *obj)
 {
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -160,6 +160,30 @@ static ssize_t driver_unbind(struct devi
 }
 static DRIVER_ATTR(unbind, S_IWUSR, NULL, driver_unbind);
 
+/*
+ * Manually attach a device to a driver.
+ * Note: the driver must want to bind to the device,
+ * it is not possible to override the driver's id table.
+ */
+static ssize_t driver_bind(struct device_driver *drv,
+			   const char *buf, size_t count)
+{
+	struct bus_type *bus = get_bus(drv->bus);
+	struct device *dev;
+	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);
+		put_device(dev);
+	}
+	return err;
+}
+static DRIVER_ATTR(bind, S_IWUSR, NULL, driver_bind);
+
 
 static struct device * next_device(struct klist_iter * i)
 {
@@ -425,6 +449,7 @@ int bus_add_driver(struct device_driver 
 
 		driver_add_attrs(bus, drv);
 		driver_create_file(drv, &driver_attr_unbind);
+		driver_create_file(drv, &driver_attr_bind);
 	}
 	return error;
 }
@@ -442,6 +467,7 @@ int bus_add_driver(struct device_driver 
 void bus_remove_driver(struct device_driver * drv)
 {
 	if (drv->bus) {
+		driver_remove_file(drv, &driver_attr_bind);
 		driver_remove_file(drv, &driver_attr_unbind);
 		driver_remove_attrs(drv->bus, drv);
 		klist_remove(&drv->knode_bus);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -65,7 +65,7 @@ void device_bind_driver(struct device * 
  *
  *	This function must be called with @dev->sem held.
  */
-static int driver_probe_device(struct device_driver * drv, struct device * dev)
+int driver_probe_device(struct device_driver * drv, struct device * dev)
 {
 	int ret = 0;
 

[PATCH] Driver core: Use klist_del() instead of klist_remove().

[Greg KH]

[PATCH] Driver core: Use klist_del() instead of klist_remove().

Use klist_del() instead of klist_remove() when unregistering devices.
This will prevent a deadlock when executing a recursive unregister using
device_for_each_child().

Signed-off-by Patrick Mochel <mochel@digitalimplant.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit d62c0f9fd2d3943a3eca85b490d86e1605000ccb
tree c9fc174992f7746f680becdeaa1bdb6924108c0f
parent 23d3d602cb96addd3c1158424fb01a49ea5e81b1
author Patrick Mochel <mochel@digitalimplant.org> Fri, 24 Jun 2005 08:39:33 -0700
committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:05 -0700

 drivers/base/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -333,7 +333,7 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 
 	if (parent)
-		klist_remove(&dev->knode_parent);
+		klist_del(&dev->knode_parent);
 
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Dmitry Torokhov]

On Thursday 30 June 2005 01:04, Greg KH wrote:
> [PATCH] driver core: Add the ability to unbind drivers to devices from userspace
> 
> This adds a single file, "unbind", to the sysfs directory of every
> device that is currently bound to a driver.  To unbind the driver from
> the device, write anything to this file and they will be disconnected
> from each other.
>

Comment and the patch disagree with each other.
 
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> ---
> commit 151ef38f7c0ec1b0420f04438b0316e3a30bf2e4
> tree 3aa6504e12c08f70cacb7f9de6ef5858b45ee86d
> parent 0edb586049e57c56e625536476931117a57671e9
> author Greg Kroah-Hartman <gregkh@suse.de> Wed, 22 Jun 2005 16:09:05 -0700
> committer Greg Kroah-Hartman <gregkh@suse.de> Wed, 29 Jun 2005 22:48:04 -0700
> 
>  drivers/base/bus.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -133,6 +133,34 @@ static struct kobj_type ktype_bus = {
>  decl_subsys(bus, &ktype_bus, NULL);
>  
>  
> +/* Manually detach a device from it's associated driver. */
> +static int driver_helper(struct device *dev, void *data)
> +{
> +	const char *name = data;
> +
> +	if (strcmp(name, dev->bus_id) == 0)
> +		return 1;
> +	return 0;
> +}
> +
> +static ssize_t driver_unbind(struct device_driver *drv,
> +			     const char *buf, size_t count)
> +{
> +	struct bus_type *bus = get_bus(drv->bus);
> +	struct device *dev;
> +	int err = -ENODEV;
> +
> +	dev = bus_find_device(bus, NULL, (void *)buf, driver_helper);
> +	if ((dev) &&
> +	    (dev->driver == drv)) {
> +		device_release_driver(dev);
> +		err = count;
> +	}
> +	return err;
> +}
> +static DRIVER_ATTR(unbind, S_IWUSR, NULL, driver_unbind);
> +
> +
>  static struct device * next_device(struct klist_iter * i)
>  {
>  	struct klist_node * n = klist_next(i);
> @@ -396,6 +424,7 @@ int bus_add_driver(struct device_driver 
>  		module_add_driver(drv->owner, drv);
>  
>  		driver_add_attrs(bus, drv);
> +		driver_create_file(drv, &driver_attr_unbind);
>  	}
>  	return error;
>  }
> @@ -413,6 +442,7 @@ int bus_add_driver(struct device_driver 
>  void bus_remove_driver(struct device_driver * drv)
>  {
>  	if (drv->bus) {
> +		driver_remove_file(drv, &driver_attr_unbind);
>  		driver_remove_attrs(drv->bus, drv);
>  		klist_remove(&drv->knode_bus);
>  		pr_debug("bus %s: remove driver %s\n", drv->bus->name, drv->name);
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Dmitry

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Thu, Jun 30, 2005 at 01:25:39AM -0500, Dmitry Torokhov wrote:
> On Thursday 30 June 2005 01:04, Greg KH wrote:
> > [PATCH] driver core: Add the ability to unbind drivers to devices from userspace
> > 
> > This adds a single file, "unbind", to the sysfs directory of every
> > device that is currently bound to a driver.  To unbind the driver from
> > the device, write anything to this file and they will be disconnected
> > from each other.
> >
> 
> Comment and the patch disagree with each other.

bleah, you are right, that was the old comment with the new patch.  Oh
well...

thanks,

greg k-h

Re: [GIT PATCH] Driver core patches for 2.6.13-rc1

[Dmitry Torokhov]

On Thursday 30 June 2005 01:02, Greg KH wrote:
> Here are some small patches for the driver core.  They fix a bug that
> has caused some people to see deadlocks when some drivers are unloaded
> (like ieee1394), and add the ability to bind and unbind drivers from
> devices from userspace (something that people have been asking for for a
> long time.)
> 

Please don't until all buses are either audited or prepared to handle
"surprise" disconnects.

-- 
Dmitry

Re: [GIT PATCH] Driver core patches for 2.6.13-rc1

[Greg KH]

On Thu, Jun 30, 2005 at 01:19:26AM -0500, Dmitry Torokhov wrote:
> On Thursday 30 June 2005 01:02, Greg KH wrote:
> > Here are some small patches for the driver core.  They fix a bug that
> > has caused some people to see deadlocks when some drivers are unloaded
> > (like ieee1394), and add the ability to bind and unbind drivers from
> > devices from userspace (something that people have been asking for for a
> > long time.)
> > 
> 
> Please don't until all buses are either audited or prepared to handle
> "surprise" disconnects.

That's what bugfixes after 2.6.13-rc2 are for :)

Seriously, I don't think this is a big deal.  But I will work with you
on getting any remaining issues you have solved.  For now, the patches
should stay for their usefulness to others.  I'll continue this
conversation in the other thread about the bind/unbind stuff.

thanks,

greg k-h

Re: [GIT PATCH] Driver core patches for 2.6.13-rc1

[John Lenz]

On Thu, June 30, 2005 1:02 am, Greg KH said:
> Here are some small patches for the driver core.  They fix a bug that
> has caused some people to see deadlocks when some drivers are unloaded
> (like ieee1394), and add the ability to bind and unbind drivers from
> devices from userspace (something that people have been asking for for a
> long time.)

As long as there are a whole bunch of class API changes going on, I would
request that the class_interface add and remove functions get passed the
class_interface pointer as well as the class_device.  This way, the same
function can be used on multiple class_interfaces.

John

Re: [GIT PATCH] Driver core patches for 2.6.13-rc1

[Greg KH]

On Thu, Jun 30, 2005 at 12:22:49PM -0500, John Lenz wrote:
> On Thu, June 30, 2005 1:02 am, Greg KH said:
> > Here are some small patches for the driver core.  They fix a bug that
> > has caused some people to see deadlocks when some drivers are unloaded
> > (like ieee1394), and add the ability to bind and unbind drivers from
> > devices from userspace (something that people have been asking for for a
> > long time.)
> 
> As long as there are a whole bunch of class API changes going on, I would
> request that the class_interface add and remove functions get passed the
> class_interface pointer as well as the class_device.  This way, the same
> function can be used on multiple class_interfaces.

I'm sorry, I seem to have missed the patch in this email that implements
this feature...

:)

thanks,

greg k-h

[PATCH] add class_interface pointer to add and remove functions

[John Lenz]

On Thu, June 30, 2005 2:45 pm, Greg KH said:
> On Thu, Jun 30, 2005 at 12:22:49PM -0500, John Lenz wrote:
>> As long as there are a whole bunch of class API changes going on, I would
>> request that the class_interface add and remove functions get passed the
>> class_interface pointer as well as the class_device.  This way, the same
>> function can be used on multiple class_interfaces.
>
> I'm sorry, I seem to have missed the patch in this email that implements
> this feature...
>

Here is a patch that updates every usage of class_interface I could find.

Signed-off-by: John Lenz <lenz@cs.wisc.edu>

Index: linux-2.6.12/drivers/message/i2o/device.c
===================================================================
--- linux-2.6.12.orig/drivers/message/i2o/device.c	2005-06-30 11:54:55.000000000 -0500
+++ linux-2.6.12/drivers/message/i2o/device.c	2005-06-30 16:00:55.756158383 -0500
@@ -385,7 +385,7 @@
  *
  *	Returns 0 on success or negative error code on failure.
  */
-static int i2o_device_class_add(struct class_device *cd)
+static int i2o_device_class_add(struct class_interface *class_intf, struct class_device *cd)
 {
 	struct i2o_device *i2o_dev, *tmp;
 	struct i2o_controller *c;
Index: linux-2.6.12/drivers/base/class.c
===================================================================
--- linux-2.6.12.orig/drivers/base/class.c	2005-06-30 11:54:52.000000000 -0500
+++ linux-2.6.12/drivers/base/class.c	2005-06-30 15:58:53.321286879 -0500
@@ -505,7 +505,7 @@
 		list_add_tail(&class_dev->node, &parent->children);
 		list_for_each_entry(class_intf, &parent->interfaces, node)
 			if (class_intf->add)
-				class_intf->add(class_dev);
+				class_intf->add(class_intf, class_dev);
 		up(&parent->sem);
 	}
 	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
@@ -585,7 +585,7 @@
 		list_del_init(&class_dev->node);
 		list_for_each_entry(class_intf, &parent->interfaces, node)
 			if (class_intf->remove)
-				class_intf->remove(class_dev);
+				class_intf->remove(class_intf, class_dev);
 		up(&parent->sem);
 	}

@@ -688,7 +688,7 @@
 	list_add_tail(&class_intf->node, &parent->interfaces);
 	if (class_intf->add) {
 		list_for_each_entry(class_dev, &parent->children, node)
-			class_intf->add(class_dev);
+			class_intf->add(class_intf, class_dev);
 	}
 	up(&parent->sem);

@@ -707,7 +707,7 @@
 	list_del_init(&class_intf->node);
 	if (class_intf->remove) {
 		list_for_each_entry(class_dev, &parent->children, node)
-			class_intf->remove(class_dev);
+			class_intf->remove(class_intf, class_dev);
 	}
 	up(&parent->sem);

Index: linux-2.6.12/drivers/pcmcia/ds.c
===================================================================
--- linux-2.6.12.orig/drivers/pcmcia/ds.c	2005-06-30 11:54:56.000000000 -0500
+++ linux-2.6.12/drivers/pcmcia/ds.c	2005-06-30 16:02:14.382619884 -0500
@@ -1148,7 +1148,7 @@
 	.requery = pcmcia_bus_rescan,
 };

-static int __devinit pcmcia_bus_add_socket(struct class_device *class_dev)
+static int __devinit pcmcia_bus_add_socket(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct pcmcia_socket *socket = class_get_devdata(class_dev);
 	int ret;
@@ -1183,7 +1183,7 @@
 	return 0;
 }

-static void pcmcia_bus_remove_socket(struct class_device *class_dev)
+static void pcmcia_bus_remove_socket(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct pcmcia_socket *socket = class_get_devdata(class_dev);

Index: linux-2.6.12/drivers/scsi/sg.c
===================================================================
--- linux-2.6.12.orig/drivers/scsi/sg.c	2005-06-30 11:54:57.000000000 -0500
+++ linux-2.6.12/drivers/scsi/sg.c	2005-06-30 16:06:23.123755439 -0500
@@ -104,8 +104,8 @@

 #define SG_DEV_ARR_LUMP 32	/* amount to over allocate sg_dev_arr by */

-static int sg_add(struct class_device *);
-static void sg_remove(struct class_device *);
+static int sg_add(struct class_interface *class_intf, struct class_device *);
+static void sg_remove(struct class_interface *class_intf, struct class_device *);

 static Scsi_Request *dummy_cmdp;	/* only used for sizeof */

@@ -1507,7 +1507,7 @@
 }

 static int
-sg_add(struct class_device *cl_dev)
+sg_add(struct class_interface *class_intf, struct class_device *cl_dev)
 {
 	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
 	struct gendisk *disk;
@@ -1583,7 +1583,7 @@
 }

 static void
-sg_remove(struct class_device *cl_dev)
+sg_remove(struct class_interface *class_intf, struct class_device *cl_dev)
 {
 	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
 	Sg_device *sdp = NULL;
Index: linux-2.6.12/drivers/pcmcia/socket_sysfs.c
===================================================================
--- linux-2.6.12.orig/drivers/pcmcia/socket_sysfs.c	2005-06-30 11:54:56.000000000 -0500
+++ linux-2.6.12/drivers/pcmcia/socket_sysfs.c	2005-06-30 16:04:24.233249703 -0500
@@ -342,7 +342,7 @@
 	.write = pccard_store_cis,
 };

-static int __devinit pccard_sysfs_add_socket(struct class_device *class_dev)
+static int __devinit pccard_sysfs_add_socket(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct class_device_attribute **attr;
 	int ret = 0;
@@ -358,7 +358,7 @@
 	return ret;
 }

-static void __devexit pccard_sysfs_remove_socket(struct class_device *class_dev)
+static void __devexit pccard_sysfs_remove_socket(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct class_device_attribute **attr;

Index: linux-2.6.12/drivers/pcmcia/rsrc_nonstatic.c
===================================================================
--- linux-2.6.12.orig/drivers/pcmcia/rsrc_nonstatic.c	2005-06-30 11:54:56.000000000 -0500
+++ linux-2.6.12/drivers/pcmcia/rsrc_nonstatic.c	2005-06-30 16:03:49.738193763 -0500
@@ -994,7 +994,7 @@
 	NULL,
 };

-static int __devinit pccard_sysfs_add_rsrc(struct class_device *class_dev)
+static int __devinit pccard_sysfs_add_rsrc(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct pcmcia_socket *s = class_get_devdata(class_dev);
 	struct class_device_attribute **attr;
@@ -1011,7 +1011,7 @@
 	return ret;
 }

-static void __devexit pccard_sysfs_remove_rsrc(struct class_device *class_dev)
+static void __devexit pccard_sysfs_remove_rsrc(struct class_interface *class_intf, struct class_device *class_dev)
 {
 	struct pcmcia_socket *s = class_get_devdata(class_dev);
 	struct class_device_attribute **attr;
Index: linux-2.6.12/include/linux/device.h
===================================================================
--- linux-2.6.12.orig/include/linux/device.h	2005-06-30 11:54:59.000000000 -0500
+++ linux-2.6.12/include/linux/device.h	2005-06-30 15:59:44.921353866 -0500
@@ -246,8 +246,8 @@
 	struct list_head	node;
 	struct class		*class;

-	int (*add)	(struct class_device *);
-	void (*remove)	(struct class_device *);
+	int (*add)	(struct class_interface *, struct class_device *);
+	void (*remove)	(struct class_interface *, struct class_device *);
 };

 extern int class_interface_register(struct class_interface *);

Re: [PATCH] add class_interface pointer to add and remove functions

[Greg KH]

On Thu, Jun 30, 2005 at 04:18:42PM -0500, John Lenz wrote:
> On Thu, June 30, 2005 2:45 pm, Greg KH said:
> > On Thu, Jun 30, 2005 at 12:22:49PM -0500, John Lenz wrote:
> >> As long as there are a whole bunch of class API changes going on, I would
> >> request that the class_interface add and remove functions get passed the
> >> class_interface pointer as well as the class_device.  This way, the same
> >> function can be used on multiple class_interfaces.
> >
> > I'm sorry, I seem to have missed the patch in this email that implements
> > this feature...
> >
> 
> Here is a patch that updates every usage of class_interface I could find.

Do you have a patch that will take advantage of this change?  I would
prefer to have that before accepting this patch.

thanks,

greg k-h

Re: [PATCH] add class_interface pointer to add and remove functions

[John Lenz]

On Sun, July 3, 2005 3:59 pm, Greg KH said:
> On Thu, Jun 30, 2005 at 04:18:42PM -0500, John Lenz wrote:
>> Here is a patch that updates every usage of class_interface I could
>> find.
>
> Do you have a patch that will take advantage of this change?  I would
> prefer to have that before accepting this patch.
>

No, not for inclusion.  I needed this change while I was working on the
touchscreen driver for Zaurus (http://www.cs.wisc.edu/~lenz/zaurus).  I
have not yet completed that driver, and am currently working on some other
drivers.  So I won't really have a patch until I (or someone else, we can
always use volunteers!) goes back and tries to work on the touchscreen
driver.

I just thought that since the class API is changing anyway, this API
change could come along.  Otherwise I will resubmit this patch when I (or
someone else) gets around to working on the touchscreen driver.

John

Re: [PATCH] add class_interface pointer to add and remove functions

[Greg KH]

On Tue, Jul 05, 2005 at 09:35:53PM -0500, John Lenz wrote:
> On Sun, July 3, 2005 3:59 pm, Greg KH said:
> > On Thu, Jun 30, 2005 at 04:18:42PM -0500, John Lenz wrote:
> >> Here is a patch that updates every usage of class_interface I could
> >> find.
> >
> > Do you have a patch that will take advantage of this change?  I would
> > prefer to have that before accepting this patch.
> >
> 
> No, not for inclusion.  I needed this change while I was working on the
> touchscreen driver for Zaurus (http://www.cs.wisc.edu/~lenz/zaurus).  I
> have not yet completed that driver, and am currently working on some other
> drivers.  So I won't really have a patch until I (or someone else, we can
> always use volunteers!) goes back and tries to work on the touchscreen
> driver.
> 
> I just thought that since the class API is changing anyway, this API
> change could come along.  Otherwise I will resubmit this patch when I (or
> someone else) gets around to working on the touchscreen driver.

Yeah, I prefer to wait until someone uses it.  There's no reason we
can't change the API any time we need to :)

thanks,

greg k-h

[PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

I just pulled from GIT to test bind/unbind. I couldn't get it to work;
it isn't taking into account the CR on the end of the input value of
the sysfs attribute.  This patch will fix it but I'm sure there is a
cleaner solution.

-- 
Jon Smirl
jonsmirl@gmail.com


diff --git a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -137,9 +137,11 @@ decl_subsys(bus, &ktype_bus, NULL);
 static int driver_helper(struct device *dev, void *data)
 {
        const char *name = data;
-
-       if (strcmp(name, dev->bus_id) == 0)
+printk(KERN_ERR "unbind: %s %s\n", name, dev->bus_id);
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0) {
+printk(KERN_ERR "match\n");
                return 1;
+       }
        return 0;
 }

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Dmitry Torokhov]

On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> it isn't taking into account the CR on the end of the input value of
> the sysfs attribute.  This patch will fix it but I'm sure there is a
> cleaner solution.
> 

"echo -n" should take care of this problem I think.

-- 
Dmitry

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > it isn't taking into account the CR on the end of the input value of
> > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > cleaner solution.
> >
> 
> "echo -n" should take care of this problem I think.

That will work around it but I think we should fix it.  Changing to
strncmp() fixes most cases.

-       if (strcmp(name, dev->bus_id) == 0)
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)

I work in this area and I couldn't figure out why it was silently not
working. I had to add the printk to the source before I could figure
it out. I suspect most people are going to have this trouble.

This has also made me realize that I have created other places in the
kernel where my sysfs attribute code is not going to work correctly.
Maybe we should adjust the sysfs driver to strip leading and trailing
white space before passing the string on.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Dmitry Torokhov]

On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > > it isn't taking into account the CR on the end of the input value of
> > > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > > cleaner solution.
> > >
> >
> > "echo -n" should take care of this problem I think.
> 
> That will work around it but I think we should fix it.  Changing to
> strncmp() fixes most cases.
> 
> -       if (strcmp(name, dev->bus_id) == 0)
> +       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)
> 

This will produce "interesting results" if you have both "blah-1" and
"blah-10" devices on the bus.
-- 
Dmitry

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/25/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> > On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > > > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > > > it isn't taking into account the CR on the end of the input value of
> > > > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > > > cleaner solution.
> > > >
> > >
> > > "echo -n" should take care of this problem I think.
> >
> > That will work around it but I think we should fix it.  Changing to
> > strncmp() fixes most cases.
> >
> > -       if (strcmp(name, dev->bus_id) == 0)
> > +       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)
> >
> 
> This will produce "interesting results" if you have both "blah-1" and
> "blah-10" devices on the bus.

Then the better solution is to fix the generic attribute set code to
strip leading and trailing white space.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Mon, Jul 25, 2005 at 12:30:43PM -0400, Jon Smirl wrote:
> On 7/25/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> > > On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > > On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > > > > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > > > > it isn't taking into account the CR on the end of the input value of
> > > > > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > > > > cleaner solution.
> > > > >
> > > >
> > > > "echo -n" should take care of this problem I think.
> > >
> > > That will work around it but I think we should fix it.  Changing to
> > > strncmp() fixes most cases.
> > >
> > > -       if (strcmp(name, dev->bus_id) == 0)
> > > +       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)
> > >
> > 
> > This will produce "interesting results" if you have both "blah-1" and
> > "blah-10" devices on the bus.

Yes, not a good thing for USB devices specifically.

> Then the better solution is to fix the generic attribute set code to
> strip leading and trailing white space.

No, that might break other things as we have not been doing this from
day one.  I'd rather just change these two places, if it's that big of a
deal.  It was documented (in a lwn.net article) and the changelog entry,
that you should use "echo -n".

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/25/05, Greg KH <greg@kroah.com> wrote:
> On Mon, Jul 25, 2005 at 12:30:43PM -0400, Jon Smirl wrote:
> > On 7/25/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> > > > On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > > > On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > > > > > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > > > > > it isn't taking into account the CR on the end of the input value of
> > > > > > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > > > > > cleaner solution.
> > > > > >
> > > > >
> > > > > "echo -n" should take care of this problem I think.
> > > >
> > > > That will work around it but I think we should fix it.  Changing to
> > > > strncmp() fixes most cases.
> > > >
> > > > -       if (strcmp(name, dev->bus_id) == 0)
> > > > +       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)
> > > >
> > >
> > > This will produce "interesting results" if you have both "blah-1" and
> > > "blah-10" devices on the bus.
> 
> Yes, not a good thing for USB devices specifically.
> 
> > Then the better solution is to fix the generic attribute set code to
> > strip leading and trailing white space.
> 
> No, that might break other things as we have not been doing this from
> day one.  I'd rather just change these two places, if it's that big of a
> deal.  It was documented (in a lwn.net article) and the changelog entry,
> that you should use "echo -n".

I didn't realize that echo was adding the CR, I thought that it always
appeared on the end of a sysfs attribute set. So now I have to go add
white space stripping to a dozen fbdev/drm sysfs attribute
implementations. Given that the param is const I may have to allocate
new buffers and copy. I also wonder how many other people have made
the same mistake.

Are you sure it would break other things? These are supposed to be
text attributes, not binary ones.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Mon, Jul 25, 2005 at 08:28:10PM -0400, Jon Smirl wrote:
> On 7/25/05, Greg KH <greg@kroah.com> wrote:
> > On Mon, Jul 25, 2005 at 12:30:43PM -0400, Jon Smirl wrote:
> > > On 7/25/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> > > > > On 7/25/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > > > > On Sunday 24 July 2005 23:09, Jon Smirl wrote:
> > > > > > > I just pulled from GIT to test bind/unbind. I couldn't get it to work;
> > > > > > > it isn't taking into account the CR on the end of the input value of
> > > > > > > the sysfs attribute.  This patch will fix it but I'm sure there is a
> > > > > > > cleaner solution.
> > > > > > >
> > > > > >
> > > > > > "echo -n" should take care of this problem I think.
> > > > >
> > > > > That will work around it but I think we should fix it.  Changing to
> > > > > strncmp() fixes most cases.
> > > > >
> > > > > -       if (strcmp(name, dev->bus_id) == 0)
> > > > > +       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)
> > > > >
> > > >
> > > > This will produce "interesting results" if you have both "blah-1" and
> > > > "blah-10" devices on the bus.
> > 
> > Yes, not a good thing for USB devices specifically.
> > 
> > > Then the better solution is to fix the generic attribute set code to
> > > strip leading and trailing white space.
> > 
> > No, that might break other things as we have not been doing this from
> > day one.  I'd rather just change these two places, if it's that big of a
> > deal.  It was documented (in a lwn.net article) and the changelog entry,
> > that you should use "echo -n".
> 
> I didn't realize that echo was adding the CR, I thought that it always
> appeared on the end of a sysfs attribute set. So now I have to go add
> white space stripping to a dozen fbdev/drm sysfs attribute
> implementations. Given that the param is const I may have to allocate
> new buffers and copy. I also wonder how many other people have made
> the same mistake.

Nah, just zero out that \n character :)

> Are you sure it would break other things? These are supposed to be
> text attributes, not binary ones.

I agree, I don't know what would break.  Care to make a patch so we
could find out?

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/25/05, Greg KH <greg@kroah.com> wrote:
> On Mon, Jul 25, 2005 at 08:28:10PM -0400, Jon Smirl wrote:
> > I didn't realize that echo was adding the CR, I thought that it always
> > appeared on the end of a sysfs attribute set. So now I have to go add
> > white space stripping to a dozen fbdev/drm sysfs attribute
> > implementations. Given that the param is const I may have to allocate
> > new buffers and copy. I also wonder how many other people have made
> > the same mistake.
> 
> Nah, just zero out that \n character :)

The input buffer is "const char * buf". I will have to override the
const to zero it out.

> 
> > Are you sure it would break other things? These are supposed to be
> > text attributes, not binary ones.
> 
> I agree, I don't know what would break.  Care to make a patch so we
> could find out?

I'll put one together to trim leading/trailing white space from the
buffer before it is passed into the attribute functions. Now that I
think about this I believe the attributes should have always had the
leading/trailing white space removed. If we don't do it in the sysfs
code then every driver has to do it.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Mon, Jul 25, 2005 at 08:56:17PM -0400, Jon Smirl wrote:
> On 7/25/05, Greg KH <greg@kroah.com> wrote:
> > On Mon, Jul 25, 2005 at 08:28:10PM -0400, Jon Smirl wrote:
> > > I didn't realize that echo was adding the CR, I thought that it always
> > > appeared on the end of a sysfs attribute set. So now I have to go add
> > > white space stripping to a dozen fbdev/drm sysfs attribute
> > > implementations. Given that the param is const I may have to allocate
> > > new buffers and copy. I also wonder how many other people have made
> > > the same mistake.
> > 
> > Nah, just zero out that \n character :)
> 
> The input buffer is "const char * buf". I will have to override the
> const to zero it out.

Yeah, hence the ":)" above.

> > > Are you sure it would break other things? These are supposed to be
> > > text attributes, not binary ones.
> > 
> > I agree, I don't know what would break.  Care to make a patch so we
> > could find out?
> 
> I'll put one together to trim leading/trailing white space from the
> buffer before it is passed into the attribute functions. Now that I
> think about this I believe the attributes should have always had the
> leading/trailing white space removed. If we don't do it in the sysfs
> code then every driver has to do it.

Ok, sounds good.

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/25/05, Greg KH <greg@kroah.com> wrote:
> > I'll put one together to trim leading/trailing white space from the
> > buffer before it is passed into the attribute functions. Now that I
> > think about this I believe the attributes should have always had the
> > leading/trailing white space removed. If we don't do it in the sysfs
> > code then every driver has to do it.
> 
> Ok, sounds good.

How does this look? This is a count based interface but a lot of
attributes don't work unless I add the terminating zero. This
interface should be documented: count or zero terminated, white space
stripped or not, etc. Are these strings ASCII, UTF8, Unicode?

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
  #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 
@@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
  	struct attribute * attr = to_attr(dentry);
  	struct kobject * kobj = to_kobj(dentry->d_parent);
  	struct sysfs_ops * ops = buffer->ops;
+	char *x, *y, *z;
+
+	/* locate leading white space */
+	x = buffer->page;
+	while( isspace(*x) && (x - buffer->page < count))
+		x++;
+
+	/* locate trailng white space */
+	z = y = x;
+	while (y - buffer->page < count) {
+		y++;
+		z = y;
+		while( isspace(*y) && (y - buffer->page < count)) {
+			y++;
+		}
+	}
+	count = z - x;
+
+	/* strip the white space */
+	if (buffer->page != x)
+		memmove(buffer->page, x, count);
+	buffer->page[count] = '\0';
 
  	return ops->store(kobj,attr,buffer->page,count);
 }


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Dmitry Torokhov]

On Monday 25 July 2005 22:15, Jon Smirl wrote:
> +	while( isspace(*x) && (x - buffer->page < count))
> +		x++;
> +
> +	/* locate trailng white space */
> +	z = y = x;
> +	while (y - buffer->page < count) {
> +		y++;
> +		z = y;
> +		while( isspace(*y) && (y - buffer->page < count)) {
> +			y++;

Can we have consistent space vs. paren placement, pretty please?

-- 
Dmitry

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

Any comments on this? I'll fix up the whitespace issues if everyone
agrees that the code works.

This patch will break all of the fbdev attributes since I was making
wrong assumptions. I have another patch ready to fix them after this
one goes in.

On 7/25/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/25/05, Greg KH <greg@kroah.com> wrote:
> > > I'll put one together to trim leading/trailing white space from the
> > > buffer before it is passed into the attribute functions. Now that I
> > > think about this I believe the attributes should have always had the
> > > leading/trailing white space removed. If we don't do it in the sysfs
> > > code then every driver has to do it.
> >
> > Ok, sounds good.
> 
> How does this look? This is a count based interface but a lot of
> attributes don't work unless I add the terminating zero. This
> interface should be documented: count or zero terminated, white space
> stripped or not, etc. Are these strings ASCII, UTF8, Unicode?
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -6,6 +6,7 @@
>  #include <linux/fsnotify.h>
>   #include <linux/kobject.h>
>  #include <linux/namei.h>
> +#include <linux/ctype.h>
>  #include <asm/uaccess.h>
>  #include <asm/semaphore.h>
> 
> @@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
>         struct attribute * attr = to_attr(dentry);
>         struct kobject * kobj = to_kobj(dentry->d_parent);
>         struct sysfs_ops * ops = buffer->ops;
> +       char *x, *y, *z;
> +
> +       /* locate leading white space */
> +       x = buffer->page;
> +       while( isspace(*x) && (x - buffer->page < count))
> +               x++;
> +
> +       /* locate trailng white space */
> +       z = y = x;
> +       while (y - buffer->page < count) {
> +               y++;
> +               z = y;
> +               while( isspace(*y) && (y - buffer->page < count)) {
> +                       y++;
> +               }
> +       }
> +       count = z - x;
> +
> +       /* strip the white space */
> +       if (buffer->page != x)
> +               memmove(buffer->page, x, count);
> +       buffer->page[count] = '\0';
> 
>         return ops->store(kobj,attr,buffer->page,count);
>  }
> 
> 
> --
> Jon Smirl
> jonsmirl@gmail.com
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Wed, Jul 27, 2005 at 10:05:34PM -0400, Jon Smirl wrote:
> Any comments on this? I'll fix up the whitespace issues if everyone
> agrees that the code works.

I'll add the patch to my tree and -mm if you clean up the whitespace
issues :)

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

New patch with fixed whitespace.

-- 
Jon Smirl
jonsmirl@gmail.com


diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 
@@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
 	struct attribute * attr = to_attr(dentry);
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 	struct sysfs_ops * ops = buffer->ops;
+	char *x, *y, *z;
+
+	/* locate leading white space */
+	x = buffer->page;
+	while (isspace(*x) && (x - buffer->page < count))
+		x++;
+
+	/* locate trailng white space */
+	z = y = x;
+	while (y - buffer->page < count) {
+		y++;
+		z = y;
+		while (isspace(*y) && (y - buffer->page < count)) {
+			y++;
+		}
+	}
+	count = z - x;
+
+	/* strip the white space */
+	if (buffer->page != x)
+		memmove(buffer->page, x, count);
+	buffer->page[count] = '\0';
 
 	return ops->store(kobj,attr,buffer->page,count);
 }

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Wed, Jul 27, 2005 at 11:59:11PM -0400, Jon Smirl wrote:
> New patch with fixed whitespace.

But no changelog info or signed-off-by line :(

Care to try it again?

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

Change log and signed off

-- 
Jon Smirl
jonsmirl@gmail.com


Remove leading and trailing whitespace when text sysfs attributes are
assigned a value.


Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 
@@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
 	struct attribute * attr = to_attr(dentry);
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 	struct sysfs_ops * ops = buffer->ops;
+	char *x, *y, *z;
+
+	/* locate leading white space */
+	x = buffer->page;
+	while (isspace(*x) && (x - buffer->page < count))
+		x++;
+
+	/* locate trailng white space */
+	z = y = x;
+	while (y - buffer->page < count) {
+		y++;
+		z = y;
+		while (isspace(*y) && (y - buffer->page < count)) {
+			y++;
+		}
+	}
+	count = z - x;
+
+	/* strip the white space */
+	if (buffer->page != x)
+		memmove(buffer->page, x, count);
+	buffer->page[count] = '\0';
 
 	return ops->store(kobj,attr,buffer->page,count);
 }

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Thu, Jul 28, 2005 at 12:49:21AM -0400, Jon Smirl wrote:
> @@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
>  	struct attribute * attr = to_attr(dentry);
>  	struct kobject * kobj = to_kobj(dentry->d_parent);
>  	struct sysfs_ops * ops = buffer->ops;
> +	char *x, *y, *z;
> +
> +	/* locate leading white space */
> +	x = buffer->page;
> +	while (isspace(*x) && (x - buffer->page < count))
> +		x++;

Ok, I can follow this.  For example
buffer->page = "  foo  "

then x = "foo  " at the end of that .

> +	/* locate trailng white space */
> +	z = y = x;
> +	while (y - buffer->page < count) {
> +		y++;
> +		z = y;
> +		while (isspace(*y) && (y - buffer->page < count)) {
> +			y++;
> +		}
> +	}
> +	count = z - x;

Hm, I _think_ this works, but I need someone else to verify this...
Anyone else?


> +
> +	/* strip the white space */
> +	if (buffer->page != x)
> +		memmove(buffer->page, x, count);
> +	buffer->page[count] = '\0';

Why move the buffer?  Why not just pass in a pointer to the start of the
"non-whitespace filled" buffer to the store() function?

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Mitchell Blank Jr]

Greg KH wrote:
> > +	/* locate trailng white space */
> > +	z = y = x;
> > +	while (y - buffer->page < count) {
> > +		y++;
> > +		z = y;
> > +		while (isspace(*y) && (y - buffer->page < count)) {
> > +			y++;
> > +		}
> > +	}
> > +	count = z - x;
> 
> Hm, I _think_ this works, but I need someone else to verify this...
> Anyone else?

It looks sane-ish to me, but also more complicated than need be.  Why can't
you just do something like:

	while (count > 0 && isspace(x[count - 1]))
		count--;

-Mitch

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/28/05, Mitchell Blank Jr <mitch@sfgoth.com> wrote:
> Greg KH wrote:
> > > +   /* locate trailng white space */
> > > +   z = y = x;
> > > +   while (y - buffer->page < count) {
> > > +           y++;
> > > +           z = y;
> > > +           while (isspace(*y) && (y - buffer->page < count)) {
> > > +                   y++;
> > > +           }
> > > +   }
> > > +   count = z - x;
> >
> > Hm, I _think_ this works, but I need someone else to verify this...
> > Anyone else?
> 
> It looks sane-ish to me, but also more complicated than need be.  Why can't
> you just do something like:
> 
>         while (count > 0 && isspace(x[count - 1]))
>                 count--;
> 
> -Mitch
> 

Do we need to deal with UTF8 here? I did the forward loop because you
can't parse UTF8 backwards. If UTF8 is possible I need to change the
pointer inc function.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Donnerstag, 28. Juli 2005 14:54 schrieb Jon Smirl:
> On 7/28/05, Mitchell Blank Jr <mitch@sfgoth.com> wrote:
> > Greg KH wrote:
> > > > +   /* locate trailng white space */
> > > > +   z = y = x;
> > > > +   while (y - buffer->page < count) {
> > > > +           y++;
> > > > +           z = y;
> > > > +           while (isspace(*y) && (y - buffer->page < count)) {
> > > > +                   y++;
> > > > +           }
> > > > +   }
> > > > +   count = z - x;
> > >
> > > Hm, I _think_ this works, but I need someone else to verify this...
> > > Anyone else?
> > 
> > It looks sane-ish to me, but also more complicated than need be.  Why can't
> > you just do something like:
> > 
> >         while (count > 0 && isspace(x[count - 1]))
> >                 count--;
> > 
> > -Mitch
> > 
> 
> Do we need to deal with UTF8 here? I did the forward loop because you
> can't parse UTF8 backwards. If UTF8 is possible I need to change the
> pointer inc function.

This considerations show that having parsing code in kernel space is
a questionable idea. Are you absolutely sure that you cannot do with less?

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Paulo Marques]

Jon Smirl wrote:
> On 7/28/05, Mitchell Blank Jr <mitch@sfgoth.com> wrote:
>>[...]
>>It looks sane-ish to me, but also more complicated than need be.  Why can't
>>you just do something like:
>>
>>        while (count > 0 && isspace(x[count - 1]))
>>                count--;
> 
> Do we need to deal with UTF8 here? I did the forward loop because you
> can't parse UTF8 backwards. If UTF8 is possible I need to change the
> pointer inc function.

I don't think it matters here. Even with UTF8, any char that makes 
isspace return true, can't be part of a multi-byte char, as every byte 
in a multi-byte char in UTF8 has the MSB set, i.e., >= 0x80.

-- 
Paulo Marques - www.grupopie.com

It is a mistake to think you can solve any major problems
just with potatoes.
Douglas Adams

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Mitchell Blank Jr]

Jon Smirl wrote:
> Do we need to deal with UTF8 here? I did the forward loop because you
> can't parse UTF8 backwards. If UTF8 is possible I need to change the
> pointer inc function.

As others have mentioned there shouldn't be a UTF8 problem with isspace().
However, even if you wanted to scan going forwards you can do it without
the complexity of a nested loop:

	char *real_end;

	for (real_end = x; x - buffer->page < count; x++)
		if (!isspace(*x))
			real_end = x + 1;
	*real_end = '\0';
	// then fix "count"

BTW, my code I posted yesterday is incomplete since I neglected to notice
that the "count = z - x" at the end is used to repair "count" damage from
both the leading and trailing whitespace stripping.  Its actually easier
to strip the trailing whitespace first, like:

	while (count > 0 && isspace(buffer->page[count - 1]))
		count--;	/* strip trailing whitespace */
	if (count > 0 && isspace(buffer->page[0])) {
		/*
		 * We have leading whitespace but also at least one
		 * non-whitespace character
		 */
		const char *x = buffer->page;
		do {
			x++;
			count--;
		} while (isspace(*x));
		memmove(buffer->page, x, count);
	}
	buffer->page[count] = '\0';

-Mitch

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Thu, Jul 28, 2005 at 08:54:53AM -0400, Jon Smirl wrote:
> On 7/28/05, Mitchell Blank Jr <mitch@sfgoth.com> wrote:
> > Greg KH wrote:
> > > > +   /* locate trailng white space */
> > > > +   z = y = x;
> > > > +   while (y - buffer->page < count) {
> > > > +           y++;
> > > > +           z = y;
> > > > +           while (isspace(*y) && (y - buffer->page < count)) {
> > > > +                   y++;
> > > > +           }
> > > > +   }
> > > > +   count = z - x;
> > >
> > > Hm, I _think_ this works, but I need someone else to verify this...
> > > Anyone else?
> > 
> > It looks sane-ish to me, but also more complicated than need be.  Why can't
> > you just do something like:
> > 
> >         while (count > 0 && isspace(x[count - 1]))
> >                 count--;
> > 
> > -Mitch
> > 
> 
> Do we need to deal with UTF8 here? I did the forward loop because you
> can't parse UTF8 backwards. If UTF8 is possible I need to change the
> pointer inc function.

No, we don't need to handle UTF8.  Or if we do, then we better not make
a general filter function, as it will be a mess.

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

New, simplified version of the sysfs whitespace strip patch...

-- 
Jon Smirl
jonsmirl@gmail.com

Remove leading and trailing whitespace when text sysfs attribute is set
signed-off-by: Jon Smirl <jonsmirl@gmail.com>

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 
@@ -207,8 +208,22 @@ flush_write_buffer(struct dentry * dentr
 	struct attribute * attr = to_attr(dentry);
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 	struct sysfs_ops * ops = buffer->ops;
+	char *x;
 
-	return ops->store(kobj,attr,buffer->page,count);
+	/* locate trailing white space */
+	while ((count > 0) && isspace(buffer->page[count - 1]))
+		count--;
+
+	/* locate leading white space */
+	x = buffer->page;
+	while (isspace(*x) && (x - buffer->page < count))
+		x++;
+	count -= (x - buffer->page);
+
+	/* terminate the string */
+	x[count] = '\0';
+
+	return ops->store(kobj, attr, x, count);
 }

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Mitchell Blank Jr]

Still one nitpick:

Jon Smirl wrote:
> +	while (isspace(*x) && (x - buffer->page < count))
> +		x++;

I think you can just do:

	if (count > 0)
		while (isspace(*x))
			x++;

If the passed-in string was fully whitespace then the trailing-whitespace
stripping would already reduce it to count==0.  So as long as you check
for that case you can be sure that the while() will stop before x falls off
the end of the string.

Other than that it looks fine to me now.

-Mitch

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

Even simpler version....

-- 
Jon Smirl
jonsmirl@gmail.com

Remove leading and trailing whitespace when text sysfs attribute is set
signed-off-by: Jon Smirl <jonsmirl@gmail.com>

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 
@@ -207,8 +208,23 @@ flush_write_buffer(struct dentry * dentr
 	struct attribute * attr = to_attr(dentry);
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 	struct sysfs_ops * ops = buffer->ops;
+	char *x;
 
-	return ops->store(kobj,attr,buffer->page,count);
+	/* locate trailing white space */
+	while ((count > 0) && isspace(buffer->page[count - 1]))
+		count--;
+
+	/* locate leading white space */
+	x = buffer->page;
+	if (count > 0) {
+		while (isspace(*x))
+			x++;
+		count -= (x - buffer->page);
+	}
+	/* terminate the string */
+	x[count] = '\0';
+
+	return ops->store(kobj, attr, x, count);
 }

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

Greg, is this ok for your tree now or does it need more work?

On 7/28/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> Even simpler version....
> 
> --
> Jon Smirl
> jonsmirl@gmail.com
> 
> Remove leading and trailing whitespace when text sysfs attribute is set
> signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -6,6 +6,7 @@
>  #include <linux/fsnotify.h>
>  #include <linux/kobject.h>
>  #include <linux/namei.h>
> +#include <linux/ctype.h>
>  #include <asm/uaccess.h>
>  #include <asm/semaphore.h>
> 
> @@ -207,8 +208,23 @@ flush_write_buffer(struct dentry * dentr
>         struct attribute * attr = to_attr(dentry);
>         struct kobject * kobj = to_kobj(dentry->d_parent);
>         struct sysfs_ops * ops = buffer->ops;
> +       char *x;
> 
> -       return ops->store(kobj,attr,buffer->page,count);
> +       /* locate trailing white space */
> +       while ((count > 0) && isspace(buffer->page[count - 1]))
> +               count--;
> +
> +       /* locate leading white space */
> +       x = buffer->page;
> +       if (count > 0) {
> +               while (isspace(*x))
> +                       x++;
> +               count -= (x - buffer->page);
> +       }
> +       /* terminate the string */
> +       x[count] = '\0';
> +
> +       return ops->store(kobj, attr, x, count);
>  }
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Fri, Jul 29, 2005 at 02:50:44PM -0400, Jon Smirl wrote:
> Greg, is this ok for your tree now or does it need more work?

It's in my queue, will add it to the tree next week.  Sorry for the
delay, was at OSCON this week...

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 8/5/05, Greg KH <greg@kroah.com> wrote:
> On Fri, Jul 29, 2005 at 02:50:44PM -0400, Jon Smirl wrote:
> > Greg, is this ok for your tree now or does it need more work?
> 
> It's in my queue, will add it to the tree next week.  Sorry for the
> delay, was at OSCON this week...

Glad to see this. After it lands in the tree I'll fix up about ten
places where I have attribute processing wrong because of this. Is
there some place in the sysfs doc that this can be mentioned?

Another thing that should be cleared up is if the attributes are
described by length or zero termination. Right now they get both. I
suspect the right answer here is supposed to be by length.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/28/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> Even simpler version....
> 
> --
> Jon Smirl
> jonsmirl@gmail.com
> 
> Remove leading and trailing whitespace when text sysfs attribute is set
> signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -6,6 +6,7 @@
>  #include <linux/fsnotify.h>
>  #include <linux/kobject.h>
>  #include <linux/namei.h>
> +#include <linux/ctype.h>
>  #include <asm/uaccess.h>
>  #include <asm/semaphore.h>
> 
> @@ -207,8 +208,23 @@ flush_write_buffer(struct dentry * dentr
>         struct attribute * attr = to_attr(dentry);
>         struct kobject * kobj = to_kobj(dentry->d_parent);
>         struct sysfs_ops * ops = buffer->ops;
> +       char *x;
> 
> -       return ops->store(kobj,attr,buffer->page,count);
> +       /* locate trailing white space */
> +       while ((count > 0) && isspace(buffer->page[count - 1]))
> +               count--;
> +
> +       /* locate leading white space */
> +       x = buffer->page;
> +       if (count > 0) {
> +               while (isspace(*x))
> +                       x++;
> +               count -= (x - buffer->page);
> +       }
> +       /* terminate the string */
> +       x[count] = '\0';

Should we add a check for a NULL string here?  It seems not all
drivers were prepared to handle a zero length store().

If (count == 0)
      return 0;

> +
> +       return ops->store(kobj, attr, x, count);
>  }
> 

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Donnerstag, 28. Juli 2005 21:57 schrieb Jon Smirl:
> New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed
input is supplied?

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/28/05, Oliver Neukum <oliver@neukum.org> wrote:
> Am Donnerstag, 28. Juli 2005 21:57 schrieb Jon Smirl:
> > New, simplified version of the sysfs whitespace strip patch...
> 
> Could you tell me why you don't just fail the operation if malformed
> input is supplied?

Leading/trailing white space should be allowed. For example echo
appends '\n' unless you know to use -n. It is easier to fix the kernel
than to teach everyone to use -n.

> 
>         Regards
>                 Oliver
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Pavel Machek]

Hi!

> > > New, simplified version of the sysfs whitespace strip patch...
> > 
> > Could you tell me why you don't just fail the operation if malformed
> > input is supplied?
> 
> Leading/trailing white space should be allowed. For example echo
> appends '\n' unless you know to use -n. It is easier to fix the kernel
> than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn.
We are not going to add such workarounds all over the kernel...
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
> 
> > > > New, simplified version of the sysfs whitespace strip patch...
> > >
> > > Could you tell me why you don't just fail the operation if malformed
> > > input is supplied?
> >
> > Leading/trailing white space should be allowed. For example echo
> > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > than to teach everyone to use -n.
> 
> Please, NO! echo -n is the right thing to do, and users will eventually learn.
> We are not going to add such workarounds all over the kernel...

It is not a work around. These are text attributes meant for human
use.  Humans have a hard time cleaning up things they can't see. And
the failure mode for this is awful, your attribute won't set but
everything on the screen looks fine.

echo is not the only source of problems. I had some trailing
whitespace in a shell script and it took me a couple of hours to
discover why the attribute set was failing. I finally had to add debug
code to the kernel and reboot to located the problem. Normal users are
never going to figure this out.

Binary attributes are for program use, they should not get cleaned up.
If you dont want the whitespace cleaning switch to a binary one.

>                                 Pavel
> --
> 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
> 
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> > 
> > > > > New, simplified version of the sysfs whitespace strip patch...
> > > >
> > > > Could you tell me why you don't just fail the operation if malformed
> > > > input is supplied?
> > >
> > > Leading/trailing white space should be allowed. For example echo
> > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > than to teach everyone to use -n.
> > 
> > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > We are not going to add such workarounds all over the kernel...
> 
> It is not a work around. These are text attributes meant for human
> use.  Humans have a hard time cleaning up things they can't see. And
> the failure mode for this is awful, your attribute won't set but
> everything on the screen looks fine.

The average user has no place poking sysfs. Root should know when
to use -n, as should shell scripts.

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> > On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > > Hi!
> > >
> > > > > > New, simplified version of the sysfs whitespace strip patch...
> > > > >
> > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > input is supplied?
> > > >
> > > > Leading/trailing white space should be allowed. For example echo
> > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > than to teach everyone to use -n.
> > >
> > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > We are not going to add such workarounds all over the kernel...
> >
> > It is not a work around. These are text attributes meant for human
> > use.  Humans have a hard time cleaning up things they can't see. And
> > the failure mode for this is awful, your attribute won't set but
> > everything on the screen looks fine.
> 
> The average user has no place poking sysfs. Root should know when
> to use -n, as should shell scripts.

So the average user never needs to change their console mode? Check
out /sys/class/graphics/fb/modes and mode.

> 
>         Regards
>                 Oliver
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:
> On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> > > On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > > > Hi!
> > > >
> > > > > > > New, simplified version of the sysfs whitespace strip patch...
> > > > > >
> > > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > > input is supplied?
> > > > >
> > > > > Leading/trailing white space should be allowed. For example echo
> > > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > > than to teach everyone to use -n.
> > > >
> > > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > > We are not going to add such workarounds all over the kernel...
> > >
> > > It is not a work around. These are text attributes meant for human
> > > use.  Humans have a hard time cleaning up things they can't see. And
> > > the failure mode for this is awful, your attribute won't set but
> > > everything on the screen looks fine.
> > 
> > The average user has no place poking sysfs. Root should know when
> > to use -n, as should shell scripts.
> 
> So the average user never needs to change their console mode? Check
> out /sys/class/graphics/fb/modes and mode.

That is what we have helper scripts for.

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:
> > On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > > Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> > > > On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > > > > Hi!
> > > > >
> > > > > > > > New, simplified version of the sysfs whitespace strip patch...
> > > > > > >
> > > > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > > > input is supplied?
> > > > > >
> > > > > > Leading/trailing white space should be allowed. For example echo
> > > > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > > > than to teach everyone to use -n.
> > > > >
> > > > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > > > We are not going to add such workarounds all over the kernel...
> > > >
> > > > It is not a work around. These are text attributes meant for human
> > > > use.  Humans have a hard time cleaning up things they can't see. And
> > > > the failure mode for this is awful, your attribute won't set but
> > > > everything on the screen looks fine.
> > >
> > > The average user has no place poking sysfs. Root should know when
> > > to use -n, as should shell scripts.
> >
> > So the average user never needs to change their console mode? Check
> > out /sys/class/graphics/fb/modes and mode.
> 
> That is what we have helper scripts for.

If we are going back to needing helper scripts then I should just
remove the entire sysfs graphics interface and switch back to using
ioctls and a helper app. Of could no one can ever find the helper app
or remember how it works. I thought one of the main reasons behind the
sysfs interface was to eliminate these helper apps.

Without doing whitespace cleanup there is a 100% probability that this
will generate bug reports. I know this for a fact because I am already
getting them.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Freitag, 5. August 2005 20:47 schrieb Jon Smirl:
> On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:
> > > On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > > > Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> > > > > On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > > > > > Hi!
> > > > > >
> > > > > > > > > New, simplified version of the sysfs whitespace strip patch...
> > > > > > > >
> > > > > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > > > > input is supplied?
> > > > > > >
> > > > > > > Leading/trailing white space should be allowed. For example echo
> > > > > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > > > > than to teach everyone to use -n.
> > > > > >
> > > > > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > > > > We are not going to add such workarounds all over the kernel...
> > > > >
> > > > > It is not a work around. These are text attributes meant for human
> > > > > use.  Humans have a hard time cleaning up things they can't see. And
> > > > > the failure mode for this is awful, your attribute won't set but
> > > > > everything on the screen looks fine.
> > > >
> > > > The average user has no place poking sysfs. Root should know when
> > > > to use -n, as should shell scripts.
> > >
> > > So the average user never needs to change their console mode? Check
> > > out /sys/class/graphics/fb/modes and mode.
> > 
> > That is what we have helper scripts for.
> 
> If we are going back to needing helper scripts then I should just
> remove the entire sysfs graphics interface and switch back to using
> ioctls and a helper app. Of could no one can ever find the helper app
> or remember how it works. I thought one of the main reasons behind the
> sysfs interface was to eliminate these helper apps.

The point is that you _can_ do it with echo, not that it is _easy_.
Nor is sysfs a solution in any case.

> Without doing whitespace cleanup there is a 100% probability that this
> will generate bug reports. I know this for a fact because I am already
> getting them.

Stupid users are not a reason for kernel bloat.

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> Am Freitag, 5. August 2005 20:47 schrieb Jon Smirl:
> > On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > > Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:
> > > > On 8/5/05, Oliver Neukum <oliver@neukum.org> wrote:
> > > > > Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:
> > > > > > On 1/1/02, Pavel Machek <pavel@ucw.cz> wrote:
> > > > > > > Hi!
> > > > > > >
> > > > > > > > > > New, simplified version of the sysfs whitespace strip patch...
> > > > > > > > >
> > > > > > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > > > > > input is supplied?
> > > > > > > >
> > > > > > > > Leading/trailing white space should be allowed. For example echo
> > > > > > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > > > > > than to teach everyone to use -n.
> > > > > > >
> > > > > > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > > > > > We are not going to add such workarounds all over the kernel...
> > > > > >
> > > > > > It is not a work around. These are text attributes meant for human
> > > > > > use.  Humans have a hard time cleaning up things they can't see. And
> > > > > > the failure mode for this is awful, your attribute won't set but
> > > > > > everything on the screen looks fine.
> > > > >
> > > > > The average user has no place poking sysfs. Root should know when
> > > > > to use -n, as should shell scripts.
> > > >
> > > > So the average user never needs to change their console mode? Check
> > > > out /sys/class/graphics/fb/modes and mode.
> > >
> > > That is what we have helper scripts for.
> >
> > If we are going back to needing helper scripts then I should just
> > remove the entire sysfs graphics interface and switch back to using
> > ioctls and a helper app. Of could no one can ever find the helper app
> > or remember how it works. I thought one of the main reasons behind the
> > sysfs interface was to eliminate these helper apps.
> 
> The point is that you _can_ do it with echo, not that it is _easy_.
> Nor is sysfs a solution in any case.
> 
> > Without doing whitespace cleanup there is a 100% probability that this
> > will generate bug reports. I know this for a fact because I am already
> > getting them.
> 
> Stupid users are not a reason for kernel bloat.

You have a very wrapped sense of kernel bloat. This is nine lines of
code whose absence is guaranteed to generate a bunch of bug reports.
Not having it is also causing various implementers to implement
attribute processing differently. Some are stripping white space in
their implementations and some are not. If you want to attack kernel
bloat there are much more productive areas.

If whitespace cleanup is rejected I believe we should eliminate text
based sysfs attributes in general and make them all binary. I'll
probably remove the fbdev sysfs interface because I don't want to deal
with the bug reports reporting the same problem over and over.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

> > Stupid users are not a reason for kernel bloat.
> 
> You have a very wrapped sense of kernel bloat. This is nine lines of
> code whose absence is guaranteed to generate a bunch of bug reports.

They are supposed to be present, but not in the kernel.

> Not having it is also causing various implementers to implement
> attribute processing differently. Some are stripping white space in
> their implementations and some are not. If you want to attack kernel
> bloat there are much more productive areas.

If somebody is stripping whitespace in the kernel, find and destroy.
 
> If whitespace cleanup is rejected I believe we should eliminate text
> based sysfs attributes in general and make them all binary. I'll
> probably remove the fbdev sysfs interface because I don't want to deal
> with the bug reports reporting the same problem over and over.

You are seeing this wrong. The problem is not binary vs. text.
If binary were the problem we'd have a generic ioctl tool that takes
arguments in any number format you'd want. The problem is untyped
data. ASCII-strings without leading or trailing whitespece is a clearly
defined datatype. Requiring a tool ensuring this format is met or care
is used if other tools are used is perfectly in order. The kernel does
not guess what an input is supposed to mean. Anything else is bloat.

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Pavel Machek]

Hi!

> > > If we are going back to needing helper scripts then I should just
> > > remove the entire sysfs graphics interface and switch back to using
> > > ioctls and a helper app. Of could no one can ever find the helper app
> > > or remember how it works. I thought one of the main reasons behind the
> > > sysfs interface was to eliminate these helper apps.
> > 
> > The point is that you _can_ do it with echo, not that it is _easy_.
> > Nor is sysfs a solution in any case.
> > 
> > > Without doing whitespace cleanup there is a 100% probability that this
> > > will generate bug reports. I know this for a fact because I am already
> > > getting them.
> > 
> > Stupid users are not a reason for kernel bloat.
> 
> You have a very wrapped sense of kernel bloat. This is nine lines of
> code whose absence is guaranteed to generate a bunch of bug reports.
> Not having it is also causing various implementers to implement
> attribute processing differently. Some are stripping white space in
> their implementations and some are not. If you want to attack kernel

Can you point place where we do strip whitespace in kernel sysfs handlers?
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Pavel Machek]

Hi!

> > > > Could you tell me why you don't just fail the operation if malformed
> > > > input is supplied?
> > >
> > > Leading/trailing white space should be allowed. For example echo
> > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > than to teach everyone to use -n.
> > 
> > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > We are not going to add such workarounds all over the kernel...
> 
> It is not a work around. These are text attributes meant for human
> use.  Humans have a hard time cleaning up things they can't see. And
> the failure mode for this is awful, your attribute won't set but
> everything on the screen looks fine.

Kernel is not a place to be user friendly. Or do you propose stripping whitespace
for open(), too? File called "foo.txt    " certainly *is* going to be confusing, but it should be allowed at kernel level.

Now... echo foo > /sys/var does not properly report errors. Thats bad, but it needs to
be fixed in bash.
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 8/7/05, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
> 
> > > > > Could you tell me why you don't just fail the operation if malformed
> > > > > input is supplied?
> > > >
> > > > Leading/trailing white space should be allowed. For example echo
> > > > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > > > than to teach everyone to use -n.
> > >
> > > Please, NO! echo -n is the right thing to do, and users will eventually learn.
> > > We are not going to add such workarounds all over the kernel...
> >
> > It is not a work around. These are text attributes meant for human
> > use.  Humans have a hard time cleaning up things they can't see. And
> > the failure mode for this is awful, your attribute won't set but
> > everything on the screen looks fine.
> 
> Kernel is not a place to be user friendly. Or do you propose stripping whitespace
> for open(), too? File called "foo.txt    " certainly *is* going to be confusing, but it should be allowed at kernel level.

open is not made for human use, it is used by programs and only
indirectly by humans. sysfs variables are used by directly humans.

> 
> Now... echo foo > /sys/var does not properly report errors. Thats bad, but it needs to
> be fixed in bash.

It is going to take a lot more code to return an error that a
parameter didn't match because of extra white space that it would take
to simply remove the whitespace.

> --
> 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
> 
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Pavel Machek]

Hi!

> > > It is not a work around. These are text attributes meant for human
> > > use.  Humans have a hard time cleaning up things they can't see. And
> > > the failure mode for this is awful, your attribute won't set but
> > > everything on the screen looks fine.
> > 
> > Kernel is not a place to be user friendly. Or do you propose stripping whitespace
> > for open(), too? File called "foo.txt    " certainly *is* going to be confusing, but it should be allowed at kernel level.
> 
> open is not made for human use, it is used by programs and only
> indirectly by humans. sysfs variables are used by directly humans.

Both are kernel interfaces; I can use open() by hand just fine...

> > Now... echo foo > /sys/var does not properly report errors. Thats bad, but it needs to
> > be fixed in bash.
> 
> It is going to take a lot more code to return an error that a
> parameter didn't match because of extra white space that it would take
> to simply remove the whitespace.

I believe we do correctly report errors in write() calls already. Bash
not reporting them to the user is different problem.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[David Weinehall]

On Tue, Jan 01, 2002 at 08:53:39AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > New, simplified version of the sysfs whitespace strip patch...
> > > 
> > > Could you tell me why you don't just fail the operation if malformed
> > > input is supplied?
> > 
> > Leading/trailing white space should be allowed. For example echo
> > appends '\n' unless you know to use -n. It is easier to fix the kernel
> > than to teach everyone to use -n.
> 
> Please, NO! echo -n is the right thing to do, and users will eventually learn.
> We are not going to add such workarounds all over the kernel...

Ahhh, this would be so much easier if people just got used to using
printf instead of echo when doing text output. =)


Regards: David Weinehall
-- 
 /) David Weinehall <tao@acc.umu.se> /) Northern lights wander      (\
//  Maintainer of the v2.0 kernel   //  Dance across the winter sky //
\)  http://www.acc.umu.se/~tao/    (/   Full colour fire           (/

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Oliver Neukum]

Am Donnerstag, 28. Juli 2005 23:12 schrieb Jon Smirl:
> On 7/28/05, Oliver Neukum <oliver@neukum.org> wrote:
> > Am Donnerstag, 28. Juli 2005 21:57 schrieb Jon Smirl:
> > > New, simplified version of the sysfs whitespace strip patch...
> > 
> > Could you tell me why you don't just fail the operation if malformed
> > input is supplied?
> 
> Leading/trailing white space should be allowed. For example echo
> appends '\n' unless you know to use -n. It is easier to fix the kernel
> than to teach everyone to use -n.

Why? If you can do it in user space, do so.

	Regards
		Oliver

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Jon Smirl]

On 7/28/05, Greg KH <greg@kroah.com> wrote:
> On Thu, Jul 28, 2005 at 12:49:21AM -0400, Jon Smirl wrote:
> > @@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
> >       struct attribute * attr = to_attr(dentry);
> >       struct kobject * kobj = to_kobj(dentry->d_parent);
> >       struct sysfs_ops * ops = buffer->ops;
> > +     char *x, *y, *z;
> > +
> > +     /* locate leading white space */
> > +     x = buffer->page;
> > +     while (isspace(*x) && (x - buffer->page < count))
> > +             x++;
> 
> Ok, I can follow this.  For example
> buffer->page = "  foo  "
> 
> then x = "foo  " at the end of that .
> 
> > +     /* locate trailng white space */
> > +     z = y = x;
> > +     while (y - buffer->page < count) {
> > +             y++;
> > +             z = y;
> > +             while (isspace(*y) && (y - buffer->page < count)) {
> > +                     y++;
> > +             }
> > +     }
> > +     count = z - x;
> 
> Hm, I _think_ this works, but I need someone else to verify this...
> Anyone else?
> 
> 
> > +
> > +     /* strip the white space */
> > +     if (buffer->page != x)
> > +             memmove(buffer->page, x, count);
> > +     buffer->page[count] = '\0';
> 
> Why move the buffer?  Why not just pass in a pointer to the start of the
> "non-whitespace filled" buffer to the store() function?

That will work if you say it does. I didn't know what happens with
buffer->page in other parts of the code.

> 
> thanks,
> 
> greg k-h
> 


-- 
Jon Smirl
jonsmirl@gmail.com

[RFC] bind and unbind drivers from userspace through sysfs

[Greg KH]

Now that we have the internal infrastructure of the driver model
reworked so the locks aren't so global and imposing, it's possible to
bind and unbind drivers from devices from userspace with only a very
tiny ammount of code.

In reply to this email, are two patches, one that adds bind and one that
adds unbind functionality.  I've added these to my trees and should show
up in the next -mm releases.  Comments appreciated.

Oh, and yes, we still need a way to add new device ids to drivers from
sysfs, like PCI currently has.  I'll be working on that next.

Even so, with these two patches, people should be able to do things that
they have been wanting to do for a while (like take over the what driver
to what device logic in userspace, as I know some distro installers
really want to do.)

thanks,

greg k-h

[PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

This adds a single file, "unbind", to the sysfs directory of every
device that is currently bound to a driver.  To unbind the driver from
the device, write anything to this file and they will be disconnected
from each other.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/dd.c |   11 +++++++++++
 1 files changed, 11 insertions(+)

--- gregkh-2.6.orig/drivers/base/dd.c	2005-06-22 17:56:48.000000000 -0700
+++ gregkh-2.6/drivers/base/dd.c	2005-06-22 17:56:59.000000000 -0700
@@ -23,6 +23,15 @@
 
 #define to_drv(node) container_of(node, struct device_driver, kobj.entry)
 
+/* manually detach a device from it's associated driver. */
+/* Any write will cause it to happen. */
+static ssize_t device_unbind(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	device_release_driver(dev);
+	return count;
+}
+static DEVICE_ATTR(unbind, S_IWUSR, NULL, device_unbind);
 
 /**
  *	device_bind_driver - bind a driver to one device.
@@ -46,6 +55,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);
 }
 
 /**
@@ -191,6 +201,7 @@
 		get_driver(drv);
 		sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
 		sysfs_remove_link(&dev->kobj, "driver");
+		device_remove_file(dev, &dev_attr_unbind);
 		klist_remove(&dev->knode_driver);
 
 		if (drv->remove)

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Patrick Mochel]

On Thu, 23 Jun 2005, Greg KH wrote:

> This adds a single file, "unbind", to the sysfs directory of every
> device that is currently bound to a driver.  To unbind the driver from
> the device, write anything to this file and they will be disconnected
> from each other.

Do you think it would be better to put the 'unbind' file in the driver's
directory and have it accept the bus ID of a device that it's bound to?
This would make it more similar to the complementary 'bind' functionality.


	Pat

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Greg KH]

On Fri, Jun 24, 2005 at 08:57:15AM -0700, Patrick Mochel wrote:
> 
> On Thu, 23 Jun 2005, Greg KH wrote:
> 
> > This adds a single file, "unbind", to the sysfs directory of every
> > device that is currently bound to a driver.  To unbind the driver from
> > the device, write anything to this file and they will be disconnected
> > from each other.
> 
> Do you think it would be better to put the 'unbind' file in the driver's
> directory and have it accept the bus ID of a device that it's bound to?
> This would make it more similar to the complementary 'bind' functionality.

Yeah, you are right, I'll make that change.  Heh, symmetry, what a
concept...

thanks,

greg k-h

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Dmitry Torokhov]

On Friday 24 June 2005 22:27, Greg KH wrote:
> On Fri, Jun 24, 2005 at 08:57:15AM -0700, Patrick Mochel wrote:
> > 
> > On Thu, 23 Jun 2005, Greg KH wrote:
> > 
> > > This adds a single file, "unbind", to the sysfs directory of every
> > > device that is currently bound to a driver.  To unbind the driver from
> > > the device, write anything to this file and they will be disconnected
> > > from each other.
> > 
> > Do you think it would be better to put the 'unbind' file in the driver's
> > directory and have it accept the bus ID of a device that it's bound to?
> > This would make it more similar to the complementary 'bind' functionality.
>

It is more complex this way. You need to do additional parsing...
 
> Yeah, you are right, I'll make that change.  Heh, symmetry, what a
> concept...
>

Actually, I think that both should be in device's directory. When unbinding
a device you normally don't care what driver it is bound to, you just want
to make sure that there is no driver bound to the device afterwards. I.e it
is a operation on device. When binding you could argue both ways, but then
again you usually have a piece of hardware you want to assign specific driver
for, so I'd say it is operation on device as well.

Also, some buses may implement other similar operatons, like rescan and
reconnect (serio/gameport buses). They are similar to "bind" except that
you do not specify driver at all. If bind/unbind is in the driver and
connect/reconnect are in the device's directory it will be complete mess.  
 
-- 
Dmitry

Re: [PATCH] driver core: Add the ability to unbind drivers to devices from userspace

[Michael Tokarev]

Dmitry Torokhov wrote:
[bind/unbind in device or driver dir in sysfs]
> 
> Actually, I think that both should be in device's directory. When unbinding
> a device you normally don't care what driver it is bound to, you just want
> to make sure that there is no driver bound to the device afterwards. I.e it
> is a operation on device. When binding you could argue both ways, but then
> again you usually have a piece of hardware you want to assign specific driver
> for, so I'd say it is operation on device as well.

A small comment.  How about having one file named 'bind', which acts like
either bind or unbind if nothing (empty string) has written to it?

(for fun.. that'd be 'driver' file, which, when read, returns the name
of the driver currently bound to the device.. but too bad, 'driver' is
a symlink already...)

/mjt