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

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

[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 <stable@vger.kernel.org>	# 4.5
Signed-off-by: Johan Hovold <johan@kernel.org>
---

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(-)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 14718a9ffcfb..460c855be0d0 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -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
 };
 
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index dcb85e3cd5a7..479187c32571 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -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)
-- 
2.7.3

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

[Alan Stern]

On Mon, 2 May 2016, Johan Hovold wrote:

> 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 <stable@vger.kernel.org>	# 4.5
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> 
> 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.

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

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

[Greg Kroah-Hartman]

On Mon, May 02, 2016 at 11:13:10AM -0400, Alan Stern wrote:
> On Mon, 2 May 2016, Johan Hovold wrote:
> 
> > 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 <stable@vger.kernel.org>	# 4.5
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> > 
> > 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.
> 
> 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.

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

thanks,

greg k-h

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

[Alan Stern]

On Mon, 2 May 2016, Greg Kroah-Hartman wrote:

> On Mon, May 02, 2016 at 11:13:10AM -0400, Alan Stern wrote:
> > On Mon, 2 May 2016, Johan Hovold wrote:
> > 
> > > 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 <stable@vger.kernel.org>	# 4.5
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > > 
> > > 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.
> > 
> > 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.
> 
> So is that an "acked-by" for this revert?

Yes.

Alan Stern

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

[Greg Kroah-Hartman]

On Mon, May 02, 2016 at 11:26:01AM -0400, Alan Stern wrote:
> On Mon, 2 May 2016, Greg Kroah-Hartman wrote:
> 
> > On Mon, May 02, 2016 at 11:13:10AM -0400, Alan Stern wrote:
> > > On Mon, 2 May 2016, Johan Hovold wrote:
> > > 
> > > > 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 <stable@vger.kernel.org>	# 4.5
> > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > ---
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > So is that an "acked-by" for this revert?
> 
> Yes.

Great, now applied, thanks.

greg k-h

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

[Johan Hovold]

On Mon, May 02, 2016 at 11:13:10AM -0400, Alan Stern wrote:
> On Mon, 2 May 2016, Johan Hovold wrote:
> 
> > 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 <stable@vger.kernel.org>	# 4.5
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> > 
> > 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.
> 
> 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.

> 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