Re: [PATCH 2/4] Driver core: add driver_probe_device

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Dmitry Torokhov]

Sorry for breaking the threading...

Greg KH wrote:
> On Fri, Oct 29, 2004 at 01:24:21PM -0500, Dmitry Torokhov wrote:
> > On Friday 29 October 2004 11:37 am, Greg KH wrote:
> > > On Tue, Oct 12, 2004 at 01:31:36AM -0500, Dmitry Torokhov wrote:
> > > > #### AUTHOR dtor_core@ameritech.net
> > > > #### COMMENT START
> > > > ### Comments for ChangeSet
> > > > Driver core: rename bus_match into driver_probe_device and export
> > > >              it so subsystems can bind an individual device to a
> > > >              specific driver without getting involved with driver
> > > >              core internals.
> > >
> > > Applied, thanks.
> > >
> >
> > Greg,
> >
> > What about "bind_mode" device and driver attributes? If you are not
> going
> > to apply them then I need to rework driver_probe_device to not call
> > bus->match() function.
> 
> Hm, I'm not going to apply them, but haven't written that email yet,
> sorry.
> 
> Is things now broken with only these 2 patches applied?
> 
> > The reason is that if bind_mode is not in the core
> > then I need to check these attributes in serio's bus match function, but
> > then I will not be able to use driver_probe_device to force binding when
> > user requests it. And if I don't check bind_mode in serio_bus_match then
> > I will have to do all driver/device mathing by hand which I wanted to
> > avoid in the first place.
> 
> Heh, I understand.  I like the ideas of your next patches, but just not
> the implementation.
> 
> I really like the "driver" part in the device.  But not as a file, let's
> make it a symlink back to the driver that is bound to the device at that
> point in time.  This makes it just like the other symlinks in the sysfs
> tree.
> 
> But if we do that, we still don't have a way to implement what you are
> really trying to do (and it breaks your code as you already have a
> driver file.)  I'll work on what I propose instead in my next message
> (will be a few hours, have real work to do for a bit, sorry...)
>

Well, we can have driver as a symlink and rename my "driver" attribute
into something like "drvctl" (just an example, maybe somebody has better
name in mind, I am not fixed on the name) and make it writable only.
I think it should work well as now device and driver objects are very
symmetrical - they both similarly linked together via device->driver
and driver->device).

Also, driver problem is tangent to the bind_mode as you can play with
bind_mode even without driver attribute (f.e. you have 2 cards and 2
modules, mark one card as manual bind, load first module, change bind
mode to automatic, load another module).

Just wanted to make these two points, now I'll wait for your poroposal
later tonight.

Thanks!

-- 
Dmitry

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Greg KH]

On Fri, Oct 29, 2004 at 11:57:52AM -0700, Dmitry Torokhov wrote:
> > I really like the "driver" part in the device.  But not as a file, let's
> > make it a symlink back to the driver that is bound to the device at that
> > point in time.  This makes it just like the other symlinks in the sysfs
> > tree.
> > 
> > But if we do that, we still don't have a way to implement what you are
> > really trying to do (and it breaks your code as you already have a
> > driver file.)  I'll work on what I propose instead in my next message
> > (will be a few hours, have real work to do for a bit, sorry...)
> >
> 
> Well, we can have driver as a symlink and rename my "driver" attribute
> into something like "drvctl" (just an example, maybe somebody has better
> name in mind, I am not fixed on the name) and make it writable only.

Yes, that's exactly what I was thinking of.  But as you wanted it to do
three different things... Hm, no, I guess you only wanted it to do 2
things.  Ah, that's not bad at all.  Been traveling too long, can't
remember threads...

> I think it should work well as now device and driver objects are very
> symmetrical - they both similarly linked together via device->driver
> and driver->device).

I agree.  Hm, I guess the remaining 2 patches aren't that far away from
what I was thinking about originally. 

Ok, care to redo the "driver" patch to be a symlink instead?  I think
the last patch doesn't really need any changes, does it?

Hm, but how does this play with the current pci "add a new device id"
scheme?  Can this "bind_mode" scheme work with that?

thanks,

greg k-h

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Dmitry Torokhov]

On Friday 29 October 2004 03:22 pm, Greg KH wrote:
> Ok, care to redo the "driver" patch to be a symlink instead?  I think
> the last patch doesn't really need any changes, does it?
>

Coming your way.

> Hm, but how does this play with the current pci "add a new device id"
> scheme?  Can this "bind_mode" scheme work with that?
> 

They really serve different purposes as far as I can see. new_id affects
matching (and is PCI specific), bind_mode affects binding of otherwise
matching device/driver pairs. Its main use is to give preference to one
driver and "punish" another. 

For example we have atkbd, psmouse and serio_raw drivers working with
PS/2 ports. atkbd and psmouse are integrated into input subsystem and we
want to encourage their use. Still we don't support (rather detect) all
existing hardware yet so in some case raw access to PS/2 port (a-la 2.4
and earlier kernels) is desirable, so we have serio_raw. But because we
don't want it grab all free serio ports even if it loaded first it is
marked as manual bind.

Another example - you for some reason don't like touchpads and prefer an
external mouse. BUt the way serio subsystem works, even if you disconnect
serio port next time there is data it will try to find the driver again
 - so you mark your port as manual bind to prevent new input device from
appearing.

The bind_mode can also be used to control experimental/deprecated features
and still have everything compiled/loaded.

-- 
Dmitry

[PATCH 1/4] Driver core: add driver symlink to device

[Dmitry Torokhov]

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


ChangeSet@1.1955, 2004-10-30 01:08:14-05:00, dtor_core@ameritech.net
  Driver core: when binding device to a driver create "driver"
               symlink in device's directory. Rename serio's
               "driver" attribute to "drvctl" (write-only)
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 base/bus.c          |    2 ++
 input/serio/serio.c |    7 +------
 2 files changed, 3 insertions(+), 6 deletions(-)


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



diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-10-30 03:14:25 -05:00
+++ b/drivers/base/bus.c	2004-10-30 03:14:25 -05:00
@@ -246,6 +246,7 @@
 	list_add_tail(&dev->driver_list, &dev->driver->devices);
 	sysfs_create_link(&dev->driver->kobj, &dev->kobj,
 			  kobject_name(&dev->kobj));
+	sysfs_create_link(&dev->kobj, &dev->driver->kobj, "driver");
 }
 
 
@@ -370,6 +371,7 @@
 	struct device_driver * drv = dev->driver;
 	if (drv) {
 		sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
+		sysfs_remove_link(&dev->kobj, "driver");
 		list_del_init(&dev->driver_list);
 		device_detach_shutdown(dev);
 		if (drv->remove)
diff -Nru a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
--- a/drivers/input/serio/serio.c	2004-10-30 03:14:25 -05:00
+++ b/drivers/input/serio/serio.c	2004-10-30 03:14:25 -05:00
@@ -246,11 +246,6 @@
 	return sprintf(buf, "%s\n", serio->name);
 }
 
-static ssize_t serio_show_driver(struct device *dev, char *buf)
-{
-	return sprintf(buf, "%s\n", dev->driver ? dev->driver->name : "(none)");
-}
-
 static ssize_t serio_rebind_driver(struct device *dev, const char *buf, size_t count)
 {
 	struct serio *serio = to_serio_port(dev);
@@ -307,7 +302,7 @@
 
 static struct device_attribute serio_device_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_show_description, NULL),
-	__ATTR(driver, S_IWUSR | S_IRUGO, serio_show_driver, serio_rebind_driver),
+	__ATTR(drvctl, S_IWUSR, NULL, serio_rebind_driver),
 	__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
 	__ATTR_NULL
 };

[PATCH 2/4] Driver core: add drvctl device attribute

[Dmitry Torokhov]

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


ChangeSet@1.1956, 2004-10-30 01:40:43-05:00, dtor_core@ameritech.net
  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.
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 drivers/base/interface.c    |   25 +++++++++++++++++
 drivers/input/serio/serio.c |   62 ++++++++++++++++++++++----------------------
 include/linux/device.h      |    3 +-
 3 files changed, 57 insertions(+), 33 deletions(-)


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



diff -Nru a/drivers/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c	2004-10-30 03:14:51 -05:00
+++ b/drivers/base/interface.c	2004-10-30 03:14:51 -05:00
@@ -42,10 +42,33 @@
 	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.
+ */
+
+static ssize_t drvctl_store(struct device * dev, const char * buf, size_t count)
+{
+	int retval = -ENOSYS;
+
+	if (dev->bus->drvctl)
+		retval = dev->bus->drvctl(dev, buf, count);
+
+	if (retval < 0)
+		return retval;
 
+	return count;
+}
+
+
+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,
 };
diff -Nru a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
--- a/drivers/input/serio/serio.c	2004-10-30 03:14:51 -05:00
+++ b/drivers/input/serio/serio.c	2004-10-30 03:14:51 -05:00
@@ -246,36 +246,6 @@
 	return sprintf(buf, "%s\n", serio->name);
 }
 
-static ssize_t serio_rebind_driver(struct device *dev, const char *buf, size_t count)
-{
-	struct serio *serio = to_serio_port(dev);
-	struct device_driver *drv;
-	int retval;
-
-	retval = down_interruptible(&serio_sem);
-	if (retval)
-		return retval;
-
-	retval = count;
-	if (!strncmp(buf, "none", count)) {
-		serio_disconnect_port(serio);
-	} else if (!strncmp(buf, "reconnect", count)) {
-		serio_reconnect_port(serio);
-	} else if (!strncmp(buf, "rescan", count)) {
-		serio_disconnect_port(serio);
-		serio_connect_port(serio, NULL);
-	} else if ((drv = driver_find(buf, &serio_bus)) != NULL) {
-		serio_disconnect_port(serio);
-		serio_connect_port(serio, to_serio_driver(drv));
-		put_driver(drv);
-	} else {
-		retval = -EINVAL;
-	}
-
-	up(&serio_sem);
-
-	return retval;
-}
 
 static ssize_t serio_show_bind_mode(struct device *dev, char *buf)
 {
@@ -302,7 +272,6 @@
 
 static struct device_attribute serio_device_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_show_description, NULL),
-	__ATTR(drvctl, S_IWUSR, NULL, serio_rebind_driver),
 	__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
 	__ATTR_NULL
 };
@@ -591,6 +560,36 @@
 	up(&serio_sem);
 }
 
+static int serio_rebind_driver(struct device *dev, const char *buf, size_t count)
+{
+	struct serio *serio = to_serio_port(dev);
+	struct device_driver *drv;
+	int retval;
+
+	retval = down_interruptible(&serio_sem);
+	if (retval)
+		return retval;
+
+	if (!strncmp(buf, "none", count)) {
+		serio_disconnect_port(serio);
+	} else if (!strncmp(buf, "reconnect", count)) {
+		serio_reconnect_port(serio);
+	} else if (!strncmp(buf, "rescan", count)) {
+		serio_disconnect_port(serio);
+		serio_connect_port(serio, NULL);
+	} else if ((drv = driver_find(buf, &serio_bus)) != NULL) {
+		serio_disconnect_port(serio);
+		serio_connect_port(serio, to_serio_driver(drv));
+		put_driver(drv);
+	} else {
+		retval = -EINVAL;
+	}
+
+	up(&serio_sem);
+
+	return retval;
+}
+
 static void serio_set_drv(struct serio *serio, struct serio_driver *drv)
 {
 	down(&serio->drv_sem);
@@ -655,6 +654,7 @@
 
 	serio_bus.dev_attrs = serio_device_attrs;
 	serio_bus.drv_attrs = serio_driver_attrs;
+	serio_bus.drvctl = serio_rebind_driver;
 	bus_register(&serio_bus);
 
 	return 0;
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	2004-10-30 03:14:51 -05:00
+++ b/include/linux/device.h	2004-10-30 03:14:51 -05:00
@@ -59,7 +59,8 @@
 	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 * buf, size_t count);
+	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);

[PATCH 3/4] Driver core: add bind_mode device/driver attributes

[Dmitry Torokhov]

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


ChangeSet@1.1957, 2004-10-30 01:50:42-05:00, dtor_core@ameritech.net
  Driver core: add "bind_mode" default device and driver attribute.
               Calls to device_attach() and driver_attach() will not
               bind device and driver if either one of them is in
               "manual" bind mode. Manual binding is expected to be
               handled by bus's drvctl()
  
           echo -n "manual" > /sus/bus/serio/devices/serio2/bind_mode
           echo -n "auto" > /sys/bus/serio/drivers/serio_raw/bind_mode
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 drivers/base/bus.c              |   20 ++++++----
 drivers/base/interface.c        |   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(-)


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



diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-10-30 03:15:15 -05:00
+++ b/drivers/base/bus.c	2004-10-30 03:15:15 -05:00
@@ -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,
 };
 
 
@@ -302,9 +305,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 */
@@ -312,8 +318,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);
 		}
 	}
 
@@ -339,12 +345,12 @@
 	struct list_head * entry;
 	int error;
 
-	if (!bus->match)
+	if (drv->manual_bind || !bus->match)
 		return;
 
 	list_for_each(entry, &bus->devices.list) {
 		struct device * dev = container_of(entry, struct device, bus_list);
-		if (!dev->driver) {
+		if (!dev->driver && !dev->manual_bind) {
 			error = driver_probe_device(drv, dev);
 			if (error && (error != -ENODEV))
 				/* driver matched but the probe failed */
diff -Nru a/drivers/base/interface.c b/drivers/base/interface.c
--- a/drivers/base/interface.c	2004-10-30 03:15:15 -05:00
+++ b/drivers/base/interface.c	2004-10-30 03:15:15 -05:00
@@ -1,6 +1,6 @@
 /*
  * drivers/base/interface.c - common driverfs interface that's exported to
- * 	the world for all devices.
+ * 	the world for all devices and drivers.
  *
  * Copyright (c) 2002-3 Patrick Mochel
  * Copyright (c) 2002-3 Open Source Development Labs
@@ -15,6 +15,35 @@
 #include <linux/string.h>
 
 /**
+ *	bind_mode - control the binding mode for the device.
+ *
+ *	When set to "auto" driver core will try to automatically bind the
+ *	device once appropriate driver becomes available. When bind mode
+ *	is "manual" intervention from userspace is required.
+ */
+
+static ssize_t dev_bind_mode_show(struct device * dev, char * buf)
+{
+	return sprintf(buf, "%s\n", dev->manual_bind ? "manual" : "auto");
+}
+
+static ssize_t dev_bind_mode_store(struct device * dev, const char * buf, size_t count)
+{
+	int retval = count;
+
+	if (!strncmp(buf, "manual", count)) {
+		dev->manual_bind = 1;
+	} else if (!strncmp(buf, "auto", count)) {
+		dev->manual_bind = 0;
+	} else {
+		retval = -EINVAL;
+	}
+
+	return retval;
+}
+
+
+/**
  *	detach_state - control the default power state for the device.
  *
  *	This is the state the device enters when it's driver module is
@@ -27,12 +56,12 @@
  *	driver's suspend method.
  */
 
-static ssize_t detach_show(struct device * dev, char * buf)
+static ssize_t dev_detach_show(struct device * dev, char * buf)
 {
 	return sprintf(buf, "%u\n", dev->detach_state);
 }
 
-static ssize_t detach_store(struct device * dev, const char * buf, size_t n)
+static ssize_t dev_detach_store(struct device * dev, const char * buf, size_t n)
 {
 	u32 state;
 	state = simple_strtoul(buf, NULL, 10);
@@ -50,7 +79,7 @@
  *	device to a specific driver.
  */
 
-static ssize_t drvctl_store(struct device * dev, const char * buf, size_t count)
+static ssize_t dev_drvctl_store(struct device * dev, const char * buf, size_t count)
 {
 	int retval = -ENOSYS;
 
@@ -64,11 +93,49 @@
 }
 
 
-static DEVICE_ATTR(detach_state, 0644, detach_show, detach_store);
-static DEVICE_ATTR(drvctl, 0200, NULL, drvctl_store);
+static DEVICE_ATTR(bind_mode, 0644, dev_bind_mode_show, dev_bind_mode_store);
+static DEVICE_ATTR(detach_state, 0644, dev_detach_show, dev_detach_store);
+static DEVICE_ATTR(drvctl, 0200, NULL, dev_drvctl_store);
 
 struct attribute * dev_default_attrs[] = {
+	&dev_attr_bind_mode.attr,
 	&dev_attr_detach_state.attr,
 	&dev_attr_drvctl.attr,
 	NULL,
 };
+
+/**
+ *	bind_mode - control the binding mode for the driver.
+ *
+ *	When set to "auto" driver core will try to automatically bind the
+ *	driver once appropriate device becomes available. When bind mode
+ *	is "manual" intervention from userspace is required.
+ */
+
+static ssize_t drv_bind_mode_show(struct device_driver * drv, char * buf)
+{
+	return sprintf(buf, "%s\n", drv->manual_bind ? "manual" : "auto");
+}
+
+static ssize_t drv_bind_mode_store(struct device_driver * drv, const char * buf, size_t count)
+{
+	int retval = count;
+
+	if (!strncmp(buf, "manual", count)) {
+		drv->manual_bind = 1;
+	} else if (!strncmp(buf, "auto", count)) {
+		drv->manual_bind = 0;
+	} else {
+		retval = -EINVAL;
+	}
+
+	return retval;
+}
+
+static DRIVER_ATTR(bind_mode, 0644, drv_bind_mode_show, drv_bind_mode_store);
+
+struct attribute * drv_default_attrs[] = {
+	&driver_attr_bind_mode.attr,
+	NULL,
+};
+
diff -Nru a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
--- a/drivers/input/serio/serio.c	2004-10-30 03:15:15 -05:00
+++ b/drivers/input/serio/serio.c	2004-10-30 03:15:15 -05:00
@@ -92,7 +92,7 @@
 	struct serio_driver *drv;
 
 	list_for_each_entry(drv, &serio_driver_list, node)
-		if (!drv->manual_bind)
+		if (!drv->driver.manual_bind)
 			if (serio_bind_driver(serio, drv))
 				break;
 }
@@ -246,33 +246,8 @@
 	return sprintf(buf, "%s\n", serio->name);
 }
 
-
-static ssize_t serio_show_bind_mode(struct device *dev, char *buf)
-{
-	struct serio *serio = to_serio_port(dev);
-	return sprintf(buf, "%s\n", serio->manual_bind ? "manual" : "auto");
-}
-
-static ssize_t serio_set_bind_mode(struct device *dev, const char *buf, size_t count)
-{
-	struct serio *serio = to_serio_port(dev);
-	int retval;
-
-	retval = count;
-	if (!strncmp(buf, "manual", count)) {
-		serio->manual_bind = 1;
-	} else if (!strncmp(buf, "auto", count)) {
-		serio->manual_bind = 0;
-	} else {
-		retval = -EINVAL;
-	}
-
-	return retval;
-}
-
 static struct device_attribute serio_device_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_show_description, NULL),
-	__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
 	__ATTR_NULL
 };
 
@@ -341,7 +316,7 @@
 
 	if (drv)
 		serio_bind_driver(serio, drv);
-	else if (!serio->manual_bind)
+	else if (!serio->dev.manual_bind)
 		serio_find_driver(serio);
 
 	/* Ok, now bind children, if any */
@@ -353,7 +328,7 @@
 
 		serio_create_port(serio);
 
-		if (!serio->manual_bind) {
+		if (!serio->dev.manual_bind) {
 			/*
 			 * With children we just _prefer_ passed in driver,
 			 * but we will try other options in case preferred
@@ -475,34 +450,8 @@
 	return sprintf(buf, "%s\n", driver->description ? driver->description : "(none)");
 }
 
-static ssize_t serio_driver_show_bind_mode(struct device_driver *drv, char *buf)
-{
-	struct serio_driver *serio_drv = to_serio_driver(drv);
-	return sprintf(buf, "%s\n", serio_drv->manual_bind ? "manual" : "auto");
-}
-
-static ssize_t serio_driver_set_bind_mode(struct device_driver *drv, const char *buf, size_t count)
-{
-	struct serio_driver *serio_drv = to_serio_driver(drv);
-	int retval;
-
-	retval = count;
-	if (!strncmp(buf, "manual", count)) {
-		serio_drv->manual_bind = 1;
-	} else if (!strncmp(buf, "auto", count)) {
-		serio_drv->manual_bind = 0;
-	} else {
-		retval = -EINVAL;
-	}
-
-	return retval;
-}
-
-
 static struct driver_attribute serio_driver_attrs[] = {
 	__ATTR(description, S_IRUGO, serio_driver_show_description, NULL),
-	__ATTR(bind_mode, S_IWUSR | S_IRUGO,
-		serio_driver_show_bind_mode, serio_driver_set_bind_mode),
 	__ATTR_NULL
 };
 
@@ -517,7 +466,7 @@
 	drv->driver.bus = &serio_bus;
 	driver_register(&drv->driver);
 
-	if (drv->manual_bind)
+	if (drv->driver.manual_bind)
 		goto out;
 
 start_over:
diff -Nru a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
--- a/drivers/input/serio/serio_raw.c	2004-10-30 03:15:15 -05:00
+++ b/drivers/input/serio/serio_raw.c	2004-10-30 03:15:15 -05:00
@@ -365,14 +365,14 @@
 
 static struct serio_driver serio_raw_drv = {
 	.driver		= {
-		.name	= "serio_raw",
+		.name		= "serio_raw",
+		.manual_bind	= 1,
 	},
 	.description	= DRIVER_DESC,
 	.interrupt	= serio_raw_interrupt,
 	.connect	= serio_raw_connect,
 	.reconnect	= serio_raw_reconnect,
 	.disconnect	= serio_raw_disconnect,
-	.manual_bind	= 1,
 };
 
 int __init serio_raw_init(void)
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	2004-10-30 03:15:15 -05:00
+++ b/include/linux/device.h	2004-10-30 03:15:15 -05:00
@@ -103,6 +103,8 @@
 	char			* name;
 	struct bus_type		* bus;
 
+	unsigned int		manual_bind;
+
 	struct semaphore	unload_sem;
 	struct kobject		kobj;
 	struct list_head	devices;
@@ -265,6 +267,9 @@
 	struct bus_type	* bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	unsigned int manual_bind;	/* indicates whether the core will
+					   try to find a driver for the
+					   device automatically */
 	void		*driver_data;	/* data private to the driver */
 	void		*platform_data;	/* Platform specific data (e.g. ACPI,
 					   BIOS data relevant to device) */
diff -Nru a/include/linux/serio.h b/include/linux/serio.h
--- a/include/linux/serio.h	2004-10-30 03:15:15 -05:00
+++ b/include/linux/serio.h	2004-10-30 03:15:15 -05:00
@@ -27,8 +27,6 @@
 	char name[32];
 	char phys[32];
 
-	unsigned int manual_bind;
-
 	unsigned short idbus;
 	unsigned short idvendor;
 	unsigned short idproduct;
@@ -57,8 +55,6 @@
 struct serio_driver {
 	void *private;
 	char *description;
-
-	unsigned int manual_bind;
 
 	void (*write_wakeup)(struct serio *);
 	irqreturn_t (*interrupt)(struct serio *, unsigned char,

[PATCH 4/4] Driver core: bus_rescan_devices improper locking

[Dmitry Torokhov]

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


ChangeSet@1.1958, 2004-10-30 03:13:25-05:00, dtor_core@ameritech.net
  Driver core: bus_rescan_devices() should take write lock to guarantee
               that 2 threads will not probe same device and therefore
               can not be implemented with bus_for_each_dev().
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 bus.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)


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



diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-10-30 03:15:58 -05:00
+++ b/drivers/base/bus.c	2004-10-30 03:15:58 -05:00
@@ -571,18 +571,6 @@
 }
 
 
-/* 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))
-		(*count)++;
-
-	return 0;
-}
-
-
 /**
  *	bus_rescan_devices - rescan devices on the bus for possible drivers
  *	@bus:	the bus to scan.
@@ -594,9 +582,19 @@
  */
 int bus_rescan_devices(struct bus_type * bus)
 {
+	struct device *dev;
 	int count = 0;
 
-	bus_for_each_dev(bus, NULL, &count, bus_rescan_devices_helper);
+	if (!(bus = get_bus(bus)))
+		return 0;
+
+	down_write(&bus->subsys.rwsem);
+	list_for_each_entry(dev, &bus->devices.list, bus_list) {
+		if (!dev->driver && device_attach(dev))
+			count++;
+	}
+	up_write(&bus->subsys.rwsem);
+	put_bus(bus);
 
 	return count;
 }

Re: [PATCH 1/4] Driver core: add driver symlink to device

[Greg KH]

On Sat, Oct 30, 2004 at 03:27:22AM -0500, Dmitry Torokhov wrote:
> 
> ===================================================================
> 
> 
> ChangeSet@1.1955, 2004-10-30 01:08:14-05:00, dtor_core@ameritech.net
>   Driver core: when binding device to a driver create "driver"
>                symlink in device's directory. Rename serio's
>                "driver" attribute to "drvctl" (write-only)
>   
>   Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Ok, I've applied this one, as it was very simple.

I'll get to your other patches tomorrow, need to get a pci and usb
update out today...

thanks for your patience, we'll get there eventually :)

greg k-h

Driver core change request

[Dmitry Torokhov]

Hi,

I am reworking my sysfs serio patches (trying to get dynamic psmouse
protocol switching) and I am wondering if we could export device_attach
function. Serio system allows user to request device rescan - force current
driver to let go off the device and find another suitable driver. Also user
can manually request device to be disconnected/connected to a driver. By
having device_attach exported I could get rid of some duplicated code.

Also serio allows user to request a specific driver to be bound to a device
in case there are several options (psmouse/serio_raw for example). To do
that and not poke in the driver core guts too much I need something like the
following:

int driver_probe_device(struct device_driver *dev, struct device *dev)
{
        int error;
        
	dev->driver = drv;
        if (drv->probe) {
        	if ((error = drv->probe(dev))) {
                	dev->driver = NULL;
                        return error;
                }
	}
        device_bind_driver(dev);
        return 0;
}


static int bus_match(struct device * dev, struct device_driver * drv)
{
        if (dev->bus->match(dev, drv))
		return driver_probe_device(drv, dev);

        return -ENODEV;
}

I.e driver_probe_device is exported. Does it have a chance to be accepted?

Thanks!

-- 
Dmitry

Re: Driver core change request

[Dmitry Torokhov]

On Friday 08 October 2004 04:48 pm, Greg KH wrote:
> On Thu, Oct 07, 2004 at 09:59:10PM -0500, Dmitry Torokhov wrote:
> > On Thursday 07 October 2004 04:40 pm, Greg KH wrote:
> > > On Wed, Oct 06, 2004 at 11:54:18PM -0500, Dmitry Torokhov wrote:
> > > > Hi,
> > > > 
> > > > I am reworking my sysfs serio patches (trying to get dynamic psmouse
> > > > protocol switching) and I am wondering if we could export device_attach
> > > > function. Serio system allows user to request device rescan - force current
> > > > driver to let go off the device and find another suitable driver. Also user
> > > > can manually request device to be disconnected/connected to a driver. By
> > > > having device_attach exported I could get rid of some duplicated code.
> > > 
> > > driver_attach() is global, so I don't have a problem with making
> > > device_attach() global either.  Just send me a patch :)
> > > 
> > 
> > OK. BTW, while driver_attach is global it is not exported. Should I mark both
> > of them EXPORT_GPL_ONLY? 
> 
> No, pci can not be a module.  Can your serio core be a module?  If not,
> just make it global.  But if so, yes, EXPORT_SYMBOL_GPL() is the proper
> marking.
> 

Yes serio and gameport (which I am planning to convert to driver model as well)
can be built as modules.

> > > > I.e driver_probe_device is exported. Does it have a chance to be accepted?
> > > 
> > > What's wrong with doing what the pci core does in this situation and
> > > call driver_attach()?
> > > 
> > 
> > Well, I need to be able to work with a specific port so when I am doing
> > 
> > 	echo -n "serio_raw" > /sys/bus/serio/devices/serio1/driver
> > 
> > I want only serio1 be affected, not every disconnected port that can work
> > with serio_raw. driver_attach() will claim all of them. 
> > 
> > The difference is that serio system allows you to have several drivers
> > that can drive the same hardware and user gets to chose which driver gets
> > the honors. I.e given 4 PS/2 ports in the system user might want to have
> > raw access to port #2 (via serio_raw) while using standard psmouse driver
> > for the rest of them. With PCI if you have 2 drivers and 2 exactly same
> > cards you do not have ability to bind first driver to one of the cards and
> > second driver to the other.
> 
> Nice.  We want to make this kind of functionality avaiable to all
> busses, and not have it be a bus only type feature.  So if you can see
> any way it could be moved into the core, I'd be all for it.
> 

I have been looking at it entire weekend and I do not presently see a way to
make this facility truly generic. That is because pretty much all buses
perform additional steps besides code in driver->remove() which driver core
does is not aware of. What is needed it bus->device_disconnect function that
would disconnect device and probably destroy all children.

For now I added:

- "driver" default device attribute that produces name of currently bound
  driver uppon read.
- bus->rebind_handler method that is called when someone writes to "driver"
  attribute and allows to perform bunctions like disconnecting device or
  rebinding it to alternative driver in bus-specific way.
- "bind_mode" default device and driver attributes that can be "auto" or
  "manual". When device or driver marked as manual bind device_attach()
  and driver_attach() will ignore them. They are expected to be bound by
  bus->rebind_handler (via driver_probe_device()).

I also renamed bus_match to driver_probe_device() and exported it, along
with device_attach and driver_attach.

Please let me know if its acceptable.

The patches are suitable for importing with bkimport utility (by Bjorn
Helgaas?) that allows preserve per-file comments.

	http://www.geocities.com/dt_or/misc/

-- 
Dmitry

[PATCH 1/4] Driver core: export device_attach

[Dmitry Torokhov]

#### AUTHOR dtor_core@ameritech.net
#### COMMENT START
### Comments for ChangeSet
Driver core: make device_attach() global and export it and
             driver_attach() so subsystems can have finer
             control over binding process.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
### Comments for drivers/base/bus.c
Make device_attach() global and export it and driver_attach()
### Comments for include/linux/device.h
Make device_attach() global.
#### COMMENT END

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/11 00:19:18-05:00 dtor_core@ameritech.net 
#   Driver core: make device_attach() global and export it and
#                driver_attach() so subsystems can have finer
#                control over binding process.
#   
#   Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
# 
# include/linux/device.h
#   2004/10/10 23:56:49-05:00 dtor_core@ameritech.net +1 -0
#   Make device_attach() global.
# 
# drivers/base/bus.c
#   2004/10/10 23:56:49-05:00 dtor_core@ameritech.net +3 -1
#   Make device_attach() global and export it and driver_attach()
# 
diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-10-12 01:27:27 -05:00
+++ b/drivers/base/bus.c	2004-10-12 01:27:27 -05:00
@@ -288,7 +288,7 @@
  *	Walk the list of drivers that the bus has and call bus_match()
  *	for each pair. If a compatible pair is found, break out and return.
  */
-static int device_attach(struct device * dev)
+int device_attach(struct device * dev)
 {
  	struct bus_type * bus = dev->bus;
 	struct list_head * entry;
@@ -728,6 +728,8 @@
 
 EXPORT_SYMBOL_GPL(device_bind_driver);
 EXPORT_SYMBOL_GPL(device_release_driver);
+EXPORT_SYMBOL_GPL(device_attach);
+EXPORT_SYMBOL_GPL(driver_attach);
 
 EXPORT_SYMBOL_GPL(bus_add_device);
 EXPORT_SYMBOL_GPL(bus_remove_device);
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	2004-10-12 01:27:27 -05:00
+++ b/include/linux/device.h	2004-10-12 01:27:27 -05:00
@@ -327,6 +327,7 @@
  */
 extern void device_bind_driver(struct device * dev);
 extern void device_release_driver(struct device * dev);
+extern int  device_attach(struct device * dev);
 extern void driver_attach(struct device_driver * drv);
 
 

[PATCH 2/4] Driver core: add driver_probe_device

[Dmitry Torokhov]

#### AUTHOR dtor_core@ameritech.net
#### COMMENT START
### Comments for ChangeSet
Driver core: rename bus_match into driver_probe_device and export
             it so subsystems can bind an individual device to a
             specific driver without getting involved with driver
             core internals.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
### Comments for drivers/base/bus.c
Rename bus_match() to driver_probe_device() and export it. 
### Comments for include/linux/device.h
Add driver_probe_device()
#### COMMENT END

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/11 00:19:57-05:00 dtor_core@ameritech.net 
#   Driver core: rename bus_match into driver_probe_device and export
#                it so subsystems can bind an individual device to a
#                specific driver without getting involved with driver
#                core internals.
#   
#   Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
# 
# include/linux/device.h
#   2004/10/10 23:57:30-05:00 dtor_core@ameritech.net +2 -1
#   Add driver_probe_device()
# 
# drivers/base/bus.c
#   2004/10/10 23:57:30-05:00 dtor_core@ameritech.net +29 -26
#   Rename bus_match() to driver_probe_device() and export it. 
# 
diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-10-12 01:28:06 -05:00
+++ b/drivers/base/bus.c	2004-10-12 01:28:06 -05:00
@@ -250,34 +250,35 @@
 
 
 /**
- *	bus_match - check compatibility between device & driver.
- *	@dev:	device.
+ *	driver_probe_device - attempt to bind device & driver.
  *	@drv:	driver.
+ *	@dev:	device.
  *
- *	First, we call the bus's match function, which should compare
- *	the device IDs the driver supports with the device IDs of the
- *	device. Note we don't do this ourselves because we don't know
- *	the format of the ID structures, nor what is to be considered
- *	a match and what is not.
+ *	First, we call the bus's match function, if one present, which
+ *	should compare the device IDs the driver supports with the
+ *	device IDs of the device. Note we don't do this ourselves
+ *	because we don't know the format of the ID structures, nor what
+ *	is to be considered a match and what is not.
  *
  *	If we find a match, we call @drv->probe(@dev) if it exists, and
- *	call attach() above.
+ *	call device_bind_driver() above.
  */
-static int bus_match(struct device * dev, struct device_driver * drv)
+int driver_probe_device(struct device_driver * drv, struct device * dev)
 {
-	int error = -ENODEV;
-	if (dev->bus->match(dev, drv)) {
-		dev->driver = drv;
-		if (drv->probe) {
-			if ((error = drv->probe(dev))) {
-				dev->driver = NULL;
-				return error;
-			}
+	if (drv->bus->match && !drv->bus->match(dev, drv))
+		return -ENODEV;
+
+	dev->driver = drv;
+	if (drv->probe) {
+		int error = drv->probe(dev);
+		if (error) {
+			dev->driver = NULL;
+			return error;
 		}
-		device_bind_driver(dev);
-		error = 0;
 	}
-	return error;
+
+	device_bind_driver(dev);
+	return 0;
 }
 
 
@@ -285,8 +286,9 @@
  *	device_attach - try to attach device to a driver.
  *	@dev:	device.
  *
- *	Walk the list of drivers that the bus has and call bus_match()
- *	for each pair. If a compatible pair is found, break out and return.
+ *	Walk the list of drivers that the bus has and call
+ *	driver_probe_device() for each pair. If a compatible
+ *	pair is found, break out and return.
  */
 int device_attach(struct device * dev)
 {
@@ -302,7 +304,7 @@
 	if (bus->match) {
 		list_for_each(entry, &bus->drivers.list) {
 			struct device_driver * drv = to_drv(entry);
-			error = bus_match(dev, drv);
+			error = driver_probe_device(drv, dev);
 			if (!error)
 				/* success, driver matched */
 				return 1;
@@ -327,8 +329,8 @@
  *	If bus_match() returns 0 and the @dev->driver is set, we've found
  *	a compatible pair.
  *
- *	Note that we ignore the -ENODEV error from bus_match(), since it's
- *	perfectly valid for a driver not to bind to any devices.
+ *	Note that we ignore the -ENODEV error from driver_probe_device(),
+ *	since it's perfectly valid for a driver not to bind to any devices.
  */
 void driver_attach(struct device_driver * drv)
 {
@@ -342,7 +344,7 @@
 	list_for_each(entry, &bus->devices.list) {
 		struct device * dev = container_of(entry, struct device, bus_list);
 		if (!dev->driver) {
-			error = bus_match(dev, drv);
+			error = driver_probe_device(drv, dev);
 			if (error && (error != -ENODEV))
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING
@@ -726,6 +728,7 @@
 EXPORT_SYMBOL_GPL(bus_for_each_dev);
 EXPORT_SYMBOL_GPL(bus_for_each_drv);
 
+EXPORT_SYMBOL_GPL(driver_probe_device);
 EXPORT_SYMBOL_GPL(device_bind_driver);
 EXPORT_SYMBOL_GPL(device_release_driver);
 EXPORT_SYMBOL_GPL(device_attach);
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	2004-10-12 01:28:06 -05:00
+++ b/include/linux/device.h	2004-10-12 01:28:06 -05:00
@@ -322,9 +322,10 @@
 		     int (*fn)(struct device *, void *));
 
 /*
- * Manual binding of a device to driver. See drivers/base/bus.c 
+ * 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 void device_bind_driver(struct device * dev);
 extern void device_release_driver(struct device * dev);
 extern int  device_attach(struct device * dev);

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Greg KH]

On Tue, Oct 12, 2004 at 01:31:36AM -0500, Dmitry Torokhov wrote:
> #### AUTHOR dtor_core@ameritech.net
> #### COMMENT START
> ### Comments for ChangeSet
> Driver core: rename bus_match into driver_probe_device and export
>              it so subsystems can bind an individual device to a
>              specific driver without getting involved with driver
>              core internals.

Applied, thanks.

greg k-h

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Dmitry Torokhov]

On Friday 29 October 2004 11:37 am, Greg KH wrote:
> On Tue, Oct 12, 2004 at 01:31:36AM -0500, Dmitry Torokhov wrote:
> > #### AUTHOR dtor_core@ameritech.net
> > #### COMMENT START
> > ### Comments for ChangeSet
> > Driver core: rename bus_match into driver_probe_device and export
> >              it so subsystems can bind an individual device to a
> >              specific driver without getting involved with driver
> >              core internals.
> 
> Applied, thanks.
> 

Greg,

What about "bind_mode" device and driver attributes? If you are not going
to apply them then I need to rework driver_probe_device to not call 
bus->match() function. The reason is that if bind_mode is not in the core
then I need to check these attributes in serio's bus match function, but
then I will not be able to use driver_probe_device to force binding when
user requests it. And if I don't check bind_mode in serio_bus_match then
I will have to do all driver/device mathing by hand which I wanted to
avoid in the first place.

Please let me know.

-- 
Dmitry

Re: [PATCH 2/4] Driver core: add driver_probe_device

[Greg KH]

On Fri, Oct 29, 2004 at 01:24:21PM -0500, Dmitry Torokhov wrote:
> On Friday 29 October 2004 11:37 am, Greg KH wrote:
> > On Tue, Oct 12, 2004 at 01:31:36AM -0500, Dmitry Torokhov wrote:
> > > #### AUTHOR dtor_core@ameritech.net
> > > #### COMMENT START
> > > ### Comments for ChangeSet
> > > Driver core: rename bus_match into driver_probe_device and export
> > >              it so subsystems can bind an individual device to a
> > >              specific driver without getting involved with driver
> > >              core internals.
> > 
> > Applied, thanks.
> > 
> 
> Greg,
> 
> What about "bind_mode" device and driver attributes? If you are not going
> to apply them then I need to rework driver_probe_device to not call 
> bus->match() function.

Hm, I'm not going to apply them, but haven't written that email yet,
sorry.

Is things now broken with only these 2 patches applied?

> The reason is that if bind_mode is not in the core
> then I need to check these attributes in serio's bus match function, but
> then I will not be able to use driver_probe_device to force binding when
> user requests it. And if I don't check bind_mode in serio_bus_match then
> I will have to do all driver/device mathing by hand which I wanted to
> avoid in the first place.

Heh, I understand.  I like the ideas of your next patches, but just not
the implementation.

I really like the "driver" part in the device.  But not as a file, let's
make it a symlink back to the driver that is bound to the device at that
point in time.  This makes it just like the other symlinks in the sysfs
tree.

But if we do that, we still don't have a way to implement what you are
really trying to do (and it breaks your code as you already have a
driver file.)  I'll work on what I propose instead in my next message
(will be a few hours, have real work to do for a bit, sorry...)

thanks,

greg k-h