On Thu, 26 Nov 2009, Oliver Neukum wrote:
> Am Donnerstag, 26. November 2009 18:03:04 schrieb Alan Stern:
> > If the interface is in the middle of binding or unbinding when the
> > reset occurs, the driver need not have a pre_reset or post_reset
> > method. This exception is for the drivers that do a reset within their
> > probe routine, of which there are (or were) several. So the test has
> > to be retained.
> >
>
> This way?
> @@ -3672,6 +3673,10 @@ int usb_reset_device(struct usb_device *udev)
> drv = to_usb_driver(cintf->dev.driver);
> if (drv->pre_reset && drv->post_reset)
> unbind = (drv->pre_reset)(cintf);
> + /*
> + * you can use usb_reset_device() from
> + * probe() without providing reset
Without providing pre_reset or post_reset. "without providing reset"
doesn't make sense.
> + */
> else if (cintf->condition ==
> USB_INTERFACE_BOUND)
> unbind = 1;
> @@ -3687,17 +3692,30 @@ int usb_reset_device(struct usb_device *udev)
> for (i = config->desc.bNumInterfaces - 1; i >= 0; --i) {
> struct usb_interface *cintf = config->interface[i];
> struct usb_driver *drv;
> - int rebind = cintf->needs_binding;
>
> - if (!rebind && cintf->dev.driver) {
> + if (!cintf->needs_binding && 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;
> + cintf->needs_binding =
> + (drv->post_reset)(cintf) ? 1 : 0;
Purely as a matter of taste, it might be clearer to write this as
if (drv->post_reset(cintf))
cintf->needs_binding = 1;
> + else if (cintf->condition == USB_INTERFACE_BOUND)
> + cintf->needs_binding = 1;
I think these two lines aren't needed, and in fact they weren't needed
in the original code. The only way to reach this spot is if you are in
the middle of binding or unbinding; hence the test can never succeed.
> }
> - if (ret == 0 && rebind)
> + }
> +
> + /*
> + * Don't rebind until after all the post_reset() calls
> + * so that a newly bound driver does not get an unbalanced
> + * call to post_reset()
> + */
> + for (i = config->desc.bNumInterfaces - 1; i >= 0; --i){
> + struct usb_interface *cintf = config->interface[i];
> +
> + /*
> + * safe w.r.t. usb_claim_interface() as
> + * that clears needs_binding
> + */
Hmm. If your probe routine claims some other interfaces and then does
a reset, and you don't provide pre_reset and post_reset methods, you're
likely to run into trouble anyway. Drivers just shouldn't do that.
They should carry out the reset first.
> + if (ret == 0 && cintf->needs_binding)
> usb_rebind_intf(cintf);
> }
> }
The rest looks right.
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
