[PATCH 0/1] Fixes to the USB 3.0 detection as 2.0 on AMD platform

[PATCH 0/1] Fixes to the USB 3.0 detection as 2.0 on AMD platform

[Gavin Guo]

Hi Sarah and Mathias,

As the discussion in http://comments.gmane.org/gmane.linux.usb.general/107011, 
I found that [AMD] FCH USB XHCI Controller [1022:7814] the USB 3.0 disk 
can't work in SuperSpeed after several times of hotplug. After doing some 
experiments and bisection, I found the bug is caused by 
41e7e056cdc662f704fa9262e5c6e213b4ab45dd (USB: Allow USB 3.0 ports to be 
disabled.). And the bug can be fixed by not executing the 
hub_usb3_port_disable() function. I also found that the port status is 
already in RxDetect before setting the port to Disabled in 
hub_usb3_port_disable() function. So, there are 2 ways to fix the bug:

1) Check if the Vendor/Device id is [1022:7814] at the beginning of 
hub_usb3_port_disable() function. If yes, return without executing the 
remaining code.

2) Check if the port status is already in RxDetect, if yes, return without 
executing the remaining code.

The second method seems more reasonable, so the patch is the implementation
of the second one. But it will affect more platforms and I don't know if 
there'll be any negative result. Otherwise, if the first one is correct, 
I can reimplement a new one.

I'm appreciated if you can give me some advice, or if there is any thing I missed.

Thanks,
Gavin

Gavin Guo (1):
  usb: Check if port status is equal to RxDetect

 drivers/usb/core/hub.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.0.0

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

[Gavin Guo]

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().

Fixes: 41e7e056cdc6 (USB: Allow USB 3.0 ports to be disabled.)
Signed-off-by: Gavin Guo <gavin.guo@canonical.com>
---
 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.
+	 */
+	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");
+		return ret;
+	}
+
 	ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
 	if (ret)
 		return ret;
-- 
2.0.0

Re: [PATCH 0/1] Fixes to the USB 3.0 detection as 2.0 on AMD platform

[Gavin Guo]

Hi all,

On Fri, Jul 11, 2014 at 2:22 PM, Gavin Guo <gavin.guo@canonical.com> wrote:
> Hi Sarah and Mathias,
>
> As the discussion in http://comments.gmane.org/gmane.linux.usb.general/107011,
> I found that [AMD] FCH USB XHCI Controller [1022:7814] the USB 3.0 disk
> can't work in SuperSpeed after several times of hotplug. After doing some
> experiments and bisection, I found the bug is caused by
> 41e7e056cdc662f704fa9262e5c6e213b4ab45dd (USB: Allow USB 3.0 ports to be
> disabled.). And the bug can be fixed by not executing the
> hub_usb3_port_disable() function. I also found that the port status is
> already in RxDetect before setting the port to Disabled in
> hub_usb3_port_disable() function. So, there are 2 ways to fix the bug:
>
> 1) Check if the Vendor/Device id is [1022:7814] at the beginning of
> hub_usb3_port_disable() function. If yes, return without executing the
> remaining code.
>
> 2) Check if the port status is already in RxDetect, if yes, return without
> executing the remaining code.
>
> The second method seems more reasonable, so the patch is the implementation
> of the second one. But it will affect more platforms and I don't know if
> there'll be any negative result. Otherwise, if the first one is correct,
> I can reimplement a new one.
>
> I'm appreciated if you can give me some advice, or if there is any thing I missed.
>
> Thanks,
> Gavin
>
> Gavin Guo (1):
>   usb: Check if port status is equal to RxDetect
>
>  drivers/usb/core/hub.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> --
> 2.0.0
>

Add Greg k-h to the list.

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

[Gavin Guo]

Hi all,

On Fri, Jul 11, 2014 at 2:22 PM, Gavin Guo <gavin.guo@canonical.com> 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().
>
> Fixes: 41e7e056cdc6 (USB: Allow USB 3.0 ports to be disabled.)
> Signed-off-by: Gavin Guo <gavin.guo@canonical.com>
> ---
>  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.
> +        */
> +       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");
> +               return ret;
> +       }
> +
>         ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
>         if (ret)
>                 return ret;
> --
> 2.0.0
>

Add Greg k-h to the list.

Re: [PATCH 0/1] Fixes to the USB 3.0 detection as 2.0 on AMD platform

[Greg KH]

On Sat, Jul 12, 2014 at 07:32:34AM +0800, Gavin Guo wrote:
> Hi all,
> 
> On Fri, Jul 11, 2014 at 2:22 PM, Gavin Guo <gavin.guo@canonical.com> wrote:
> > Hi Sarah and Mathias,
> >
> > As the discussion in http://comments.gmane.org/gmane.linux.usb.general/107011,
> > I found that [AMD] FCH USB XHCI Controller [1022:7814] the USB 3.0 disk
> > can't work in SuperSpeed after several times of hotplug. After doing some
> > experiments and bisection, I found the bug is caused by
> > 41e7e056cdc662f704fa9262e5c6e213b4ab45dd (USB: Allow USB 3.0 ports to be
> > disabled.). And the bug can be fixed by not executing the
> > hub_usb3_port_disable() function. I also found that the port status is
> > already in RxDetect before setting the port to Disabled in
> > hub_usb3_port_disable() function. So, there are 2 ways to fix the bug:
> >
> > 1) Check if the Vendor/Device id is [1022:7814] at the beginning of
> > hub_usb3_port_disable() function. If yes, return without executing the
> > remaining code.
> >
> > 2) Check if the port status is already in RxDetect, if yes, return without
> > executing the remaining code.
> >
> > The second method seems more reasonable, so the patch is the implementation
> > of the second one. But it will affect more platforms and I don't know if
> > there'll be any negative result. Otherwise, if the first one is correct,
> > I can reimplement a new one.
> >
> > I'm appreciated if you can give me some advice, or if there is any thing I missed.
> >
> > Thanks,
> > Gavin
> >
> > Gavin Guo (1):
> >   usb: Check if port status is equal to RxDetect
> >
> >  drivers/usb/core/hub.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > --
> > 2.0.0
> >
> 
> Add Greg k-h to the list.

Why?  Mathias can handle this just fine, right?

And I read all linux-usb@vger email anyway...

greg k-h

Re: [PATCH 0/1] Fixes to the USB 3.0 detection as 2.0 on AMD platform

[Gavin Guo]

Hi Greg,

On Sat, Jul 12, 2014 at 8:04 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Jul 12, 2014 at 07:32:34AM +0800, Gavin Guo wrote:
>> Hi all,
>>
>> On Fri, Jul 11, 2014 at 2:22 PM, Gavin Guo <gavin.guo@canonical.com> wrote:
>> > Hi Sarah and Mathias,
>> >
>> > As the discussion in http://comments.gmane.org/gmane.linux.usb.general/107011,
>> > I found that [AMD] FCH USB XHCI Controller [1022:7814] the USB 3.0 disk
>> > can't work in SuperSpeed after several times of hotplug. After doing some
>> > experiments and bisection, I found the bug is caused by
>> > 41e7e056cdc662f704fa9262e5c6e213b4ab45dd (USB: Allow USB 3.0 ports to be
>> > disabled.). And the bug can be fixed by not executing the
>> > hub_usb3_port_disable() function. I also found that the port status is
>> > already in RxDetect before setting the port to Disabled in
>> > hub_usb3_port_disable() function. So, there are 2 ways to fix the bug:
>> >
>> > 1) Check if the Vendor/Device id is [1022:7814] at the beginning of
>> > hub_usb3_port_disable() function. If yes, return without executing the
>> > remaining code.
>> >
>> > 2) Check if the port status is already in RxDetect, if yes, return without
>> > executing the remaining code.
>> >
>> > The second method seems more reasonable, so the patch is the implementation
>> > of the second one. But it will affect more platforms and I don't know if
>> > there'll be any negative result. Otherwise, if the first one is correct,
>> > I can reimplement a new one.
>> >
>> > I'm appreciated if you can give me some advice, or if there is any thing I missed.
>> >
>> > Thanks,
>> > Gavin
>> >
>> > Gavin Guo (1):
>> >   usb: Check if port status is equal to RxDetect
>> >
>> >  drivers/usb/core/hub.c | 19 +++++++++++++++++++
>> >  1 file changed, 19 insertions(+)
>> >
>> > --
>> > 2.0.0
>> >
>>
>> Add Greg k-h to the list.
>
> Why?  Mathias can handle this just fine, right?
>
> And I read all linux-usb@vger email anyway...
>
> greg k-h

I'm sorry if there is any misunderstanding. Because the code is under
drivers/usb/core/hub.c, I found that in MAINTAINERS you are the
maintainer of drivers/usb/*. so I think it's better to let you know
the change.

Thanks,
Gavin Guo

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@canonical.com>
> ---
>  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