diff mbox series

[2/2] usb: core: Attempt power cycle when port is in eSS.Disabled state

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

Commit Message

Kai-Heng Feng Oct. 7, 2019, 6:28 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. The USB spec mentioned this can be
caused by invalid VBus, and after some expiremets, it does show that the
SMSC hub can be brought back after a power cycle.

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>
---
 drivers/usb/core/hub.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Kai-Heng Feng Oct. 30, 2019, 8:05 a.m. UTC | #1 
> On Oct 8, 2019, at 02:28, 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. The USB spec mentioned this can be
> caused by invalid VBus, and after some expiremets, it does show that the
> SMSC hub can be brought back after a power cycle.
> 
> So let's power cycle the port at the end of reset resume attempt, if
> it's in eSS.Disabled state.

Is there any suggestion to make this series get merged?

Kai-Heng

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/usb/core/hub.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 6655a6a1651b..5f50aca7cf67 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2739,20 +2739,33 @@ 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];
> 	int ret;
> 
> 	ret = usb_hub_set_port_power(hub, port1, false);
> 	if (ret) {
> -		dev_info(&udev->dev, "failed to disable port power\n");
> +		dev_info(&port_dev->dev, "failed to disable port power\n");
> 		return;
> 	}
> 
> 	msleep(2 * hub_power_on_good_delay(hub));
> 	ret = usb_hub_set_port_power(hub, port1, true);
> 	if (ret) {
> -		dev_info(&udev->dev, "failed to enable port power\n");
> +		dev_info(&port_dev->dev, "failed to enable port power\n");
> 		return;
> 	}
> 
> @@ -3600,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_info(&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
>
Greg KH Nov. 4, 2019, 2:38 p.m. UTC | #2 
On Tue, Oct 08, 2019 at 02:28:40AM +0800, Kai-Heng Feng 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. The USB spec mentioned this can be
> caused by invalid VBus, and after some expiremets, it does show that the
> SMSC hub can be brought back after a power cycle.
> 
> 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>
> ---
>  drivers/usb/core/hub.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 6655a6a1651b..5f50aca7cf67 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2739,20 +2739,33 @@ 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];
>  	int ret;
>  
>  	ret = usb_hub_set_port_power(hub, port1, false);
>  	if (ret) {
> -		dev_info(&udev->dev, "failed to disable port power\n");
> +		dev_info(&port_dev->dev, "failed to disable port power\n");
>  		return;
>  	}
>  
>  	msleep(2 * hub_power_on_good_delay(hub));
>  	ret = usb_hub_set_port_power(hub, port1, true);
>  	if (ret) {
> -		dev_info(&udev->dev, "failed to enable port power\n");
> +		dev_info(&port_dev->dev, "failed to enable port power\n");
>  		return;
>  	}
>  
> @@ -3600,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_info(&udev->dev, "device in disabled state, attempt power cycle\n");

Why dev_info()?  Shouldn't we only care if this fails?

> +			hub_port_power_cycle(hub, port1);

Weren't we only going to do this for the broken types of devices?  And
not for everything?

thanks,

greg k-h
Kai-Heng Feng Nov. 5, 2019, 5:14 a.m. UTC | #3 
> On Nov 4, 2019, at 10:38 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Oct 08, 2019 at 02:28:40AM +0800, Kai-Heng Feng 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. The USB spec mentioned this can be
>> caused by invalid VBus, and after some expiremets, it does show that the
>> SMSC hub can be brought back after a power cycle.
>> 
>> 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>
>> ---
>> drivers/usb/core/hub.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 6655a6a1651b..5f50aca7cf67 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2739,20 +2739,33 @@ 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];
>> 	int ret;
>> 
>> 	ret = usb_hub_set_port_power(hub, port1, false);
>> 	if (ret) {
>> -		dev_info(&udev->dev, "failed to disable port power\n");
>> +		dev_info(&port_dev->dev, "failed to disable port power\n");
>> 		return;
>> 	}
>> 
>> 	msleep(2 * hub_power_on_good_delay(hub));
>> 	ret = usb_hub_set_port_power(hub, port1, true);
>> 	if (ret) {
>> -		dev_info(&udev->dev, "failed to enable port power\n");
>> +		dev_info(&port_dev->dev, "failed to enable port power\n");
>> 		return;
>> 	}
>> 
>> @@ -3600,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_info(&udev->dev, "device in disabled state, attempt power cycle\n");
> 
> Why dev_info()?  Shouldn't we only care if this fails?

I’ll lower the level to dev_dbg().

> 
>> +			hub_port_power_cycle(hub, port1);
> 
> Weren't we only going to do this for the broken types of devices?  And
> not for everything?

From what I can understand from the spec, if the device is in eSS.Disabled state, there’s no way out.
So "power cycling as a last resort” is indeed targets everything.

Kai-Heng

> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 6655a6a1651b..5f50aca7cf67 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2739,20 +2739,33 @@  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];
 	int ret;
 
 	ret = usb_hub_set_port_power(hub, port1, false);
 	if (ret) {
-		dev_info(&udev->dev, "failed to disable port power\n");
+		dev_info(&port_dev->dev, "failed to disable port power\n");
 		return;
 	}
 
 	msleep(2 * hub_power_on_good_delay(hub));
 	ret = usb_hub_set_port_power(hub, port1, true);
 	if (ret) {
-		dev_info(&udev->dev, "failed to enable port power\n");
+		dev_info(&port_dev->dev, "failed to enable port power\n");
 		return;
 	}
 
@@ -3600,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_info(&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);