On Wed, 16 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.
>
> Cc: sta...@vger.kernel.org
> Cc: Alan Stern <st...@rowland.harvard.edu>
> Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com>
>
> ---
>
> Changes since RFC-PATCH:
> - send port_dev as an argument
> - move checking and disabling remote wakeup to a function under CONFIG_PM
> - set device state to USB_STATE_NOTATTACHED _after_ disabling remote wakup
> and port disable.
> ---
> drivers/usb/core/hub.c | 100
> +++++++++++++++++--------------------------------
> 1 file changed, 35 insertions(+), 65 deletions(-)
Just one little thing; see below. After that is changed, you can add:
Acked-by: Alan Stern <st...@rowland.harvard.edu>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 76e80d8..3775424 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -103,6 +103,8 @@
>
> static void hub_release(struct kref *kref);
> static int usb_reset_and_verify_device(struct usb_device *udev);
> +static void hub_usb3_port_prepare_disable(struct usb_hub *hub,
> + struct usb_port *port_dev);
>
> static inline char *portspeed(struct usb_hub *hub, int portstatus)
> {
> @@ -901,82 +903,28 @@ 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.
> - *
> - * 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.
> + * 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. Disable remote wake and set link state to U3 for USB-3
> devices
> */
> -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;
> -
> - /*
> - * 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;
> - }
> -
> - ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
> - 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);
> -
> - return hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_RX_DETECT);
> -}
> -
> static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
> {
> struct usb_port *port_dev = hub->ports[port1 - 1];
> struct usb_device *hdev = hub->hdev;
> int ret = 0;
>
> - if (port_dev->child && set_state)
> - usb_set_device_state(port_dev->child, USB_STATE_NOTATTACHED);
> if (!hub->error) {
> - if (hub_is_superspeed(hub->hdev))
> - ret = hub_usb3_port_disable(hub, port1);
> - else
> + if (hub_is_superspeed(hub->hdev)) {
> + hub_usb3_port_prepare_disable(hub, port_dev);
> + ret = hub_set_port_link_state(hub, port_dev->portnum,
> + USB_SS_PORT_LS_U3);
> + } else {
> ret = usb_clear_port_feature(hdev, port1,
> USB_PORT_FEAT_ENABLE);
> + }
> }
> + if (port_dev->child && set_state)
> + usb_set_device_state(port_dev->child, USB_STATE_NOTATTACHED);
> if (ret && ret != -ENODEV)
> dev_err(&port_dev->dev, "cannot disable (err = %d)\n", ret);
> return ret;
> @@ -4142,6 +4090,25 @@ void usb_unlocked_enable_lpm(struct usb_device *udev)
> }
> EXPORT_SYMBOL_GPL(usb_unlocked_enable_lpm);
>
> +/* usb3 devices use U3 for disabled, make sure remote wakeup is disabled */
> +static void hub_usb3_port_prepare_disable(struct usb_hub *hub,
> + struct usb_port *port_dev)
> +{
> + struct usb_device *udev = port_dev->child;
> + int ret;
> +
> + if (udev && udev->port_is_suspended && udev->do_remote_wakeup) {
> + ret = hub_set_port_link_state(hub, port_dev->portnum,
> + USB_SS_PORT_LS_U0);
> + if (!ret) {
> + msleep(USB_RESUME_TIMEOUT);
> + ret = usb_disable_remote_wakeup(udev);
> + }
> + if (ret)
> + dev_warn(&udev->dev,
> + "Port disable: can't disable remote wake\n");
At this point you should clear udev->do_remote_wakeup. It doesn't make
a lot of difference, but it will help if this routine gets called twice
for the same device.
Alan Stern
> + }
> +}
>
> #else /* CONFIG_PM */
>
> @@ -4149,6 +4116,9 @@ void usb_unlocked_enable_lpm(struct usb_device *udev)
> #define hub_resume NULL
> #define hub_reset_resume NULL
>
> +static inline void hub_usb3_port_prepare_disable(struct usb_hub *hub,
> + struct usb_port *port_dev) { }
> +
> int usb_disable_lpm(struct usb_device *udev)
> {
> return 0;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
