[RFC] consolidate /sbin/hotplug call for pci and usb

[RFC] consolidate /sbin/hotplug call for pci and usb

[Greg KH]

Hi all,

Here's a patch against 2.5.38 that does the following:
	- adds a call to /sbin/hotplug to the driver core
	- removes the call to /sbin/hotplug from the pci and usb cores
	- adds a callback to the struct bus_type for the driver core to
	  use to set up bus specific /sbin/hotplug arguments.

One nice side effect of this patch, is that all iterfaces for a USB
device generate a call to /sbin/hotplug, so complex devices should now
be properly recognized by userspace much easier.

I've successfully tested this with both PCI Hotplug systems, and USB
devices.  There is still a bug remaining that causes an oops when a USB
pci controller is unloaded from the system that I am still tracking
down.  After I find this problem, and if there are no complaints, I'll
send it on to Linus.

Comments appreciated.

thanks,

greg k-h


diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
--- a/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
+++ b/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
@@ -5,6 +5,10 @@
 
 obj-y		+= fs/
 
+ifeq ($(CONFIG_HOTPLUG),y)
+	obj-y	+= hotplug.o
+endif
+
 export-objs	:= core.o power.o sys.o bus.o driver.o \
 			class.o intf.o
 
diff -Nru a/drivers/base/base.h b/drivers/base/base.h
--- a/drivers/base/base.h	Wed Sep 25 14:22:28 2002
+++ b/drivers/base/base.h	Wed Sep 25 14:22:28 2002
@@ -50,3 +50,13 @@
 
 extern int driver_attach(struct device_driver * drv);
 extern void driver_detach(struct device_driver * drv);
+
+#ifdef CONFIG_HOTPLUG
+extern int dev_hotplug(struct device *dev, const char *action);
+#else
+static inline int dev_hotplug(struct device *dev, const char *action)
+{
+	return 0;
+}
+#endif
+
diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Wed Sep 25 14:22:28 2002
+++ b/drivers/base/core.c	Wed Sep 25 14:22:28 2002
@@ -198,6 +198,9 @@
 	if (platform_notify)
 		platform_notify(dev);
 
+	/* notify userspace of device entry */
+	dev_hotplug(dev, "add");
+
  register_done:
 	if (error) {
 		spin_lock(&device_lock);
@@ -255,6 +258,9 @@
 
 	device_detach(dev);
 	bus_remove_device(dev);
+
+	/* notify userspace that this device just disappeared */
+	dev_hotplug (dev, "remove");
 
 	/* remove the driverfs directory */
 	device_remove_dir(dev);
diff -Nru a/drivers/base/hotplug.c b/drivers/base/hotplug.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/base/hotplug.c	Wed Sep 25 14:22:28 2002
@@ -0,0 +1,114 @@
+/*
+ * drivers/base/hotplug.c - hotplug call code
+ * 
+ * Copyright (c) 2002 Greg Kroah-Hartman <greg@kroah.com>
+ *		 2002 IBM Corp.
+ */
+
+#define DEBUG 0
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/kmod.h>
+#include <linux/interrupt.h>
+#include "base.h"
+
+/*
+ * hotplugging invokes what /proc/sys/kernel/hotplug says (normally
+ * /sbin/hotplug) when devices get added or removed.
+ *
+ * This invokes a user mode policy agent, typically helping to load driver
+ * or other modules, configure the device, and more.  Drivers can provide
+ * a MODULE_DEVICE_TABLE to help with module loading subtasks.
+ *
+ * Some synchronization is important: removes can't start processing
+ * before the add-device processing completes, and vice versa.  That keeps
+ * a stack of USB-related identifiers stable while they're in use.  If we
+ * know that agents won't complete after they return (such as by forking
+ * a process that completes later), it's enough to just waitpid() for the
+ * agent -- as is currently done.
+ *
+ * The reason: we know we're called either from khubd (the typical case)
+ * or from root hub initialization (init, kapmd, modprobe, etc).  In both
+ * cases, we know no other thread can recycle our address, since we must
+ * already have been serialized enough to prevent that.
+ */
+#define BUFFER_SIZE	1024	/* should be enough memory for the env */
+#define NUM_ENVP	32	/* number of env pointers */
+int dev_hotplug (struct device *dev, const char *action)
+{
+	char *argv [3], **envp, *buffer, *scratch;
+	int retval;
+	int i = 0;
+
+	pr_debug ("%s\n", __FUNCTION__);
+	if (!dev)
+		return -ENODEV;
+
+	if (!dev->bus || !dev->bus->hotplug)
+		return -ENODEV;
+
+	if (!hotplug_path [0])
+		return -ENODEV;
+
+	if (in_interrupt ()) {
+		pr_debug ("%s - in_interrupt, not allowed!", __FUNCTION__);
+		return -EIO;
+	}
+
+	if (!current->fs->root) {
+		/* don't try to do anything unless we have a root partition */
+		pr_debug ("%s - %s -- no FS yet\n", __FUNCTION__, action);
+		return -EIO;
+	}
+
+	envp = (char **) kmalloc (NUM_ENVP * sizeof (char *), GFP_KERNEL);
+	if (!envp)
+		return -ENOMEM;
+
+	buffer = kmalloc (BUFFER_SIZE, GFP_KERNEL);
+	if (!buffer) {
+		kfree (envp);
+		return -ENOMEM;
+	}
+
+	/* only one standardized param to hotplug command: the bus name */
+	argv [0] = hotplug_path;
+	argv [1] = dev->bus->name;
+	argv [2] = 0;
+
+	/* minimal command environment */
+	envp [i++] = "HOME=/";
+	envp [i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
+
+#ifdef DEBUG
+	/* hint that policy agent should enter no-stdout debug mode */
+	envp [i++] = "DEBUG=kernel";
+#endif
+
+	scratch = buffer;
+	/* action:  add, remove */
+	envp [i++] = scratch;
+	scratch += sprintf (scratch, "ACTION=%s", action) + 1;
+
+	/* have the bus specific function set up the rest of the environment */
+	retval = dev->bus->hotplug (dev, &envp[i], NUM_ENVP - i,
+				    scratch, BUFFER_SIZE - (scratch - buffer));
+	if (retval) {
+		pr_debug ("%s - hotplug() returned %d\n", __FUNCTION__, retval);
+		goto exit;
+	}
+
+	pr_debug ("%s: %s %s %s %s %s %s\n", __FUNCTION__, argv [0], argv[1],
+		  action, envp[0], envp[1], envp[2]);
+	retval = call_usermodehelper (argv [0], argv, envp);
+	if (retval)
+		pr_debug ("%s - call_usermodehelper returned %d\n",
+			  __FUNCTION__, retval);
+
+exit:
+	kfree (buffer);
+	kfree (envp);
+	return retval;
+}
diff -Nru a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c
--- a/drivers/pci/hotplug.c	Wed Sep 25 14:22:28 2002
+++ b/drivers/pci/hotplug.c	Wed Sep 25 14:22:28 2002
@@ -1,52 +1,42 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/kmod.h>		/* for hotplug_path */
+#include "pci.h"
 
-#ifndef FALSE
-#define FALSE	(0)
-#define TRUE	(!FALSE)
-#endif
 
 #ifdef CONFIG_HOTPLUG
-static void run_sbin_hotplug(struct pci_dev *pdev, int insert)
+int pci_hotplug (struct device *dev, char **envp, int num_envp,
+		 char *buffer, int buffer_size)
 {
-	int i;
-	char *argv[3], *envp[8];
-	char id[20], sub_id[24], bus_id[24], class_id[20];
-
-	if (!hotplug_path[0])
-		return;
-
-	sprintf(class_id, "PCI_CLASS=%04X", pdev->class);
-	sprintf(id, "PCI_ID=%04X:%04X", pdev->vendor, pdev->device);
-	sprintf(sub_id, "PCI_SUBSYS_ID=%04X:%04X", pdev->subsystem_vendor, pdev->subsystem_device);
-	sprintf(bus_id, "PCI_SLOT_NAME=%s", pdev->slot_name);
-
-	i = 0;
-	argv[i++] = hotplug_path;
-	argv[i++] = "pci";
-	argv[i] = 0;
-
-	i = 0;
-	/* minimal command environment */
-	envp[i++] = "HOME=/";
-	envp[i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
-	
-	/* other stuff we want to pass to /sbin/hotplug */
-	envp[i++] = class_id;
-	envp[i++] = id;
-	envp[i++] = sub_id;
-	envp[i++] = bus_id;
-	if (insert)
-		envp[i++] = "ACTION=add";
-	else
-		envp[i++] = "ACTION=remove";
+	struct pci_dev *pdev = to_pci_dev(dev);
+	char *scratch;
+	int i = 0;
+
+	scratch = buffer;
+
+	/* stuff we want to pass to /sbin/hotplug */
+	envp[i++] = scratch;
+	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
+
+	envp[i++] = scratch;
+	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
+			    pdev->vendor, pdev->device) + 1;
+	envp[i++] = scratch;
+	scratch += sprintf (scratch, "PCI_SUBSYS_ID=%04X:%04X",
+			    pdev->subsystem_vendor, pdev->subsystem_device) + 1;
+	envp[i++] = scratch;
+	scratch += sprintf (scratch, "PCI_SLOT_NAME=%s", pdev->slot_name) + 1;
 	envp[i] = 0;
 
-	call_usermodehelper (argv [0], argv, envp);
+	/* check to see if we went over buffer? */
+	return 0;
 }
 #else
-static void run_sbin_hotplug(struct pci_dev *pdev, int insert) { }
+int pci_hotplug (struct device *dev, char **envp, int num_envp,
+		 char *buffer, int buffer_size)
+{
+	return -ENODEV;
+}
 #endif
 
 /**
@@ -66,8 +56,6 @@
 #ifdef CONFIG_PROC_FS
 	pci_proc_attach_device(dev);
 #endif
-	/* notify userspace of new hotplug device */
-	run_sbin_hotplug(dev, TRUE);
 }
 
 static void
@@ -99,8 +87,6 @@
 #ifdef CONFIG_PROC_FS
 	pci_proc_detach_device(dev);
 #endif
-	/* notify userspace of hotplug device removal */
-	run_sbin_hotplug(dev, FALSE);
 }
 
 #ifdef CONFIG_HOTPLUG
diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	Wed Sep 25 14:22:28 2002
+++ b/drivers/pci/pci-driver.c	Wed Sep 25 14:22:28 2002
@@ -6,6 +6,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include "pci.h"
 
 /*
  *  Registration of PCI drivers and handling of hot-pluggable devices.
@@ -199,8 +200,9 @@
 }
 
 struct bus_type pci_bus_type = {
-	name:	"pci",
-	match:	pci_bus_match,
+	name:		"pci",
+	match:		pci_bus_match,
+	hotplug:	pci_hotplug,
 };
 
 static int __init pci_driver_init(void)
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/pci/pci.h	Wed Sep 25 14:22:28 2002
@@ -0,0 +1,5 @@
+/* Functions internal to the PCI core code */
+
+extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
+			 char *buffer, int buffer_size);
+
diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c	Wed Sep 25 14:22:28 2002
+++ b/drivers/usb/core/usb.c	Wed Sep 25 14:22:28 2002
@@ -505,57 +505,41 @@
  * cases, we know no other thread can recycle our address, since we must
  * already have been serialized enough to prevent that.
  */
-static void call_policy (char *verb, struct usb_device *dev)
+static int usb_hotplug (struct device *dev, char **envp, int num_envp,
+			char *buffer, int buffer_size)
 {
-	char *argv [3], **envp, *buf, *scratch;
-	int i = 0, value;
+	struct usb_interface *intf;
+	struct usb_device *usb_dev;
+	char *scratch;
+	int i = 0;
 
-	if (!hotplug_path [0])
-		return;
-	if (in_interrupt ()) {
-		dbg ("In_interrupt");
-		return;
-	}
-	if (!current->fs->root) {
-		/* statically linked USB is initted rather early */
-		dbg ("call_policy %s, num %d -- no FS yet", verb, dev->devnum);
-		return;
-	}
-	if (dev->devnum < 0) {
+	dbg ("%s", __FUNCTION__);
+
+	if (!dev)
+		return -ENODEV;
+
+	/* check for generic driver, we do not call do hotplug calls for it */
+	if (dev->driver == &usb_generic_driver)
+		return -ENODEV;
+
+	intf = to_usb_interface(dev);
+	if (!intf)
+		return -ENODEV;
+
+	usb_dev = interface_to_usbdev (intf);
+	if (!usb_dev)
+		return -ENODEV;
+	
+	if (usb_dev->devnum < 0) {
 		dbg ("device already deleted ??");
-		return;
+		return -ENODEV;
 	}
-	if (!(envp = (char **) kmalloc (20 * sizeof (char *), GFP_KERNEL))) {
-		dbg ("enomem");
-		return;
-	}
-	if (!(buf = kmalloc (256, GFP_KERNEL))) {
-		kfree (envp);
-		dbg ("enomem2");
-		return;
+	if (!usb_dev->bus) {
+		dbg ("bus already removed?");
+		return -ENODEV;
 	}
 
-	/* only one standardized param to hotplug command: type */
-	argv [0] = hotplug_path;
-	argv [1] = "usb";
-	argv [2] = 0;
-
-	/* minimal command environment */
-	envp [i++] = "HOME=/";
-	envp [i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
-
-#ifdef	DEBUG
-	/* hint that policy agent should enter no-stdout debug mode */
-	envp [i++] = "DEBUG=kernel";
-#endif
-	/* extensible set of named bus-specific parameters,
-	 * supporting multiple driver selection algorithms.
-	 */
-	scratch = buf;
-
-	/* action:  add, remove */
-	envp [i++] = scratch;
-	scratch += sprintf (scratch, "ACTION=%s", verb) + 1;
+	scratch = buffer;
 
 #ifdef	CONFIG_USB_DEVICEFS
 	/* If this is available, userspace programs can directly read
@@ -565,26 +549,35 @@
 	 * FIXME reduce hardwired intelligence here
 	 */
 	envp [i++] = "DEVFS=/proc/bus/usb";
+	if (i >= num_envp)
+		return -ENOMEM;
+
 	envp [i++] = scratch;
 	scratch += sprintf (scratch, "DEVICE=/proc/bus/usb/%03d/%03d",
-		dev->bus->busnum, dev->devnum) + 1;
+		usb_dev->bus->busnum, usb_dev->devnum) + 1;
+	if (i >= num_envp)
+		return -ENOMEM;
 #endif
-
 	/* per-device configuration hacks are common */
 	envp [i++] = scratch;
 	scratch += sprintf (scratch, "PRODUCT=%x/%x/%x",
-		dev->descriptor.idVendor,
-		dev->descriptor.idProduct,
-		dev->descriptor.bcdDevice) + 1;
+		usb_dev->descriptor.idVendor,
+		usb_dev->descriptor.idProduct,
+		usb_dev->descriptor.bcdDevice) + 1;
+	if (i >= num_envp)
+		return -ENOMEM;
 
 	/* class-based driver binding models */
 	envp [i++] = scratch;
 	scratch += sprintf (scratch, "TYPE=%d/%d/%d",
-			    dev->descriptor.bDeviceClass,
-			    dev->descriptor.bDeviceSubClass,
-			    dev->descriptor.bDeviceProtocol) + 1;
-	if (dev->descriptor.bDeviceClass == 0) {
-		int alt = dev->actconfig->interface [0].act_altsetting;
+			    usb_dev->descriptor.bDeviceClass,
+			    usb_dev->descriptor.bDeviceSubClass,
+			    usb_dev->descriptor.bDeviceProtocol) + 1;
+	if (i >= num_envp)
+		return -ENOMEM;
+
+	if (usb_dev->descriptor.bDeviceClass == 0) {
+		int alt = intf->act_altsetting;
 
 		/* a simple/common case: one config, one interface, one driver
 		 * with current altsetting being a reasonable setting.
@@ -593,30 +586,27 @@
 		 */
 		envp [i++] = scratch;
 		scratch += sprintf (scratch, "INTERFACE=%d/%d/%d",
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceClass,
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceSubClass,
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceProtocol)
+			intf->altsetting[alt].bInterfaceClass,
+			intf->altsetting[alt].bInterfaceSubClass,
+			intf->altsetting[alt].bInterfaceProtocol)
 			+ 1;
-		/* INTERFACE-0, INTERFACE-1, ... ? */
+		if (i >= num_envp)
+			return -ENOMEM;
+
 	}
 	envp [i++] = 0;
-	/* assert: (scratch - buf) < sizeof buf */
+	/* assert: (scratch - buffer) < buffer_size */
 
-	/* NOTE: user mode daemons can call the agents too */
-
-	dbg ("kusbd: %s %s %d", argv [0], verb, dev->devnum);
-	value = call_usermodehelper (argv [0], argv, envp);
-	kfree (buf);
-	kfree (envp);
-	if (value != 0)
-		dbg ("kusbd policy returned 0x%x", value);
+	return 0;
 }
 
 #else
 
-static inline void
-call_policy (char *verb, struct usb_device *dev)
-{ } 
+static int usb_hotplug (struct device *dev, char **envp,
+			char *buffer, int buffer_size)
+{
+	return -ENODEV;
+}
 
 #endif	/* CONFIG_HOTPLUG */
 
@@ -889,9 +879,6 @@
 		put_device(&dev->dev);
 	}
 
-	/* Let policy agent unload modules etc */
-	call_policy ("remove", dev);
-
 	/* Decrement the reference count, it'll auto free everything when */
 	/* it hits 0 which could very well be now */
 	usb_put_dev(dev);
@@ -1169,9 +1156,6 @@
 	/* add a /proc/bus/usb entry */
 	usbfs_add_device(dev);
 
-	/* userspace may load modules and/or configure further */
-	call_policy ("add", dev);
-
 	return 0;
 }
 
@@ -1434,6 +1418,7 @@
 struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
+	.hotplug =	usb_hotplug,
 };
 
 /*
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Wed Sep 25 14:22:28 2002
+++ b/include/linux/device.h	Wed Sep 25 14:22:28 2002
@@ -67,6 +67,8 @@
 
 	int		(*match)(struct device * dev, struct device_driver * drv);
 	struct device * (*add)	(struct device * parent, char * bus_id);
+	int		(*hotplug) (struct device *dev, char **envp, 
+				    int num_envp, char *buffer, int buffer_size);
 };
 
 

Re: [RFC] consolidate /sbin/hotplug call for pci and usb

[Kai Germaschewski]

On Wed, 25 Sep 2002, Greg KH wrote:

> diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
> --- a/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
> +++ b/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
> @@ -5,6 +5,10 @@
>  
>  obj-y		+= fs/
>  
> +ifeq ($(CONFIG_HOTPLUG),y)
> +	obj-y	+= hotplug.o
> +endif
> +
>  export-objs	:= core.o power.o sys.o bus.o driver.o \
>  			class.o intf.o
>  

Why not the simpler

obj-$(CONFIG_HOTPLUG)	+= hotplug.o

?

--Kai

Re: [RFC] consolidate /sbin/hotplug call for pci and usb

[Greg KH]

On Wed, Sep 25, 2002 at 05:04:25PM -0500, Kai Germaschewski wrote:
> On Wed, 25 Sep 2002, Greg KH wrote:
> 
> > diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
> > --- a/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
> > +++ b/drivers/base/Makefile	Wed Sep 25 14:22:28 2002
> > @@ -5,6 +5,10 @@
> >  
> >  obj-y		+= fs/
> >  
> > +ifeq ($(CONFIG_HOTPLUG),y)
> > +	obj-y	+= hotplug.o
> > +endif
> > +
> >  export-objs	:= core.o power.o sys.o bus.o driver.o \
> >  			class.o intf.o
> >  
> 
> Why not the simpler
> 
> obj-$(CONFIG_HOTPLUG)	+= hotplug.o

Thanks, Pat also pointed this out to me.  
I'll fix it.

thanks,

greg k-h

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[David Brownell]

> +	/* stuff we want to pass to /sbin/hotplug */
> +	envp[i++] = scratch;
> +	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
> +
> +	envp[i++] = scratch;
> +	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
> +			    pdev->vendor, pdev->device) + 1;

And so forth.  Use "snprintf" and prevent overrunning those buffers...

- Dave

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Greg KH]

On Wed, Sep 25, 2002 at 05:11:57PM -0700, David Brownell wrote:
> 
> >+	/* stuff we want to pass to /sbin/hotplug */
> >+	envp[i++] = scratch;
> >+	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
> >+
> >+	envp[i++] = scratch;
> >+	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
> >+			    pdev->vendor, pdev->device) + 1;
> 
> And so forth.  Use "snprintf" and prevent overrunning those buffers...

Doh, will do.

I also found the unload USB module problem.  The driver core was calling
hotplug after the device was already removed.  Made it a bit difficult
to be able to describe the device that way :)

thanks,

greg k-h

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[David Brownell]

> I also found the unload USB module problem.  The driver core was calling
> hotplug after the device was already removed.  Made it a bit difficult
> to be able to describe the device that way :)

On the other hand, since there's no internal "this is an ex-device"
state, that's also insurance that nothing could use "usbfs" to try
to re-activate the device.  I seem to recall oopses going away by
reporting the hotplug "remove" events after the usbfs path could
no longer be used.  Not that I ever liked that consequence, but
a fix adding such a "zombie" state would have taken a bit of time.

The real "module unload problem" has a lot to do with not having any
way to track how many devices a module is bound to ... that aren't
necessarily opened at the moment.  (Does Rusty's patch set touch
any of that?)

Without having a way to answer that question, today's un-helpful
"driver is in active use" refcount would encourage rmmodding drivers
that users will expect to still be available.  Plug in two devices,
look at one, decide to use the other, unplug the first ... and just
because you hadn't yet opened the second device, its driver module
vanishes.  As you start to use it ... huge frustration quotient! :)

- Dave

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Greg KH]

On Wed, Sep 25, 2002 at 07:44:47PM -0700, David Brownell wrote:
> 
> >I also found the unload USB module problem.  The driver core was calling
> >hotplug after the device was already removed.  Made it a bit difficult
> >to be able to describe the device that way :)
> 
> On the other hand, since there's no internal "this is an ex-device"
> state, that's also insurance that nothing could use "usbfs" to try
> to re-activate the device.  I seem to recall oopses going away by
> reporting the hotplug "remove" events after the usbfs path could
> no longer be used.  Not that I ever liked that consequence, but
> a fix adding such a "zombie" state would have taken a bit of time.

Yes, Pat and I have talked a lot about the need for a driver "state".  I
think the current goal was to see how far we can get without needing it.
I was certainly cursing the lack of it today when trying to debug this
problem, but in the end, having it would have only masked over the
real problem that was there.

So personally, I keep going back and forth on if it is really necessary
or not to have.  Right now, the USB drivers and developers are very used
to the fact that when the device disappears from the system, they can
not access it anymore, and that this needs to be constantly checked.  I
think that as time goes on, and more subsystems become "hot-pluggable"
either this paranoia will have to spread to the other subsystems, or we
will have to create the notion of a device "state" to make things
easier on everyone.

> The real "module unload problem" has a lot to do with not having any
> way to track how many devices a module is bound to ... that aren't
> necessarily opened at the moment.  (Does Rusty's patch set touch
> any of that?)

As far as I know, it doesn't, but I'm not sure.

> Without having a way to answer that question, today's un-helpful
> "driver is in active use" refcount would encourage rmmodding drivers
> that users will expect to still be available.  Plug in two devices,
> look at one, decide to use the other, unplug the first ... and just
> because you hadn't yet opened the second device, its driver module
> vanishes.  As you start to use it ... huge frustration quotient! :)

Well, that's a driver unload issue, which I think everyone agrees on the
fact that it's not ok to do automatic driver unload when a device is
removed, because of this very problem.

thanks,

greg k-h

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[David Brownell]

> Yes, Pat and I have talked a lot about the need for a driver "state".  I
> think the current goal was to see how far we can get without needing it.
> I was certainly cursing the lack of it today when trying to debug this
> problem, but in the end, having it would have only masked over the
> real problem that was there.

It'd actually be a "device state", not a "driver state" ...

And I suspect that the specific usbfs issue got fixed by that patch
to make its disconnect() method actually do its job, earlier this
year.  If there's an open issue, I'd likely characterize it as needing
to trust device drivers to disconnect() correctly ... without having
good ways to verify (later on) they really did so.  Unfortunately
that's an area where we know drivers like to make mistakes.



>>Without having a way to answer that question, today's un-helpful
>>"driver is in active use" refcount would encourage rmmodding drivers
>>that users will expect to still be available.  Plug in two devices,
>>look at one, decide to use the other, unplug the first ... and just
>>because you hadn't yet opened the second device, its driver module
>>vanishes.  As you start to use it ... huge frustration quotient! :)
> 
> 
> Well, that's a driver unload issue, which I think everyone agrees on the
> fact that it's not ok to do automatic driver unload when a device is
> removed, because of this very problem.

I think it _could_ be fine to do such rmmods, if all the module
remove races were removed ... and (for this issue) if the primitve
were actually "remove if the driver is not (a) in active use, or
(b) bound to any device".  Today we have races and (a) ... but it's
the lack of (b) that prevents hotplug from even trying to rmmod,
on the optimistic assumption there are no races.

- Dave

[RFC] consolidate /sbin/hotplug call for pci and usb - take 2

[Greg KH]

Ok, here's an updated version of the previous patch I sent out, that
address all of the different comments that I received.  It also has the
previously mentioned bug fixed.  It is against a clean 2.5.38 kernel.

I changed the sprintf() calls to snprintf() and if people would check my
math, I would appreciate it :)

This works for me on both PCI Hotplug and USB systems.  If there are no
complaints about it, I'll send it on to Linus.

Again, comments appreciated.

thanks,

greg k-h


diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
--- a/drivers/base/Makefile	Thu Sep 26 10:41:08 2002
+++ b/drivers/base/Makefile	Thu Sep 26 10:41:08 2002
@@ -5,6 +5,8 @@
 
 obj-y		+= fs/
 
+obj-$(CONFIG_HOTPLUG)	+= hotplug.o
+
 export-objs	:= core.o power.o sys.o bus.o driver.o \
 			class.o intf.o
 
diff -Nru a/drivers/base/base.h b/drivers/base/base.h
--- a/drivers/base/base.h	Thu Sep 26 10:41:08 2002
+++ b/drivers/base/base.h	Thu Sep 26 10:41:08 2002
@@ -50,3 +50,13 @@
 
 extern int driver_attach(struct device_driver * drv);
 extern void driver_detach(struct device_driver * drv);
+
+#ifdef CONFIG_HOTPLUG
+extern int dev_hotplug(struct device *dev, const char *action);
+#else
+static inline int dev_hotplug(struct device *dev, const char *action)
+{
+	return 0;
+}
+#endif
+
diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Thu Sep 26 10:41:08 2002
+++ b/drivers/base/core.c	Thu Sep 26 10:41:08 2002
@@ -198,6 +198,9 @@
 	if (platform_notify)
 		platform_notify(dev);
 
+	/* notify userspace of device entry */
+	dev_hotplug(dev, "add");
+
  register_done:
 	if (error) {
 		spin_lock(&device_lock);
@@ -252,6 +255,9 @@
 	 */
 	if (platform_notify_remove)
 		platform_notify_remove(dev);
+
+	/* notify userspace that this device is about to disappear */
+	dev_hotplug (dev, "remove");
 
 	device_detach(dev);
 	bus_remove_device(dev);
diff -Nru a/drivers/base/hotplug.c b/drivers/base/hotplug.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/base/hotplug.c	Thu Sep 26 10:41:08 2002
@@ -0,0 +1,98 @@
+/*
+ * drivers/base/hotplug.c - hotplug call code
+ * 
+ * Copyright (c) 2002 Greg Kroah-Hartman <greg@kroah.com>
+ *		 2002 IBM Corp.
+ */
+
+#define DEBUG 0
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/kmod.h>
+#include <linux/interrupt.h>
+#include "base.h"
+
+/*
+ * hotplugging invokes what /proc/sys/kernel/hotplug says (normally
+ * /sbin/hotplug) when devices get added or removed.
+ *
+ * This invokes a user mode policy agent, typically helping to load driver
+ * or other modules, configure the device, and more.  Drivers can provide
+ * a MODULE_DEVICE_TABLE to help with module loading subtasks.
+ */
+#define BUFFER_SIZE	1024	/* should be enough memory for the env */
+#define NUM_ENVP	32	/* number of env pointers */
+int dev_hotplug (struct device *dev, const char *action)
+{
+	char *argv [3], **envp, *buffer, *scratch;
+	int retval;
+	int i = 0;
+
+	pr_debug ("%s\n", __FUNCTION__);
+	if (!dev)
+		return -ENODEV;
+
+	if (!dev->bus || !dev->bus->hotplug)
+		return -ENODEV;
+
+	if (!hotplug_path [0])
+		return -ENODEV;
+
+	if (in_interrupt ()) {
+		pr_debug ("%s - in_interrupt, not allowed!", __FUNCTION__);
+		return -EIO;
+	}
+
+	if (!current->fs->root) {
+		/* don't try to do anything unless we have a root partition */
+		pr_debug ("%s - %s -- no FS yet\n", __FUNCTION__, action);
+		return -EIO;
+	}
+
+	envp = (char **) kmalloc (NUM_ENVP * sizeof (char *), GFP_KERNEL);
+	if (!envp)
+		return -ENOMEM;
+
+	buffer = kmalloc (BUFFER_SIZE, GFP_KERNEL);
+	if (!buffer) {
+		kfree (envp);
+		return -ENOMEM;
+	}
+
+	/* only one standardized param to hotplug command: the bus name */
+	argv [0] = hotplug_path;
+	argv [1] = dev->bus->name;
+	argv [2] = 0;
+
+	/* minimal command environment */
+	envp [i++] = "HOME=/";
+	envp [i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
+
+	scratch = buffer;
+
+	/* action:  add, remove */
+	envp [i++] = scratch;
+	scratch += sprintf (scratch, "ACTION=%s", action) + 1;
+
+	/* have the bus specific function set up the rest of the environment */
+	retval = dev->bus->hotplug (dev, &envp[i], NUM_ENVP - i,
+				    scratch, BUFFER_SIZE - (scratch - buffer));
+	if (retval) {
+		pr_debug ("%s - hotplug() returned %d\n", __FUNCTION__, retval);
+		goto exit;
+	}
+
+	pr_debug ("%s: %s %s %s %s %s %s\n", __FUNCTION__, argv [0], argv[1],
+		  action, envp[0], envp[1], envp[2]);
+	retval = call_usermodehelper (argv [0], argv, envp);
+	if (retval)
+		pr_debug ("%s - call_usermodehelper returned %d\n",
+			  __FUNCTION__, retval);
+
+exit:
+	kfree (buffer);
+	kfree (envp);
+	return retval;
+}
diff -Nru a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c
--- a/drivers/pci/hotplug.c	Thu Sep 26 10:41:08 2002
+++ b/drivers/pci/hotplug.c	Thu Sep 26 10:41:08 2002
@@ -1,52 +1,68 @@
 #include <linux/pci.h>
 #include <linux/module.h>
-#include <linux/kmod.h>		/* for hotplug_path */
+#include "pci.h"
 
-#ifndef FALSE
-#define FALSE	(0)
-#define TRUE	(!FALSE)
-#endif
 
 #ifdef CONFIG_HOTPLUG
-static void run_sbin_hotplug(struct pci_dev *pdev, int insert)
+int pci_hotplug (struct device *dev, char **envp, int num_envp,
+		 char *buffer, int buffer_size)
 {
-	int i;
-	char *argv[3], *envp[8];
-	char id[20], sub_id[24], bus_id[24], class_id[20];
-
-	if (!hotplug_path[0])
-		return;
+	struct pci_dev *pdev;
+	char *scratch;
+	int i = 0;
+	int length = 0;
+
+	if (!dev)
+		return -ENODEV;
+
+	pdev = to_pci_dev(dev);
+	if (!pdev)
+		return -ENODEV;
+
+	scratch = buffer;
+
+	/* stuff we want to pass to /sbin/hotplug */
+	envp[i++] = scratch;
+	length += snprintf (scratch, buffer_size - length, "PCI_CLASS=%04X",
+			    pdev->class);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
+
+	envp[i++] = scratch;
+	length += snprintf (scratch, buffer_size - length, "PCI_ID=%04X:%04X",
+			    pdev->vendor, pdev->device);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
+
+	envp[i++] = scratch;
+	length += snprintf (scratch, buffer_size - length,
+			    "PCI_SUBSYS_ID=%04X:%04X", pdev->subsystem_vendor,
+			    pdev->subsystem_device);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
+
+	envp[i++] = scratch;
+	length += snprintf (scratch, buffer_size - length, "PCI_SLOT_NAME=%s",
+			    pdev->slot_name);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
 
-	sprintf(class_id, "PCI_CLASS=%04X", pdev->class);
-	sprintf(id, "PCI_ID=%04X:%04X", pdev->vendor, pdev->device);
-	sprintf(sub_id, "PCI_SUBSYS_ID=%04X:%04X", pdev->subsystem_vendor, pdev->subsystem_device);
-	sprintf(bus_id, "PCI_SLOT_NAME=%s", pdev->slot_name);
-
-	i = 0;
-	argv[i++] = hotplug_path;
-	argv[i++] = "pci";
-	argv[i] = 0;
-
-	i = 0;
-	/* minimal command environment */
-	envp[i++] = "HOME=/";
-	envp[i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
-	
-	/* other stuff we want to pass to /sbin/hotplug */
-	envp[i++] = class_id;
-	envp[i++] = id;
-	envp[i++] = sub_id;
-	envp[i++] = bus_id;
-	if (insert)
-		envp[i++] = "ACTION=add";
-	else
-		envp[i++] = "ACTION=remove";
 	envp[i] = 0;
 
-	call_usermodehelper (argv [0], argv, envp);
+	return 0;
 }
 #else
-static void run_sbin_hotplug(struct pci_dev *pdev, int insert) { }
+int pci_hotplug (struct device *dev, char **envp, int num_envp,
+		 char *buffer, int buffer_size)
+{
+	return -ENODEV;
+}
 #endif
 
 /**
@@ -66,8 +82,6 @@
 #ifdef CONFIG_PROC_FS
 	pci_proc_attach_device(dev);
 #endif
-	/* notify userspace of new hotplug device */
-	run_sbin_hotplug(dev, TRUE);
 }
 
 static void
@@ -99,8 +113,6 @@
 #ifdef CONFIG_PROC_FS
 	pci_proc_detach_device(dev);
 #endif
-	/* notify userspace of hotplug device removal */
-	run_sbin_hotplug(dev, FALSE);
 }
 
 #ifdef CONFIG_HOTPLUG
diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	Thu Sep 26 10:41:08 2002
+++ b/drivers/pci/pci-driver.c	Thu Sep 26 10:41:08 2002
@@ -6,6 +6,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include "pci.h"
 
 /*
  *  Registration of PCI drivers and handling of hot-pluggable devices.
@@ -199,8 +200,9 @@
 }
 
 struct bus_type pci_bus_type = {
-	name:	"pci",
-	match:	pci_bus_match,
+	name:		"pci",
+	match:		pci_bus_match,
+	hotplug:	pci_hotplug,
 };
 
 static int __init pci_driver_init(void)
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/pci/pci.h	Thu Sep 26 10:41:08 2002
@@ -0,0 +1,5 @@
+/* Functions internal to the PCI core code */
+
+extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
+			 char *buffer, int buffer_size);
+
diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c	Thu Sep 26 10:41:08 2002
+++ b/drivers/usb/core/usb.c	Thu Sep 26 10:41:08 2002
@@ -505,57 +505,42 @@
  * cases, we know no other thread can recycle our address, since we must
  * already have been serialized enough to prevent that.
  */
-static void call_policy (char *verb, struct usb_device *dev)
+static int usb_hotplug (struct device *dev, char **envp, int num_envp,
+			char *buffer, int buffer_size)
 {
-	char *argv [3], **envp, *buf, *scratch;
-	int i = 0, value;
+	struct usb_interface *intf;
+	struct usb_device *usb_dev;
+	char *scratch;
+	int i = 0;
+	int length = 0;
 
-	if (!hotplug_path [0])
-		return;
-	if (in_interrupt ()) {
-		dbg ("In_interrupt");
-		return;
-	}
-	if (!current->fs->root) {
-		/* statically linked USB is initted rather early */
-		dbg ("call_policy %s, num %d -- no FS yet", verb, dev->devnum);
-		return;
-	}
-	if (dev->devnum < 0) {
+	dbg ("%s", __FUNCTION__);
+
+	if (!dev)
+		return -ENODEV;
+
+	/* check for generic driver, we do not call do hotplug calls for it */
+	if (dev->driver == &usb_generic_driver)
+		return -ENODEV;
+
+	intf = to_usb_interface(dev);
+	if (!intf)
+		return -ENODEV;
+
+	usb_dev = interface_to_usbdev (intf);
+	if (!usb_dev)
+		return -ENODEV;
+	
+	if (usb_dev->devnum < 0) {
 		dbg ("device already deleted ??");
-		return;
+		return -ENODEV;
 	}
-	if (!(envp = (char **) kmalloc (20 * sizeof (char *), GFP_KERNEL))) {
-		dbg ("enomem");
-		return;
-	}
-	if (!(buf = kmalloc (256, GFP_KERNEL))) {
-		kfree (envp);
-		dbg ("enomem2");
-		return;
+	if (!usb_dev->bus) {
+		dbg ("bus already removed?");
+		return -ENODEV;
 	}
 
-	/* only one standardized param to hotplug command: type */
-	argv [0] = hotplug_path;
-	argv [1] = "usb";
-	argv [2] = 0;
-
-	/* minimal command environment */
-	envp [i++] = "HOME=/";
-	envp [i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
-
-#ifdef	DEBUG
-	/* hint that policy agent should enter no-stdout debug mode */
-	envp [i++] = "DEBUG=kernel";
-#endif
-	/* extensible set of named bus-specific parameters,
-	 * supporting multiple driver selection algorithms.
-	 */
-	scratch = buf;
-
-	/* action:  add, remove */
-	envp [i++] = scratch;
-	scratch += sprintf (scratch, "ACTION=%s", verb) + 1;
+	scratch = buffer;
 
 #ifdef	CONFIG_USB_DEVICEFS
 	/* If this is available, userspace programs can directly read
@@ -564,27 +549,48 @@
 	 *
 	 * FIXME reduce hardwired intelligence here
 	 */
-	envp [i++] = "DEVFS=/proc/bus/usb";
 	envp [i++] = scratch;
-	scratch += sprintf (scratch, "DEVICE=/proc/bus/usb/%03d/%03d",
-		dev->bus->busnum, dev->devnum) + 1;
+	length += snprintf (scratch, buffer_size - length,
+			    "%s", "DEVFS=/proc/bus/usb");
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
+
+	envp [i++] = scratch;
+	length += snprintf (scratch, buffer_size - length,
+			    "DEVICE=/proc/bus/usb/%03d/%03d",
+			    usb_dev->bus->busnum, usb_dev->devnum);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
 #endif
 
 	/* per-device configuration hacks are common */
 	envp [i++] = scratch;
-	scratch += sprintf (scratch, "PRODUCT=%x/%x/%x",
-		dev->descriptor.idVendor,
-		dev->descriptor.idProduct,
-		dev->descriptor.bcdDevice) + 1;
+	length += snprintf (scratch, buffer_size - length, "PRODUCT=%x/%x/%x",
+			    usb_dev->descriptor.idVendor,
+			    usb_dev->descriptor.idProduct,
+			    usb_dev->descriptor.bcdDevice);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
 
 	/* class-based driver binding models */
 	envp [i++] = scratch;
-	scratch += sprintf (scratch, "TYPE=%d/%d/%d",
-			    dev->descriptor.bDeviceClass,
-			    dev->descriptor.bDeviceSubClass,
-			    dev->descriptor.bDeviceProtocol) + 1;
-	if (dev->descriptor.bDeviceClass == 0) {
-		int alt = dev->actconfig->interface [0].act_altsetting;
+	length += snprintf (scratch, buffer_size - length, "TYPE=%d/%d/%d",
+			    usb_dev->descriptor.bDeviceClass,
+			    usb_dev->descriptor.bDeviceSubClass,
+			    usb_dev->descriptor.bDeviceProtocol);
+	if ((buffer_size - length <= 0) || (i >= num_envp))
+		return -ENOMEM;
+	++length;
+	scratch += length;
+
+	if (usb_dev->descriptor.bDeviceClass == 0) {
+		int alt = intf->act_altsetting;
 
 		/* a simple/common case: one config, one interface, one driver
 		 * with current altsetting being a reasonable setting.
@@ -592,31 +598,29 @@
 		 * device-specific binding policies.
 		 */
 		envp [i++] = scratch;
-		scratch += sprintf (scratch, "INTERFACE=%d/%d/%d",
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceClass,
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceSubClass,
-			dev->actconfig->interface [0].altsetting [alt].bInterfaceProtocol)
-			+ 1;
-		/* INTERFACE-0, INTERFACE-1, ... ? */
+		length += snprintf (scratch, buffer_size - length,
+				    "INTERFACE=%d/%d/%d",
+				    intf->altsetting[alt].bInterfaceClass,
+				    intf->altsetting[alt].bInterfaceSubClass,
+				    intf->altsetting[alt].bInterfaceProtocol);
+		if ((buffer_size - length <= 0) || (i >= num_envp))
+			return -ENOMEM;
+		++length;
+		scratch += length;
+
 	}
 	envp [i++] = 0;
-	/* assert: (scratch - buf) < sizeof buf */
 
-	/* NOTE: user mode daemons can call the agents too */
-
-	dbg ("kusbd: %s %s %d", argv [0], verb, dev->devnum);
-	value = call_usermodehelper (argv [0], argv, envp);
-	kfree (buf);
-	kfree (envp);
-	if (value != 0)
-		dbg ("kusbd policy returned 0x%x", value);
+	return 0;
 }
 
 #else
 
-static inline void
-call_policy (char *verb, struct usb_device *dev)
-{ } 
+static int usb_hotplug (struct device *dev, char **envp,
+			char *buffer, int buffer_size)
+{
+	return -ENODEV;
+}
 
 #endif	/* CONFIG_HOTPLUG */
 
@@ -889,9 +893,6 @@
 		put_device(&dev->dev);
 	}
 
-	/* Let policy agent unload modules etc */
-	call_policy ("remove", dev);
-
 	/* Decrement the reference count, it'll auto free everything when */
 	/* it hits 0 which could very well be now */
 	usb_put_dev(dev);
@@ -1169,9 +1170,6 @@
 	/* add a /proc/bus/usb entry */
 	usbfs_add_device(dev);
 
-	/* userspace may load modules and/or configure further */
-	call_policy ("add", dev);
-
 	return 0;
 }
 
@@ -1434,6 +1432,7 @@
 struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
+	.hotplug =	usb_hotplug,
 };
 
 /*
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Thu Sep 26 10:41:08 2002
+++ b/include/linux/device.h	Thu Sep 26 10:41:08 2002
@@ -67,6 +67,8 @@
 
 	int		(*match)(struct device * dev, struct device_driver * drv);
 	struct device * (*add)	(struct device * parent, char * bus_id);
+	int		(*hotplug) (struct device *dev, char **envp, 
+				    int num_envp, char *buffer, int buffer_size);
 };
 
 

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Greg KH]

On Thu, Sep 26, 2002 at 09:14:48AM -0700, David Brownell wrote:
> 
> >Yes, Pat and I have talked a lot about the need for a driver "state".  I
> >think the current goal was to see how far we can get without needing it.
> >I was certainly cursing the lack of it today when trying to debug this
> >problem, but in the end, having it would have only masked over the
> >real problem that was there.
> 
> It'd actually be a "device state", not a "driver state" ...

Doh, yes, that's what I meant, sorry.

> >Well, that's a driver unload issue, which I think everyone agrees on the
> >fact that it's not ok to do automatic driver unload when a device is
> >removed, because of this very problem.
> 
> I think it _could_ be fine to do such rmmods, if all the module
> remove races were removed ... and (for this issue) if the primitve
> were actually "remove if the driver is not (a) in active use, or
> (b) bound to any device".  Today we have races and (a) ... but it's
> the lack of (b) that prevents hotplug from even trying to rmmod,
> on the optimistic assumption there are no races.

But how do we accomplish (b) for devices that we can't remove from the
system?  Like 99.9% of the pci systems?

I agree it would be "nice", but probably never realistic :)

thanks,

greg k-h

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[David Brownell]

>>>Well, that's a driver unload issue, which I think everyone agrees on the
>>>fact that it's not ok to do automatic driver unload when a device is
>>>removed, because of this very problem.
>>
>>I think it _could_ be fine to do such rmmods, if all the module
>>remove races were removed ... and (for this issue) if the primitve
>>were actually "remove if the driver is not (a) in active use, or
>>(b) bound to any device".  Today we have races and (a) ... but it's
>>the lack of (b) that prevents hotplug from even trying to rmmod,
>>on the optimistic assumption there are no races.
> 
> 
> But how do we accomplish (b) for devices that we can't remove from the
> system?  Like 99.9% of the pci systems?
> 
> I agree it would be "nice", but probably never realistic :)

There would be two modes for 'rmmod'.  One is the "remove if the
kernel can tell it can't be used" (as described above), suitable
for automation.  The other gives the rude end-user failure modes:
"remove this module if it's not in active use", which is all we
have today ... a mode suitable only for developers or sysadmins.

Userland needs to make the policy choice, then tell the kernel
whether to ignore any unopened (but bound) devices.

Highly realistic, IMO.  It's just a question of ensuring some data
the kernel already holds (N devices are bound to this driver, even
if none are currently opened) is considered by rmmod.  I almost put
together a patch for it at one time.  There seems to be a change
in maintainer under way, maybe it'd be worth revisiting this.

- Dave

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Alan Stern]

On Thu, 26 Sep 2002, Greg KH wrote:

> On Thu, Sep 26, 2002 at 09:14:48AM -0700, David Brownell wrote:
> > I think it _could_ be fine to do such rmmods, if all the module
> > remove races were removed ... and (for this issue) if the primitve
> > were actually "remove if the driver is not (a) in active use, or
> > (b) bound to any device".  Today we have races and (a) ... but it's
> > the lack of (b) that prevents hotplug from even trying to rmmod,
> > on the optimistic assumption there are no races.
>
> But how do we accomplish (b) for devices that we can't remove from the
> system?  Like 99.9% of the pci systems?
>
> I agree it would be "nice", but probably never realistic :)


This raises a generally interesting question:  When should a driver module
be loaded?

Should it be there all the time? -- usually not.  Or only when the device
is in active use?  Also at detection (hotplug) and removal time?  As long
as the device is installed?  What if the device is built into the system
and can never be removed?  What if the device is not subject to
hotplugging itself but is required in order to operate another device that
can be hotplugged (e.g., a USB host controller driver)?

I think it's clear that the answer must depend on the particular driver,
and that no single scheme involving usage counts (or the equivalent) can
suffice for every situation.

Is there a way to let the kernel provide a variety of mechanisms and let
the device driver (or even the user) select which one gets used?

Alan Stern

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pciand usb

[Oliver Neukum]

> This raises a generally interesting question:  When should a driver
> module be loaded?

It is an interesting question, but it is not a kernel problem :-)

[..]
> I think it's clear that the answer must depend on the particular driver,
> and that no single scheme involving usage counts (or the equivalent) can
> suffice for every situation.
>
> Is there a way to let the kernel provide a variety of mechanisms and let
> the device driver (or even the user) select which one gets used?

That is more or less what is in place. There's a kernel aspect to the problem,
because the kernel needs to implement an unload if nothing is bound to method,
as user space cannot do that without a race.

	Regards
		Oliver

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[David Brownell]

>>>>+	envp[i++] = scratch;
>>>>+	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
>>>>+			    pdev->vendor, pdev->device) + 1;
>>>
>>>And so forth.  Use "snprintf" and prevent overrunning those buffers...
>>
>>Hmm? An %04X format is perfectly bounded.

Which was my thought when I originally wrote the code which has been
widely cut'n'pasted.  Safe enough at that moment, but it's now become
dangerous to leave that around as a copy/paste coding example.

Those code fragments are now being used in contexts that aren't
as controlled as the original:  the code _using_ the buffer is no
longer in charge of allocating it.  There's no longer any way to
guarantee that adding the next parameter to the environment isn't
going to overrun the available space.

Except by using "snprintf" and tracking how much space is left.

Easy enough to do, and that's a habit that IMO _everyone_ should
be getting into whenever they program in languages that permit
such buffer overflows.


> Technically, it isn't bounded.  The field will expand if the value exceeds
> 4 digits.  
> 
> However, these values can't do that.  At least not now.
> 
> But, as a good programming practice, snprintf should be used.  Heck, PCI
> 3.0 might use 32-bit vendor and device values, instead of 8 bit.  So, if
> nothing else, do it as insurance for the future.

And to help ensure that as people continue to copy/paste code from the
core hotpluggable systems, they won't break things when they add the
parameters needed to set up their new subsystem or class, using the
/sbin/hotplug mechanism.

- Dave

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Andi Kleen]

On Wed, Sep 25, 2002 at 05:46:12PM -0700, Matthew Dharm wrote:
> On Thu, Sep 26, 2002 at 02:33:50AM +0200, Andi Kleen wrote:
> > David Brownell <david-b@pacbell.net> writes:
> > 
> > > > +	/* stuff we want to pass to /sbin/hotplug */
> > > > +	envp[i++] = scratch;
> > > > +	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
> > > > +
> > > > +	envp[i++] = scratch;
> > > > +	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
> > > > +			    pdev->vendor, pdev->device) + 1;
> > > 
> > > And so forth.  Use "snprintf" and prevent overrunning those buffers...
> > 
> > Hmm? An %04X format is perfectly bounded.
> 
> Technically, it isn't bounded.  The field will expand if the value exceeds
> 4 digits.  

It is bounded to 8 characters on linux systems (where int is always 32bit)

-Andi

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Matthew Dharm]

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

On Thu, Sep 26, 2002 at 02:33:50AM +0200, Andi Kleen wrote:
> David Brownell <david-b@pacbell.net> writes:
> 
> > > +	/* stuff we want to pass to /sbin/hotplug */
> > > +	envp[i++] = scratch;
> > > +	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
> > > +
> > > +	envp[i++] = scratch;
> > > +	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
> > > +			    pdev->vendor, pdev->device) + 1;
> > 
> > And so forth.  Use "snprintf" and prevent overrunning those buffers...
> 
> Hmm? An %04X format is perfectly bounded.

Technically, it isn't bounded.  The field will expand if the value exceeds
4 digits.  

However, these values can't do that.  At least not now.

But, as a good programming practice, snprintf should be used.  Heck, PCI
3.0 might use 32-bit vendor and device values, instead of 8 bit.  So, if
nothing else, do it as insurance for the future.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pci and usb

[Andi Kleen]

David Brownell <david-b@pacbell.net> writes:

> > +	/* stuff we want to pass to /sbin/hotplug */
> > +	envp[i++] = scratch;
> > +	scratch += sprintf (scratch, "PCI_CLASS=%04X", pdev->class) + 1;
> > +
> > +	envp[i++] = scratch;
> > +	scratch += sprintf (scratch, "PCI_ID=%04X:%04X",
> > +			    pdev->vendor, pdev->device) + 1;
> 
> And so forth.  Use "snprintf" and prevent overrunning those buffers...

Hmm? An %04X format is perfectly bounded.

-Andi