On Sat, 17 Mar 2012, Ming Lei wrote:
> 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?
Why bother?
> >> + � � � � � � for (i = 0; i < config->desc.bNumInterfaces; i++)
If config is NULL then this statement will get an invalid memory
access. The effect will be the same as your BUG_ON.
> >> + � � � � � � � � � � 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.
Look, you can't check the entire state of the system. At some point
you have to trust the other parts of the kernel to do their jobs
properly. Do you have any reason to think that this will ever happen?
If you want to include this second BUG_ON, go ahead. I don't think it
will do any good. But don't include the first one.
Alan Stern
--
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
