Revert "USB / PM: Allow USB devices to remain runtime-suspended when sleeping"

PATCH In review Johan Hovold 3 participants 6 messages May 2, 2016 – May 3, 2016

Subsystems (2)

Stable branch USB

lore

Share & tools

lore

Johan Hovold

author

2016-05-02 13:37 UTC #0 lore raw

PATCH 1/1 Revert "USB / PM: Allow USB devices to remain runtime-suspended when sleeping"

This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.

Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.

During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.

Cc stable <[email protected]> # 4.5

Signed-off-by Johan Hovold <[email protected]>

---

Greg, Alan,

This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.

We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.

Thanks,
Johan

 drivers/usb/core/port.c | 6 ------
 drivers/usb/core/usb.c  | 8 +-------
 2 files changed, 1 insertion(+), 13 deletions(-)

drivers/usb/core/port.c +0 -6

index 14718a9ffcfb..460c855be0d0 100644
@@ -249,18 +249,12 @@ static int usb_port_runtime_suspend(struct device *dev)
 
 	return retval;
 }
-
-static int usb_port_prepare(struct device *dev)
-{
-	return 1;
-}
 #endif
 
 static const struct dev_pm_ops usb_port_pm_ops = {
 #ifdef CONFIG_PM
 	.runtime_suspend =	usb_port_runtime_suspend,
 	.runtime_resume =	usb_port_runtime_resume,
-	.prepare =		usb_port_prepare,
 #endif
 };

drivers/usb/core/usb.c +1 -7

index dcb85e3cd5a7..479187c32571 100644
@@ -312,13 +312,7 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static int usb_dev_prepare(struct device *dev)
 {
-	struct usb_device *udev = to_usb_device(dev);
-
-	/* Return 0 if the current wakeup setting is wrong, otherwise 1 */
-	if (udev->do_remote_wakeup != device_may_wakeup(dev))
-		return 0;
-
-	return 1;
+	return 0;		/* Implement eventually? */
 }
 
 static void usb_dev_complete(struct device *dev)

Signature · 1 line

2.7.3

Alan Stern

2016-05-02 15:13 UTC #1 lore raw

Quoted 19 lines · Johan Hovold

This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.
Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.
During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.
Cc stable <[email protected]> # 4.5
Signed-off-by Johan Hovold <[email protected]>
---
Greg, Alan,

Quoted 7 lines

This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.
We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.

The description of the problem doesn't sound right to me. For instance, would it help if usb_runtime_resume() did honor the needs_binding flag? I doubt it. Things like the wakeup setting would still be lost before the runtime resume occurred.

I suspect the right answer is always to resume a USB device if it needs a reset-resume, but otherwise allow it to remain in runtime suspend.

Reverting the patch for now is okay with me. Tomeu may want to work on a better solution. Part of the difficulty is that the PM core wants to know before suspending whether skipping resume will be okay, but the USB stack doesn't know until after the host controller has been resumed.

In the end, we'll probably have the PM core call usb_resume all the time, but usb_resume will leave the device in runtime suspend if it can. This isn't ideal but it may be the best we can do.

Alan Stern

Greg Kroah-Hartman

maintainer

2016-05-02 15:22 UTC #2 lore raw

Quoted 43 lines · Alan Stern

Quoted 19 lines · Johan Hovold
This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.
Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.
During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.
Cc stable <[email protected]> # 4.5
Signed-off-by Johan Hovold <[email protected]>
---
Greg, Alan,
Quoted 7 lines
This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.
We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.
The description of the problem doesn't sound right to me. For instance, would it help if usb_runtime_resume() did honor the needs_binding flag? I doubt it. Things like the wakeup setting would still be lost before the runtime resume occurred.
I suspect the right answer is always to resume a USB device if it needs a reset-resume, but otherwise allow it to remain in runtime suspend.
Reverting the patch for now is okay with me. Tomeu may want to work on a better solution. Part of the difficulty is that the PM core wants to know before suspending whether skipping resume will be okay, but the USB stack doesn't know until after the host controller has been resumed.

Quoted 3 lines

In the end, we'll probably have the PM core call usb_resume all the time, but usb_resume will leave the device in runtime suspend if it can. This isn't ideal but it may be the best we can do.

So is that an "acked-by" for this revert?

thanks,

greg k-h

Alan Stern

2016-05-02 15:26 UTC #3 lore raw

Quoted 44 lines · Greg Kroah-Hartman

Quoted 43 lines · Alan Stern
Quoted 19 lines · Johan Hovold
This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.
Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.
During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.
Cc stable <[email protected]> # 4.5
Signed-off-by Johan Hovold <[email protected]>
---
Greg, Alan,
Quoted 7 lines
This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.
We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.
The description of the problem doesn't sound right to me. For instance, would it help if usb_runtime_resume() did honor the needs_binding flag? I doubt it. Things like the wakeup setting would still be lost before the runtime resume occurred.
I suspect the right answer is always to resume a USB device if it needs a reset-resume, but otherwise allow it to remain in runtime suspend.
Reverting the patch for now is okay with me. Tomeu may want to work on a better solution. Part of the difficulty is that the PM core wants to know before suspending whether skipping resume will be okay, but the USB stack doesn't know until after the host controller has been resumed.

Quoted 5 lines

Quoted 3 lines
In the end, we'll probably have the PM core call usb_resume all the time, but usb_resume will leave the device in runtime suspend if it can. This isn't ideal but it may be the best we can do.
So is that an "acked-by" for this revert?

Yes.

Alan Stern

Greg Kroah-Hartman

maintainer

2016-05-02 15:54 UTC #4 lore raw

Quoted 46 lines · Alan Stern

Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Alan Stern
Quoted 19 lines · Johan Hovold
This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.
Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.
During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.
Cc stable <[email protected]> # 4.5
Signed-off-by Johan Hovold <[email protected]>
---
Greg, Alan,
Quoted 7 lines
This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.
We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.
The description of the problem doesn't sound right to me. For instance, would it help if usb_runtime_resume() did honor the needs_binding flag? I doubt it. Things like the wakeup setting would still be lost before the runtime resume occurred.
I suspect the right answer is always to resume a USB device if it needs a reset-resume, but otherwise allow it to remain in runtime suspend.
Reverting the patch for now is okay with me. Tomeu may want to work on a better solution. Part of the difficulty is that the PM core wants to know before suspending whether skipping resume will be okay, but the USB stack doesn't know until after the host controller has been resumed.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
In the end, we'll probably have the PM core call usb_resume all the time, but usb_resume will leave the device in runtime suspend if it can. This isn't ideal but it may be the best we can do.
So is that an "acked-by" for this revert?
Yes.

Great, now applied, thanks.

greg k-h

Johan Hovold

author

2016-05-03 07:51 UTC #5 lore raw

In reply to Alan Stern

Quoted 29 lines · Alan Stern

Quoted 19 lines · Johan Hovold
This reverts commit e3345db85068ddb937fc0ba40dfc39c293dad977, which broke system resume for a large class of devices.
Devices that after having been reset during resume need to be rebound due to a missing reset_resume callback, are now left in a suspended state. This specifically broke resume of common USB-serial devices, which are now unusable after system suspend (until disconnected and reconnected) when USB persist is enabled.
During resume, usb_resume_interface will set the needs_binding flag for such interfaces, but unlike system resume, run-time resume does not honour it.
Cc stable <[email protected]> # 4.5
Signed-off-by Johan Hovold <[email protected]>
---
Greg, Alan,
Quoted 7 lines
This patch for v4.6-rc7 fixes a 4.5-regression that broke system suspend for a large class of devices, including USB-serial devices, for example when USB persist is enabled.
We may be able to find a way around this, but since it's a user-visible regression and late in the rc-cycle, I believe reverting the offending commit is the right thing to do.

Quoted 7 lines

The description of the problem doesn't sound right to me. For instance, would it help if usb_runtime_resume() did honor the needs_binding flag? I doubt it. Things like the wakeup setting would still be lost before the runtime resume occurred.
I suspect the right answer is always to resume a USB device if it needs a reset-resume, but otherwise allow it to remain in runtime suspend.

I was trying to describe how things broke but not necessarily to suggest how this should be fixed. Could have put the last paragraph below the cut-off line I guess.

Quoted 9 lines

Reverting the patch for now is okay with me. Tomeu may want to work on a better solution. Part of the difficulty is that the PM core wants to know before suspending whether skipping resume will be okay, but the USB stack doesn't know until after the host controller has been resumed.
In the end, we'll probably have the PM core call usb_resume all the time, but usb_resume will leave the device in runtime suspend if it can. This isn't ideal but it may be the best we can do.

Thanks,
Johan