| Message ID | 1427818501-10201-7-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Tue, 31 Mar 2015, Tomeu Vizoso wrote: > Have dev_pm_ops.prepare return 1 for USB devices, interfaces, endpoints > and ports so that USB devices can remain runtime-suspended when the > system goes to a sleep state. > > Also enable runtime PM for endpoints, which is another requirement for > the above to work. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> ... > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index b1fb9ae..1498faa 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -300,7 +300,7 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env) > > static int usb_dev_prepare(struct device *dev) > { > - return 0; /* Implement eventually? */ > + return 1; /* Implement eventually? */ > } The rest of the patch is okay, but this part is wrong. The documented requirement is that the prepare callback should return 1 if the device is already in the appropriate state. This means it has to have the right wakeup setting. In other words, if the device is currently in runtime suspend with remote wakeup enabled, but device_may_wakeup() returns 0 (so that the device should be disabled for wakeup when the system goes into suspend), then the prepare callback has to return 0. Therefore what you need to do here is something like this: 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; Aside from this issue, I like the patch set. Do you think you can do something similar for drivers/scsi/scsi_pm.c? I'm not aware of any wakeup-capable SCSI devices -- not disk or CD/DVD drives, anyway. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote: > In other words, if the device is currently in runtime suspend with > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the > device should be disabled for wakeup when the system goes into > suspend), then the prepare callback has to return 0. > > Therefore what you need to do here is something like this: > > struct usb_device *udev = to_usb_device(dev); > > /* Return 0 if the current wakeup setting is wrong, otherwise > 1 */ And the other way round? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 1 Apr 2015, Oliver Neukum wrote: > On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote: > > In other words, if the device is currently in runtime suspend with > > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the > > device should be disabled for wakeup when the system goes into > > suspend), then the prepare callback has to return 0. > > > > Therefore what you need to do here is something like this: > > > > struct usb_device *udev = to_usb_device(dev); > > > > /* Return 0 if the current wakeup setting is wrong, otherwise > > 1 */ > > And the other way round? Your meaning isn't clear. Are you asking what should happen if the device is in runtime suspend with remote wakeup disabled, but device_may_wakeup() returns 1? That case should never happen -- but if it does then the prepare callback should return 0. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-04-01 at 10:01 -0400, Alan Stern wrote: > On Wed, 1 Apr 2015, Oliver Neukum wrote: > > > On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote: > > > In other words, if the device is currently in runtime suspend with > > > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the > > > device should be disabled for wakeup when the system goes into > > > suspend), then the prepare callback has to return 0. > > > > > > Therefore what you need to do here is something like this: > > > > > > struct usb_device *udev = to_usb_device(dev); > > > > > > /* Return 0 if the current wakeup setting is wrong, otherwise > > > 1 */ > > > > And the other way round? > > Your meaning isn't clear. Are you asking what should happen if the > device is in runtime suspend with remote wakeup disabled, but > device_may_wakeup() returns 1? Yes. I was thinking about that case. > That case should never happen -- but > if it does then the prepare callback should return 0. Exactly. It didn't seem to do so. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 1 Apr 2015, Oliver Neukum wrote: > On Wed, 2015-04-01 at 10:01 -0400, Alan Stern wrote: > > On Wed, 1 Apr 2015, Oliver Neukum wrote: > > > > > On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote: > > > > In other words, if the device is currently in runtime suspend with > > > > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the > > > > device should be disabled for wakeup when the system goes into > > > > suspend), then the prepare callback has to return 0. > > > > > > > > Therefore what you need to do here is something like this: > > > > > > > > struct usb_device *udev = to_usb_device(dev); > > > > > > > > /* Return 0 if the current wakeup setting is wrong, otherwise > > > > 1 */ > > > > > > And the other way round? > > > > Your meaning isn't clear. Are you asking what should happen if the > > device is in runtime suspend with remote wakeup disabled, but > > device_may_wakeup() returns 1? > > Yes. I was thinking about that case. > > > That case should never happen -- but > > if it does then the prepare callback should return 0. > > Exactly. It didn't seem to do so. Well, Tomeu can fix it up to handle both cases properly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 March 2015 at 19:09, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 31 Mar 2015, Tomeu Vizoso wrote: > >> Have dev_pm_ops.prepare return 1 for USB devices, interfaces, endpoints >> and ports so that USB devices can remain runtime-suspended when the >> system goes to a sleep state. >> >> Also enable runtime PM for endpoints, which is another requirement for >> the above to work. >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > ... > >> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c >> index b1fb9ae..1498faa 100644 >> --- a/drivers/usb/core/usb.c >> +++ b/drivers/usb/core/usb.c >> @@ -300,7 +300,7 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env) >> >> static int usb_dev_prepare(struct device *dev) >> { >> - return 0; /* Implement eventually? */ >> + return 1; /* Implement eventually? */ >> } > > The rest of the patch is okay, but this part is wrong. The documented > requirement is that the prepare callback should return 1 if the device > is already in the appropriate state. This means it has to have the > right wakeup setting. > > In other words, if the device is currently in runtime suspend with > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the > device should be disabled for wakeup when the system goes into > suspend), then the prepare callback has to return 0. > > Therefore what you need to do here is something like this: > > 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; Thanks, in v2 I have also covered the other case, as suggested by Oliver. > Aside from this issue, I like the patch set. Do you think you can do > something similar for drivers/scsi/scsi_pm.c? I'm not aware of any > wakeup-capable SCSI devices -- not disk or CD/DVD drives, anyway. I think I will be able to allocate some time on monday to look at this. Thanks, Tomeu > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c index 39a2402..7c82bb7 100644 --- a/drivers/usb/core/endpoint.c +++ b/drivers/usb/core/endpoint.c @@ -160,6 +160,19 @@ static const struct attribute_group *ep_dev_groups[] = { NULL }; +#ifdef CONFIG_PM + +static int usb_ep_device_prepare(struct device *dev) +{ + return 1; +} + +static const struct dev_pm_ops usb_ep_device_pm_ops = { + .prepare = usb_ep_device_prepare, +}; + +#endif /* CONFIG_PM */ + static void ep_device_release(struct device *dev) { struct ep_device *ep_dev = to_ep_device(dev); @@ -170,6 +183,9 @@ static void ep_device_release(struct device *dev) struct device_type usb_ep_device_type = { .name = "usb_endpoint", .release = ep_device_release, +#ifdef CONFIG_PM + .pm = &usb_ep_device_pm_ops, +#endif }; int usb_create_ep_devs(struct device *parent, @@ -197,6 +213,7 @@ int usb_create_ep_devs(struct device *parent, goto error_register; device_enable_async_suspend(&ep_dev->dev); + pm_runtime_enable(&ep_dev->dev); endpoint->ep_dev = ep_dev; return retval; diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index f368d20..9041aee 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1589,10 +1589,26 @@ static int usb_if_uevent(struct device *dev, struct kobj_uevent_env *env) return 0; } +#ifdef CONFIG_PM + +static int usb_if_prepare(struct device *dev) +{ + return 1; +} + +static const struct dev_pm_ops usb_if_pm_ops = { + .prepare = usb_if_prepare, +}; + +#endif /* CONFIG_PM */ + struct device_type usb_if_device_type = { .name = "usb_interface", .release = usb_release_interface, .uevent = usb_if_uevent, +#ifdef CONFIG_PM + .pm = &usb_if_pm_ops, +#endif }; static struct usb_interface_assoc_descriptor *find_iad(struct usb_device *dev, diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 2106183..f49707d 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -168,12 +168,18 @@ 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 b1fb9ae..1498faa 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -300,7 +300,7 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env) static int usb_dev_prepare(struct device *dev) { - return 0; /* Implement eventually? */ + return 1; /* Implement eventually? */ } static void usb_dev_complete(struct device *dev)
Have dev_pm_ops.prepare return 1 for USB devices, interfaces, endpoints and ports so that USB devices can remain runtime-suspended when the system goes to a sleep state. Also enable runtime PM for endpoints, which is another requirement for the above to work. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- drivers/usb/core/endpoint.c | 17 +++++++++++++++++ drivers/usb/core/message.c | 16 ++++++++++++++++ drivers/usb/core/port.c | 6 ++++++ drivers/usb/core/usb.c | 2 +- 4 files changed, 40 insertions(+), 1 deletion(-)