[RFC PATCH v2] USB: hub: fix port suspend/resume

[RFC PATCH v2] USB: hub: fix port suspend/resume

[Marco Felsch]

At the momemnt the usb-port driver has only runime_pm hooks.
Suspending the port and turn off the VBUS supply should be triggered by
the hub device suspend callback usb_port_suspend() which calls the
pm_runtime_put_sync() if all pre-conditions are meet. This mechanism
don't work correctly due to the global PM behaviour, for more information
see [1]. According [1] I added the suspend/resume callbacks for the port
device to fix this.

[1] https://www.spinics.net/lists/linux-usb/msg190537.html

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Hi,

this v2 contains the fixes

Reported-by: kbuild test robot <lkp@intel.com>

Regards,
  Marco

Changes:
- init retval to zero
- keep CONFIG_PM due to do_remote_wakeup availability
- adapt commit message

 drivers/usb/core/hub.c  | 13 -------------
 drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3405b146edc9..c294484e478d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		usb_set_device_state(udev, USB_STATE_SUSPENDED);
 	}
 
-	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
-			&& test_and_clear_bit(port1, hub->child_usage_bits))
-		pm_runtime_put_sync(&port_dev->dev);
-
 	usb_mark_last_busy(hub->hdev);
 
 	usb_unlock_port(port_dev);
@@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	int		status;
 	u16		portchange, portstatus;
 
-	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
-		status = pm_runtime_get_sync(&port_dev->dev);
-		if (status < 0) {
-			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
-					status);
-			return status;
-		}
-	}
-
 	usb_lock_port(port_dev);
 
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index bbbb35fa639f..13f130b67efe 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev)
 
 	return retval;
 }
-#endif
+
+static int __maybe_unused _usb_port_suspend(struct device *dev)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+	int retval = 0;
+
+	if (!udev->do_remote_wakeup && udev->persist_enabled)
+		retval = usb_port_runtime_suspend(dev);
+
+	/* Do not force the user to enable the power-off feature */
+	if (retval && retval != -EAGAIN)
+		return retval;
+
+	return 0;
+}
+
+static int __maybe_unused _usb_port_resume(struct device *dev)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+
+	if (!udev->do_remote_wakeup && udev->persist_enabled)
+		return usb_port_runtime_resume(dev);
+
+	return 0;
+}
+#endif /* CONFIG_PM */
 
 static void usb_port_shutdown(struct device *dev)
 {
@@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev)
 }
 
 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,
-#endif
+	SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume)
+	SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
 };
 
 struct device_type usb_port_device_type = {
-- 
2.20.1

Re: [RFC PATCH v2] USB: hub: fix port suspend/resume

[Alan Stern]

On Thu, 27 Feb 2020, Marco Felsch wrote:

> At the momemnt the usb-port driver has only runime_pm hooks.
> Suspending the port and turn off the VBUS supply should be triggered by
> the hub device suspend callback usb_port_suspend() which calls the

Strictly speaking it's just a routine, not a callback.  That is, it 
doesn't get invoked through a function pointer.

> pm_runtime_put_sync() if all pre-conditions are meet. This mechanism
> don't work correctly due to the global PM behaviour, for more information
> see [1]. According [1] I added the suspend/resume callbacks for the port
> device to fix this.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg190537.html

Please put at least a short description of the problem here; don't 
force people to go look up some random web page just to find out what's 
really going on.

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Hi,
> 
> this v2 contains the fixes
> 
> Reported-by: kbuild test robot <lkp@intel.com>

Everything below the "---" line, except the patch itself, gets ignored.  
You need to move this Reported-by: up higher.

> Regards,
>   Marco
> 
> Changes:
> - init retval to zero
> - keep CONFIG_PM due to do_remote_wakeup availability
> - adapt commit message
> 
>  drivers/usb/core/hub.c  | 13 -------------
>  drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3405b146edc9..c294484e478d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  		usb_set_device_state(udev, USB_STATE_SUSPENDED);
>  	}
>  
> -	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
> -			&& test_and_clear_bit(port1, hub->child_usage_bits))
> -		pm_runtime_put_sync(&port_dev->dev);
> -
>  	usb_mark_last_busy(hub->hdev);
>  
>  	usb_unlock_port(port_dev);
> @@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  	int		status;
>  	u16		portchange, portstatus;
>  
> -	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
> -		status = pm_runtime_get_sync(&port_dev->dev);
> -		if (status < 0) {
> -			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> -					status);
> -			return status;
> -		}
> -	}
> -

Why do you get rid of these two sections of code?  Won't that cause
runtime PM to stop working properly?

>  	usb_lock_port(port_dev);
>  
>  	/* Skip the initial Clear-Suspend step for a remote wakeup */
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index bbbb35fa639f..13f130b67efe 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev)
>  
>  	return retval;
>  }
> -#endif
> +
> +static int __maybe_unused _usb_port_suspend(struct device *dev)

Don't say _maybe_unused.  Instead, protect these two routines with 
#ifdef CONFIG_PM_SLEEP.  That way they won't be compiled on systems 
that can't use them.

Also, try to find better names.  Maybe usb_port_sleep and 
usb_port_wake, or usb_port_system_suspend and usb_port_system_resume.

> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	struct usb_device *udev = port_dev->child;
> +	int retval = 0;
> +
> +	if (!udev->do_remote_wakeup && udev->persist_enabled)
> +		retval = usb_port_runtime_suspend(dev);
> +
> +	/* Do not force the user to enable the power-off feature */
> +	if (retval && retval != -EAGAIN)
> +		return retval;
> +
> +	return 0;

IMO it would be a lot more understandable if you wrote

	if (retval == -EAGAIN)
		retval = 0;

Also, the relation between this code and the preceding comment is not
obvious.  The comment should say something more like: If the
PM_QOS_FLAG setting prevents us from powering off the port, it's not an
error.

Alan Stern

> +}
> +
> +static int __maybe_unused _usb_port_resume(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	struct usb_device *udev = port_dev->child;
> +
> +	if (!udev->do_remote_wakeup && udev->persist_enabled)
> +		return usb_port_runtime_resume(dev);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
>  
>  static void usb_port_shutdown(struct device *dev)
>  {
> @@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev)
>  }
>  
>  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,
> -#endif
> +	SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume)
> +	SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
>  };
>  
>  struct device_type usb_port_device_type = {
> 

Re: [RFC PATCH v2] USB: hub: fix port suspend/resume

[Marco Felsch]

On 20-02-27 11:18, Alan Stern wrote:
> On Thu, 27 Feb 2020, Marco Felsch wrote:
> 
> > At the momemnt the usb-port driver has only runime_pm hooks.
> > Suspending the port and turn off the VBUS supply should be triggered by
> > the hub device suspend callback usb_port_suspend() which calls the
> 
> Strictly speaking it's just a routine, not a callback.  That is, it 
> doesn't get invoked through a function pointer.

Damn, you right it gets called from the generic_suspend callback.

> > pm_runtime_put_sync() if all pre-conditions are meet. This mechanism
> > don't work correctly due to the global PM behaviour, for more information
> > see [1]. According [1] I added the suspend/resume callbacks for the port
> > device to fix this.
> > 
> > [1] https://www.spinics.net/lists/linux-usb/msg190537.html
> 
> Please put at least a short description of the problem here; don't 
> force people to go look up some random web page just to find out what's 
> really going on.

Okay, I will do that.

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Hi,
> > 
> > this v2 contains the fixes
> > 
> > Reported-by: kbuild test robot <lkp@intel.com>
> 
> Everything below the "---" line, except the patch itself, gets ignored.  
> You need to move this Reported-by: up higher.

I know, I put it here because the patch isn't part of the kernel. IMHO a

Signed-off-by:
Reported-by: 

looks a bit strange.

> > Regards,
> >   Marco
> > 
> > Changes:
> > - init retval to zero
> > - keep CONFIG_PM due to do_remote_wakeup availability
> > - adapt commit message
> > 
> >  drivers/usb/core/hub.c  | 13 -------------
> >  drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++-----
> >  2 files changed, 30 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3405b146edc9..c294484e478d 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> >  		usb_set_device_state(udev, USB_STATE_SUSPENDED);
> >  	}
> >  
> > -	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
> > -			&& test_and_clear_bit(port1, hub->child_usage_bits))
> > -		pm_runtime_put_sync(&port_dev->dev);
> > -
> >  	usb_mark_last_busy(hub->hdev);
> >  
> >  	usb_unlock_port(port_dev);
> > @@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> >  	int		status;
> >  	u16		portchange, portstatus;
> >  
> > -	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
> > -		status = pm_runtime_get_sync(&port_dev->dev);
> > -		if (status < 0) {
> > -			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> > -					status);
> > -			return status;
> > -		}
> > -	}
> > -
> 
> Why do you get rid of these two sections of code?  Won't that cause
> runtime PM to stop working properly?

Both runtime_pm calls are part of the suspend/resume logic so this code
isn't called during runtime PM. As far as I understood it correctly the
purpose of those section was to trigger port poweroff if the device
supports it upon a system-suspend. Therefore I came up with my question:
https://www.spinics.net/lists/linux-usb/msg190537.html.

> >  	usb_lock_port(port_dev);
> >  
> >  	/* Skip the initial Clear-Suspend step for a remote wakeup */
> > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> > index bbbb35fa639f..13f130b67efe 100644
> > --- a/drivers/usb/core/port.c
> > +++ b/drivers/usb/core/port.c
> > @@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev)
> >  
> >  	return retval;
> >  }
> > -#endif
> > +
> > +static int __maybe_unused _usb_port_suspend(struct device *dev)
> 
> Don't say _maybe_unused.  Instead, protect these two routines with 
> #ifdef CONFIG_PM_SLEEP.  That way they won't be compiled on systems 
> that can't use them.

Okay, as you like. I find the __maybe_unused better than #ifdefs.

> Also, try to find better names.  Maybe usb_port_sleep and 
> usb_port_wake, or usb_port_system_suspend and usb_port_system_resume.

IMHO usb_port_suspend/resume should be the best ;)

> > +{
> > +	struct usb_port *port_dev = to_usb_port(dev);
> > +	struct usb_device *udev = port_dev->child;
> > +	int retval = 0;
> > +
> > +	if (!udev->do_remote_wakeup && udev->persist_enabled)
> > +		retval = usb_port_runtime_suspend(dev);
> > +
> > +	/* Do not force the user to enable the power-off feature */
> > +	if (retval && retval != -EAGAIN)
> > +		return retval;
> > +
> > +	return 0;
> 
> IMO it would be a lot more understandable if you wrote
> 
> 	if (retval == -EAGAIN)
> 		retval = 0;

As you like.

> Also, the relation between this code and the preceding comment is not
> obvious.  The comment should say something more like: If the
> PM_QOS_FLAG setting prevents us from powering off the port, it's not an
> error.

Okay I will change that.

Regards,
  Marco

> Alan Stern
> 
> > +}
> > +
> > +static int __maybe_unused _usb_port_resume(struct device *dev)
> > +{
> > +	struct usb_port *port_dev = to_usb_port(dev);
> > +	struct usb_device *udev = port_dev->child;
> > +
> > +	if (!udev->do_remote_wakeup && udev->persist_enabled)
> > +		return usb_port_runtime_resume(dev);
> > +
> > +	return 0;
> > +}
> > +#endif /* CONFIG_PM */
> >  
> >  static void usb_port_shutdown(struct device *dev)
> >  {
> > @@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev)
> >  }
> >  
> >  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,
> > -#endif
> > +	SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume)
> > +	SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
> >  };
> >  
> >  struct device_type usb_port_device_type = {
> > 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH v2] USB: hub: fix port suspend/resume

[Alan Stern]

On Thu, 27 Feb 2020, Marco Felsch wrote:

> On 20-02-27 11:18, Alan Stern wrote:

> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > Hi,
> > > 
> > > this v2 contains the fixes
> > > 
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > Everything below the "---" line, except the patch itself, gets ignored.  
> > You need to move this Reported-by: up higher.
> 
> I know, I put it here because the patch isn't part of the kernel. IMHO a
> 
> Signed-off-by:
> Reported-by: 
> 
> looks a bit strange.

Not at all.  That sort of thing occurs all the time; just look at a few 
commits in the kernel or patches on the mailing lists.  Especially ones 
that are bug fixes.

> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 3405b146edc9..c294484e478d 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> > >  		usb_set_device_state(udev, USB_STATE_SUSPENDED);
> > >  	}
> > >  
> > > -	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
> > > -			&& test_and_clear_bit(port1, hub->child_usage_bits))
> > > -		pm_runtime_put_sync(&port_dev->dev);
> > > -
> > >  	usb_mark_last_busy(hub->hdev);
> > >  
> > >  	usb_unlock_port(port_dev);
> > > @@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> > >  	int		status;
> > >  	u16		portchange, portstatus;
> > >  
> > > -	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
> > > -		status = pm_runtime_get_sync(&port_dev->dev);
> > > -		if (status < 0) {
> > > -			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> > > -					status);
> > > -			return status;
> > > -		}
> > > -	}
> > > -
> > 
> > Why do you get rid of these two sections of code?  Won't that cause
> > runtime PM to stop working properly?
> 
> Both runtime_pm calls are part of the suspend/resume logic so this code
> isn't called during runtime PM.

I'm not quite sure what you mean by that.  In any case, it would be 
completely wrong to think that usb_port_suspend isn't involved in 
runtime PM.

In fact, usb_port_suspend is _more_ important for runtime suspend than
for system sleep.  The reason is simple: If you want to put a USB
device into runtime suspend, you have to tell its upstream hub's port
to enable the suspend feature (i.e., call usb_port_suspend).  But if
you want to put an entire bus of USB devices to sleep for a system
suspend, all you have to do is tell the host controller to stop sending
packets; the ports don't need any notification.

(Actually the situation is more complicated for USB 3.  But you get the 
idea.)

> As far as I understood it correctly the
> purpose of those section was to trigger port poweroff if the device
> supports it upon a system-suspend.

No, the purpose of the sections you removed is to trigger port poweroff
when the device goes into any type of suspend, either system or
runtime.  Of course, as you discovered, during system sleep the code
doesn't actually turn off the port power -- that's a bug.  But during
runtime PM it does.

> Therefore I came up with my question:
> https://www.spinics.net/lists/linux-usb/msg190537.html.

> > Also, try to find better names.  Maybe usb_port_sleep and 
> > usb_port_wake, or usb_port_system_suspend and usb_port_system_resume.
> 
> IMHO usb_port_suspend/resume should be the best ;)

Okay, so long as they are static and won't conflict with the functions 
in hub.c.

Alan Stern

PS: There's one more thing you need to know -- I completely forgot 
about it until just now.  During system sleep, we have to make sure 
that the child device gets suspended _before_ and resumed _after_ the 
port.  If it happened the other way, we'd be in trouble.

(The proper ordering would be automatic if the child USB device was
registered under the port device, but for historical reasons it isn't;
it gets registered directly under the parent hub.)

This means you'll have to call device_pm_wait_for_dev() at the 
appropriate places in the suspend and resume pathways.

RE: [RFC PATCH v2] USB: hub: fix port suspend/resume

[Peter Chen]

 
> 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > > Hi,
> > > >
> > > > this v2 contains the fixes
> > > >
> > > > Reported-by: kbuild test robot <lkp@intel.com>
> > >
> > > Everything below the "---" line, except the patch itself, gets ignored.
> > > You need to move this Reported-by: up higher.
> >
> > I know, I put it here because the patch isn't part of the kernel. IMHO
> > a
> >
> > Signed-off-by:
> > Reported-by:
> >
> > looks a bit strange.
> 
> Not at all.  That sort of thing occurs all the time; just look at a few commits in the
> kernel or patches on the mailing lists.  Especially ones that are bug fixes.
> 
> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > > > 3405b146edc9..c294484e478d 100644
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev,
> pm_message_t msg)
> > > >  		usb_set_device_state(udev, USB_STATE_SUSPENDED);
> > > >  	}
> > > >
> > > > -	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
> > > > -			&& test_and_clear_bit(port1, hub->child_usage_bits))
> > > > -		pm_runtime_put_sync(&port_dev->dev);
> > > > -
> > > >  	usb_mark_last_busy(hub->hdev);
> > > >
> > > >  	usb_unlock_port(port_dev);
> > > > @@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
> > > >  	int		status;
> > > >  	u16		portchange, portstatus;
> > > >
> > > > -	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
> > > > -		status = pm_runtime_get_sync(&port_dev->dev);
> > > > -		if (status < 0) {
> > > > -			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> > > > -					status);
> > > > -			return status;
> > > > -		}
> > > > -	}
> > > > -
> > >
> > > Why do you get rid of these two sections of code?  Won't that cause
> > > runtime PM to stop working properly?
> >
> > Both runtime_pm calls are part of the suspend/resume logic so this
> > code isn't called during runtime PM.
> 
> I'm not quite sure what you mean by that.  In any case, it would be completely
> wrong to think that usb_port_suspend isn't involved in runtime PM.
> 
> In fact, usb_port_suspend is _more_ important for runtime suspend than for system
> sleep.  The reason is simple: If you want to put a USB device into runtime suspend,
> you have to tell its upstream hub's port to enable the suspend feature (i.e., call
> usb_port_suspend).  But if you want to put an entire bus of USB devices to sleep
> for a system suspend, all you have to do is tell the host controller to stop sending
> packets; the ports don't need any notification.
> 
> (Actually the situation is more complicated for USB 3.  But you get the
> idea.)
> 
> > As far as I understood it correctly the purpose of those section was
> > to trigger port poweroff if the device supports it upon a
> > system-suspend.
> 
> No, the purpose of the sections you removed is to trigger port poweroff when the
> device goes into any type of suspend, either system or runtime.  Of course, as you
> discovered, during system sleep the code doesn't actually turn off the port power --
> that's a bug.  But during runtime PM it does.
> 
> > Therefore I came up with my question:
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.
> net%2Flists%2Flinux-
> usb%2Fmsg190537.html&amp;data=02%7C01%7Cpeter.chen%40nxp.com%7Ce2
> 47403d3a8c420ef66d08d7bbbb63a5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637184285813366300&amp;sdata=MviQpc4vhyVu496wyNQ%2Bb3T
> hNE7Gh6hpenjzxn6%2FLwE%3D&amp;reserved=0.
> 
> > > Also, try to find better names.  Maybe usb_port_sleep and
> > > usb_port_wake, or usb_port_system_suspend and usb_port_system_resume.
> >
> > IMHO usb_port_suspend/resume should be the best ;)
> 
> Okay, so long as they are static and won't conflict with the functions in hub.c.
> 
> Alan Stern
> 
> PS: There's one more thing you need to know -- I completely forgot about it until
> just now.  During system sleep, we have to make sure that the child device gets
> suspended _before_ and resumed _after_ the port.  If it happened the other way,
> we'd be in trouble.
> 
> (The proper ordering would be automatic if the child USB device was registered
> under the port device, but for historical reasons it isn't; it gets registered directly
> under the parent hub.)
> 
> This means you'll have to call device_pm_wait_for_dev() at the appropriate places
> in the suspend and resume pathways.

Hi Alan & Marco,

I tried this VBUS off feature at one chipidea controller board with USB mouse, it works
well, at least with my expectation. See below log:

// There is a USB mouse at USB2 port

root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  1        1        0   480000000          0     0  50000
       usb_phy2_clk                   1        1        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       1        1        0   135000000          0     0  50000
// clock is on

root@imx6ul7d:~# echo auto > /sys/bus/usb/devices/1-1/power/control 
root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  0        0        0   480000000          0     0  50000
       usb_phy2_clk                   0        0        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       0        0        0   135000000          0     0  50000

//clock is off after enable mouse's autosuspend

root@imx6ul7d:~# echo 0 > //sys/bus/usb/devices/1-0\:1.0/usb1-port1/power/
autosuspend_delay_ms    pm_qos_no_power_off     runtime_status
control                 runtime_active_time     runtime_suspended_time
_no_power_off ~# echo 0 > //sys/bus/usb/devices/1-0\:1.0/usb1-port1/power/pm_qos_
root@imx6ul7d:~# [  250.865933] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, disable vbus regulator

//VBUS is off after set pm_qos_no_power_off 0. It stands for pm_qos_no_power_off works
well for runtime suspend.

root@imx6ul7d:~# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
root@imx6ul7d:~# echo mem > /sys/power/state

// Enable tty wakeup, and suspend system.

[  292.530680] PM: suspend entry (s2idle)
[  292.547108] Filesystems sync: 0.012 seconds
[  292.575311] Freezing user space processes ... (elapsed 0.003 seconds) done.
[  292.586275] OOM killer disabled.
[  292.589726] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[  292.599891] printk: Suspending console(s) (use no_console_suspend to debug)

[  292.674257] fec 30be0000.ethernet eth0: Link is Down
[  292.677564] PM: suspend devices took 0.070 seconds
[  295.690990] imx6q-pcie 33800000.pcie: Phy link never came up
[  295.691012] imx6q-pcie 33800000.pcie: pcie link is down after resume.
[  295.715262] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, enable vbus regulator

// The VBUS is only on after resume, but not during the system suspend routine.

[  296.321299] usb 1-1: reset low-speed USB device number 2 using ci_hdrc
[  296.680700] PM: resume devices took 0.990 seconds
[  296.721685] OOM killer enabled.
[  296.724847] Restarting tasks ... done.
[  296.750136] PM: suspend exit
root@imx6ul7d:~# 
root@imx6ul7d:~# [  298.811004] fec 30be0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
[  299.169794] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, disable vbus regulator

root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  0        0        0   480000000          0     0  50000
       usb_phy2_clk                   0        0        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       0        0        0   135000000          0     0  50000

// As USB mouse is auto-suspend, and pm_qos_no_power_off is 0. The clock is off, and VBUS is off after autosuspend
timeout.

Linux version 5.6.0-rc1-00027-g2671f46409d5-dirty (b29397@b29397-desktop) (gcc version 6.2.0 (GCC)) #7 SMP Fri Feb 28 
10:20:18 CST 2020

// I use the usb-next tree, it is the new upstream kernel

Peter