Am Dienstag, 3. Januar 2012, 17:00:18 schrieb Alan Stern:
> On Tue, 3 Jan 2012, Oliver Neukum wrote:
>
> This patch needs to be fixed up.
Roger
>
> > Signed-off-by: Oliver Neukum <oneukum@xxxxxxx>
> > ---
> > drivers/usb/core/driver.c | 54 ++++++++++++++++++++++++++++----------------
> > 1 files changed, 34 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 07f8718..9d708c3 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -974,12 +974,11 @@ void usb_rebind_intf(struct usb_interface *intf)
> > #define DO_UNBIND 0
> > #define DO_REBIND 1
> >
> > -/* Unbind drivers for @udev's interfaces that don't support suspend/resume,
> > - * or rebind interfaces that have been unbound, according to @action.
> > +/* Unbind drivers for @udev's interfaces that don't support suspend/resume
> > *
> > * The caller must hold @udev's device lock.
> > */
> > -static void do_unbind_rebind(struct usb_device *udev, int action)
> > +static void kick_unsupportive_drivers_from_interfaces(struct usb_device *udev)
>
> Okay, I admit I combined multiple routines into one in order to save
> code space. But I don't like your choice of a new name. How about
> unbind_drivers_with_no_suspend_or_resume_method? Still pretty bad, but
> it's more precise.
Well, it does not state that interfaces are affected.
> Also, if you're going to do this cleanup, change usb_rebind_intf().
> Its first "delayed unbind" section is basically a copy of
> usb_forced_unbind_intf() -- have it call that routine instead of
> duplicating the code.
Roger
> > +static void do_rebind(struct usb_device *udev)
> > +{
> > + struct usb_host_config *config;
> > + int i;
> > + struct usb_interface *intf;
> > +
> > + config = udev->actconfig;
> > + if (config) {
> > + for (i = 0; i < config->desc.bNumInterfaces; ++i) {
> > + intf = config->interface[i];
> > + if (intf->needs_binding)
> > + usb_rebind_intf(intf);
> > + }
> > + }
> > +}
>
> This is okay. But maybe change the name to rebind_interfaces.
Fine by me, but we should be consistent.
> > @@ -1296,7 +1304,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> > {
> > struct usb_device *udev = to_usb_device(dev);
> >
> > - do_unbind_rebind(udev, DO_UNBIND);
> > + kick_unsupportive_drivers_from_interfaces(udev);
>
> Blank line before the comment, please.
Roger
> > + /* From now on we are sure all drivers support suspend/resume
> > + * but not necessarily reset_resume()
> > + * so we may still need to rebind upon resumption
>
> "... may still need to unbind and rebind upon resume"
>
> > + */
> > choose_wakeup(udev, msg);
> > return usb_suspend_both(udev, msg);
> > }
> > @@ -1307,15 +1319,17 @@ int usb_resume(struct device *dev, pm_message_t msg)
> > struct usb_device *udev = to_usb_device(dev);
> > int status;
> >
> > - /* For PM complete calls, all we do is rebind interfaces */
> > + /* For PM complete calls, all we do is rebind interfaces
> > + * if the need has arisen earlier
>
> How about: "... all we do is rebind interfaces that need it" ?
That would be unclearer as it might be understodd to mean that we
determine the need at this point
> > + */
> > if (msg.event == PM_EVENT_ON) {
> > if (udev->state != USB_STATE_NOTATTACHED)
> > - do_unbind_rebind(udev, DO_REBIND);
> > + do_rebind(udev);
> > status = 0;
> >
> > /* For all other calls, take the device back to full power and
> > * tell the PM core in case it was autosuspended previously.
> > - * Unbind the interfaces that will need rebinding later.
> > + * Rebind the interfaces that need it.
>
> The original comment was correct and the new comment is wrong. See
> below.
>
> > */
> > } else {
> > status = usb_resume_both(udev, msg);
> > @@ -1323,7 +1337,7 @@ int usb_resume(struct device *dev, pm_message_t msg)
> > pm_runtime_disable(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > - do_unbind_rebind(udev, DO_REBIND);
> > + do_rebind(udev);
>
> This is what Linus was complaining about. The action here really is an
> unbind, not a rebind, but it applies only to interfaces that have
> needs_binding set already.
>
> "Why would an interface have needs_binding set without already being
> unbound", you ask. Well, that's what happens when a driver doesn't
> support reset-resume. usb_resume_interface() sets intf->needs_binding,
> but it can't unbind the interface because it doesn't own the right set
> of locks.
I'll add a big fat comment.
> Thus what you really need here is yet another routine, with a name like
> unbind_drivers_because_of_reset_resume. That routine will be just like
> unbind_drivers_with_no_suspend_or_resume_method, except that it will
> check for needs_binding set rather than looking at method pointers.
Roger.
Regards
Oliver
--
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
