On Tue, 3 Jan 2012, Oliver Neukum wrote:
> From 186ed6425f31636d1bb52de0f48a76dddf8e6224 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@xxxxxxxxxx>
> Date: Tue, 3 Jan 2012 12:34:31 +0100
> Subject: [PATCH] USB: code cleanup in suspend/resume path
>
> Do the cleanup to avoid behaviorial parameters Linus
> requested.
This patch needs to be fixed up.
> 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.
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.
...
> +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.
> @@ -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.
> + /* 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" ?
> + */
> 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.
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.
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
