On Fri, Mar 16, 2012 at 9:55 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>> + if (config)
>
> This test isn't needed. The code will never run if udev->actconfig is
> NULL.
If there are bugs on kernel/usb somewhere, it still may happen, so could we
add BUD_ON() to catch it?
>
>> + for (i = 0; i < config->desc.bNumInterfaces; i++)
>> + if (config->interface[i] == intf) {
>> + lockdep_set_class(mutex,
>> + &us_interface_key[i]);
>> + return;
>> + }
>> + dev_err(&intf->dev, "something weird!");
>
> Ugh. "something weird!" is not a useful message; it doesn't tell the
> reader anything about what happened. You can simply ignore this
> case; it should never occur.
>
> You can move the lockdep_set_class call after the loop and replace
> the "return" with "break". Then if you want to be really safe, define
> the length of us_interface_key[] to be USB_MAXINTERFACES + 1. But I
> don't think that's necessary.
I think +1 is not good, which may hide a bug, even it is very impossible, but
some kernel/usb bugs still may trigger it, so BUG_ON can catch it, and the
check doesn't hurt performance since it is in the .probe path and only available
with LOCKDEP.
>
> And also, please fix the braces as Peter requested.
Will do it.
So how about the blow?
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index c18538e..99b48ea 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -132,6 +132,37 @@ static struct us_unusual_dev for_dynamic_ids =
#undef COMPLIANT_DEV
#undef USUAL_DEV
+#ifdef CONFIG_LOCKDEP
+
+static struct lock_class_key us_interface_key[USB_MAXINTERFACES];
+
+static void us_set_lock_class(struct mutex *mutex,
+ struct usb_interface *intf)
+{
+ struct usb_device *udev = interface_to_usbdev(intf);
+ struct usb_host_config *config = udev->actconfig;
+ int i;
+
+ BUG_ON(!config);
+
+ for (i = 0; i < config->desc.bNumInterfaces; i++) {
+ if (config->interface[i] == intf)
+ break;
+ }
+
+ BUG_ON(i == config->desc.bNumInterfaces);
+
+ lockdep_set_class(mutex, &us_interface_key[i]);
+}
+
+#else
+
+static void us_set_lock_class(struct mutex *mutex,
+ struct usb_interface *intf)
+{
+}
+
+#endif
#ifdef CONFIG_PM /* Minimal support for suspend and resume */
@@ -895,6 +926,7 @@ int usb_stor_probe1(struct us_data **pus,
*pus = us = host_to_us(host);
memset(us, 0, sizeof(struct us_data));
mutex_init(&(us->dev_mutex));
+ us_set_lock_class(&us->dev_mutex, intf);
init_completion(&us->cmnd_ready);
init_completion(&(us->notify));
init_waitqueue_head(&us->delay_wait);
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
