On Sat, 8 Oct 2005, Pete Zaitcev wrote:
> This patch adds a shim driver libusual, which routes devices between
> usb-storage and ub according to the common table, based on unusual_devs.h.
> The help and example syntax is in Kconfig.
>
> Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
>
> ---
>
> I think libusual is ready for Andrew's tree now. I realize that the use
> of request_module is an inkblot in this copybook, so maybe it's not the
> time for Linus' tree yet. Another concern I have is if we have any races
> with hotplug, udev, and HAL.
>
> I haven't heard from Adrian yet, but I suppose it's a good sign.
> No word from Matt Dharm either, which is not so good... But I do have
> Alan Stern's buy-in.
After reading through the libusual code carefully, I have some more
suggestions for changes. (Slightly off-topic, I noticed in ub.c your
struct ub_completion. What's the reason for that? Why not just use an
atomic_t? Or even set_bit/test_bit?)
The changes are all given below in a single big patch. Untested, but it
compiles. Here's a list of the individual items, broken out. You may not
want to accept all these suggestions, but the first one fixes a potential
bug.
When assigning the flags value in usb-storage, the extra
usb_usual module-type information should be masked out.
Apparently ub's probe routine won't accept any devices unless
libusual is configured. That's what you intended, right?
So there's no need for it to have an entry in its usb_ids table.
It doesn't matter where usb_usual_set_present is called in a
modules's init routine. (This is another one of those races
involving request_module.) So I moved the calls to the end, to
avoid calling usb_usual_clear_present when registration fails.
I converted libusual to use the kthread API.
There's not much extra overhead if you check the bias string
every time a new device is probed. By doing so, libusual can
react to changes made through sysfs. A new routine was added
to handle this: usb_usual_check_type.
The call to up(&usu_init_notify) can be moved to the end of the
probe_thread routine. We may as well extend the region of mutual
exclusion over the whole routine, since most of the time a
spinlock is held anyway.
Alan Stern
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
--- a/include/linux/usb_usual.h Sat Oct 8 22:03:02 2005
+++ b/include/linux/usb_usual.h Sun Oct 9 11:29:40 2005
@@ -56,7 +56,8 @@
#define USB_US_TYPE_STOR 1 /* usb-storage */
#define USB_US_TYPE_UB 2 /* ub */
-#define USB_US_TYPE(flags) (((flags) >> 24) & 0xFF)
+#define USB_US_TYPE(flags) (((flags) >> 24) & 0xFF)
+#define USB_US_ORIG_FLAGS(flags) ((flags) & 0x00ffffff)
/*
* This is probably not the best place to keep these constants, conceptually.
@@ -111,10 +112,12 @@
extern struct usb_device_id storage_usb_ids[];
extern void usb_usual_set_present(int type);
extern void usb_usual_clear_present(int type);
+extern int usb_usual_check_type(int type);
#else
#define usb_usual_set_present(t) do { } while(0)
#define usb_usual_clear_present(t) do { } while(0)
+#define usb_usual_check_type(t) (t)
#endif /* CONFIG_USB_LIBUSUAL */
#endif /* __LINUX_USB_USUAL_H */
--- a/drivers/block/ub.c Sat Oct 8 22:03:02 2005
+++ b/drivers/block/ub.c Sun Oct 9 12:03:07 2005
@@ -417,7 +417,6 @@
#else
static struct usb_device_id ub_usb_ids[] = {
- { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, US_SC_SCSI, US_PR_BULK) },
{ }
};
@@ -2166,7 +2165,8 @@
int rc;
int i;
- if (USB_US_TYPE(dev_id->driver_info) != USB_US_TYPE_UB)
+ if (usb_usual_check_type(USB_US_TYPE(dev_id->driver_info)) !=
+ USB_US_TYPE_UB)
return -ENXIO;
rc = -ENOMEM;
@@ -2468,20 +2468,18 @@
{
int rc;
- usb_usual_set_present(USB_US_TYPE_UB);
-
if ((rc = register_blkdev(UB_MAJOR, DRV_NAME)) != 0)
goto err_regblkdev;
if ((rc = usb_register(&ub_driver)) != 0)
goto err_register;
+ usb_usual_set_present(USB_US_TYPE_UB);
return 0;
err_register:
unregister_blkdev(UB_MAJOR, DRV_NAME);
err_regblkdev:
- usb_usual_clear_present(USB_US_TYPE_UB);
return rc;
}
--- a/drivers/usb/storage/libusual.c Sat Oct 8 22:03:02 2005
+++ b/drivers/usb/storage/libusual.c Sun Oct 9 12:15:59 2005
@@ -4,6 +4,7 @@
* The libusual contains the table of devices common for ub and usb-storage.
*/
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/usb.h>
#include <linux/usb_usual.h>
@@ -34,15 +35,6 @@
static atomic_t total_threads = ATOMIC_INIT(0);
static int usu_probe_thread(void *arg);
-static void bias_storage_ids(int to_bias);
-static int parse_bias(const char *bias_s);
-
-/*
- * The UNUSUAL_DEV_FL is a version of UNUSUAL_DEV for "flags-only"
- * devices which only need an entry in usb_device_id array.
- * It keeps arguments for vendor and product names because
- * they are valuable comments, even though they are not used by the code.
- */
#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
vendorName, productName,useProtocol, useTransport, \
@@ -97,12 +89,35 @@
EXPORT_SYMBOL(usb_usual_clear_present);
/*
+ * Figure out the current bias for a particular device entry.
+ */
+int usb_usual_check_type(int type)
+{
+ int i;
+
+ if (type > 0 && type < 3)
+ return type;
+ if (bias[0]) {
+ bias[BIAS_NAME_SIZE-1] = 0;
+ for (i = 1; i < 3; i++) {
+ if (strcmp(bias, bias_names[i]) == 0)
+ return i;
+ }
+ printk(KERN_INFO
+ "libusual: unknown bias \"%s\", using \"%s\"\n",
+ bias, bias_names[USB_US_DEFAULT_BIAS]);
+ }
+ return USB_US_DEFAULT_BIAS;
+}
+EXPORT_SYMBOL(usb_usual_check_type);
+
+/*
*/
static int usu_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
int type = USB_US_TYPE(id->driver_info);
- int rc;
+ struct task_struct *ts;
unsigned long flags;
if (type == 0)
@@ -116,15 +131,16 @@
stat[type].fls |= USU_MOD_FL_THREAD;
spin_unlock_irqrestore(&usu_lock, flags);
- rc = kernel_thread(usu_probe_thread, (void*)type, CLONE_VM);
- if (rc < 0) {
+ ts = kthread_run(usu_probe_thread, (void *) type, "libusual_%d", type);
+ if (IS_ERR(ts)) {
printk(KERN_WARNING "libusual: "
"Unable to start the thread for %s: %d\n",
- bias_names[type], rc);
+ bias_names[type], (int) PTR_ERR(ts));
spin_lock_irqsave(&usu_lock, flags);
stat[type].fls &= ~USU_MOD_FL_THREAD;
spin_unlock_irqrestore(&usu_lock, flags);
- return rc; /* Not being -ENXIO causes a message printed */
+ return PTR_ERR(ts);
+ /* Not returning -ENXIO causes a message to be printed */
}
atomic_inc(&total_threads);
@@ -160,11 +176,8 @@
int rc;
unsigned long flags;
- daemonize("libusual_%d", type); /* "usb-storage" is kinda too long */
-
/* A completion does not work here because it's counted. */
down(&usu_init_notify);
- up(&usu_init_notify);
rc = request_module(bias_names[type]);
spin_lock_irqsave(&usu_lock, flags);
@@ -178,42 +191,20 @@
}
st->fls &= ~USU_MOD_FL_THREAD;
spin_unlock_irqrestore(&usu_lock, flags);
- complete_and_exit(&usu_end_notify, 0);
-}
-/*
- */
-static void bias_storage_ids(int to_bias)
-{
- struct usb_device_id *id;
-
- for (id = storage_usb_ids;
- id->idVendor || id->bDeviceClass || id->bInterfaceClass ||
- id->driver_info;
- id++) {
- if (USB_US_TYPE(id->driver_info) == 0) {
- id->driver_info |= to_bias << 24;
- }
- }
+ up(&usu_init_notify);
+ complete_and_exit(&usu_end_notify, 0);
}
/*
*/
static int __init usb_usual_init(void)
{
- int usb_usual_bias;
int rc;
- bias[BIAS_NAME_SIZE-1] = 0;
- usb_usual_bias = parse_bias(bias);
- bias_storage_ids(usb_usual_bias);
-
- if ((rc = usb_register(&usu_driver)) != 0) {
- up(&usu_init_notify);
- return rc;
- }
+ rc = usb_register(&usu_driver);
up(&usu_init_notify);
- return 0;
+ return rc;
}
static void __exit usb_usual_exit(void)
@@ -231,33 +222,6 @@
}
}
-/*
- * Validate and accept the bias parameter.
- * Maybe make an sysfs method later. XXX
- */
-static int parse_bias(const char *bias_s)
-{
- int i;
- int bias_n = 0;
-
- if (bias_s[0] == 0 || bias_s[0] == ' ') {
- bias_n = USB_US_DEFAULT_BIAS;
- } else {
- for (i = 1; i < 3; i++) {
- if (strcmp(bias_s, bias_names[i]) == 0) {
- bias_n = i;
- break;
- }
- }
- if (bias_n == 0) {
- bias_n = USB_US_DEFAULT_BIAS;
- printk(KERN_INFO
- "libusual: unknown bias \"%s\", using \"%s\"\n",
- bias_s, bias_names[bias_n]);
- }
- }
- return bias_n;
-}
module_init(usb_usual_init);
module_exit(usb_usual_exit);
--- a/drivers/usb/storage/usb.c Sat Oct 8 22:03:02 2005
+++ b/drivers/usb/storage/usb.c Sun Oct 9 12:01:21 2005
@@ -419,7 +419,8 @@
}
/* Find an unusual_dev descriptor (always succeeds in the current code) */
-static struct us_unusual_dev *find_unusual_entry(const struct usb_device_id
*id)
+static struct us_unusual_dev *find_unusual_entry(
+ const struct usb_device_id *id)
{
const int id_index = id - storage_usb_ids;
return &us_unusual_dev_list[id_index];
@@ -441,7 +442,7 @@
us->protocol = (unusual_dev->useTransport == US_PR_DEVICE) ?
idesc->bInterfaceProtocol :
unusual_dev->useTransport;
- us->flags = id->driver_info;
+ us->flags = USB_US_ORIG_FLAGS(id->driver_info);
/*
* This flag is only needed when we're in high-speed, so let's
@@ -877,7 +878,8 @@
struct us_data *us;
int result;
- if (USB_US_TYPE(id->driver_info) != USB_US_TYPE_STOR)
+ if (usb_usual_check_type(USB_US_TYPE(id->driver_info)) !=
+ USB_US_TYPE_STOR)
return -ENXIO;
US_DEBUGP("USB Mass Storage device detected\n");
@@ -999,15 +1001,12 @@
int retval;
printk(KERN_INFO "Initializing USB Mass Storage driver...\n");
- usb_usual_set_present(USB_US_TYPE_STOR);
-
/* register the driver, return usb_register return code if error */
retval = usb_register(&usb_storage_driver);
- if (retval == 0)
+ if (retval == 0) {
printk(KERN_INFO "USB Mass Storage support registered.\n");
- else
- usb_usual_clear_present(USB_US_TYPE_STOR);
-
+ usb_usual_set_present(USB_US_TYPE_STOR);
+ }
return retval;
}
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
