On Fri, Aug 11, 2006 at 02:03:21PM -0400, Alan Stern wrote:
> On Fri, 11 Aug 2006, Greg KH wrote:
>
> > > This won't work right if an error occurs. The value you return to the
> > > caller of usb_new_device() reflects only errors in creating the new
> > > thread. It doesn't reflect any errors the thread itself may encounter.
> > >
> > > So the caller won't realize it if __usb_new_device() fails.
> >
> > Yes, I realise this, but the callers of usb_new_device() don't always do
> > something "useful" with that information anyway, so it's not a real loss
> > here :)
>
> Well, let's take a look at the callers. There are only two. One is
> hub_port_connect_change() in hub.c. Here's what it does:
>
> if (!status) {
> status = usb_new_device(udev);
> if (status) {
> spin_lock_irq(&device_state_lock);
> hdev->children[port1-1] = NULL;
> spin_unlock_irq(&device_state_lock);
> }
> }
>
> if (status)
> goto loop_disable;
>
> You could fold the assignment to hdev->children into __usb_new_device, but
> what about the conditional jump to loop_disable?
>
> The other caller is register_root_hub() in hcd.c. Here's what it does:
>
> retval = usb_new_device (usb_dev);
> if (retval) {
> dev_err (parent_dev, "can't register root hub for %s, %d\n",
> usb_dev->dev.bus_id, retval);
> }
> mutex_unlock(&usb_bus_list_lock);
>
> if (retval == 0) {
> spin_lock_irq (&hcd_root_hub_lock);
> hcd->rh_registered = 1;
> spin_unlock_irq (&hcd_root_hub_lock);
>
> /* Did the HC die before the root hub was registered? */
> if (hcd->state == HC_STATE_HALT)
> usb_hc_died (hcd); /* This time clean up */
> }
>
> return retval;
>
> Merging that into __usb_new_device will be hard, since hcd_root_hub_lock
> is private to hcd.c.
>
> Furthermore, there are serial dependencies here. What if
> register_root_hub apparently succeeds, but then the HCD quickly decides to
> unregister the root hub before the child thread has had a chance to
> register it?
Then the HCD is really messed up :)
> As I see it, there's no advantage in making register_root_hub run in a
> new thread. There _is_ an advantage in making the call from the hub
> driver run in a new thread, but there are also dangers. Just as one
> example, what's to prevent someone from doing "rmmod usbcore" while these
> sub-threads are still running?
Here's an updated version that prevents that from happening.
I'll look into moving this further up the call chain so that the root
hub stuff is not affected by this.
thanks,
greg k-h
-------------
From: Greg Kroah-Hartman <[EMAIL PROTECTED]>
Subject: USB: create a new thread for every USB device found during the probe
sequence
Might speed up some systems. If nothing else, a bad driver should not
take the whole USB subsystem down with it.
Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]>
---
drivers/usb/core/Kconfig | 14 ++++++++
drivers/usb/core/hub.c | 81 ++++++++++++++++++++++++++++++++++-------------
2 files changed, 73 insertions(+), 22 deletions(-)
--- gregkh-2.6.orig/drivers/usb/core/Kconfig
+++ gregkh-2.6/drivers/usb/core/Kconfig
@@ -72,6 +72,20 @@ config USB_SUSPEND
If you are unsure about this, say N here.
+config USB_MULTITHREAD_PROBE
+ bool "USB Multi-threaded probe (EXPERIMENTAL)"
+ depends on USB && EXPERIMENTAL
+ help
+ Say Y here if you want the USB core to spawn a new thread for
+ every USB device that is probed. This can cause a small speedup
+ in boot times on systems with a lot of different USB devices.
+
+ This option should be safe to enable, but if any odd probing
+ problems are found, please disable it, or dynamically turn it
+ off in the /sys/module/usbcore/parameters/multithread_probe
+ file
+
+ When in doubt, say N.
config USB_OTG
bool
--- gregkh-2.6.orig/drivers/usb/core/hub.c
+++ gregkh-2.6/drivers/usb/core/hub.c
@@ -45,6 +45,16 @@ static DECLARE_WAIT_QUEUE_HEAD(khubd_wai
static struct task_struct *khubd_task;
+/* multithreaded probe logic */
+static int multithread_probe =
+#ifdef CONFIG_USB_MULTITHREAD_PROBE
+ 1;
+#else
+ 0;
+#endif
+module_param(multithread_probe, bool, S_IRUGO);
+MODULE_PARM_DESC(multithread_probe, "Run each USB device probe in a new
thread");
+
/* cycle leds on hubs that aren't blinking for attention */
static int blinkenlights = 0;
module_param (blinkenlights, bool, S_IRUGO);
@@ -1185,29 +1195,17 @@ static inline void show_string(struct us
#include "otg_whitelist.h"
#endif
-/**
- * usb_new_device - perform initial device setup (usbcore-internal)
- * @udev: newly addressed device (in ADDRESS state)
- *
- * This is called with devices which have been enumerated, but not yet
- * configured. The device descriptor is available, but not descriptors
- * for any device configuration. The caller must have locked either
- * the parent hub (if udev is a normal device) or else the
- * usb_bus_list_lock (if udev is a root hub). The parent's pointer to
- * udev has already been installed, but udev is not yet visible through
- * sysfs or other filesystem code.
- *
- * Returns 0 for success (device is configured and listed, with its
- * interfaces, in sysfs); else a negative errno value.
- *
- * This call is synchronous, and may not be used in an interrupt context.
- *
- * Only the hub driver or root-hub registrar should ever call this.
- */
-int usb_new_device(struct usb_device *udev)
+static int __usb_new_device(void *void_data)
{
+ struct usb_device *udev = void_data;
int err;
+ /* Lock ourself into memory in order to keep a probe sequence
+ * sleeping in a new thread from allowing us to be unloaded.
+ */
+ if (!try_module_get(THIS_MODULE))
+ return -EINVAL;
+
err = usb_get_configuration(udev);
if (err < 0) {
dev_err(&udev->dev, "can't read configurations, error %d\n",
@@ -1306,13 +1304,52 @@ int usb_new_device(struct usb_device *ud
goto fail;
}
- return 0;
+exit:
+ module_put(THIS_MODULE);
+ return err;
fail:
usb_set_device_state(udev, USB_STATE_NOTATTACHED);
- return err;
+ goto exit;
}
+/**
+ * usb_new_device - perform initial device setup (usbcore-internal)
+ * @udev: newly addressed device (in ADDRESS state)
+ *
+ * This is called with devices which have been enumerated, but not yet
+ * configured. The device descriptor is available, but not descriptors
+ * for any device configuration. The caller must have locked either
+ * the parent hub (if udev is a normal device) or else the
+ * usb_bus_list_lock (if udev is a root hub). The parent's pointer to
+ * udev has already been installed, but udev is not yet visible through
+ * sysfs or other filesystem code.
+ *
+ * The return value for this function depends on if the
+ * multithread_probe variable is set or not. If it's set, it will
+ * return a if the probe thread was successfully created or not. If the
+ * variable is not set, it will return if the device is configured
+ * properly or not. interfaces, in sysfs); else a negative errno value.
+ *
+ * This call is synchronous, and may not be used in an interrupt context.
+ *
+ * Only the hub driver or root-hub registrar should ever call this.
+ */
+int usb_new_device(struct usb_device *udev)
+{
+ struct task_struct *probe_task;
+ int ret = 0;
+
+ if (multithread_probe) {
+ probe_task = kthread_run(__usb_new_device, udev,
+ "usb-probe-%s", udev->devnum);
+ if (IS_ERR(probe_task))
+ ret = PTR_ERR(probe_task);
+ } else
+ ret = __usb_new_device(udev);
+
+ return ret;
+}
static int hub_port_status(struct usb_hub *hub, int port1,
u16 *status, u16 *change)
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
