diff mbox series

[v2,2/2] USB: core: Attempt power cycle port when it's in eSS.Disabled state

Message ID 20191129174115.31683-2-kai.heng.feng@canonical.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] USB: core: Make port power cycle a seperate helper function | expand

Commit Message

Kai-Heng Feng Nov. 29, 2019, 5:41 p.m. UTC 
On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
then connects to ASMedia xHCI's root hub:

/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
            |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M

Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter

The SMSC hub may disconnect after system resume from suspend. When this
happens, the reset resume attempt fails, and the last resort to disable
the port and see something comes up later, also fails.

When the issue occurs, the link state stays in eSS.Disabled state
despite the warm reset attempts. Accoding to spec this can be caused by
invalid VBus, after some expiremets, the SMSC hub can be brought back
after a powercycle.

So let's power cycle the port at the end of reset resume attempt, if
it's in eSS.Disabled state.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
- Lower dev_info() to dev_dbg().

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

Comments

Kai-Heng Feng Dec. 11, 2019, 1:45 p.m. UTC | #1 
> On Nov 30, 2019, at 01:41, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
> then connects to ASMedia xHCI's root hub:
> 
> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>            |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
> 
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
> 
> The SMSC hub may disconnect after system resume from suspend. When this
> happens, the reset resume attempt fails, and the last resort to disable
> the port and see something comes up later, also fails.
> 
> When the issue occurs, the link state stays in eSS.Disabled state
> despite the warm reset attempts. Accoding to spec this can be caused by
> invalid VBus, after some expiremets, the SMSC hub can be brought back
> after a powercycle.
> 
> So let's power cycle the port at the end of reset resume attempt, if
> it's in eSS.Disabled state.

If there's no objection, can this fix be reviewed and merged?

Kai-Heng

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
> - Lower dev_info() to dev_dbg().
> 
> drivers/usb/core/hub.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 6b6cd76ac5e6..a2e6001046f5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2739,6 +2739,18 @@ static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
> 		|| link_state == USB_SS_PORT_LS_COMP_MOD;
> }
> 
> +static bool hub_port_power_cycle_required(struct usb_hub *hub, int port1,
> +		u16 portstatus)
> +{
> +	u16 link_state;
> +
> +	if (!hub_is_superspeed(hub->hdev))
> +		return false;
> +
> +	link_state = portstatus & USB_PORT_STAT_LINK_STATE;
> +	return link_state == USB_SS_PORT_LS_SS_DISABLED;
> +}
> +
> static void hub_port_power_cycle(struct usb_hub *hub, int port1)
> {
> 	struct usb_port *port_dev = hub->ports[port1  - 1];
> @@ -3601,6 +3613,10 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> 	if (status < 0) {
> 		dev_dbg(&udev->dev, "can't resume, status %d\n", status);
> 		hub_port_logical_disconnect(hub, port1);
> +		if (hub_port_power_cycle_required(hub, port1, portstatus)) {
> +			dev_dbg(&udev->dev, "device in disabled state, attempt power cycle\n");
> +			hub_port_power_cycle(hub, port1);
> +		}
> 	} else  {
> 		/* Try to enable USB2 hardware LPM */
> 		usb_enable_usb2_hardware_lpm(udev);
> -- 
> 2.17.1
>
Alan Stern Dec. 11, 2019, 3:08 p.m. UTC | #2 
On Wed, 11 Dec 2019, Kai-Heng Feng wrote:

> 
> 
> > On Nov 30, 2019, at 01:41, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> > 
> > On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
> > then connects to ASMedia xHCI's root hub:
> > 
> > /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
> >    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
> >            |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
> > 
> > Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
> > Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
> > 
> > The SMSC hub may disconnect after system resume from suspend. When this
> > happens, the reset resume attempt fails, and the last resort to disable
> > the port and see something comes up later, also fails.
> > 
> > When the issue occurs, the link state stays in eSS.Disabled state
> > despite the warm reset attempts. Accoding to spec this can be caused by
> > invalid VBus, after some expiremets, the SMSC hub can be brought back
> > after a powercycle.
> > 
> > So let's power cycle the port at the end of reset resume attempt, if
> > it's in eSS.Disabled state.
> 
> If there's no objection, can this fix be reviewed and merged?
> 
> Kai-Heng

The patch is okay as far as I'm concerned, since it doesn't do anything 
for non-SuperSpeed connections.  It should be reviewed by Mathias.

Alan Stern

> > 
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v2:
> > - Lower dev_info() to dev_dbg().
> > 
> > drivers/usb/core/hub.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 6b6cd76ac5e6..a2e6001046f5 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2739,6 +2739,18 @@ static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
> > 		|| link_state == USB_SS_PORT_LS_COMP_MOD;
> > }
> > 
> > +static bool hub_port_power_cycle_required(struct usb_hub *hub, int port1,
> > +		u16 portstatus)
> > +{
> > +	u16 link_state;
> > +
> > +	if (!hub_is_superspeed(hub->hdev))
> > +		return false;
> > +
> > +	link_state = portstatus & USB_PORT_STAT_LINK_STATE;
> > +	return link_state == USB_SS_PORT_LS_SS_DISABLED;
> > +}
> > +
> > static void hub_port_power_cycle(struct usb_hub *hub, int port1)
> > {
> > 	struct usb_port *port_dev = hub->ports[port1  - 1];
> > @@ -3601,6 +3613,10 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> > 	if (status < 0) {
> > 		dev_dbg(&udev->dev, "can't resume, status %d\n", status);
> > 		hub_port_logical_disconnect(hub, port1);
> > +		if (hub_port_power_cycle_required(hub, port1, portstatus)) {
> > +			dev_dbg(&udev->dev, "device in disabled state, attempt power cycle\n");
> > +			hub_port_power_cycle(hub, port1);
> > +		}
> > 	} else  {
> > 		/* Try to enable USB2 hardware LPM */
> > 		usb_enable_usb2_hardware_lpm(udev);
> > -- 
> > 2.17.1
Mathias Nyman Dec. 17, 2019, 11:16 a.m. UTC | #3 
On 11.12.2019 17.08, Alan Stern wrote:
> On Wed, 11 Dec 2019, Kai-Heng Feng wrote:
> 
>>
>>
>>> On Nov 30, 2019, at 01:41, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>
>>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>>> then connects to ASMedia xHCI's root hub:
>>>
>>> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>>>             |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>>
>>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>>
>>> The SMSC hub may disconnect after system resume from suspend. When this
>>> happens, the reset resume attempt fails, and the last resort to disable
>>> the port and see something comes up later, also fails.
>>>
>>> When the issue occurs, the link state stays in eSS.Disabled state
>>> despite the warm reset attempts. Accoding to spec this can be caused by
>>> invalid VBus, after some expiremets, the SMSC hub can be brought back
>>> after a powercycle.

Could you open up a bit more how this happens, mainly codepaths how the
USB3 port ends up in eSS.Disabled state

There might be something else wrong here.
My impression is that port should only end up in eSS.Disabled when directed
to with a Set_Port_Feature(PORT_LINK_STATE, eSS.Disabled) Request.
After this we shouldn't attempt warm resets, they won't work for USB3 ports in
ss.Disabled state.

-Mathias
Kai-Heng Feng Dec. 23, 2019, 5:32 p.m. UTC | #4 
> On Dec 17, 2019, at 19:16, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> 
> On 11.12.2019 17.08, Alan Stern wrote:
>> On Wed, 11 Dec 2019, Kai-Heng Feng wrote:
>>> 
>>> 
>>>> On Nov 30, 2019, at 01:41, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>> 
>>>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>>>> then connects to ASMedia xHCI's root hub:
>>>> 
>>>> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>>>>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>>>>            |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>>> 
>>>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>>>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>>>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>>> 
>>>> The SMSC hub may disconnect after system resume from suspend. When this
>>>> happens, the reset resume attempt fails, and the last resort to disable
>>>> the port and see something comes up later, also fails.
>>>> 
>>>> When the issue occurs, the link state stays in eSS.Disabled state
>>>> despite the warm reset attempts. Accoding to spec this can be caused by
>>>> invalid VBus, after some expiremets, the SMSC hub can be brought back
>>>> after a powercycle.
> 
> Could you open up a bit more how this happens, mainly codepaths how the
> USB3 port ends up in eSS.Disabled state

It happens right after hub_resume(), the first get_port_status():
Dec 23 09:46:58 u-XPS-13-9370 kernel: usb usb4: usb resume
Dec 23 09:46:58 u-XPS-13-9370 kernel: hub 4-0:1.0: hub_resume
Dec 23 09:46:58 u-XPS-13-9370 kernel: xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
Dec 23 09:46:58 u-XPS-13-9370 kernel: usb usb4-port1: status 0280 change 0000
Dec 23 09:46:58 u-XPS-13-9370 kernel: xhci_hcd 0000:3f:00.0: Get port status 4-2 read: 0x2a0, return 0x2a0
Dec 23 09:46:58 u-XPS-13-9370 kernel: xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
Dec 23 09:46:58 u-XPS-13-9370 kernel: xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
Dec 23 09:46:58 u-XPS-13-9370 kernel: xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280

And it keeps getting 0x280. It should mean PP = 1 and PLS = 4, so it's in Disabled state.

> 
> There might be something else wrong here.
> My impression is that port should only end up in eSS.Disabled when directed
> to with a Set_Port_Feature(PORT_LINK_STATE, eSS.Disabled) Request.
> After this we shouldn't attempt warm resets, they won't work for USB3 ports in
> ss.Disabled state.

Do you suggest that we should power cycle the port in xHCI instead of USB core?

Kai-Heng

> 
> -Mathias
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 6b6cd76ac5e6..a2e6001046f5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2739,6 +2739,18 @@  static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
 		|| link_state == USB_SS_PORT_LS_COMP_MOD;
 }
 
+static bool hub_port_power_cycle_required(struct usb_hub *hub, int port1,
+		u16 portstatus)
+{
+	u16 link_state;
+
+	if (!hub_is_superspeed(hub->hdev))
+		return false;
+
+	link_state = portstatus & USB_PORT_STAT_LINK_STATE;
+	return link_state == USB_SS_PORT_LS_SS_DISABLED;
+}
+
 static void hub_port_power_cycle(struct usb_hub *hub, int port1)
 {
 	struct usb_port *port_dev = hub->ports[port1  - 1];
@@ -3601,6 +3613,10 @@  int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	if (status < 0) {
 		dev_dbg(&udev->dev, "can't resume, status %d\n", status);
 		hub_port_logical_disconnect(hub, port1);
+		if (hub_port_power_cycle_required(hub, port1, portstatus)) {
+			dev_dbg(&udev->dev, "device in disabled state, attempt power cycle\n");
+			hub_port_power_cycle(hub, port1);
+		}
 	} else  {
 		/* Try to enable USB2 hardware LPM */
 		usb_enable_usb2_hardware_lpm(udev);