From 41341261aa1707b49f937ba2c20d1a0daa5afac3 Mon Sep 17 00:00:00 2001 From: Mathias Nyman Date: Tue, 18 Jun 2013 17:28:48 +0300 Subject: usb: check usb_hub_to_struct_hub() return value usb_hub_to_struct_hub() can return NULL in some unlikely cases. Add checks where appropriate, or pass the hub pointer as an additional argument if it's known to be valid. The places it makes sense to check usb_hub_to_struct_hub() are picked based on feedback from Alan Stern. Signed-off-by: Mathias Nyman Acked-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hub.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index feef9351463..4191db32f12 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -718,18 +718,18 @@ static void hub_tt_work(struct work_struct *work) /** * usb_hub_set_port_power - control hub port's power state - * @hdev: target hub + * @hdev: USB device belonging to the usb hub + * @hub: target hub * @port1: port index * @set: expected status * * call this function to control port's power via setting or * clearing the port's PORT_POWER feature. */ -int usb_hub_set_port_power(struct usb_device *hdev, int port1, - bool set) +int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub, + int port1, bool set) { int ret; - struct usb_hub *hub = usb_hub_to_struct_hub(hdev); struct usb_port *port_dev = hub->ports[port1 - 1]; if (set) @@ -1769,15 +1769,17 @@ hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data) static int find_port_owner(struct usb_device *hdev, unsigned port1, struct dev_state ***ppowner) { + struct usb_hub *hub = usb_hub_to_struct_hub(hdev); + if (hdev->state == USB_STATE_NOTATTACHED) return -ENODEV; if (port1 == 0 || port1 > hdev->maxchild) return -EINVAL; - /* This assumes that devices not managed by the hub driver + /* Devices not managed by the hub driver * will always have maxchild equal to 0. */ - *ppowner = &(usb_hub_to_struct_hub(hdev)->ports[port1 - 1]->port_owner); + *ppowner = &(hub->ports[port1 - 1]->port_owner); return 0; } @@ -5323,7 +5325,8 @@ void usb_set_hub_port_connect_type(struct usb_device *hdev, int port1, { struct usb_hub *hub = usb_hub_to_struct_hub(hdev); - hub->ports[port1 - 1]->connect_type = type; + if (hub) + hub->ports[port1 - 1]->connect_type = type; } /** @@ -5339,6 +5342,9 @@ usb_get_hub_port_connect_type(struct usb_device *hdev, int port1) { struct usb_hub *hub = usb_hub_to_struct_hub(hdev); + if (!hub) + return USB_PORT_CONNECT_TYPE_UNKNOWN; + return hub->ports[port1 - 1]->connect_type; } @@ -5397,6 +5403,9 @@ acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev, { struct usb_hub *hub = usb_hub_to_struct_hub(hdev); + if (!hub) + return NULL; + return DEVICE_ACPI_HANDLE(&hub->ports[port1 - 1]->dev); } #endif -- cgit v1.2.3-70-g09d2 From e583d9db9960cf40e0bc8afee4946baa9d71596e Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Thu, 11 Jul 2013 14:58:04 -0400 Subject: USB: global suspend and remote wakeup don't mix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hub driver was recently changed to use "global" suspend for system suspend transitions on non-SuperSpeed buses. This means that we don't suspend devices individually by setting the suspend feature on the upstream hub port; instead devices all go into suspend automatically when the root hub stops transmitting packets. The idea was to save time and to avoid certain kinds of wakeup races. Now it turns out that many hubs are buggy; they don't relay wakeup requests from a downstream port to their upstream port if the downstream port's suspend feature is not set (depending on the speed of the downstream port, whether or not the hub is enabled for remote wakeup, and possibly other factors). We can't have hubs dropping wakeup requests. Therefore this patch goes partway back to the old policy: It sets the suspend feature for a port if the device attached to that port or any of its descendants is enabled for wakeup. People will still be able to benefit from the time savings if they don't care about wakeup and leave it disabled on all their devices. In order to accomplish this, the patch adds a new field to the usb_hub structure: wakeup_enabled_descendants is a count of how many devices below a suspended hub are enabled for remote wakeup. A corresponding new subroutine determines the number of wakeup-enabled devices at or below an arbitrary suspended USB device. This should be applied to the 3.10 stable kernel. Signed-off-by: Alan Stern Reported-and-tested-by: Toralf Förster Cc: stable Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++++++++-------- drivers/usb/core/hub.h | 3 +++ 2 files changed, 34 insertions(+), 8 deletions(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 4191db32f12..c857471519e 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2848,6 +2848,15 @@ static int usb_disable_function_remotewakeup(struct usb_device *udev) USB_CTRL_SET_TIMEOUT); } +/* Count of wakeup-enabled devices at or below udev */ +static unsigned wakeup_enabled_descendants(struct usb_device *udev) +{ + struct usb_hub *hub = usb_hub_to_struct_hub(udev); + + return udev->do_remote_wakeup + + (hub ? hub->wakeup_enabled_descendants : 0); +} + /* * usb_port_suspend - suspend a usb device's upstream port * @udev: device that's no longer in active use, not a root hub @@ -2888,8 +2897,8 @@ static int usb_disable_function_remotewakeup(struct usb_device *udev) * Linux (2.6) currently has NO mechanisms to initiate that: no khubd * timer, no SRP, no requests through sysfs. * - * If Runtime PM isn't enabled or used, non-SuperSpeed devices really get - * suspended only when their bus goes into global suspend (i.e., the root + * If Runtime PM isn't enabled or used, non-SuperSpeed devices may not get + * suspended until their bus goes into global suspend (i.e., the root * hub is suspended). Nevertheless, we change @udev->state to * USB_STATE_SUSPENDED as this is the device's "logical" state. The actual * upstream port setting is stored in @udev->port_is_suspended. @@ -2960,15 +2969,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) /* see 7.1.7.6 */ if (hub_is_superspeed(hub->hdev)) status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3); - else if (PMSG_IS_AUTO(msg)) - status = set_port_feature(hub->hdev, port1, - USB_PORT_FEAT_SUSPEND); + /* * For system suspend, we do not need to enable the suspend feature * on individual USB-2 ports. The devices will automatically go * into suspend a few ms after the root hub stops sending packets. * The USB 2.0 spec calls this "global suspend". + * + * However, many USB hubs have a bug: They don't relay wakeup requests + * from a downstream port if the port's suspend feature isn't on. + * Therefore we will turn on the suspend feature if udev or any of its + * descendants is enabled for remote wakeup. */ + else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0) + status = set_port_feature(hub->hdev, port1, + USB_PORT_FEAT_SUSPEND); else { really_suspend = false; status = 0; @@ -3003,15 +3018,16 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) if (!PMSG_IS_AUTO(msg)) status = 0; } else { - /* device has up to 10 msec to fully suspend */ dev_dbg(&udev->dev, "usb %ssuspend, wakeup %d\n", (PMSG_IS_AUTO(msg) ? "auto-" : ""), udev->do_remote_wakeup); - usb_set_device_state(udev, USB_STATE_SUSPENDED); if (really_suspend) { udev->port_is_suspended = 1; + + /* device has up to 10 msec to fully suspend */ msleep(10); } + usb_set_device_state(udev, USB_STATE_SUSPENDED); } /* @@ -3293,7 +3309,11 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg) unsigned port1; int status; - /* Warn if children aren't already suspended */ + /* + * Warn if children aren't already suspended. + * Also, add up the number of wakeup-enabled descendants. + */ + hub->wakeup_enabled_descendants = 0; for (port1 = 1; port1 <= hdev->maxchild; port1++) { struct usb_device *udev; @@ -3303,6 +3323,9 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg) if (PMSG_IS_AUTO(msg)) return -EBUSY; } + if (udev) + hub->wakeup_enabled_descendants += + wakeup_enabled_descendants(udev); } if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) { diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index 6508e02b3da..4e4790dea34 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -59,6 +59,9 @@ struct usb_hub { struct usb_tt tt; /* Transaction Translator */ unsigned mA_per_port; /* current for each child */ +#ifdef CONFIG_PM + unsigned wakeup_enabled_descendants; +#endif unsigned limited_power:1; unsigned quiescing:1; -- cgit v1.2.3-70-g09d2 From 2c7b871b9102c497ba8f972aa5d38532f05b654d Mon Sep 17 00:00:00 2001 From: William Gulland Date: Thu, 27 Jun 2013 16:10:20 -0700 Subject: usb: Clear both buffers when clearing a control transfer TT buffer. Control transfers have both IN and OUT (or SETUP) packets, so when clearing TT buffers for a control transfer it's necessary to send two HUB_CLEAR_TT_BUFFER requests to the hub. Signed-off-by: William Gulland Acked-by: Alan Stern Cc: stable Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hub.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index c857471519e..4a8a1d68002 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -668,6 +668,15 @@ resubmit: static inline int hub_clear_tt_buffer (struct usb_device *hdev, u16 devinfo, u16 tt) { + /* Need to clear both directions for control ep */ + if (((devinfo >> 11) & USB_ENDPOINT_XFERTYPE_MASK) == + USB_ENDPOINT_XFER_CONTROL) { + int status = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), + HUB_CLEAR_TT_BUFFER, USB_RT_PORT, + devinfo ^ 0x8000, tt, NULL, 0, 1000); + if (status) + return status; + } return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), HUB_CLEAR_TT_BUFFER, USB_RT_PORT, devinfo, tt, NULL, 0, 1000); -- cgit v1.2.3-70-g09d2 From 481f2d4f89f87a0baa26147f323380e31cfa7c44 Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 30 Jul 2013 19:51:20 -0700 Subject: usb: core: don't try to reset_device() a port that got just disconnected The USB hub driver's event handler contains a check to catch SuperSpeed devices that transitioned into the SS.Inactive state and tries to fix them with a reset. It decides whether to do a plain hub port reset or call the usb_reset_device() function based on whether there was a device attached to the port. However, there are device/hub combinations (found with a JetFlash Transcend mass storage stick (8564:1000) on the root hub of an Intel LynxPoint PCH) which can transition to the SS.Inactive state on disconnect (and stay there long enough for the host to notice). In this case, above-mentioned reset check will call usb_reset_device() on the stale device data structure. The kernel will send pointless LPM control messages to the no longer connected device address and can even cause several 5 second khubd stalls on some (buggy?) host controllers, before finally accepting the device's fate amongst a flurry of error messages. This patch makes the choice of reset dependent on the port status that has just been read from the hub in addition to the existence of an in-kernel data structure for the device, and only proceeds with the more extensive reset if both are valid. Signed-off-by: Julius Werner Signed-off-by: Sarah Sharp --- drivers/usb/core/hub.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 4a8a1d68002..558313de491 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4798,7 +4798,8 @@ static void hub_events(void) hub->ports[i - 1]->child; dev_dbg(hub_dev, "warm reset port %d\n", i); - if (!udev) { + if (!udev || !(portstatus & + USB_PORT_STAT_CONNECTION)) { status = hub_port_reset(hub, i, NULL, HUB_BH_RESET_TIME, true); @@ -4808,8 +4809,8 @@ static void hub_events(void) usb_lock_device(udev); status = usb_reset_device(udev); usb_unlock_device(udev); + connect_change = 0; } - connect_change = 0; } if (connect_change) -- cgit v1.2.3-70-g09d2