On Fri, 4 Nov 2016, Mathias Nyman wrote:
> USB-3 does not have any link state that will avoid negotiating a connection
> with a plugged-in cable but will signal the host when the cable is
> unplugged.
>
> For USB-3 we used to first set the link to Disabled, then to RxDdetect to
> be able to detect cable connects or disconnects. But in RxDetect the
> connected device is detected again and eventually enabled.
>
> Instead set the link into U3 and disable remote wakeups for the device.
> This is what Windows does, and what Alan Stern suggested.
The general idea is okay, but there is one problem. See below.
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/core/hub.c | 68 +++++++++++++-------------------------------------
> 1 file changed, 17 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 1d5fc32..8e55fbd 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -101,6 +101,7 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>
> static void hub_release(struct kref *kref);
> static int usb_reset_and_verify_device(struct usb_device *udev);
> +static int usb_disable_remote_wakeup(struct usb_device *udev);
>
> static inline char *portspeed(struct usb_hub *hub, int portstatus)
> {
> @@ -899,65 +900,30 @@ static int hub_set_port_link_state(struct usb_hub *hub, int port1,
> }
>
> /*
> - * If USB 3.0 ports are placed into the Disabled state, they will no longer
> - * detect any device connects or disconnects. This is generally not what the
> - * USB core wants, since it expects a disabled port to produce a port status
> - * change event when a new device connects.
> + * USB-3 does not have a similar link state as USB-2 that will avoid negotiating
> + * a connection with a plugged-in cable but will signal the host when the cable
> + * is unplugged.
> *
> - * Instead, set the link state to Disabled, wait for the link to settle into
> - * that state, clear any change bits, and then put the port into the RxDetect
> - * state.
> + * Instead set the link into U3 and disable remote wakeups for the device.
> */
> static int hub_usb3_port_disable(struct usb_hub *hub, int port1)
> {
> - int ret;
> - int total_time;
> - u16 portchange, portstatus;
> -
> - if (!hub_is_superspeed(hub->hdev))
> - return -EINVAL;
> -
> - ret = hub_port_status(hub, port1, &portstatus, &portchange);
> - if (ret < 0)
> - return ret;
> + struct usb_port *port_dev = hub->ports[port1 - 1];
> + struct usb_device *udev = port_dev->child;
You could pass port_dev as an argument from hub_port_disable() instead
of recomputing it.
> + int ret = 0;
>
> - /*
> - * USB controller Advanced Micro Devices, Inc. [AMD] FCH USB XHCI
> - * Controller [1022:7814] will have spurious result making the following
> - * usb 3.0 device hotplugging route to the 2.0 root hub and recognized
> - * as high-speed device if we set the usb 3.0 port link state to
> - * Disabled. Since it's already in USB_SS_PORT_LS_RX_DETECT state, we
> - * check the state here to avoid the bug.
> - */
> - if ((portstatus & USB_PORT_STAT_LINK_STATE) ==
> - USB_SS_PORT_LS_RX_DETECT) {
> - dev_dbg(&hub->ports[port1 - 1]->dev,
> - "Not disabling port; link state is RxDetect\n");
> - return ret;
> + if (udev && udev->port_is_suspended && udev->do_remote_wakeup) {
> + ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U0);
> + if (ret)
> + goto out;
> + msleep(USB_RESUME_TIMEOUT);
> + ret = usb_disable_remote_wakeup(udev);
This won't work. When this routine runs udev->state has already been
set to USB_STATE_NOTATTACHED, and therefore it's impossible to send any
URBs to udev.
You may be able to avoid this problem by changing hub_port_disable().
Have it call usb_set_device_state() after disabling the port instead of
before.
> }
> -
> - ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
> +out:
> if (ret)
> - return ret;
> -
> - /* Wait for the link to enter the disabled state. */
> - for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) {
> - ret = hub_port_status(hub, port1, &portstatus, &portchange);
> - if (ret < 0)
> - return ret;
> -
> - if ((portstatus & USB_PORT_STAT_LINK_STATE) ==
> - USB_SS_PORT_LS_SS_DISABLED)
> - break;
> - if (total_time >= HUB_DEBOUNCE_TIMEOUT)
> - break;
> - msleep(HUB_DEBOUNCE_STEP);
> - }
> - if (total_time >= HUB_DEBOUNCE_TIMEOUT)
> - dev_warn(&hub->ports[port1 - 1]->dev,
> - "Could not disable after %d ms\n", total_time);
> + dev_warn(&udev->dev, "Port disable: can't disable remote wake\n");
>
> - return hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_RX_DETECT);
> + return hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
> }
>
> static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
>
Otherwise this seems reasonable.
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
