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

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

[Dmitry Torokhov]

> Greg KH wrote:
> On Thu, Nov 04, 2004 at 12:05:31PM -0500, Dmitry Torokhov wrote:
> > 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

What about connecting? I am pretty ignorant of USB inner workings
but when I took a glance there seems to be a lot of preparations
before device is ready to be probed...

> problems (as long as it's not the hub driver from a hub device, we need
> to never be able to disconnect those.)

Never say never ;) That was the first thing I did after playing with
PCI devices when I tried doing what Tejun did.

If kernel advertises an userspace interface it will be used. I can see
myself wanting to disconnect my hub and all its devices so my wireless
explorer does not use batteries and I do not want to figure out what
port it is connected to... Someone else will find another reason,
I don't know.

I also think that even PCI should kill children devices behind a bridge
if bridge driver is disconnected to manage resources in more strict way.
But I think that would require notion of generic/specialized driver and
require automatic rebinding of specialized driver over generic one so
every PCI device has a driver attached to it.

-- 
Dmitry

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

[Tejun Heo]

 Hello Dmitry.  Hello Greg.

On Thu, Nov 04, 2004 at 11:06:19AM -0800, Dmitry Torokhov wrote:
> What about connecting? I am pretty ignorant of USB inner workings
> but when I took a glance there seems to be a lot of preparations
> before device is ready to be probed...

 IMHO, it would be better to coerce whatever bus to follow common
driver-model synchronization/attach/detach rules and be able to do
straight-forward implementation of features in the core driver-model.
If the current driver-model isn't enough, the core code should be
expanded rather than doing bus-specific dances in individual buses.
But I don't really know about any bus other than PCI, so maybe I'm
being too naive.

> > problems (as long as it's not the hub driver from a hub device, we need
> > to never be able to disconnect those.)
> 
> Never say never ;) That was the first thing I did after playing with
> PCI devices when I tried doing what Tejun did.
> 
> If kernel advertises an userspace interface it will be used. I can see
> myself wanting to disconnect my hub and all its devices so my wireless
> explorer does not use batteries and I do not want to figure out what
> port it is connected to... Someone else will find another reason,
> I don't know.
> 
> I also think that even PCI should kill children devices behind a bridge
> if bridge driver is disconnected to manage resources in more strict way.
> But I think that would require notion of generic/specialized driver and
> require automatic rebinding of specialized driver over generic one so
> every PCI device has a driver attached to it.

 I think above can be cleanly solved by enforcing that no device can
be attached to a driver unless all its ancestors are attached to a
driver.  The check can be made easiliy inside the driver-model, and,
if needed, making dummy drivers for internal node devices which
orignally didn't need one shouldn't be difficult.  We can just return
-EBUSY for any attempts to detach an internal device which has
driver-attached children.  This way, recursing and all other chores
can be dumped to user-space where they belong.

 And regarding the duplicate works, my work on manual-attach was
primarily to show how dynamic device-driver binding can work with
devparam; also, Dmitry seems to understand the problem better than me.
So, I think I should back off on manualattach.  Dmitry, what do you
think about integrating devparam with your work?

 Thanks.

-- 
tejun

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

[Dmitry Torokhov]

On Thursday 04 November 2004 11:45 pm, Tejun Heo wrote:
>  Hello Dmitry.  Hello Greg.
> 
> On Thu, Nov 04, 2004 at 11:06:19AM -0800, Dmitry Torokhov wrote:
> > What about connecting? I am pretty ignorant of USB inner workings
> > but when I took a glance there seems to be a lot of preparations
> > before device is ready to be probed...
> 
>  IMHO, it would be better to coerce whatever bus to follow common
> driver-model synchronization/attach/detach rules and be able to do
> straight-forward implementation of features in the core driver-model.
> If the current driver-model isn't enough, the core code should be
> expanded rather than doing bus-specific dances in individual buses.
> But I don't really know about any bus other than PCI, so maybe I'm
> being too naive.
>

The main problem I guess is that driver model does not allow registering
or removal of child devices during driver->probe() on the same bus. So
every bus does it its own way. Plus most buses were around way before
driver model so their historical locking rules affect this as well.
Down the road, as the stuff gets cleaned we can have a shot of pulling
bus code into drivre core, but not yet I think.
 
> > > problems (as long as it's not the hub driver from a hub device, we need
> > > to never be able to disconnect those.)
> > 
> > Never say never ;) That was the first thing I did after playing with
> > PCI devices when I tried doing what Tejun did.
> > 
> > If kernel advertises an userspace interface it will be used. I can see
> > myself wanting to disconnect my hub and all its devices so my wireless
> > explorer does not use batteries and I do not want to figure out what
> > port it is connected to... Someone else will find another reason,
> > I don't know.
> > 
> > I also think that even PCI should kill children devices behind a bridge
> > if bridge driver is disconnected to manage resources in more strict way.
> > But I think that would require notion of generic/specialized driver and
> > require automatic rebinding of specialized driver over generic one so
> > every PCI device has a driver attached to it.
> 
>  I think above can be cleanly solved by enforcing that no device can
> be attached to a driver unless all its ancestors are attached to a
> driver.  The check can be made easiliy inside the driver-model, and,
> if needed, making dummy drivers for internal node devices which
> orignally didn't need one shouldn't be difficult.

Right now not all parent PCI devices have drivers. IIRC there were talks
about implementing generic bridge driver but then some bridges need special
handling and the concern was that generic driver would prevent binding of
a special driver.

> We can just return 
> -EBUSY for any attempts to detach an internal device which has
> driver-attached children.  This way, recursing and all other chores
> can be dumped to user-space where they belong.
> 

The system needs to handle situation when you physically yanking part of a
device tree and get rid of all devices, like in cases when you pull an USB
cable connected to a HUB with a mouse and a scanner and a pinter and whatever
else out there. We need to get rid of all these devices and I am not sure
that offloading this task to userspace is good idea. Especially if removal
happens when the box is suspended and at wakeup we have completely different
set of devices...

>  And regarding the duplicate works, my work on manual-attach was
> primarily to show how dynamic device-driver binding can work with
> devparam; also, Dmitry seems to understand the problem better than me.
> So, I think I should back off on manualattach.  Dmitry, what do you
> think about integrating devparam with your work?
>

I do not see why we shoudln't but first I need to persuade Greg to apply
my patches ;)
 
-- 
Dmitry

[PATCH 2.6.10-rc1 0/4] driver-model: manual device attach

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

 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

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

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


Signed-off-by: Tejun Heo <tj@home-tj.org>


Index: linux-export/drivers/base/interface.c
===================================================================
--- linux-export.orig/drivers/base/interface.c	2004-11-04 11:04:15.000000000 +0900
+++ linux-export/drivers/base/interface.c	2004-11-04 11:04:15.000000000 +0900
@@ -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,
 };

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

[Dmitry Torokhov]

On Thursday 04 November 2004 02:46 am, Tejun Heo wrote:
>  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.
> 
...
> +/**
> + *   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;
> +}

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

-- 
Dmitry

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

[Greg KH]

On Thu, Nov 04, 2004 at 12:05:31PM -0500, Dmitry Torokhov wrote:
> On Thursday 04 November 2004 02:46 am, Tejun Heo wrote:
> > ?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.
> > 
> ...
> > +/**
> > + *???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;
> > +}
> 
> 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.

> 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