On Mon, 9 Jan 2012, Sarah Sharp wrote:
> Ok, what about something like this? I feel like portions of the
> hub_events really need to get refactored into smaller functions that do
> one thing (like handle remote wakeup), with the hub_events() function
> basically keeping track of the future actions it needs to take care of
> (like handling a connect status change).
Okay, that's sensible.
> Sarah Sharp
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index db6b751..cee525c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3442,6 +3442,47 @@ done:
> hcd->driver->relinquish_port(hcd, port1);
> }
>
> +/* Returns 1 if there was a remote wakeup and a connect status change. */
> +static int hub_check_remote_wakeup(struct usb_hub *hub, unsigned int port,
> + u16 portchange)
> +{
> + struct usb_device *hdev;
> + struct usb_device *udev;
> + int connect_change = 0;
> + int ret;
> +
> + hdev = hub->hdev;
> + if (!hub_is_superspeed(hdev) &&
> + !(portchange & USB_PORT_STAT_C_SUSPEND))
> + return 0;
> + if (hub_is_superspeed(hdev) &&
> + !(portchange & USB_PORT_STAT_C_LINK_STATE))
> + return 0;
> +
> + if (!hub_is_superspeed(hdev))
> + clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND);
> + else
> + clear_port_feature(hdev, port, USB_PORT_FEAT_C_PORT_LINK_STATE);
I would have arranged the code to avoid repeated tests:
if (hub_is_superspeed(hdev)) {
if (!(portchange & USB_PORT_STAT_C_LINK_STATE))
return 0;
clear_port_feature(hdev, port, USB_PORT_FEAT_C_LINK_STATE);
} else {
if (!(portchange & USB_PORT_STAT_C_SUSPEND))
return 0;
clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND)
}
> + udev = hdev->children[port-1];
> + if (udev) {
> + /* TRSMRCY = 10 msec */
> + msleep(10);
> +
> + usb_lock_device(udev);
> + ret = usb_remote_wakeup(hdev->children[port-1]);
Although this is simply a copy of the existing code, you might as well
replace "hdev->children[port-1]" with "udev". I think the
usb_remote_wakeup() call was there first, and when the udev local
variable was added, the call's argument didn't get simplified.
> + usb_unlock_device(udev);
> + if (ret < 0)
> + connect_change = 1;
> + } else {
> + ret = -ENODEV;
> + hub_port_disable(hub, port, 1);
> + }
> + dev_dbg(hub->intfdev, "resume on port %d, status %d\n",
> + port, ret);
> + return connect_change;
> +}
Otherwise fine.
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
