[RFC] xhci: clear port_remote_wakeup on device disconnection

[RFC] xhci: clear port_remote_wakeup on device disconnection

[Nicolas Saenz Julienne]

This was reported on a Dell Precision 5520 using it's WD15 dock. The
dock's Ethernet device interfaces with the laptop through one of it's
USB3 ports. While idle, the Ethernet device and HCD are suspended by
runtime PM, being the only active device connected to the bus. They are
both generally resumed on behalf of the Ethernet device, which has
remote wake-up capabilities.

The Ethernet device was observed randomly disconnecting from the USB
port shortly after submitting it's remote wake-up request. This causes
the PM to busyloop, as the port is in the middle of a remote wake-up
operation, waiting for the device to switch to U0. This leaves a bit set
in "port_remote_wakeup", which stops any further suspend/resume
operations.

The logs look like this (I added the port status print in the first
one):

[28128.472101] xhci_hcd:handle_port_status: xhci_hcd 0000:3e:00.0: port resume event for port 3, port status 0xc4013e3
[28128.472165] xhci_hcd:xhci_hub_control: xhci_hcd 0000:3e:00.0: get port status, actual port 0 status  = 0x1e0
[28128.472200] usbcore:hub_event: hub 4-0:1.0: state 7 ports 2 chg 0002 evt 0000
[28128.472221] usb 4-1: USB disconnect, device number 2

In between the first two logs we acknowledge the remote wake-up request,
setting the link state as U0 and setting the relevant bit in
"port_remote_wakeup". We also call usb_hcd_resume_root_hub() which will
wake up the hub device. Note the CCS & PED bits on the port status are
still set (see 5.4.8 in spec). In between the second and third log, the
hub device finds out about the disconnection (CSS & PED are now 0),
which triggers a call to hub_port_connect() which finds an already
existing USB device and disconnects it. Runtime PM, seeing no devices
connected to the HCD busyloops trying to suspend the device, which fails
every time because of "port_remote_wakeup", generating a tangible CPU
load.

This patch clears "port_remote_wakeup" upon detecting a device
disconnection. Making sure the above mentioned situation doesn't trigger
a PM busyloop.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/host/xhci-hub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index e2eece693655..bea853f45aec 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
 			bus_state->suspended_ports &= ~(1 << portnum);
 	}
 
+	if (!(portsc & PORT_CONNECT))
+		bus_state->port_remote_wakeup &= ~(1 << portnum);
+
 	xhci_hub_report_usb3_link_state(xhci, status, portsc);
 	xhci_del_comp_mod_timer(xhci, portsc, portnum);
 }
-- 
2.21.0

[RFC] xhci: clear port_remote_wakeup on device disconnection

[Nicolas Saenz Julienne]

This was reported on a Dell Precision 5520 using it's WD15 dock. The
dock's Ethernet device interfaces with the laptop through one of it's
USB3 ports. While idle, the Ethernet device and HCD are suspended by
runtime PM, being the only active device connected to the bus. They are
both generally resumed on behalf of the Ethernet device, which has
remote wake-up capabilities.

The Ethernet device was observed randomly disconnecting from the USB
port shortly after submitting it's remote wake-up request. This causes
the PM to busyloop, as the port is in the middle of a remote wake-up
operation, waiting for the device to switch to U0. This leaves a bit set
in "port_remote_wakeup", which stops any further suspend/resume
operations.

The logs look like this (I added the port status print in the first
one):

[28128.472101] xhci_hcd:handle_port_status: xhci_hcd 0000:3e:00.0: port resume event for port 3, port status 0xc4013e3
[28128.472165] xhci_hcd:xhci_hub_control: xhci_hcd 0000:3e:00.0: get port status, actual port 0 status  = 0x1e0
[28128.472200] usbcore:hub_event: hub 4-0:1.0: state 7 ports 2 chg 0002 evt 0000
[28128.472221] usb 4-1: USB disconnect, device number 2

In between the first two logs we acknowledge the remote wake-up request,
setting the link state as U0 and setting the relevant bit in
"port_remote_wakeup". We also call usb_hcd_resume_root_hub() which will
wake up the hub device. Note the CCS & PED bits on the port status are
still set (see 5.4.8 in spec). In between the second and third log, the
hub device finds out about the disconnection (CSS & PED are now 0),
which triggers a call to hub_port_connect() which finds an already
existing USB device and disconnects it. Runtime PM, seeing no devices
connected to the HCD busyloops trying to suspend the device, which fails
every time because of "port_remote_wakeup", generating a tangible CPU
load.

This patch clears "port_remote_wakeup" upon detecting a device
disconnection. Making sure the above mentioned situation doesn't trigger
a PM busyloop.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/host/xhci-hub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index e2eece693655..bea853f45aec 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
 			bus_state->suspended_ports &= ~(1 << portnum);
 	}
 
+	if (!(portsc & PORT_CONNECT))
+		bus_state->port_remote_wakeup &= ~(1 << portnum);
+
 	xhci_hub_report_usb3_link_state(xhci, status, portsc);
 	xhci_del_comp_mod_timer(xhci, portsc, portnum);
 }

Re: [RFC] xhci: clear port_remote_wakeup on device disconnection

[Oliver Neukum]

On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
> 
> This patch clears "port_remote_wakeup" upon detecting a device
> disconnection. Making sure the above mentioned situation doesn't trigger
> a PM busyloop.

Hi,

that is an interesting race condition.

> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/xhci-hub.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index e2eece693655..bea853f45aec 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
>  			bus_state->suspended_ports &= ~(1 << portnum);
>  	}
>  
> +	if (!(portsc & PORT_CONNECT))
> +		bus_state->port_remote_wakeup &= ~(1 << portnum);
> +
>  	xhci_hub_report_usb3_link_state(xhci, status, portsc);
>  	xhci_del_comp_mod_timer(xhci, portsc, portnum);
>  }

Why are you putting that logic into xhci_get_usb3_port_status()?
It looks to me like there is already something related in the caller

        /* USB2 and USB3 specific bits, including Port Link State */
        if (hcd->speed >= HCD_USB3)
                xhci_get_usb3_port_status(port, &status, raw_port_status);
        else
                xhci_get_usb2_port_status(port, &status, raw_port_status,
                                          flags);
        /*
         * Clear stale usb2 resume signalling variables in case port changed
         * state during resume signalling. For example on error
         */
        if ((bus_state->resume_done[wIndex] ||
             test_bit(wIndex, &bus_state->resuming_ports)) &&
            (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
            (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
                bus_state->resume_done[wIndex] = 0;
                clear_bit(wIndex, &bus_state->resuming_ports);
                usb_hcd_end_port_resume(&hcd->self, wIndex);
        }

Otherwise very good catch!

	Regards
		Oliver

[RFC] xhci: clear port_remote_wakeup on device disconnection

[Oliver Neukum]

On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
> 
> This patch clears "port_remote_wakeup" upon detecting a device
> disconnection. Making sure the above mentioned situation doesn't trigger
> a PM busyloop.

Hi,

that is an interesting race condition.

> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/xhci-hub.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index e2eece693655..bea853f45aec 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
>  			bus_state->suspended_ports &= ~(1 << portnum);
>  	}
>  
> +	if (!(portsc & PORT_CONNECT))
> +		bus_state->port_remote_wakeup &= ~(1 << portnum);
> +
>  	xhci_hub_report_usb3_link_state(xhci, status, portsc);
>  	xhci_del_comp_mod_timer(xhci, portsc, portnum);
>  }

Why are you putting that logic into xhci_get_usb3_port_status()?
It looks to me like there is already something related in the caller

        /* USB2 and USB3 specific bits, including Port Link State */
        if (hcd->speed >= HCD_USB3)
                xhci_get_usb3_port_status(port, &status, raw_port_status);
        else
                xhci_get_usb2_port_status(port, &status, raw_port_status,
                                          flags);
        /*
         * Clear stale usb2 resume signalling variables in case port changed
         * state during resume signalling. For example on error
         */
        if ((bus_state->resume_done[wIndex] ||
             test_bit(wIndex, &bus_state->resuming_ports)) &&
            (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
            (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
                bus_state->resume_done[wIndex] = 0;
                clear_bit(wIndex, &bus_state->resuming_ports);
                usb_hcd_end_port_resume(&hcd->self, wIndex);
        }

Otherwise very good catch!

	Regards
		Oliver

Re: [RFC] xhci: clear port_remote_wakeup on device disconnection

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

Hi Oliver, thanks for the review!

On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote:
> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
> > This patch clears "port_remote_wakeup" upon detecting a device
> > disconnection. Making sure the above mentioned situation doesn't trigger
> > a PM busyloop.
> 
> Hi,
> 
> that is an interesting race condition.
> 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-hub.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index e2eece693655..bea853f45aec 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port
> > *port, u32 *status,
> >  			bus_state->suspended_ports &= ~(1 << portnum);
> >  	}
> >  
> > +	if (!(portsc & PORT_CONNECT))
> > +		bus_state->port_remote_wakeup &= ~(1 << portnum);
> > +
> >  	xhci_hub_report_usb3_link_state(xhci, status, portsc);
> >  	xhci_del_comp_mod_timer(xhci, portsc, portnum);
> >  }
> 
> Why are you putting that logic into xhci_get_usb3_port_status()?
> It looks to me like there is already something related in the caller
> 
>         /* USB2 and USB3 specific bits, including Port Link State */
>         if (hcd->speed >= HCD_USB3)
>                 xhci_get_usb3_port_status(port, &status, raw_port_status);
>         else
>                 xhci_get_usb2_port_status(port, &status, raw_port_status,
>                                           flags);
>         /*
>          * Clear stale usb2 resume signalling variables in case port changed
>          * state during resume signalling. For example on error
>          */
>         if ((bus_state->resume_done[wIndex] ||
>              test_bit(wIndex, &bus_state->resuming_ports)) &&
>             (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
>             (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
>                 bus_state->resume_done[wIndex] = 0;
>                 clear_bit(wIndex, &bus_state->resuming_ports);
>                 usb_hcd_end_port_resume(&hcd->self, wIndex);
>         }

You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the
code there.

> 
> Otherwise very good catch!
> 
> 	Regards
> 		Oliver
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC] xhci: clear port_remote_wakeup on device disconnection

[Nicolas Saenz Julienne]

Hi Oliver, thanks for the review!

On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote:
> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
> > This patch clears "port_remote_wakeup" upon detecting a device
> > disconnection. Making sure the above mentioned situation doesn't trigger
> > a PM busyloop.
> 
> Hi,
> 
> that is an interesting race condition.
> 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-hub.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index e2eece693655..bea853f45aec 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port
> > *port, u32 *status,
> >  			bus_state->suspended_ports &= ~(1 << portnum);
> >  	}
> >  
> > +	if (!(portsc & PORT_CONNECT))
> > +		bus_state->port_remote_wakeup &= ~(1 << portnum);
> > +
> >  	xhci_hub_report_usb3_link_state(xhci, status, portsc);
> >  	xhci_del_comp_mod_timer(xhci, portsc, portnum);
> >  }
> 
> Why are you putting that logic into xhci_get_usb3_port_status()?
> It looks to me like there is already something related in the caller
> 
>         /* USB2 and USB3 specific bits, including Port Link State */
>         if (hcd->speed >= HCD_USB3)
>                 xhci_get_usb3_port_status(port, &status, raw_port_status);
>         else
>                 xhci_get_usb2_port_status(port, &status, raw_port_status,
>                                           flags);
>         /*
>          * Clear stale usb2 resume signalling variables in case port changed
>          * state during resume signalling. For example on error
>          */
>         if ((bus_state->resume_done[wIndex] ||
>              test_bit(wIndex, &bus_state->resuming_ports)) &&
>             (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
>             (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
>                 bus_state->resume_done[wIndex] = 0;
>                 clear_bit(wIndex, &bus_state->resuming_ports);
>                 usb_hcd_end_port_resume(&hcd->self, wIndex);
>         }

You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the
code there.

> 
> Otherwise very good catch!
> 
> 	Regards
> 		Oliver
>

Re: [RFC] xhci: clear port_remote_wakeup on device disconnection

[Mathias Nyman]

On 19.3.2019 16.54, Nicolas Saenz Julienne wrote:
> Hi Oliver, thanks for the review!
> 
> On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote:
>> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
>>> This patch clears "port_remote_wakeup" upon detecting a device
>>> disconnection. Making sure the above mentioned situation doesn't trigger
>>> a PM busyloop.
>>
>> Hi,
>>
>> that is an interesting race condition.
>>

Turns out that port_remote_wakeup was only properly cleared in case of a
successful resume where port stays in U0, or in case of an error where port
goes to inactive state.

In all other cases port_remote_wakeup bit was left uncleared.

>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> ---
>>>   drivers/usb/host/xhci-hub.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>> index e2eece693655..bea853f45aec 100644
>>> --- a/drivers/usb/host/xhci-hub.c
>>> +++ b/drivers/usb/host/xhci-hub.c
>>> @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port
>>> *port, u32 *status,
>>>   			bus_state->suspended_ports &= ~(1 << portnum);
>>>   	}
>>>   
>>> +	if (!(portsc & PORT_CONNECT))
>>> +		bus_state->port_remote_wakeup &= ~(1 << portnum);
>>> +

A check like this should cover the failing resume cases, thanks

We also saw a case when a successful resume was missed as port went into U1
Link power save state (resume->U0->U1) before xhci driver read the port status.

A patch adding U1 and U2 to successful resume states is pending in my for-usb-linus
branch for this.

>>
>> Why are you putting that logic into xhci_get_usb3_port_status()?
>> It looks to me like there is already something related in the caller
> 
> You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the
> code there.
> 

Sounds good, thanks

-Mathias

[RFC] xhci: clear port_remote_wakeup on device disconnection

[Mathias Nyman]

On 19.3.2019 16.54, Nicolas Saenz Julienne wrote:
> Hi Oliver, thanks for the review!
> 
> On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote:
>> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
>>> This patch clears "port_remote_wakeup" upon detecting a device
>>> disconnection. Making sure the above mentioned situation doesn't trigger
>>> a PM busyloop.
>>
>> Hi,
>>
>> that is an interesting race condition.
>>

Turns out that port_remote_wakeup was only properly cleared in case of a
successful resume where port stays in U0, or in case of an error where port
goes to inactive state.

In all other cases port_remote_wakeup bit was left uncleared.

>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> ---
>>>   drivers/usb/host/xhci-hub.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>> index e2eece693655..bea853f45aec 100644
>>> --- a/drivers/usb/host/xhci-hub.c
>>> +++ b/drivers/usb/host/xhci-hub.c
>>> @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port
>>> *port, u32 *status,
>>>   			bus_state->suspended_ports &= ~(1 << portnum);
>>>   	}
>>>   
>>> +	if (!(portsc & PORT_CONNECT))
>>> +		bus_state->port_remote_wakeup &= ~(1 << portnum);
>>> +

A check like this should cover the failing resume cases, thanks

We also saw a case when a successful resume was missed as port went into U1
Link power save state (resume->U0->U1) before xhci driver read the port status.

A patch adding U1 and U2 to successful resume states is pending in my for-usb-linus
branch for this.

>>
>> Why are you putting that logic into xhci_get_usb3_port_status()?
>> It looks to me like there is already something related in the caller
> 
> You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the
> code there.
> 

Sounds good, thanks

-Mathias