On Wed, 11 Nov 2009, Oliver Neukum wrote:
> Hi,
>
> it seems to me that we have a subtle bug in usb_reset_device().
> If we reset a device we may reprobe an interface. If that causes
> another interface to be claimed we may call post_reset() for that
> interface without having called pre_reset().
>
> I think it is necessary to do the rebinding in a second pass after
> doing the notifications. But this is subtle. Alan, what do you think?
You are right. But I would make the code a little cleaner...
> @@ -3696,13 +3696,15 @@ int usb_reset_device(struct usb_device *udev)
>
A few lines above this, it would be good to change
if (config) {
to
if (ret != -ENODEV && config) {
There's no point sending post_reset notifications if the reset caused
the device to go away.
> if (!rebind && cintf->dev.driver) {
> drv = to_usb_driver(cintf->dev.driver);
> - if (drv->post_reset)
> - rebind = (drv->post_reset)(cintf);
> - else if (cintf->condition ==
> - USB_INTERFACE_BOUND)
> - rebind = 1;
> + rebind = (drv->post_reset)(cintf);
> }
> - if (ret == 0 && rebind)
> + cintf->needs_binding = rebind;
You don't need the "rebind" variable here. Directly set
cintf->needs_binding to the result of the method call. Or rather, if
the method call returns nonzero, set cintf->needs_binding to 1 (since
it is a bitflag, not an int).
> + }
> +
> + /* second pass to not call post_reset on new drivers */
Rephrase the comment to make it more clear:
/* Don't rebind until after all the post_reset calls */
> + for (i = config->desc.bNumInterfaces - 1; i >= 0; --i){
> + struct usb_interface *cintf = config->interface[i];
Missing blank line.
> + if (ret == 0 && cintf->needs_binding)
If ret is tested above then it doesn't need to be tested here. And in
fact the existing test is wrong; it should be against -ENODEV rather
than 0. A reset can fail for reasons that don't cause the device to go
away.
> usb_rebind_intf(cintf);
> }
> }
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
