Re: [PATCH v2 1/1] usb: hub: Fix auto-remount of safely removed or ejected USB-3 devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: stable@xxxxxxxxxxxxxxx
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> 
> ---
> 
> 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 <stern@xxxxxxxxxxxxxxxxxxx>

> 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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]