USB Port Power-Off during suspend Bug?

USB Port Power-Off during suspend Bug?

[Marco Felsch]

Hi Alan, Rafael, Greg,

long story short: I want to disable a usb-port completely during suspend
because it isn't needed and we need to save energy, because is a 32bit ARM
(OF-based) handheld device. I use the port to connect a usb-ethernet
dongle (all needed drivers are builtin no modules) which is needed for
the NFS. The usb-ethernet dongle supports the persist setting because it
does a hw-reset during resume anyway.

So what I did is:
 1) Set the persist bit for the usb device
 2) Set the control to auto for the usb device
 3) Unset the pm_qos_no_power_off flag for the usb-port

But the port gets not disabled. I debugged it and found a problem in
usb_port_suspend() logic [1] and the generic PM-framework more precisely
the dpm mechanism. The usbcore does the correct pm_runtime counting but
the call [2] don't trigger the usb_port_runtime_suspend() [3] because
the dpm enables all runtime-pm device before the shutdown is executed.
IMHO both subsystem behaviours are correct and I don't know the
_correct_ fix, therefore I wrote this email.

As far as I understood it all non-ACPI platforms are affected.

Regards,
  Marco

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3238
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3328
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/port.c?h=v5.5#n247

Re: USB Port Power-Off during suspend Bug?

[Alan Stern]

On Wed, 29 Jan 2020, Marco Felsch wrote:

> Hi Alan, Rafael, Greg,
> 
> long story short: I want to disable a usb-port completely during suspend

You're talking about what happens during a full system suspend, right?

> because it isn't needed and we need to save energy, because is a 32bit ARM
> (OF-based) handheld device. I use the port to connect a usb-ethernet
> dongle (all needed drivers are builtin no modules) which is needed for
> the NFS. The usb-ethernet dongle supports the persist setting because it
> does a hw-reset during resume anyway.
> 
> So what I did is:
>  1) Set the persist bit for the usb device
>  2) Set the control to auto for the usb device
>  3) Unset the pm_qos_no_power_off flag for the usb-port
> 
> But the port gets not disabled. I debugged it and found a problem in
> usb_port_suspend() logic [1] and the generic PM-framework more precisely
> the dpm mechanism. The usbcore does the correct pm_runtime counting but
> the call [2] don't trigger the usb_port_runtime_suspend() [3] because
> the dpm enables all runtime-pm device before the shutdown is executed.

That's right; it's supposed to work that way.  We don't want runtime 
suspend kicking in and messing things up during a system suspend.

> IMHO both subsystem behaviours are correct and I don't know the
> _correct_ fix, therefore I wrote this email.

The correct fix is to add support for system suspend to the USB port 
driver.  Currently it only supports runtime suspend, as you can see 
from the definition of usb_port_pm_ops in port.c.

Alan Stern

> As far as I understood it all non-ACPI platforms are affected.
> 
> Regards,
>   Marco
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3238
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3328
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/port.c?h=v5.5#n247

Re: USB Port Power-Off during suspend Bug?

[Marco Felsch]

On 20-01-29 12:59, Alan Stern wrote:
> On Wed, 29 Jan 2020, Marco Felsch wrote:
> 
> > Hi Alan, Rafael, Greg,
> > 
> > long story short: I want to disable a usb-port completely during suspend
> 
> You're talking about what happens during a full system suspend, right?

Yes.

> > because it isn't needed and we need to save energy, because is a 32bit ARM
> > (OF-based) handheld device. I use the port to connect a usb-ethernet
> > dongle (all needed drivers are builtin no modules) which is needed for
> > the NFS. The usb-ethernet dongle supports the persist setting because it
> > does a hw-reset during resume anyway.
> > 
> > So what I did is:
> >  1) Set the persist bit for the usb device
> >  2) Set the control to auto for the usb device
> >  3) Unset the pm_qos_no_power_off flag for the usb-port
> > 
> > But the port gets not disabled. I debugged it and found a problem in
> > usb_port_suspend() logic [1] and the generic PM-framework more precisely
> > the dpm mechanism. The usbcore does the correct pm_runtime counting but
> > the call [2] don't trigger the usb_port_runtime_suspend() [3] because
> > the dpm enables all runtime-pm device before the shutdown is executed.
> 
> That's right; it's supposed to work that way.  We don't want runtime 
> suspend kicking in and messing things up during a system suspend.

I'm absolutly fine with that behaviour.

> > IMHO both subsystem behaviours are correct and I don't know the
> > _correct_ fix, therefore I wrote this email.
> 
> The correct fix is to add support for system suspend to the USB port 
> driver.  Currently it only supports runtime suspend, as you can see 
> from the definition of usb_port_pm_ops in port.c.

I tought that this was intentionally to support only the runtime-pm ops.
Okay so this means that we need to check the:
  - persist
  - do_wakeup
  - pm_qos_power_off
bits again for the suspend case. I tought I miss something and we can
reuse the current checks.

Regards,
  Marco

> Alan Stern
> 
> > As far as I understood it all non-ACPI platforms are affected.
> > 
> > Regards,
> >   Marco
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3238
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/hub.c?h=v5.5#n3328
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/core/port.c?h=v5.5#n247
> 
> 

-- 
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: USB Port Power-Off during suspend Bug?

[Alan Stern]

On Wed, 29 Jan 2020, Marco Felsch wrote:

> On 20-01-29 12:59, Alan Stern wrote:
> > On Wed, 29 Jan 2020, Marco Felsch wrote:
> > 
> > > Hi Alan, Rafael, Greg,
> > > 
> > > long story short: I want to disable a usb-port completely during suspend
> > 
> > You're talking about what happens during a full system suspend, right?
> 
> Yes.
> 
> > > because it isn't needed and we need to save energy, because is a 32bit ARM
> > > (OF-based) handheld device. I use the port to connect a usb-ethernet
> > > dongle (all needed drivers are builtin no modules) which is needed for
> > > the NFS. The usb-ethernet dongle supports the persist setting because it
> > > does a hw-reset during resume anyway.
> > > 
> > > So what I did is:
> > >  1) Set the persist bit for the usb device
> > >  2) Set the control to auto for the usb device
> > >  3) Unset the pm_qos_no_power_off flag for the usb-port
> > > 
> > > But the port gets not disabled. I debugged it and found a problem in
> > > usb_port_suspend() logic [1] and the generic PM-framework more precisely
> > > the dpm mechanism. The usbcore does the correct pm_runtime counting but
> > > the call [2] don't trigger the usb_port_runtime_suspend() [3] because
> > > the dpm enables all runtime-pm device before the shutdown is executed.
> > 
> > That's right; it's supposed to work that way.  We don't want runtime 
> > suspend kicking in and messing things up during a system suspend.
> 
> I'm absolutly fine with that behaviour.
> 
> > > IMHO both subsystem behaviours are correct and I don't know the
> > > _correct_ fix, therefore I wrote this email.
> > 
> > The correct fix is to add support for system suspend to the USB port 
> > driver.  Currently it only supports runtime suspend, as you can see 
> > from the definition of usb_port_pm_ops in port.c.
> 
> I tought that this was intentionally to support only the runtime-pm ops.

No, it wasn't intentional as far as I know.

> Okay so this means that we need to check the:
>   - persist
>   - do_wakeup
>   - pm_qos_power_off
> bits again for the suspend case. I tought I miss something and we can
> reuse the current checks.

We can.  Something like the patch below ought to work.  But I have not 
tested it, and it may very well cause problems for some people.

Alan Stern



Index: usb-devel/drivers/usb/core/port.c
===================================================================
--- usb-devel.orig/drivers/usb/core/port.c
+++ usb-devel/drivers/usb/core/port.c
@@ -283,7 +283,23 @@ static int usb_port_runtime_suspend(stru
 
 	return retval;
 }
+
+#ifdef CONFIG_PM_SLEEP
+
+/* Same as runtime suspend, but no error return */
+static int usb_port_system_suspend(struct device *dev)
+{
+	usb_port_runtime_suspend(dev);
+	return 0;
+}
+
+static int usb_port_system_resume(struct device *dev)
+{
+	return usb_port_runtime_resume(dev);
+}
+
 #endif
+#endif /* CONFIG_PM */
 
 static void usb_port_shutdown(struct device *dev)
 {
@@ -294,10 +310,8 @@ static void usb_port_shutdown(struct 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_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
+SET_SYSTEM_SLEEP_PM_OPS(usb_port_system_suspend, usb_port_system_resume)
 };
 
 struct device_type usb_port_device_type = {

Re: USB Port Power-Off during suspend Bug?

[Marco Felsch]

On 20-01-30 10:32, Alan Stern wrote:
> On Wed, 29 Jan 2020, Marco Felsch wrote:
> 
> > On 20-01-29 12:59, Alan Stern wrote:
> > > On Wed, 29 Jan 2020, Marco Felsch wrote:
> > > 
> > > > Hi Alan, Rafael, Greg,
> > > > 
> > > > long story short: I want to disable a usb-port completely during suspend
> > > 
> > > You're talking about what happens during a full system suspend, right?
> > 
> > Yes.
> > 
> > > > because it isn't needed and we need to save energy, because is a 32bit ARM
> > > > (OF-based) handheld device. I use the port to connect a usb-ethernet
> > > > dongle (all needed drivers are builtin no modules) which is needed for
> > > > the NFS. The usb-ethernet dongle supports the persist setting because it
> > > > does a hw-reset during resume anyway.
> > > > 
> > > > So what I did is:
> > > >  1) Set the persist bit for the usb device
> > > >  2) Set the control to auto for the usb device
> > > >  3) Unset the pm_qos_no_power_off flag for the usb-port
> > > > 
> > > > But the port gets not disabled. I debugged it and found a problem in
> > > > usb_port_suspend() logic [1] and the generic PM-framework more precisely
> > > > the dpm mechanism. The usbcore does the correct pm_runtime counting but
> > > > the call [2] don't trigger the usb_port_runtime_suspend() [3] because
> > > > the dpm enables all runtime-pm device before the shutdown is executed.
> > > 
> > > That's right; it's supposed to work that way.  We don't want runtime 
> > > suspend kicking in and messing things up during a system suspend.
> > 
> > I'm absolutly fine with that behaviour.
> > 
> > > > IMHO both subsystem behaviours are correct and I don't know the
> > > > _correct_ fix, therefore I wrote this email.
> > > 
> > > The correct fix is to add support for system suspend to the USB port 
> > > driver.  Currently it only supports runtime suspend, as you can see 
> > > from the definition of usb_port_pm_ops in port.c.
> > 
> > I tought that this was intentionally to support only the runtime-pm ops.
> 
> No, it wasn't intentional as far as I know.

Okay, adding a suspend handler seemed to be to easy ^^

> > Okay so this means that we need to check the:
> >   - persist
> >   - do_wakeup
> >   - pm_qos_power_off
> > bits again for the suspend case. I tought I miss something and we can
> > reuse the current checks.
> 
> We can.  Something like the patch below ought to work.  But I have not 
> tested it, and it may very well cause problems for some people.

Yes, something like that with a few more checks for the bits listed
above except the pm_qos_power_off. Thanks for your reply I will prepare
a patch next week.

Regards,
  Marco

> 
> Alan Stern
> 
> 
> 
> Index: usb-devel/drivers/usb/core/port.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/port.c
> +++ usb-devel/drivers/usb/core/port.c
> @@ -283,7 +283,23 @@ static int usb_port_runtime_suspend(stru
>  
>  	return retval;
>  }
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +/* Same as runtime suspend, but no error return */
> +static int usb_port_system_suspend(struct device *dev)
> +{
> +	usb_port_runtime_suspend(dev);
> +	return 0;
> +}
> +
> +static int usb_port_system_resume(struct device *dev)
> +{
> +	return usb_port_runtime_resume(dev);
> +}
> +
>  #endif
> +#endif /* CONFIG_PM */
>  
>  static void usb_port_shutdown(struct device *dev)
>  {
> @@ -294,10 +310,8 @@ static void usb_port_shutdown(struct 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_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
> +SET_SYSTEM_SLEEP_PM_OPS(usb_port_system_suspend, usb_port_system_resume)
>  };
>  
>  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 |