Skip to contentSkip to conversation

driver-model: manual device attach

PATCH In review Tejun Heo 5 participants 17 messages Nov 4, 2004 – Nov 8, 2004

Subsystems (4)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

cover driver-model: manual device attach

Hello, again. :-)

These are the manual device attach patches I was talking about in the
previous posting. These patches need devparam patches to be applied first. It's composed of two parts.

1. sysctl node dev.autoattach

dev.autoattach is read/write integer sysctl node which controls
driver-model's behavior regarding device - driver association.

0: autoattach disabled. devices are not associated with drivers
automatically. i.e. insmod'ing e100.ko won't cause it to attach to the actual e100 devices.
1: autoattach enabled. The default value. This is the same as the
current driver model behavior. Driver model automatically associates devices to drivers.
2: rescan command. If this value is written, bus_rescan_devices() is
invoked for all the registered bus types; thus attaching all devices to available drivers. After rescan is complete, the
autoattach value is set to 1.

2. per-device attach and detach sysfs node.

Two files named attach and detach are created under each device's
sysfs directory. Reading attach node shows the name of applicable drivers. Writing a driver name attaches the device to the driver. Also, per-device parameters can be specified when writing to an attach node. Writing anything to the write-only detach node detaches the driver from the currently associated driver.

========= So, for example, on my machine which has two e100's...

# pwd
/sys/bus/pci/devices/0000:00:04.0
# sysctl dev.autoattach
dev.autoattach = 1
# modprobe e100
e100: Intel(R) PRO/100 Network Driver, 3.2.3-k2-NAPI
e100: Copyright(c) 1999-2004 Intel Corporation
e100: eth0: e100_probe: addr 0xfeafe000, irq 20, MAC addr 00:E0:81:01:7F:F3
e100: eth1: e100_probe: addr 0xfeafd000, irq 21, MAC addr 00:E0:81:01:7F:F4
# modprobe eepro100
eepro100.c:v1.09j-t 9/29/99 Donald Becker http://www.scyld.com/network...
eepro100.c: $Revision: 1.36 $ 2000/11/17 Modified by Andrey V. Savochkin...
# ls -l driver
lrwxrwxrwx  1 root root 0 Nov  4 16:26 driver -> ../../../bus/pci/drivers/e100
# rmmod e100
# rmmod eepro100
# sysctl -w dev.autoattach=0
dev.autoattach = 0
# modprobe e100
e100: Intel(R) PRO/100 Network Driver, 3.2.3-k2-NAPI
e100: Copyright(c) 1999-2004 Intel Corporation
# modprobe eepro100
eepro100.c:v1.09j-t 9/29/99 Donald Becker http://www.scyld.com/network...
eepro100.c: $Revision: 1.36 $ 2000/11/17 Modified by Andrey V. Savochkin...
# ls -l driver
ls: driver: No such file or directory
# cat attach
e100
eepro100
# echo eepro100 > attach
eth0: OEM i82557/i82558 10/100 Ethernet, 00:E0:81:01:7F:F3, IRQ 20.
  Receiver lock-up bug exists -- enabling work-around.
  Board assembly 123456-120, Physical connectors present: RJ45
  ...
# ls -l driver
lrwxrwxrwx  1 root root 0 Nov  4 16:27 driver -> ../../../bus/pci/drivers/eepro100
# sysctl -w dev.autoattach=2
e100: eth1: e100_probe: addr 0xfeafd000, irq 21, MAC addr 00:E0:81:01:7F:F4
dev.autoattach = 2

======== And, drivers can accept per-device parameters like the following.

# pwd
/sys/bus/dp/devices/dp_dev1
# ls -l
total 0
-rw-r--r--  1 root root 4096 Nov  4 16:34 attach
--w-------  1 root root 4096 Nov  4 16:34 detach
-rw-r--r--  1 root root 4096 Nov  4 16:34 detach_state
# cat attach
babo
# echo babo n=15 opts=0,9,8 dp_class.integer=12 > attach
dp_bus: 0 1 2 1,2,3,0,0,0 cnt=3
dp_drv: 15 0,9,8,0 cnt=3
dp_class: 12 120 "dp_class"
# ls -l driver
lrwxrwxrwx  1 root root 0 Nov  4 16:35 driver -> ../../bus/dp/drivers/babo
# ls parameters/
a  ar  b  byte  c  charp  integer  n  opts

===========

We'll need to expand user-space hotplug facility to make full use of
manualattach feature. I think with a bit more information exported to userland and some standardized parameters (i.e. `name' parameter for all class devices), we'll be able to give high-granuality control over driver-model to users.

What do you guys think? I think this can be quite useful with right
user-space tools.

Thanks.

--
tejun

author
#1 lore raw

Re: [PATCH 2.6.10-rc1 1/4] driver-model: sysctl node dev.autoattach

ma_01_sysctl_dev_autoattach.patch

This patch implements sysctl node dev.autoattach.

drivers/base/bus.c +61 -2
Index: linux-export/drivers/base/bus.c
===================================================================
@@ -17,6 +17,12 @@
 #include "base.h"
 #include "power/power.h"
 
+int dev_autoattach = 1, dev_autoattach_min = 0, dev_autoattach_max = 2;
+
+static LIST_HEAD(all_buses);
+static spinlock_t all_buses_lock = SPIN_LOCK_UNLOCKED;
+static DECLARE_MUTEX(all_buses_traverse_mutex);
+
 #define to_dev(node) container_of(node, struct device, bus_list)
 #define to_drv(node) container_of(node, struct device_driver, kobj.entry)
 
@@ -329,7 +335,7 @@ int device_attach(struct device * dev)
 		return 1;
 	}
 
-	if (bus->match) {
+	if (dev_autoattach && bus->match) {
 		list_for_each(entry, &bus->drivers.list) {
 			struct device_driver * drv = to_drv(entry);
 			error = driver_probe_device(drv, dev);
@@ -366,7 +372,7 @@ void driver_attach(struct device_driver 
 	struct list_head * entry;
 	int error;
 
-	if (!bus->match)
+	if (!dev_autoattach || !bus->match)
 		return;
 
 	list_for_each(entry, &bus->devices.list) {
@@ -723,6 +729,10 @@ int bus_register(struct bus_type * bus)
 		goto bus_drivers_fail;
 	bus_add_attrs(bus);
 
+	spin_lock(&all_buses_lock);
+	list_add_tail(&bus->node, &all_buses);
+	spin_unlock(&all_buses_lock);
+
 	pr_debug("bus type '%s' registered\n", bus->name);
 	return 0;
 
@@ -745,12 +755,61 @@ out:
 void bus_unregister(struct bus_type * bus)
 {
 	pr_debug("bus %s: unregistering\n", bus->name);
+
+	spin_lock(&all_buses_lock);
+	list_del(&bus->node);
+	spin_unlock(&all_buses_lock);
+
 	bus_remove_attrs(bus);
 	kset_unregister(&bus->drivers);
 	kset_unregister(&bus->devices);
 	subsystem_unregister(&bus->subsys);
 }
 
+
+/**
+ *	dev_autoattach_handler - proc_handler for sysctl node dev.autoattach
+ */
+int dev_autoattach_handler(ctl_table *table, int write, struct file *filp,
+			   void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret;
+
+	ret = proc_dointvec_minmax(table, write, filp, buffer, lenp, ppos);
+
+	down(&all_buses_traverse_mutex);
+
+	if (dev_autoattach == 2) {
+		struct list_head marker;
+
+		spin_lock(&all_buses_lock);
+		list_add(&marker, &all_buses);
+		while (marker.next != &all_buses) {
+			struct bus_type *bus;
+			bus = container_of(marker.next, struct bus_type, node);
+			if (!(bus = get_bus(bus)))
+				continue;
+			/* Okay, we have the next bus, move it ahead
+			   of the marker and perform rescan. */
+			list_del(&bus->node);
+			list_add_tail(&bus->node, &marker);
+			spin_unlock(&all_buses_lock);
+
+			bus_rescan_devices(bus);
+
+			put_bus(bus);
+			spin_lock(&all_buses_lock);
+		}
+		list_del(&marker);
+		spin_unlock(&all_buses_lock);
+		dev_autoattach = 1;
+	}
+
+	up(&all_buses_traverse_mutex);
+
+	return ret;
+}
+
 int __init buses_init(void)
 {
 	return subsystem_register(&bus_subsys);
include/linux/device.h +8 -0
Index: linux-export/include/linux/device.h
===================================================================
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/deviceparam.h>
+#include <linux/sysctl.h>
 #include <asm/semaphore.h>
 #include <asm/atomic.h>
 
@@ -54,6 +55,7 @@ struct bus_type {
 	struct subsystem	subsys;
 	struct kset		drivers;
 	struct kset		devices;
+	struct list_head	node;
 
 	struct bus_attribute	* bus_attrs;
 	struct device_attribute	* dev_attrs;
@@ -414,6 +416,12 @@ extern void device_shutdown(void);
 extern int firmware_register(struct subsystem *);
 extern void firmware_unregister(struct subsystem *);
 
+/* dev.autoattach sysctl node */
+extern int dev_autoattach, dev_autoattach_min, dev_autoattach_max;
+extern int dev_autoattach_handler(ctl_table *table, int write,
+				  struct file *filp, void __user *buffer,
+				  size_t *lenp, loff_t *ppos);
+
 /* debugging and troubleshooting/diagnostic helpers. */
 #define dev_printk(level, dev, format, arg...)	\
 	printk(level "%s %s: " format , (dev)->driver ? (dev)->driver->name : "" , (dev)->bus_id , ## arg)
include/linux/sysctl.h +1 -0
Index: linux-export/include/linux/sysctl.h
===================================================================
@@ -691,6 +691,7 @@ enum {
 	DEV_RAID=4,
 	DEV_MAC_HID=5,
 	DEV_SCSI=6,
+	DEV_AUTOATTACH=7,
 };
 
 /* /proc/sys/dev/cdrom */
kernel/sysctl.c +11 -0
Index: linux-export/kernel/sysctl.c
===================================================================
@@ -41,6 +41,7 @@
 #include <linux/limits.h>
 #include <linux/dcache.h>
 #include <linux/syscalls.h>
+#include <linux/device.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -936,6 +937,16 @@ static ctl_table debug_table[] = {
 };
 
 static ctl_table dev_table[] = {
+	{
+		.ctl_name	= DEV_AUTOATTACH,
+		.procname	= "autoattach",
+		.data		= &dev_autoattach,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &dev_autoattach_handler,
+		.extra1		= &dev_autoattach_min,
+		.extra2		= &dev_autoattach_max,
+	},
 	{ .ctl_name = 0 }
 };  
 
author
#2 lore raw

Re: [PATCH 2.6.10-rc1 3/4] driver-model: detach_state functions renamed

In reply to Tejun Heo

ma_03_detach_state_fn_rename.patch

This patch renames detach_{show,store}() functions which is used for
detach_state device interface node to detach_state_{show,store}().

drivers/base/interface.c +4 -3
Index: linux-export/drivers/base/interface.c
===================================================================
@@ -27,12 +27,13 @@
  *	driver's suspend method.
  */
 
-static ssize_t detach_show(struct device * dev, char * buf)
+static ssize_t detach_state_show(struct device * dev, char * buf)
 {
 	return sprintf(buf, "%u\n", dev->detach_state);
 }
 
-static ssize_t detach_store(struct device * dev, const char * buf, size_t n)
+static ssize_t detach_state_store(struct device * dev,
+				  const char * buf, size_t n)
 {
 	u32 state;
 	state = simple_strtoul(buf, NULL, 10);
@@ -42,7 +43,7 @@ static ssize_t detach_store(struct devic
 	return n;
 }
 
-static DEVICE_ATTR(detach_state, 0644, detach_show, detach_store);
+static DEVICE_ATTR(detach_state, 0644, detach_state_show, detach_state_store);
 
 
 struct attribute * dev_default_attrs[] = {
author
#3 lore raw

Re: [PATCH 2.6.10-rc1 2/4] driver-model: devparam expanded to accept direct per-device parameters via @args argument

In reply to Tejun Heo

ma_02_devparam_set_by_args.patch

This patch implements set_devparams_by_args() and expands related
functions to accept an const char * @args argument. This enables setting per-device parameters directly from argument string rather than from parameters stored while booting or loading modules.

drivers/base/bus.c +5 -4
Index: linux-export/drivers/base/bus.c
===================================================================
@@ -287,7 +287,8 @@ void device_bind_driver(struct device * 
  *	If we find a match, we call @drv->probe(@dev) if it exists, and
  *	call device_bind_driver() above.
  */
-int driver_probe_device(struct device_driver * drv, struct device * dev)
+int driver_probe_device(struct device_driver * drv, struct device * dev,
+			const char *args)
 {
 	int error;
 
@@ -296,7 +297,7 @@ int driver_probe_device(struct device_dr
 
 	dev->driver = drv;
 
-	if ((error = devparam_set_params(dev)) != 0)
+	if ((error = devparam_set_params(dev, args)) != 0)
 		goto devparam_fail;
 
 	if (drv->probe) {
@@ -338,7 +339,7 @@ int device_attach(struct device * dev)
 	if (dev_autoattach && bus->match) {
 		list_for_each(entry, &bus->drivers.list) {
 			struct device_driver * drv = to_drv(entry);
-			error = driver_probe_device(drv, dev);
+			error = driver_probe_device(drv, dev, NULL);
 			if (!error)
 				/* success, driver matched */
 				return 1;
@@ -378,7 +379,7 @@ void driver_attach(struct device_driver 
 	list_for_each(entry, &bus->devices.list) {
 		struct device * dev = container_of(entry, struct device, bus_list);
 		if (!dev->driver) {
-			error = driver_probe_device(drv, dev);
+			error = driver_probe_device(drv, dev, NULL);
 			if (error && (error != -ENODEV))
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING
drivers/base/deviceparam.c +98 -6

138 lines not on this page · raw message

include/linux/device.h +2 -1
Index: linux-export/include/linux/device.h
===================================================================
@@ -342,7 +342,8 @@ extern int device_for_each_child(struct 
  * Manual binding of a device to driver. See drivers/base/bus.c
  * for information on use.
  */
-extern int  driver_probe_device(struct device_driver * drv, struct device * dev);
+extern int  driver_probe_device(struct device_driver * drv, struct device * dev,
+				const char *args);
 extern void device_bind_driver(struct device * dev);
 extern void device_release_driver(struct device * dev);
 extern int  device_attach(struct device * dev);
include/linux/deviceparam.h +1 -1
Index: linux-export/include/linux/deviceparam.h
===================================================================
@@ -220,7 +220,7 @@ int devparam_module_init(struct module *
 int devparam_unknown_modparam(char *name, char *val, void *arg);
 void devparam_module_done(struct module *module);
 
-int devparam_set_params(struct device *dev);
+int devparam_set_params(struct device *dev, const char *args);
 void devparam_release_params(struct device *dev);
 
 #endif /*__LINUX_DEVICEPARAM_H*/
author
#4 lore raw

Re: [PATCH 2.6.10-rc1 4/4] driver-model: attach/detach sysfs node implemented

In reply to Tejun Heo

ma_04_manual_attach.patch

This patch implements device interface nodes attach and detach.
Reading attach node shows the name of applicable drivers. Writing a driver name attaches the device to the driver. Writing anything to the write-only detach node detaches the driver from the currently associated driver.

drivers/base/interface.c +107 -0
Index: linux-export/drivers/base/interface.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/err.h>
 #include <linux/stat.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 
 /**
  *	detach_state - control the default power state for the device.
@@ -46,7 +47,113 @@ static ssize_t detach_state_store(struct
 static DEVICE_ATTR(detach_state, 0644, detach_state_show, detach_state_store);
 
 
+/**
+ *	attach - manually attaches the device to the specified driver
+ *
+ *	When read, this node shows the list of the attachable drivers.
+ *	Writing the name of a driver attaches the device to the
+ *	driver.
+ */
+
+struct attach_show_arg {
+	struct device * dev;
+	char * buf;
+	size_t left;
+};
+
+static int attach_show_helper(struct device_driver * drv, void * void_arg)
+{
+	struct attach_show_arg * arg = void_arg;
+	int ret;
+
+	if (drv->bus->match(arg->dev, drv)) {
+		ret = snprintf(arg->buf, arg->left, "%s\n", drv->name);
+		if (ret >= arg->left)
+			return -ENOSPC;
+		arg->buf += ret;
+		arg->left -= ret;
+	}
+
+	return 0;
+}
+
+static ssize_t attach_show(struct device * dev, char * buf)
+{
+	struct attach_show_arg arg = { dev, buf, PAGE_SIZE };
+	int ret = 0;
+
+	if (dev->bus->match)
+		ret = bus_for_each_drv(dev->bus, NULL, &arg, attach_show_helper);
+
+	return ret ?: PAGE_SIZE - arg.left;
+}
+
+static int attach_store_helper(struct device_driver * drv, void * arg)
+{
+	const char * p = *(void **)arg;
+	int len;
+
+	len = strlen(drv->name);
+	if (!strncmp(drv->name, p, len) &&
+	    (p[len] == '\0' || isspace(p[len]))) {
+		*(void **)(arg) = get_driver(drv);
+		return 1;
+	}
+
+	return 0;
+}
+
+static ssize_t attach_store(struct device * dev, const char * buf, size_t n)
+{
+	void * arg = (void *)buf;
+	struct device_driver *drv;
+	int error;
+
+	if (bus_for_each_drv(dev->bus, NULL, &arg, attach_store_helper) == 0)
+		return -ENOENT;
+	drv = arg;
+
+	/* Skip driver name */
+	while (*buf != '\0' && !isspace(*buf))
+		buf++;
+
+	/* Attach */
+	error = -EBUSY;
+	down_write(&dev->bus->subsys.rwsem);
+	if (dev->driver == NULL)
+		error = driver_probe_device(drv, dev, buf);
+	up_write(&dev->bus->subsys.rwsem);
+
+	if (error)
+		printk(KERN_WARNING "%s: probe of %s failed with error %d\n",
+		       drv->name, dev->bus_id, error);
+
+	return error ?: n;
+}
+
+static DEVICE_ATTR(attach, 0644, attach_show, attach_store);
+
+
+/**
+ *	detach - manually detaches the device from its associated driver.
+ *
+ *	This is a write-only node.  When any value is written, it detaches
+ *	the device from its associated driver.
+ */
+static ssize_t detach_store(struct device * dev, const char * buf, size_t n)
+{
+	down_write(&dev->bus->subsys.rwsem);
+	device_release_driver(dev);
+	up_write(&dev->bus->subsys.rwsem);
+	return n;
+}
+
+static DEVICE_ATTR(detach, 0200, NULL, detach_store);
+
+
 struct attribute * dev_default_attrs[] = {
 	&dev_attr_detach_state.attr,
+	&dev_attr_attach.attr,
+	&dev_attr_detach.attr,
 	NULL,
 };
#5 lore raw

Re: [PATCH 2.6.10-rc1 4/4] driver-model: attach/detach sysfs node implemented

Quoted 8 lines · Tejun Heo

 ma_04_manual_attach.patch

 This patch implements device interface nodes attach and detach. Reading attach node shows the name of applicable drivers.  Writing a driver name attaches the device to the driver.  Writing anything to the write-only detach node detaches the driver from the currently associated driver.

...

Quoted 8 lines
drivers/base/interface.c +7 -0
+/**
+ *   detach - manually detaches the device from its associated driver.
+ *
+ *   This is a write-only node.  When any value is written, it detaches
+ *   the device from its associated driver.
+ */
+static ssize_t detach_store(struct device * dev, const char * buf, size_t

n)

drivers/base/interface.c static ssize_t detach_state_store(struct · line 149

Quoted 6 lines
drivers/base/interface.c +6 -0
+{
+     down_write(&dev->bus->subsys.rwsem);
+     device_release_driver(dev);
+     up_write(&dev->bus->subsys.rwsem);
+     return n;
+}

This will not work for pretty much any bus but PCI because only PCI allows to detach a driver leaving children devices on the bus. The rest of buses remove children devices when disconnecting parent.

Also, there usually much more going on with regard to locking and other bus-specific actions besides taking bus's rwsem when binding devices. Serio bus will definitely get upset if you try to disconnect even a leaf device in the manner presented above and I think USB will get upset as well.

I have tried the naïve approach as well but in the end we need bus -specific helper to do manual connect/disconnect. Please take a look at these:

http://marc.theaimsgroup.com/?l=linux-kernel&m=109908274124446&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=109912528510337&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=109912553831130&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=109912553827412&w=2

Signature · 1 line
Dmitry
#6 lore raw

Re: [PATCH 2.6.10-rc1 4/4] driver-model: attach/detach sysfs node implemented

Quoted 20 lines · Dmitry Torokhov
Quoted 8 lines · Tejun Heo

?ma_04_manual_attach.patch

?This patch implements device interface nodes attach and detach. Reading attach node shows the name of applicable drivers. ?Writing a driver name attaches the device to the driver. ?Writing anything to the write-only detach node detaches the driver from the currently associated driver.

...

Quoted 10 lines
(quoted diff) +7 -0
+/**
+ *???detach - manually detaches the device from its associated driver.
+ *
+ *???This is a write-only node. ?When any value is written, it detaches
+ *???the device from its associated driver.
+ */
+static ssize_t detach_store(struct device * dev, const char * buf, size_t

n)
+{
+?????down_write(&dev->bus->subsys.rwsem);

Quoted 8 lines
Quoted 4 lines

+?????device_release_driver(dev);
+?????up_write(&dev->bus->subsys.rwsem);
+?????return n;
+}

This will not work for pretty much any bus but PCI because only PCI allows to detach a driver leaving children devices on the bus. The rest of buses remove children devices when disconnecting parent.

Yeah, I was glad you stepped in. Both of you are trying to work on the same problem, in different ways. It would be great if you both could work out a common method together.

Quoted 5 lines

Also, there usually much more going on with regard to locking and other bus-specific actions besides taking bus's rwsem when binding devices. Serio bus will definitely get upset if you try to disconnect even a leaf device in the manner presented above and I think USB will get upset as well.

No, we can disconnect a driver from a device just fine for USB with no problems (as long as it's not the hub driver from a hub device, we need to never be able to disconnect those.)

thanks,

greg k-h

#7 lore raw
In reply to Tejun Heo

hoi :)

Quoted 6 lines · Tejun Heo

Two files named attach and detach are created under each device's
sysfs directory. Reading attach node shows the name of applicable drivers. Writing a driver name attaches the device to the driver. Also, per-device parameters can be specified when writing to an attach node. Writing anything to the write-only detach node detaches the driver from the currently associated driver.

perhaps it'll be simpler with only the attach file and using a special magic value ("", "none", "detach", whatever) to manually detach a device from the driver.

Is it possible (and worthwhile) to reattach a manually detached device to the default driver? Perhaps using a magic value "auto" for attach. Something like your dev.autoattach=2 rescan method, but for one device only. (What is the use case to rescan all busses, anyway?)

Signature · 1 line
Martin Waitz
#8 lore raw
In reply to Tejun Heo
Quoted 10 lines · Tejun Heo

Hello, again. :-)

These are the manual device attach patches I was talking about in the
previous posting. These patches need devparam patches to be applied first. It's composed of two parts.

1. sysctl node dev.autoattach

dev.autoattach is read/write integer sysctl node which controls
driver-model's behavior regarding device - driver association.

Ick, no new sysctls please. Make this a per-bus attribute that gets written to in sysfs. Much nicer and much finer control then.

Quoted 10 lines

0: autoattach disabled. devices are not associated with drivers
automatically. i.e. insmod'ing e100.ko won't cause it to attach to the actual e100 devices.
1: autoattach enabled. The default value. This is the same as the
current driver model behavior. Driver model automatically associates devices to drivers.
2: rescan command. If this value is written, bus_rescan_devices() is
invoked for all the registered bus types; thus attaching all devices to available drivers. After rescan is complete, the
autoattach value is set to 1.

Make this a different sysfs file. "rescan" would be good.

Look at how pci can handle adding new devices to their drivers from sysfs. If we can move that kind of functionality to the driver core, so that all busses get it (it will require a new per-bus callback though, se the other patches recently posted to lkml for an example of this), that would be what I would like to see happen.

Quoted 5 lines

2. per-device attach and detach sysfs node.

Two files named attach and detach are created under each device's
sysfs directory. Reading attach node shows the name of applicable drivers.

How does a device know what drivers could be bound to it? It's the other way around, drivers know what kind of devices they can bind to. Let's add the ability to add more devices to a driver through sysfs, again, like PCI does.

Quoted 1 line

Writing a driver name attaches the device to the driver.

No, do it the other way, attach a driver to a device.

thanks,

greg k-h

author
#9 lore raw

Hello, Greg,

Quoted 8 lines · Greg KH

How does a device know what drivers could be bound to it? It's the other way around, drivers know what kind of devices they can bind to. Let's add the ability to add more devices to a driver through sysfs, again, like PCI does.

Quoted 1 line

Writing a driver name attaches the device to the driver.

No, do it the other way, attach a driver to a device.

Well, I just happen to think that devices attach to drivers as in
"attaching pci device 0000:00:08.0 to e1000 driver". Maybe I'm just weird. :-P

Signature · 1 line
tejun
#10 lore raw
In reply to Greg KH
Quoted 7 lines · Greg KH
Quoted 6 lines · Tejun Heo

Hello, again. :-)

These are the manual device attach patches I was talking about in the
previous posting. These patches need devparam patches to be applied first. It's composed of two parts.

Quoted 8 lines
Quoted 4 lines

1. sysctl node dev.autoattach

dev.autoattach is read/write integer sysctl node which controls
driver-model's behavior regarding device - driver association.

Ick, no new sysctls please. Make this a per-bus attribute that gets written to in sysfs. Much nicer and much finer control then.

I think that my bind)mode patches which allow to control binding through sysfs on pre-device and per-driver base should suffice here. I really doubt that anybody would want to keep autoattach disabled and do all matching manually ;). Besides having per-driver attribute allows drivers authors control binding.

Quoted 11 lines
Quoted 10 lines

0: autoattach disabled. devices are not associated with drivers
automatically. i.e. insmod'ing e100.ko won't cause it to attach to the actual e100 devices.
1: autoattach enabled. The default value. This is the same as the
current driver model behavior. Driver model automatically associates devices to drivers.
2: rescan command. If this value is written, bus_rescan_devices() is
invoked for all the registered bus types; thus attaching all devices to available drivers. After rescan is complete, the
autoattach value is set to 1.

Quoted 14 lines

Make this a different sysfs file. "rescan" would be good.

Look at how pci can handle adding new devices to their drivers from sysfs. If we can move that kind of functionality to the driver core, so that all busses get it (it will require a new per-bus callback though, se the other patches recently posted to lkml for an example of this), that would be what I would like to see happen.

Quoted 5 lines

2. per-device attach and detach sysfs node.

Two files named attach and detach are created under each device's
sysfs directory. Reading attach node shows the name of applicable drivers.

Do we really need 2 or even 3 files ("attach", "detach" and "rescan")? Given that you really can't (at least not yet) do all there operations for all buses from the core that woudl require 3 per-bus callbacks. I think reserving special values such as "none" or "detach" and "rescan" shoudl work just fine and also willallow extending supported operations on per-bus basis. For example serio bus supports "reconnect" option which tries to re-initialize device if something happened to it. It does not want to do rescan as that would generate new input devices while it is much more convenient to re-use old ones.

Quoted 2 lines

How does a device know what drivers could be bound to it? It's the other way around, drivers know what kind of devices they can bind to.

But when 2+ drivers can be bound to a device then particular _device_ gets to decide which driver is best suited for it, like in cases of e100/eepro100 or psmouse/serio_raw.

Quoted 3 lines

Let's add the ability to add more devices to a driver through sysfs, again, like PCI does.

Well, PCI does add a new ID to a driver allowing it to bind to a whole new set of devices. I agree that this is a "driver" operation.

Quoted 4 lines
Quoted 1 line

Writing a driver name attaches the device to the driver.

No, do it the other way, attach a driver to a device.

I disagree. Here you working with particular device. You are not saying "from now on I want e100 to bind all my 5 new network cards that happen to have id XXXX:YYYY". Instead you are saying "I want to bind e100 driver to this card residing at /sys/bus/pci/0000.....". In other word it is operation on particular device and should be done by manipulating device attribute.

Signature · 1 line
Dmitry
author
#11 lore raw
Quoted 5 lines · Dmitry Torokhov

I think that my bind)mode patches which allow to control binding through sysfs on pre-device and per-driver base should suffice here. I really doubt that anybody would want to keep autoattach disabled and do all matching manually ;). Besides having per-driver attribute allows drivers authors control binding.

Think about extending hotplug to cover all device bindings. It will
kick in very early in the booting process (maybe in the initrd image) and names/binds every device on the device with appropriate arguments as user requested. I was thinking about usages like that when I was making the sysctl node. Maybe I was going too far. :-)

Quoted 9 lines

Do we really need 2 or even 3 files ("attach", "detach" and "rescan")? Given that you really can't (at least not yet) do all there operations for all buses from the core that woudl require 3 per-bus callbacks. I think reserving special values such as "none" or "detach" and "rescan" shoudl work just fine and also willallow extending supported operations on per-bus basis. For example serio bus supports "reconnect" option which tries to re-initialize device if something happened to it. It does not want to do rescan as that would generate new input devices while it is much more convenient to re-use old ones.

How about making the command format "CMD ARGS" rather than
"{CMD|DRIVERNAME}" i.e.

not

 # echo e100 > drvctl
 # echo detach > drvctl

but

 # echo attach e100 > drvctl
 # echo detach > drvctl

But, I don't know. It now just seems too much like a proc node.

Quoted 6 lines

I disagree. Here you working with particular device. You are not saying "from now on I want e100 to bind all my 5 new network cards that happen to have id XXXX:YYYY". Instead you are saying "I want to bind e100 driver to this card residing at /sys/bus/pci/0000.....". In other word it is operation on particular device and should be done by manipulating device attribute.

I kind of agree with you but I think either way is fine.

Thanks.

Signature · 1 line
tejun
maintainer
#12 lore raw
Quoted 20 lines · Tejun Heo
Quoted 9 lines · Dmitry Torokhov

Do we really need 2 or even 3 files ("attach", "detach" and "rescan")? Given that you really can't (at least not yet) do all there operations for all buses from the core that woudl require 3 per-bus callbacks. I think reserving special values such as "none" or "detach" and "rescan" shoudl work just fine and also willallow extending supported operations on per-bus basis. For example serio bus supports "reconnect" option which tries to re-initialize device if something happened to it. It does not want to do rescan as that would generate new input devices while it is much more convenient to re-use old ones.

How about making the command format "CMD ARGS" rather than "{CMD|DRIVERNAME}" i.e.

not

# echo e100 > drvctl
# echo detach > drvctl
Quoted 7 lines

but

# echo attach e100 > drvctl
# echo detach > drvctl

But, I don't know. It now just seems too much like a proc node.

Well, I was lazy and did not want to do any parsing at all, but I do not have anything against "CMD ARG ARG ARG" form, especially if integrate drvparm.

Signature · 1 line
Dmitry
#13 lore raw
In reply to Tejun Heo
Quoted 10 lines · Tejun Heo
Quoted 9 lines

Do we really need 2 or even 3 files ("attach", "detach" and "rescan")? Given that you really can't (at least not yet) do all there operations for all buses from the core that woudl require 3 per-bus callbacks. I think reserving special values such as "none" or "detach" and "rescan" shoudl work just fine and also willallow extending supported operations on per-bus basis. For example serio bus supports "reconnect" option which tries to re-initialize device if something happened to it. It does not want to do rescan as that would generate new input devices while it is much more convenient to re-use old ones.

Quoted 3 lines

 How about making the command format "CMD ARGS" rather than "{CMD|DRIVERNAME}"  i.e.

Hi guys,

What do you think about following set of patches. It adds drvctl default device attribute (write only) that controls device/driver binding. It expects commands in form of "CMD [DRIVER_NAME] [ARG ARG...]" so I think it will be very easy to adapt it to Tejun's per-device parameters.

I adjusted serio bus to the new form of commands so now it accepts:
"detach", "attach <driver>", "rescan", "reconnect"

And I added PCI bus drvctl that understands "detach", "attach <driver>", and "rescan".

As we add drvctl methods to the rest of the buses we can review what can be pulled into the core and what has to stay bus-specific.

Plus there is the bind_mode patch that allows switching between automatic and manual binding on per-device/per-driver base.

Signature · 1 line
Dmitry
#14 lore raw

PATCH 1/3 Add drvctl default device attribute

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

[email protected], 2004-11-08 02:03:59-05:00, [email protected]
Driver core: add "drvctl" default device attribute that allows
userspace request execution of bus-specific actions for devices. Used to manually control driver and device binding/unbinding/rebinding, causes execution of bus->drvctl() helper. Expects commands in form of "CMD [DRIVER NAME] [ARG ARG...]]".

Serio bus's drvctl rearranged to accept the following
commands: "detach", "attach <driver>", "rescan", and "reconnect".

 drivers/base/interface.c    |   74 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/input/serio/serio.c |   61 +++++++++++++++++-------------------
 include/linux/device.h      |    4 +-
 3 files changed, 106 insertions(+), 33 deletions(-)

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

drivers/base/interface.c +73 -1
diff -Nru a/drivers/base/interface.c b/drivers/base/interface.c
@@ -13,6 +13,7 @@
 #include <linux/err.h>
 #include <linux/stat.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 
 /**
  *	detach_state - control the default power state for the device.
@@ -42,10 +43,81 @@
 	return n;
 }
 
-static DEVICE_ATTR(detach_state, 0644, detach_show, detach_store);
+/**
+ *	drvctl - controls device and driver binding.
+ *
+ *	Writing to the attribute causes execution of bus-specific drvctl()
+ *	helper that is used to disconnect device from its driver or rebind
+ *	device to a specific driver.
+ *	Commands are expected in the following format:
+ *	CMD [DRIVER_NAME] [ARG ARG ARG]
+ *	drvctl handler is free to mangle the args string.
+ */
+
+#define GET_WORD(x)					\
+do {							\
+	while (isspace(*args)) args++;			\
+	(x) = args;					\
+	while (*args && !isspace(*args)) args++;	\
+	if (isspace(*args)) {				\
+		save_chr = *args;			\
+		*args++ = '\0';				\
+	}						\
+} while (0)						\
 
+static ssize_t drvctl_store(struct device * dev, const char * buf, size_t count)
+{
+	int retval = -ENOSYS;
+	struct device_driver *drv = NULL;
+	char *str, *action, *drv_name, *args;
+	char save_chr = 0;
+
+	if (!dev->bus->drvctl)
+		return -ENOSYS;
+
+	/* copy buffer so we can mangle it */
+	if (!(args = str = kmalloc(count + 1, GFP_KERNEL)))
+		return -ENOMEM;
+
+	memcpy(str, buf, count);
+	str[count] = '\0';
+
+	GET_WORD(action);
+	GET_WORD(drv_name);
+
+	if (*drv_name) {
+		if (!(drv = driver_find(drv_name, dev->bus))) {
+			/*
+			 * if this is not a name of existing driver
+			 * merge it with thye rest of the string and
+			 * pass to drvctl as 'args'
+			 */
+			if (*args)
+				*--args = save_chr;
+			args = drv_name;
+		}
+	}
+
+	while (*args == ' ') args++;
+
+	pr_debug("drvctl -  action: '%s', driver: '%s', args: '%s'\n",
+		 action, drv ? drv->name : "", args);
+
+	retval = dev->bus->drvctl(dev, action, drv, args);
+
+	if (drv)
+		put_driver(drv);
+	kfree(str);
+
+	return (retval < 0) ? retval : count;
+}
+#undef GET_WORD
+
+static DEVICE_ATTR(detach_state, 0644, detach_show, detach_store);
+static DEVICE_ATTR(drvctl, 0200, NULL, drvctl_store);
 
 struct attribute * dev_default_attrs[] = {
 	&dev_attr_detach_state.attr,
+	&dev_attr_drvctl.attr,
 	NULL,
 };
drivers/input/serio/serio.c +30 -31

85 lines not on this page · raw message

include/linux/device.h +3 -1
diff -Nru a/include/linux/device.h b/include/linux/device.h
@@ -59,7 +59,9 @@
 	struct driver_attribute	* drv_attrs;
 
 	int		(*match)(struct device * dev, struct device_driver * drv);
-	int		(*hotplug) (struct device *dev, char **envp, 
+	int		(*drvctl)(struct device * dev, const char * action,
+				  struct device_driver * drv, char * args);
+	int		(*hotplug) (struct device *dev, char **envp,
 				    int num_envp, char *buffer, int buffer_size);
 	int		(*suspend)(struct device * dev, u32 state);
 	int		(*resume)(struct device * dev);
#15 lore raw

PATCH 2/3 Add drvctl handler to PCI bus

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

[email protected], 2004-11-08 02:06:19-05:00, [email protected]
PCI: Add devctl method to PCI bus. The following commands are
available: "detach", "attach <driver>", and "rescan".

 pci-driver.c |   58 ++++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 44 insertions(+), 14 deletions(-)

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

drivers/pci/pci-driver.c +44 -14
diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
@@ -186,7 +186,7 @@
  *                    PCI device id structure
  * @ids: array of PCI device id structures to search in
  * @dev: the PCI device structure to match against
- * 
+ *
  * Used by a driver to check whether a PCI device present in the
  * system is in its list of supported devices.Returns the matching
  * pci_device_id structure or %NULL if there is no match.
@@ -204,12 +204,12 @@
 
 /**
  * pci_device_probe_static()
- * 
+ *
  * returns 0 and sets pci_dev->driver when drv claims pci_dev, else error.
  */
 static int
 pci_device_probe_static(struct pci_driver *drv, struct pci_dev *pci_dev)
-{		   
+{
 	int error = -ENODEV;
 	const struct pci_device_id *id;
 
@@ -227,13 +227,13 @@
 
 /**
  * __pci_device_probe()
- * 
+ *
  * returns 0  on success, else error.
  * side-effect: pci_dev->driver is set to drv when drv claims pci_dev.
  */
 static int
 __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
-{		   
+{
 	int error = 0;
 
 	if (!pci_dev->driver && drv->probe) {
@@ -314,7 +314,7 @@
 }
 
 
-/* 
+/*
  * Default resume method for devices that have no driver provided resume,
  * or not even a driver at all.
  */
@@ -394,10 +394,10 @@
 /**
  * pci_register_driver - register a new pci driver
  * @drv: the driver structure to register
- * 
+ *
  * Adds the driver structure to the list of registered drivers.
- * Returns a negative value on error, otherwise 0. 
- * If no error occured, the driver remains registered even if 
+ * Returns a negative value on error, otherwise 0.
+ * If no error occured, the driver remains registered even if
  * no device was claimed during registration.
  */
 int pci_register_driver(struct pci_driver *drv)
@@ -425,7 +425,7 @@
 /**
  * pci_unregister_driver - unregister a pci driver
  * @drv: the driver structure to unregister
- * 
+ *
  * Deletes the driver structure from the list of registered PCI drivers,
  * gives it a chance to clean up by calling its remove() function for
  * each device it was responsible for, and marks those devices as
@@ -447,7 +447,7 @@
  * pci_dev_driver - get the pci_driver of a device
  * @dev: the device to query
  *
- * Returns the appropriate pci_driver structure or %NULL if there is no 
+ * Returns the appropriate pci_driver structure or %NULL if there is no
  * registered driver for the device.
  */
 struct pci_driver *
@@ -457,7 +457,7 @@
 		return dev->driver;
 	else {
 		int i;
-		for(i=0; i<=PCI_ROM_RESOURCE; i++)
+		for(i = 0; i <= PCI_ROM_RESOURCE; i++)
 			if (dev->resource[i].flags & IORESOURCE_BUSY)
 				return &pci_compat_driver;
 	}
@@ -468,12 +468,12 @@
  * pci_bus_match - Tell if a PCI device structure has a matching PCI device id structure
  * @ids: array of PCI device id structures to search in
  * @dev: the PCI device structure to match against
- * 
+ *
  * Used by a driver to check whether a PCI device present in the
  * system is in its list of supported devices.Returns the matching
  * pci_device_id structure or %NULL if there is no match.
  */
-static int pci_bus_match(struct device * dev, struct device_driver * drv) 
+static int pci_bus_match(struct device * dev, struct device_driver * drv)
 {
 	const struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * pci_drv = to_pci_driver(drv);
@@ -490,6 +490,35 @@
 	return pci_bus_match_dynids(pci_dev, pci_drv);
 }
 
+/*
+ * This is PCI bus's drvctl method that handles manual device binding.
+ */
+static int pci_rebind_driver(struct device *dev, const char *action,
+			     struct device_driver *drv, char *args)
+{
+	int retval = 0;
+
+	if (!strcmp(action, "detach")) {
+		down_write(&dev->bus->subsys.rwsem);
+		device_release_driver(dev);
+		up_write(&dev->bus->subsys.rwsem);
+	} else if (!strcmp(action, "rescan")) {
+		down_write(&dev->bus->subsys.rwsem);
+		device_release_driver(dev);
+		device_attach(dev);
+		up_write(&dev->bus->subsys.rwsem);
+	} else if (!strcmp(action, "attach") && drv) {
+		down_write(&dev->bus->subsys.rwsem);
+		device_release_driver(dev);
+		driver_probe_device(drv, dev);
+		up_write(&dev->bus->subsys.rwsem);
+	} else {
+		retval = -EINVAL;
+	}
+
+	return retval;
+}
+
 /**
  * pci_dev_get - increments the reference count of the pci device structure
  * @dev: the device being referenced
@@ -534,6 +563,7 @@
 	.name		= "pci",
 	.match		= pci_bus_match,
 	.hotplug	= pci_hotplug,
+	.drvctl		= pci_rebind_driver,
 	.suspend	= pci_device_suspend,
 	.resume		= pci_device_resume,
 	.dev_attrs	= pci_dev_attrs,
#16 lore raw

PATCH 3/3 Add bind_mode default device/driver attributes

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

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

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

 drivers/base/bus.c              |   20 ++++++----
 drivers/base/interface.c        |   79 ++++++++++++++++++++++++++++++++++++----
 drivers/input/serio/serio.c     |   59 ++---------------------------
 drivers/input/serio/serio_raw.c |    4 +-
 include/linux/device.h          |    5 ++
 include/linux/serio.h           |    4 --
 6 files changed, 97 insertions(+), 74 deletions(-)

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

drivers/base/bus.c +13 -7
diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
@@ -68,9 +68,12 @@
 	up(&drv->unload_sem);
 }
 
+extern struct attribute * drv_default_attrs[];
+
 static struct kobj_type ktype_driver = {
 	.sysfs_ops	= &driver_sysfs_ops,
 	.release	= driver_release,
+	.default_attrs	= drv_default_attrs,
 };
 
 
@@ -319,9 +322,12 @@
 		return 1;
 	}
 
-	if (bus->match) {
-		list_for_each(entry, &bus->drivers.list) {
-			struct device_driver * drv = to_drv(entry);
+	if (dev->manual_bind || !bus->match)
+		return 0;
+
+	list_for_each(entry, &bus->drivers.list) {
+		struct device_driver * drv = to_drv(entry);
+		if (!drv->manual_bind) {
 			error = driver_probe_device(drv, dev);
 			if (!error)
 				/* success, driver matched */
@@ -329,8 +335,8 @@
 			if (error != -ENODEV)
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING
-				    "%s: probe of %s failed with error %d\n",
-				    drv->name, dev->bus_id, error);
+					"%s: probe of %s failed with error %d\n",
+					drv->name, dev->bus_id, error);
 		}
 	}
 
@@ -356,12 +362,12 @@
 	struct list_head * entry;
 	int error;
 
-	if (!bus->match)
+	if (drv->manual_bind || !bus->match)
 		return;
 
 	list_for_each(entry, &bus->devices.list) {
 		struct device * dev = container_of(entry, struct device, bus_list);
-		if (!dev->driver) {
+		if (!dev->driver && !dev->manual_bind) {
 			error = driver_probe_device(drv, dev);
 			if (error && (error != -ENODEV))
 				/* driver matched but the probe failed */
drivers/base/interface.c +73 -6

115 lines not on this page · raw message

drivers/input/serio/serio.c +4 -55

99 lines not on this page · raw message

drivers/input/serio/serio_raw.c +2 -2
diff -Nru a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
@@ -365,14 +365,14 @@
 
 static struct serio_driver serio_raw_drv = {
 	.driver		= {
-		.name	= "serio_raw",
+		.name		= "serio_raw",
+		.manual_bind	= 1,
 	},
 	.description	= DRIVER_DESC,
 	.interrupt	= serio_raw_interrupt,
 	.connect	= serio_raw_connect,
 	.reconnect	= serio_raw_reconnect,
 	.disconnect	= serio_raw_disconnect,
-	.manual_bind	= 1,
 };
 
 int __init serio_raw_init(void)
include/linux/device.h +5 -0
diff -Nru a/include/linux/device.h b/include/linux/device.h
@@ -104,6 +104,8 @@
 	char			* name;
 	struct bus_type		* bus;
 
+	unsigned int		manual_bind;
+
 	struct semaphore	unload_sem;
 	struct kobject		kobj;
 	struct list_head	devices;
@@ -266,6 +268,9 @@
 	struct bus_type	* bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	unsigned int manual_bind;	/* indicates whether the core will
+					   try to find a driver for the
+					   device automatically */
 	void		*driver_data;	/* data private to the driver */
 	void		*platform_data;	/* Platform specific data (e.g. ACPI,
 					   BIOS data relevant to device) */
include/linux/serio.h +0 -4
diff -Nru a/include/linux/serio.h b/include/linux/serio.h
@@ -27,8 +27,6 @@
 	char name[32];
 	char phys[32];
 
-	unsigned int manual_bind;
-
 	unsigned short idbus;
 	unsigned short idvendor;
 	unsigned short idproduct;
@@ -59,8 +57,6 @@
 struct serio_driver {
 	void *private;
 	char *description;
-
-	unsigned int manual_bind;
 
 	void (*write_wakeup)(struct serio *);
 	irqreturn_t (*interrupt)(struct serio *, unsigned char,