On Sun, 9 Oct 2005 12:47:33 -0400 (EDT), Alan Stern <[EMAIL PROTECTED]> wrote:
> When assigning the flags value in usb-storage, the extra
> usb_usual module-type information should be masked out.
OK
> 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 was a mistake. Fortunately, users of usb-storage were not affected.
So, I put the entry back and changed the check.
> 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.
Hmm. The new API looks promising with the explicit starting and
stopping. This is a good idea, but I have to revisit it separately.
> 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.
I think that open-coding the comparison was wrong and usb_usual_check_type
is a better way to do it, but it is a wrong time to parse strings and
print diagnostics. So I kept the parsing where it was, but also adopted
usb_usual_check_type(). The libusual does not change usb_device_id
array on the fly anymore.
> 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.
No, I do not like that, because I do not want request_module to be
covered with any extra locks. That thing does too much, and there may
be deadlocks if I do that. Not currently, but I do not like leaving
things like this sitting in ambush.
It's not a lock really, just a notification done through a semaphore
because the stock completion primitive required a precise balance
between calls to notify and to test. I think that we should get rid
of it by using kthread_start.
Please see the attached patch. It goes on top of what I sent yesterday.
This way Greg may take it as an update. Otherwise, he may reject both.
Then I'll resend a unified libusual for -rc4.
-- Pete
diff -urp -X dontdiff linux-2.6.14-rc3-wip6/drivers/block/ub.c
linux-2.6.14-rc3-lem/drivers/block/ub.c
--- linux-2.6.14-rc3-wip6/drivers/block/ub.c 2005-10-09 12:25:18.000000000
-0700
+++ linux-2.6.14-rc3-lem/drivers/block/ub.c 2005-10-09 12:24:34.000000000
-0700
@@ -2168,7 +2168,7 @@ static int ub_probe(struct usb_interface
int rc;
int i;
- if (USB_US_TYPE(dev_id->driver_info) != USB_US_TYPE_UB)
+ if (usb_usual_check_type(dev_id, USB_US_TYPE_UB))
return -ENXIO;
rc = -ENOMEM;
@@ -2471,8 +2471,6 @@ static int __init ub_init(void)
{
int rc;
- usb_usual_set_present(USB_US_TYPE_UB);
-
if ((rc = register_blkdev(UB_MAJOR, DRV_NAME)) != 0)
goto err_regblkdev;
devfs_mk_dir(DEVFS_NAME);
@@ -2480,13 +2478,13 @@ static int __init ub_init(void)
if ((rc = usb_register(&ub_driver)) != 0)
goto err_register;
+ usb_usual_set_present(USB_US_TYPE_UB);
return 0;
err_register:
devfs_remove(DEVFS_NAME);
unregister_blkdev(UB_MAJOR, DRV_NAME);
err_regblkdev:
- usb_usual_clear_present(USB_US_TYPE_UB);
return rc;
}
diff -urp -X dontdiff linux-2.6.14-rc3-wip6/drivers/usb/storage/libusual.c
linux-2.6.14-rc3-lem/drivers/usb/storage/libusual.c
--- linux-2.6.14-rc3-wip6/drivers/usb/storage/libusual.c 2005-10-09
12:25:18.000000000 -0700
+++ linux-2.6.14-rc3-lem/drivers/usb/storage/libusual.c 2005-10-09
13:09:40.000000000 -0700
@@ -27,6 +27,7 @@ static DEFINE_SPINLOCK(usu_lock);
#define BIAS_NAME_SIZE (sizeof("usb-storage"))
static char bias[BIAS_NAME_SIZE];
+static int usb_usual_bias;
static const char *bias_names[3] = { "none", "usb-storage", "ub" };
static DECLARE_MUTEX_LOCKED(usu_init_notify);
@@ -34,16 +35,11 @@ static DECLARE_COMPLETION(usu_end_notify
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.
+ * The table.
*/
-
#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
vendorName, productName,useProtocol, useTransport, \
initFunction, flags) \
@@ -97,16 +93,38 @@ void usb_usual_clear_present(int type)
EXPORT_SYMBOL(usb_usual_clear_present);
/*
+ * Match the calling driver type against the table.
+ * Returns: 0 if the device matches.
+ */
+int usb_usual_check_type(const struct usb_device_id *id, int caller_type)
+{
+ int id_type = USB_US_TYPE(id->driver_info);
+
+ if (caller_type <= 0 || caller_type >= 3)
+ return -EINVAL;
+
+ /* Drivers grab fixed assignment devices */
+ if (id_type == caller_type)
+ return 0;
+ /* Drivers grab devices biased to them */
+ if (id_type == USB_US_TYPE_NONE && caller_type == usb_usual_bias)
+ return 0;
+ return -ENODEV;
+}
+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 type;
int rc;
unsigned long flags;
+ type = USB_US_TYPE(id->driver_info);
if (type == 0)
- return -ENXIO;
+ type = usb_usual_bias;
spin_lock_irqsave(&usu_lock, flags);
if ((stat[type].fls & (USU_MOD_FL_THREAD|USU_MOD_FL_PRESENT)) != 0) {
@@ -178,42 +196,22 @@ static int usu_probe_thread(void *arg)
}
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;
- }
- }
+ 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)
diff -urp -X dontdiff linux-2.6.14-rc3-wip6/drivers/usb/storage/usb.c
linux-2.6.14-rc3-lem/drivers/usb/storage/usb.c
--- linux-2.6.14-rc3-wip6/drivers/usb/storage/usb.c 2005-10-09
12:25:18.000000000 -0700
+++ linux-2.6.14-rc3-lem/drivers/usb/storage/usb.c 2005-10-09
12:31:17.000000000 -0700
@@ -419,7 +419,7 @@ static int associate_dev(struct us_data
}
/* 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(const struct usb_device_id *id)
{
const int id_index = id - storage_usb_ids;
return &us_unusual_dev_list[id_index];
@@ -431,7 +431,7 @@ static void get_device_info(struct us_da
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
&us->pusb_intf->cur_altsetting->desc;
- struct us_unusual_dev *unusual_dev = find_unusual_entry(id);
+ struct us_unusual_dev *unusual_dev = find_unusual(id);
/* Store the entries */
us->unusual_dev = unusual_dev;
@@ -441,7 +441,7 @@ static void get_device_info(struct us_da
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 +899,7 @@ static int storage_probe(struct usb_inte
struct us_data *us;
int result;
- if (USB_US_TYPE(id->driver_info) != USB_US_TYPE_STOR)
+ if (usb_usual_check_type(id, USB_US_TYPE_STOR))
return -ENXIO;
US_DEBUGP("USB Mass Storage device detected\n");
@@ -999,15 +1021,12 @@ static int __init usb_stor_init(void)
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;
}
diff -urp -X dontdiff linux-2.6.14-rc3-wip6/include/linux/usb_usual.h
linux-2.6.14-rc3-lem/include/linux/usb_usual.h
--- linux-2.6.14-rc3-wip6/include/linux/usb_usual.h 2005-10-09
12:25:18.000000000 -0700
+++ linux-2.6.14-rc3-lem/include/linux/usb_usual.h 2005-10-09
12:31:53.000000000 -0700
@@ -56,7 +56,8 @@ enum { US_DO_ALL_FLAGS };
#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 @@ enum { US_DO_ALL_FLAGS };
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(const struct usb_device_id *, 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(id, t) (0)
#endif /* CONFIG_USB_LIBUSUAL */
#endif /* __LINUX_USB_USUAL_H */
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
