Re: [PATCH 1/1] usb: Check if port status is equal to RxDetect

[Alan Stern]

On Fri, 11 Jul 2014, Gavin Guo wrote:

> When using USB 3.0 pen drive with the [AMD] FCH USB XHCI Controller
> [1022:7814], the second hotplugging will experience the USB 3.0 pen
> drive is recognized as high-speed device. After bisecting the kernel,
> I found the commit number 41e7e056cdc662f704fa9262e5c6e213b4ab45dd
> (USB: Allow USB 3.0 ports to be disabled.) causes the bug. After doing
> some experiments, the bug can be fixed by avoiding executing the function
> hub_usb3_port_disable(). Because the port status with [AMD] FCH USB
> XHCI Controlleris [1022:7814] is already in RxDetect
> (I tried printing out the port status before setting to Disabled state),
> it's reasonable to check the port status before really executing
> hub_usb3_port_disable().

This seems like a race.  Even if the port isn't in RxDetect when you 
check it, it could switch to RxDetect before the kernel sets it to 
Disabled.

But maybe this is the best we can do.

> Fixes: 41e7e056cdc6 (USB: Allow USB 3.0 ports to be disabled.)
> Signed-off-by: Gavin Guo <gavin.guo@xxxxxxxxxxxxx>
> ---
>  drivers/usb/core/hub.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 21b99b4..e02ab62 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -889,6 +889,25 @@ static int hub_usb3_port_disable(struct usb_hub *hub, int port1)
>  	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.
> +	 */

Comments should not extend beyond column 80.  And there should be only 
one space, not two, after the '*' in the 6th and 7th lines.

> +	if ((portstatus & USB_PORT_STAT_LINK_STATE) ==
> +				USB_SS_PORT_LS_RX_DETECT) {
> +		dev_dbg(&hub->ports[port1 - 1]->dev,
> +			 "The link state is already in USB_SS_PORT_LS_RX_DETECT\n");

This message does not explain what has happened.  It should say
something like "Not disabling port; link state is RxDetect".

> +		return ret;
> +	}
> +
>  	ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
>  	if (ret)
>  		return ret;

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/