Re: [PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
On Thu, Dec 13, 2018 at 02:24:14PM +0200, Mathias Nyman wrote:
> On 13.12.2018 09:36, Greg Kroah-Hartman wrote:
> > On Wed, Dec 12, 2018 at 11:53:34PM +0100, Thomas Zeitlhofer wrote:
> > > Hello,
> > >
> > > On Thu, Nov 29, 2018 at 03:11:45PM +0100, Greg Kroah-Hartman wrote:
> > > > 4.19-stable review patch. If anyone has any objections, please let me
> > > > know.
> > > >
> > > > --
> > > >
> > > > From: Mathias Nyman
> > > >
> > > > commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream.
> > > [...]
> > >
> > > on a current Thinkpad X1 Yoga, this breaks resume from hibernate such
> > > that opening the lid has (in the regular use case, see below) no effect
> > > any more:
> > >
> > > The system is configured to hibernate when the lid is closed. So, the
> > > expected behavior, which is restored by reverting this patch, is:
> > >
> > > close lid => system hibernates
> > > open lid => system resumes
> > >
> > > With this patch, the following two cases are observed:
> > >
> > > 1)
> > > close lid => system hibernates
> > > open lid => system stays off
> > > press power button => system boots and resumes
> > >
> > > 2)
> > > # systemctl hibernate => system hibernates
> > > close lid
> > > open lid => system resumes
> > >
> >
> > So this is a problem in Linus's tree? If so, let's work to get this
> > fixed there first.
> >
> > If not, then we have other issues :)
> >
>
> That patch incorrectly reacts to USB2 polling states as well,
> which could cause issues like this.
>
> Does applying the below code help?
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 94aca1b..01b5818 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1507,7 +1507,8 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> portsc_buf[port_index] = 0;
> /* Bail out if a USB3 port has a new device in link training
> */
> - if ((t1 & PORT_PLS_MASK) == XDEV_POLLING) {
> + if ((hcd->speed >= HCD_USB3) &&
> + (t1 & PORT_PLS_MASK) == XDEV_POLLING) {
> bus_state->bus_suspended = 0;
> spin_unlock_irqrestore(>lock, flags);
> xhci_dbg(xhci, "Bus suspend bailout, port in
> polling\n");
Yes, this fixes the problem.
Thanks,
Thomas
Re: [PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
On 13.12.2018 09:36, Greg Kroah-Hartman wrote:
On Wed, Dec 12, 2018 at 11:53:34PM +0100, Thomas Zeitlhofer wrote:
Hello,
On Thu, Nov 29, 2018 at 03:11:45PM +0100, Greg Kroah-Hartman wrote:
4.19-stable review patch. If anyone has any objections, please let me
know.
--
From: Mathias Nyman
commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream.
[...]
on a current Thinkpad X1 Yoga, this breaks resume from hibernate such
that opening the lid has (in the regular use case, see below) no effect
any more:
The system is configured to hibernate when the lid is closed. So, the
expected behavior, which is restored by reverting this patch, is:
close lid => system hibernates
open lid => system resumes
With this patch, the following two cases are observed:
1)
close lid => system hibernates
open lid => system stays off
press power button => system boots and resumes
2)
# systemctl hibernate => system hibernates
close lid
open lid => system resumes
So this is a problem in Linus's tree? If so, let's work to get this
fixed there first.
If not, then we have other issues :)
That patch incorrectly reacts to USB2 polling states as well,
which could cause issues like this.
Does applying the below code help?
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 94aca1b..01b5818 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1507,7 +1507,8 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
portsc_buf[port_index] = 0;
/* Bail out if a USB3 port has a new device in link training */
- if ((t1 & PORT_PLS_MASK) == XDEV_POLLING) {
+ if ((hcd->speed >= HCD_USB3) &&
+ (t1 & PORT_PLS_MASK) == XDEV_POLLING) {
bus_state->bus_suspended = 0;
spin_unlock_irqrestore(>lock, flags);
xhci_dbg(xhci, "Bus suspend bailout, port in
polling\n");
Re: [PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
On Wed, Dec 12, 2018 at 11:53:34PM +0100, Thomas Zeitlhofer wrote: > Hello, > > On Thu, Nov 29, 2018 at 03:11:45PM +0100, Greg Kroah-Hartman wrote: > > 4.19-stable review patch. If anyone has any objections, please let me > > know. > > > > -- > > > > From: Mathias Nyman > > > > commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. > [...] > > on a current Thinkpad X1 Yoga, this breaks resume from hibernate such > that opening the lid has (in the regular use case, see below) no effect > any more: > > The system is configured to hibernate when the lid is closed. So, the > expected behavior, which is restored by reverting this patch, is: > > close lid => system hibernates > open lid => system resumes > > With this patch, the following two cases are observed: > > 1) > close lid => system hibernates > open lid => system stays off > press power button => system boots and resumes > > 2) > # systemctl hibernate => system hibernates > close lid > open lid => system resumes > So this is a problem in Linus's tree? If so, let's work to get this fixed there first. If not, then we have other issues :) thanks, greg k-h
Re: [PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
Hello, On Thu, Nov 29, 2018 at 03:11:45PM +0100, Greg Kroah-Hartman wrote: > 4.19-stable review patch. If anyone has any objections, please let me > know. > > -- > > From: Mathias Nyman > > commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. [...] on a current Thinkpad X1 Yoga, this breaks resume from hibernate such that opening the lid has (in the regular use case, see below) no effect any more: The system is configured to hibernate when the lid is closed. So, the expected behavior, which is restored by reverting this patch, is: close lid => system hibernates open lid => system resumes With this patch, the following two cases are observed: 1) close lid => system hibernates open lid => system stays off press power button => system boots and resumes 2) # systemctl hibernate => system hibernates close lid open lid => system resumes Regards, Thomas
Re: [PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
[PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
4.19-stable review patch. If anyone has any objections, please let me know.
--
From: Mathias Nyman
commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream.
USB3 roothub might autosuspend before a plugged USB3 device is detected,
causing USB3 device enumeration failure.
USB3 devices don't show up as connected and enabled until USB3 link trainig
completes. On a fast booting platform with a slow USB3 link training the
link might reach the connected enabled state just as the bus is suspending.
If this device is discovered first time by the xhci_bus_suspend() routine
it will be put to U3 suspended state like the other ports which failed to
suspend earlier.
The hub thread will notice the connect change and resume the bus,
moving the port back to U0
This U0 -> U3 -> U0 transition right after being connected seems to be
too much for some devices, causing them to first go to SS.Inactive state,
and finally end up stuck in a polling state with reset asserted
Fix this by failing the bus suspend if a port has a connect change or is
in a polling state in xhci_bus_suspend().
Don't do any port changes until all ports are checked, buffer all port
changes and only write them in the end if suspend can proceed
Cc: sta...@vger.kernel.org
Signed-off-by: Mathias Nyman
Signed-off-by: Greg Kroah-Hartman
---
drivers/usb/host/xhci-hub.c | 60 +---
1 file changed, 46 insertions(+), 14 deletions(-)
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1474,15 +1474,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd
unsigned long flags;
struct xhci_hub *rhub;
struct xhci_port **ports;
+ u32 portsc_buf[USB_MAXCHILDREN];
+ bool wake_enabled;
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
bus_state = >bus_state[hcd_index(hcd)];
+ wake_enabled = hcd->self.root_hub->do_remote_wakeup;
spin_lock_irqsave(>lock, flags);
- if (hcd->self.root_hub->do_remote_wakeup) {
+ if (wake_enabled) {
if (bus_state->resuming_ports ||/* USB2 */
bus_state->port_remote_wakeup) {/* USB3 */
spin_unlock_irqrestore(>lock, flags);
@@ -1490,26 +1493,36 @@ int xhci_bus_suspend(struct usb_hcd *hcd
return -EBUSY;
}
}
-
- port_index = max_ports;
+ /*
+* Prepare ports for suspend, but don't write anything before all ports
+* are checked and we know bus suspend can proceed
+*/
bus_state->bus_suspended = 0;
+ port_index = max_ports;
while (port_index--) {
- /* suspend the port if the port is not suspended */
u32 t1, t2;
- int slot_id;
t1 = readl(ports[port_index]->addr);
t2 = xhci_port_state_to_neutral(t1);
+ portsc_buf[port_index] = 0;
- if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
- xhci_dbg(xhci, "port %d not suspended\n", port_index);
- slot_id = xhci_find_slot_id_by_port(hcd, xhci,
- port_index + 1);
- if (slot_id) {
+ /* Bail out if a USB3 port has a new device in link training */
+ if ((t1 & PORT_PLS_MASK) == XDEV_POLLING) {
+ bus_state->bus_suspended = 0;
+ spin_unlock_irqrestore(>lock, flags);
+ xhci_dbg(xhci, "Bus suspend bailout, port in
polling\n");
+ return -EBUSY;
+ }
+
+ /* suspend ports in U0, or bail out for new connect changes */
+ if ((t1 & PORT_PE) && (t1 & PORT_PLS_MASK) == XDEV_U0) {
+ if ((t1 & PORT_CSC) && wake_enabled) {
+ bus_state->bus_suspended = 0;
spin_unlock_irqrestore(>lock, flags);
- xhci_stop_device(xhci, slot_id, 1);
- spin_lock_irqsave(>lock, flags);
+ xhci_dbg(xhci, "Bus suspend bailout, port
connect change\n");
+ return -EBUSY;
}
+ xhci_dbg(xhci, "port %d not suspended\n", port_index);
t2 &= ~PORT_PLS_MASK;
t2 |= PORT_LINK_STROBE | XDEV_U3;
set_bit(port_index, _state->bus_suspended);
@@ -1518,7 +1531,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd
* including the USB 3.0 roothub, but only if CONFIG_PM
* is enabled, so also enable remote wake here.
*/
- if (hcd->self.root_hub->do_remote_wakeup) {
+ if (wake_enabled) {
if (t1 & PORT_CONNECT) {
[PATCH 4.19 014/110] usb: xhci: Prevent bus suspend if a port connect change or polling state is detected
4.19-stable review patch. If anyone has any objections, please let me know.
--
From: Mathias Nyman
commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream.
USB3 roothub might autosuspend before a plugged USB3 device is detected,
causing USB3 device enumeration failure.
USB3 devices don't show up as connected and enabled until USB3 link trainig
completes. On a fast booting platform with a slow USB3 link training the
link might reach the connected enabled state just as the bus is suspending.
If this device is discovered first time by the xhci_bus_suspend() routine
it will be put to U3 suspended state like the other ports which failed to
suspend earlier.
The hub thread will notice the connect change and resume the bus,
moving the port back to U0
This U0 -> U3 -> U0 transition right after being connected seems to be
too much for some devices, causing them to first go to SS.Inactive state,
and finally end up stuck in a polling state with reset asserted
Fix this by failing the bus suspend if a port has a connect change or is
in a polling state in xhci_bus_suspend().
Don't do any port changes until all ports are checked, buffer all port
changes and only write them in the end if suspend can proceed
Cc: sta...@vger.kernel.org
Signed-off-by: Mathias Nyman
Signed-off-by: Greg Kroah-Hartman
---
drivers/usb/host/xhci-hub.c | 60 +---
1 file changed, 46 insertions(+), 14 deletions(-)
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1474,15 +1474,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd
unsigned long flags;
struct xhci_hub *rhub;
struct xhci_port **ports;
+ u32 portsc_buf[USB_MAXCHILDREN];
+ bool wake_enabled;
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
bus_state = >bus_state[hcd_index(hcd)];
+ wake_enabled = hcd->self.root_hub->do_remote_wakeup;
spin_lock_irqsave(>lock, flags);
- if (hcd->self.root_hub->do_remote_wakeup) {
+ if (wake_enabled) {
if (bus_state->resuming_ports ||/* USB2 */
bus_state->port_remote_wakeup) {/* USB3 */
spin_unlock_irqrestore(>lock, flags);
@@ -1490,26 +1493,36 @@ int xhci_bus_suspend(struct usb_hcd *hcd
return -EBUSY;
}
}
-
- port_index = max_ports;
+ /*
+* Prepare ports for suspend, but don't write anything before all ports
+* are checked and we know bus suspend can proceed
+*/
bus_state->bus_suspended = 0;
+ port_index = max_ports;
while (port_index--) {
- /* suspend the port if the port is not suspended */
u32 t1, t2;
- int slot_id;
t1 = readl(ports[port_index]->addr);
t2 = xhci_port_state_to_neutral(t1);
+ portsc_buf[port_index] = 0;
- if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
- xhci_dbg(xhci, "port %d not suspended\n", port_index);
- slot_id = xhci_find_slot_id_by_port(hcd, xhci,
- port_index + 1);
- if (slot_id) {
+ /* Bail out if a USB3 port has a new device in link training */
+ if ((t1 & PORT_PLS_MASK) == XDEV_POLLING) {
+ bus_state->bus_suspended = 0;
+ spin_unlock_irqrestore(>lock, flags);
+ xhci_dbg(xhci, "Bus suspend bailout, port in
polling\n");
+ return -EBUSY;
+ }
+
+ /* suspend ports in U0, or bail out for new connect changes */
+ if ((t1 & PORT_PE) && (t1 & PORT_PLS_MASK) == XDEV_U0) {
+ if ((t1 & PORT_CSC) && wake_enabled) {
+ bus_state->bus_suspended = 0;
spin_unlock_irqrestore(>lock, flags);
- xhci_stop_device(xhci, slot_id, 1);
- spin_lock_irqsave(>lock, flags);
+ xhci_dbg(xhci, "Bus suspend bailout, port
connect change\n");
+ return -EBUSY;
}
+ xhci_dbg(xhci, "port %d not suspended\n", port_index);
t2 &= ~PORT_PLS_MASK;
t2 |= PORT_LINK_STROBE | XDEV_U3;
set_bit(port_index, _state->bus_suspended);
@@ -1518,7 +1531,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd
* including the USB 3.0 roothub, but only if CONFIG_PM
* is enabled, so also enable remote wake here.
*/
- if (hcd->self.root_hub->do_remote_wakeup) {
+ if (wake_enabled) {
if (t1 & PORT_CONNECT) {
