Hi,
this is v4 of an attempt to make easier for devices to remain in runtime
PM when the system ges to sleep, mainly to reduce the time spent
resuming devices.
In this version there's a patch from Alan that relaxes the conditions
that allow a device to go directly to the complete phase, thus allowing
its ancestors to do the same.
Also, we interpret the absence of all PM callback implementations as
being safe to do direct_complete as well.
With these changes, a uvcvideo device (for example) stays in runtime
suspend when the system goes to sleep and is left in that state when the
system resumes, not delaying it unnecessarily.
Thanks,
Tomeu
Alan Stern (1):
PM / sleep: Allow devices without runtime PM to do direct-complete
Tomeu Vizoso (2):
PM / sleep: Go direct_complete if driver has no callbacks
USB / PM: Allow USB devices to remain runtime-suspended when sleeping
Documentation/power/devices.txt | 7 +++++++
Documentation/power/runtime_pm.txt | 4 ----
drivers/base/power/main.c | 19 ++++++++++++++++++-
drivers/usb/core/port.c | 6 ++++++
drivers/usb/core/usb.c | 11 ++++++++++-
include/linux/pm_runtime.h | 6 ------
6 files changed, 41 insertions(+), 12 deletions(-)
--
2.4.3
From: Alan Stern <[email protected]>
Don't unset the direct_complete flag on devices that have runtime PM
disabled, if they are runtime suspended.
This is needed because otherwise ancestor devices wouldn't be able to
do direct_complete without adding runtime PM support to all its
descendants.
Also removes pm_runtime_suspended_if_enabled() because it's now unused.
Signed-off-by: Tomeu Vizoso <[email protected]>
Signed-off-by: Alan Stern <[email protected]>
---
Documentation/power/devices.txt | 7 +++++++
Documentation/power/runtime_pm.txt | 4 ----
drivers/base/power/main.c | 2 +-
include/linux/pm_runtime.h | 6 ------
4 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index d172bce0fd49..8ba6625fdd63 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -341,6 +341,13 @@ the phases are:
and is entirely responsible for bringing the device back to the
functional state as appropriate.
+ Note that this direct-complete procedure applies even if the device is
+ disabled for runtime PM; only the runtime-PM status matters. It follows
+ that if a device has system-sleep callbacks but does not support runtime
+ PM, then its prepare callback must never return a positive value. This
+ is because all devices are initially set to runtime-suspended with
+ runtime PM disabled.
+
2. The suspend methods should quiesce the device to stop it from performing
I/O. They also may save the device registers and put it into the
appropriate low-power state, depending on the bus type the device is on,
diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index e76dc0ad4d2b..0784bc3a2ab5 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -445,10 +445,6 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
bool pm_runtime_status_suspended(struct device *dev);
- return true if the device's runtime PM status is 'suspended'
- bool pm_runtime_suspended_if_enabled(struct device *dev);
- - return true if the device's runtime PM status is 'suspended' and its
- 'power.disable_depth' field is equal to 1
-
void pm_runtime_allow(struct device *dev);
- set the power.runtime_auto flag for the device and decrease its usage
counter (used by the /sys/devices/.../power/control interface to
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 30b7bbfdc558..1710c26ba097 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1377,7 +1377,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
if (dev->power.direct_complete) {
if (pm_runtime_status_suspended(dev)) {
pm_runtime_disable(dev);
- if (pm_runtime_suspended_if_enabled(dev))
+ if (pm_runtime_status_suspended(dev))
goto Complete;
pm_runtime_enable(dev);
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 30e84d48bfea..3bdbb4189780 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -98,11 +98,6 @@ static inline bool pm_runtime_status_suspended(struct device *dev)
return dev->power.runtime_status == RPM_SUSPENDED;
}
-static inline bool pm_runtime_suspended_if_enabled(struct device *dev)
-{
- return pm_runtime_status_suspended(dev) && dev->power.disable_depth == 1;
-}
-
static inline bool pm_runtime_enabled(struct device *dev)
{
return !dev->power.disable_depth;
@@ -164,7 +159,6 @@ static inline void device_set_run_wake(struct device *dev, bool enable) {}
static inline bool pm_runtime_suspended(struct device *dev) { return false; }
static inline bool pm_runtime_active(struct device *dev) { return true; }
static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
-static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return false; }
static inline bool pm_runtime_enabled(struct device *dev) { return false; }
static inline void pm_runtime_no_callbacks(struct device *dev) {}
--
2.4.3
If a suitable prepare callback cannot be found for a given device and
its driver has no PM callbacks at all, assume that it can go direct to
complete when the system goes to sleep.
The reason for this is that there's lots of devices in a system that do
no PM at all and there's no reason for them to prevent their ancestors
to do direct_complete if they can support it.
Signed-off-by: Tomeu Vizoso <[email protected]>
---
drivers/base/power/main.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26ba097..edda3f233c7c 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1540,6 +1540,21 @@ int dpm_suspend(pm_message_t state)
return error;
}
+static bool driver_has_no_pm_callbacks(struct device_driver *drv)
+{
+ if (!drv->pm)
+ return true;
+
+ return !drv->pm->prepare &&
+ !drv->pm->suspend &&
+ !drv->pm->suspend_late &&
+ !drv->pm->suspend_noirq &&
+ !drv->pm->resume_noirq &&
+ !drv->pm->resume_early &&
+ !drv->pm->resume &&
+ !drv->pm->complete;
+}
+
/**
* device_prepare - Prepare a device for system power transition.
* @dev: Device to handle.
@@ -1590,6 +1605,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
if (callback)
ret = callback(dev);
+ else if (!dev->driver || driver_has_no_pm_callbacks(dev->driver))
+ ret = 1; /* Let device go direct_complete */
device_unlock(dev);
--
2.4.3
Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
devices can remain runtime-suspended when the system goes to a sleep
state, if their wakeup state is correct and they have runtime PM enabled.
Signed-off-by: Tomeu Vizoso <[email protected]>
---
drivers/usb/core/port.c | 6 ++++++
drivers/usb/core/usb.c | 11 ++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 210618319f10..f49707d73b5a 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 8d5b2f4113cd..cf4dde11db1c 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -316,7 +316,16 @@ 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? */
+ struct usb_device *udev = to_usb_device(dev);
+
+ if (!pm_runtime_enabled(dev))
+ return 0;
+
+ /* Return 0 if the current wakeup setting is wrong, otherwise 1 */
+ if (udev->do_remote_wakeup != device_may_wakeup(dev))
+ return 0;
+
+ return 1;
}
static void usb_dev_complete(struct device *dev)
--
2.4.3
On Wed, 15 Jul 2015, Tomeu Vizoso wrote:
> If a suitable prepare callback cannot be found for a given device and
> its driver has no PM callbacks at all, assume that it can go direct to
> complete when the system goes to sleep.
>
> The reason for this is that there's lots of devices in a system that do
> no PM at all and there's no reason for them to prevent their ancestors
> to do direct_complete if they can support it.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> ---
>
> drivers/base/power/main.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 1710c26ba097..edda3f233c7c 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1540,6 +1540,21 @@ int dpm_suspend(pm_message_t state)
> return error;
> }
>
> +static bool driver_has_no_pm_callbacks(struct device_driver *drv)
> +{
> + if (!drv->pm)
> + return true;
> +
> + return !drv->pm->prepare &&
> + !drv->pm->suspend &&
> + !drv->pm->suspend_late &&
> + !drv->pm->suspend_noirq &&
> + !drv->pm->resume_noirq &&
> + !drv->pm->resume_early &&
> + !drv->pm->resume &&
> + !drv->pm->complete;
> +}
This isn't exactly what I meant. We also need to check the dev_pm_ops
fields in dev->pm_domain, dev->type, dev->class, and dev->bus. Only if
_all_ of these callbacks are missing should we use direct_complete.
Alan Stern
On Wednesday, July 15, 2015 02:40:06 PM Tomeu Vizoso wrote:
> From: Alan Stern <[email protected]>
>
> Don't unset the direct_complete flag on devices that have runtime PM
> disabled, if they are runtime suspended.
>
> This is needed because otherwise ancestor devices wouldn't be able to
> do direct_complete without adding runtime PM support to all its
> descendants.
>
> Also removes pm_runtime_suspended_if_enabled() because it's now unused.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> Signed-off-by: Alan Stern <[email protected]>
Applied, thanks!
> ---
>
> Documentation/power/devices.txt | 7 +++++++
> Documentation/power/runtime_pm.txt | 4 ----
> drivers/base/power/main.c | 2 +-
> include/linux/pm_runtime.h | 6 ------
> 4 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index d172bce0fd49..8ba6625fdd63 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -341,6 +341,13 @@ the phases are:
> and is entirely responsible for bringing the device back to the
> functional state as appropriate.
>
> + Note that this direct-complete procedure applies even if the device is
> + disabled for runtime PM; only the runtime-PM status matters. It follows
> + that if a device has system-sleep callbacks but does not support runtime
> + PM, then its prepare callback must never return a positive value. This
> + is because all devices are initially set to runtime-suspended with
> + runtime PM disabled.
> +
> 2. The suspend methods should quiesce the device to stop it from performing
> I/O. They also may save the device registers and put it into the
> appropriate low-power state, depending on the bus type the device is on,
> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
> index e76dc0ad4d2b..0784bc3a2ab5 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -445,10 +445,6 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
> bool pm_runtime_status_suspended(struct device *dev);
> - return true if the device's runtime PM status is 'suspended'
>
> - bool pm_runtime_suspended_if_enabled(struct device *dev);
> - - return true if the device's runtime PM status is 'suspended' and its
> - 'power.disable_depth' field is equal to 1
> -
> void pm_runtime_allow(struct device *dev);
> - set the power.runtime_auto flag for the device and decrease its usage
> counter (used by the /sys/devices/.../power/control interface to
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 30b7bbfdc558..1710c26ba097 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1377,7 +1377,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
> if (dev->power.direct_complete) {
> if (pm_runtime_status_suspended(dev)) {
> pm_runtime_disable(dev);
> - if (pm_runtime_suspended_if_enabled(dev))
> + if (pm_runtime_status_suspended(dev))
> goto Complete;
>
> pm_runtime_enable(dev);
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 30e84d48bfea..3bdbb4189780 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -98,11 +98,6 @@ static inline bool pm_runtime_status_suspended(struct device *dev)
> return dev->power.runtime_status == RPM_SUSPENDED;
> }
>
> -static inline bool pm_runtime_suspended_if_enabled(struct device *dev)
> -{
> - return pm_runtime_status_suspended(dev) && dev->power.disable_depth == 1;
> -}
> -
> static inline bool pm_runtime_enabled(struct device *dev)
> {
> return !dev->power.disable_depth;
> @@ -164,7 +159,6 @@ static inline void device_set_run_wake(struct device *dev, bool enable) {}
> static inline bool pm_runtime_suspended(struct device *dev) { return false; }
> static inline bool pm_runtime_active(struct device *dev) { return true; }
> static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
> -static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return false; }
> static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>
> static inline void pm_runtime_no_callbacks(struct device *dev) {}
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wednesday, July 15, 2015 02:47:50 PM Alan Stern wrote:
> On Wed, 15 Jul 2015, Tomeu Vizoso wrote:
>
> > If a suitable prepare callback cannot be found for a given device and
> > its driver has no PM callbacks at all, assume that it can go direct to
> > complete when the system goes to sleep.
> >
> > The reason for this is that there's lots of devices in a system that do
> > no PM at all and there's no reason for them to prevent their ancestors
> > to do direct_complete if they can support it.
> >
> > Signed-off-by: Tomeu Vizoso <[email protected]>
> > ---
> >
> > drivers/base/power/main.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 1710c26ba097..edda3f233c7c 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1540,6 +1540,21 @@ int dpm_suspend(pm_message_t state)
> > return error;
> > }
> >
> > +static bool driver_has_no_pm_callbacks(struct device_driver *drv)
> > +{
> > + if (!drv->pm)
> > + return true;
> > +
> > + return !drv->pm->prepare &&
> > + !drv->pm->suspend &&
> > + !drv->pm->suspend_late &&
> > + !drv->pm->suspend_noirq &&
> > + !drv->pm->resume_noirq &&
> > + !drv->pm->resume_early &&
> > + !drv->pm->resume &&
> > + !drv->pm->complete;
> > +}
>
> This isn't exactly what I meant. We also need to check the dev_pm_ops
> fields in dev->pm_domain, dev->type, dev->class, and dev->bus. Only if
> _all_ of these callbacks are missing should we use direct_complete.
Also checking that on every suspend is kind of wasteful, because those things
do not change very often.
Thanks,
Rafael
On Wednesday, July 15, 2015 02:40:08 PM Tomeu Vizoso wrote:
> Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
> devices can remain runtime-suspended when the system goes to a sleep
> state, if their wakeup state is correct and they have runtime PM enabled.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> ---
>
> drivers/usb/core/port.c | 6 ++++++
> drivers/usb/core/usb.c | 11 ++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 210618319f10..f49707d73b5a 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 8d5b2f4113cd..cf4dde11db1c 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -316,7 +316,16 @@ 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? */
> + struct usb_device *udev = to_usb_device(dev);
> +
> + if (!pm_runtime_enabled(dev))
Why just enabled and not suspended?
> + return 0;
> +
> + /* Return 0 if the current wakeup setting is wrong, otherwise 1 */
> + if (udev->do_remote_wakeup != device_may_wakeup(dev))
> + return 0;
> +
> + return 1;
> }
>
> static void usb_dev_complete(struct device *dev)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 16 July 2015 at 02:41, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, July 15, 2015 02:47:50 PM Alan Stern wrote:
>> On Wed, 15 Jul 2015, Tomeu Vizoso wrote:
>>
>> > If a suitable prepare callback cannot be found for a given device and
>> > its driver has no PM callbacks at all, assume that it can go direct to
>> > complete when the system goes to sleep.
>> >
>> > The reason for this is that there's lots of devices in a system that do
>> > no PM at all and there's no reason for them to prevent their ancestors
>> > to do direct_complete if they can support it.
>> >
>> > Signed-off-by: Tomeu Vizoso <[email protected]>
>> > ---
>> >
>> > drivers/base/power/main.c | 17 +++++++++++++++++
>> > 1 file changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> > index 1710c26ba097..edda3f233c7c 100644
>> > --- a/drivers/base/power/main.c
>> > +++ b/drivers/base/power/main.c
>> > @@ -1540,6 +1540,21 @@ int dpm_suspend(pm_message_t state)
>> > return error;
>> > }
>> >
>> > +static bool driver_has_no_pm_callbacks(struct device_driver *drv)
>> > +{
>> > + if (!drv->pm)
>> > + return true;
>> > +
>> > + return !drv->pm->prepare &&
>> > + !drv->pm->suspend &&
>> > + !drv->pm->suspend_late &&
>> > + !drv->pm->suspend_noirq &&
>> > + !drv->pm->resume_noirq &&
>> > + !drv->pm->resume_early &&
>> > + !drv->pm->resume &&
>> > + !drv->pm->complete;
>> > +}
>>
>> This isn't exactly what I meant. We also need to check the dev_pm_ops
>> fields in dev->pm_domain, dev->type, dev->class, and dev->bus. Only if
>> _all_ of these callbacks are missing should we use direct_complete.
>
> Also checking that on every suspend is kind of wasteful, because those things
> do not change very often.
Do you have any suggestion on when would be a good time to do that
check? device_pm_sleep_init() and device_pm_add() are unfortunately
too early.
Alternatively we could check once on the first suspend and cache it,
but I'm not sure that complexity would be worth it.
Thanks,
Tomeu
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 16 July 2015 at 02:42, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, July 15, 2015 02:40:08 PM Tomeu Vizoso wrote:
>> Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
>> devices can remain runtime-suspended when the system goes to a sleep
>> state, if their wakeup state is correct and they have runtime PM enabled.
>>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>> ---
>>
>> drivers/usb/core/port.c | 6 ++++++
>> drivers/usb/core/usb.c | 11 ++++++++++-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 210618319f10..f49707d73b5a 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 8d5b2f4113cd..cf4dde11db1c 100644
>> --- a/drivers/usb/core/usb.c
>> +++ b/drivers/usb/core/usb.c
>> @@ -316,7 +316,16 @@ 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? */
>> + struct usb_device *udev = to_usb_device(dev);
>> +
>> + if (!pm_runtime_enabled(dev))
>
> Why just enabled and not suspended?
Hmm, the core checks if it's runtime suspended before going
direct_complete, but it's true that the API docs say that it should
return a positive value only if it's runtime suspended.
Is there a reason why the prepare() implementations have to check that
instead of leaving it to the core?
Thanks,
Tomeu
>> + return 0;
>> +
>> + /* Return 0 if the current wakeup setting is wrong, otherwise 1 */
>> + if (udev->do_remote_wakeup != device_may_wakeup(dev))
>> + return 0;
>> +
>> + return 1;
>> }
>>
>> static void usb_dev_complete(struct device *dev)
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Thursday, July 16, 2015 10:47:51 AM Tomeu Vizoso wrote:
> On 16 July 2015 at 02:41, Rafael J. Wysocki <[email protected]> wrote:
> > On Wednesday, July 15, 2015 02:47:50 PM Alan Stern wrote:
> >> On Wed, 15 Jul 2015, Tomeu Vizoso wrote:
> >>
> >> > If a suitable prepare callback cannot be found for a given device and
> >> > its driver has no PM callbacks at all, assume that it can go direct to
> >> > complete when the system goes to sleep.
> >> >
> >> > The reason for this is that there's lots of devices in a system that do
> >> > no PM at all and there's no reason for them to prevent their ancestors
> >> > to do direct_complete if they can support it.
> >> >
> >> > Signed-off-by: Tomeu Vizoso <[email protected]>
> >> > ---
> >> >
> >> > drivers/base/power/main.c | 17 +++++++++++++++++
> >> > 1 file changed, 17 insertions(+)
> >> >
> >> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> > index 1710c26ba097..edda3f233c7c 100644
> >> > --- a/drivers/base/power/main.c
> >> > +++ b/drivers/base/power/main.c
> >> > @@ -1540,6 +1540,21 @@ int dpm_suspend(pm_message_t state)
> >> > return error;
> >> > }
> >> >
> >> > +static bool driver_has_no_pm_callbacks(struct device_driver *drv)
> >> > +{
> >> > + if (!drv->pm)
> >> > + return true;
> >> > +
> >> > + return !drv->pm->prepare &&
> >> > + !drv->pm->suspend &&
> >> > + !drv->pm->suspend_late &&
> >> > + !drv->pm->suspend_noirq &&
> >> > + !drv->pm->resume_noirq &&
> >> > + !drv->pm->resume_early &&
> >> > + !drv->pm->resume &&
> >> > + !drv->pm->complete;
> >> > +}
> >>
> >> This isn't exactly what I meant. We also need to check the dev_pm_ops
> >> fields in dev->pm_domain, dev->type, dev->class, and dev->bus. Only if
> >> _all_ of these callbacks are missing should we use direct_complete.
> >
> > Also checking that on every suspend is kind of wasteful, because those things
> > do not change very often.
>
> Do you have any suggestion on when would be a good time to do that
> check? device_pm_sleep_init() and device_pm_add() are unfortunately
> too early.
The time to check that is (a) when the device is registered (for the bus
type/class/device type), (b) when it is added to (or removed from) a PM
domain and (c) when a driver is bound to (unbound from) it.
> Alternatively we could check once on the first suspend and cache it,
That information may be stale by the time we suspend next time.
> but I'm not sure that complexity would be worth it.
I don't think that adding extra overhead to *every* suspend can be
justified easily, however.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thursday, July 16, 2015 02:09:41 PM Tomeu Vizoso wrote:
> On 16 July 2015 at 02:42, Rafael J. Wysocki <[email protected]> wrote:
> > On Wednesday, July 15, 2015 02:40:08 PM Tomeu Vizoso wrote:
> >> Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
> >> devices can remain runtime-suspended when the system goes to a sleep
> >> state, if their wakeup state is correct and they have runtime PM enabled.
> >>
> >> Signed-off-by: Tomeu Vizoso <[email protected]>
> >> ---
> >>
> >> drivers/usb/core/port.c | 6 ++++++
> >> drivers/usb/core/usb.c | 11 ++++++++++-
> >> 2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> >> index 210618319f10..f49707d73b5a 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 8d5b2f4113cd..cf4dde11db1c 100644
> >> --- a/drivers/usb/core/usb.c
> >> +++ b/drivers/usb/core/usb.c
> >> @@ -316,7 +316,16 @@ 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? */
> >> + struct usb_device *udev = to_usb_device(dev);
> >> +
> >> + if (!pm_runtime_enabled(dev))
> >
> > Why just enabled and not suspended?
>
> Hmm, the core checks if it's runtime suspended before going
> direct_complete, but it's true that the API docs say that it should
> return a positive value only if it's runtime suspended.
>
> Is there a reason why the prepare() implementations have to check that
> instead of leaving it to the core?
The concern is that they generally need to check the state of the device
which is meaningless from the suspend suitability (so to speak) perspective
if the device is not (runtime) suspended when the check is made.
But this particular case seems to be exceptional as ->prepare doesn't realy
check the device's (hardware) state, but only its software configuration
(if I'm not mistaken). So checking the RPM status is not needed here.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.