[PATCH v7 0/2] Allow USB devices to remain runtime-suspended when sleeping

[PATCH v7 0/2] Allow USB devices to remain runtime-suspended when sleeping

[Tomeu Vizoso]

Hi,

this is v7 of an attempt to make it easier for devices to remain in
runtime PM when the system goes to sleep, mainly to reduce the time
spent resuming devices.

For this, we interpret the absence of all PM callback implementations as
it being safe to do direct_complete, so their ancestors aren't prevented
from remaining runtime-suspended.

Additionally, the prepare() callback of USB devices will return 1 if
runtime PM is enabled and the current wakeup settings are correct.

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

Changes in v7:
- Reduce indentation by adding a label in device_prepare()

Changes in v6:
- Add stub for !CONFIG_PM.
- Move implementation of device_check_pm_callbacks to power/main.c as it
  doesn't belong to CONFIG_PM_SLEEP.
- Take dev->power.lock before modifying flag.

Changes in v5:
- Check for all dev_pm_ops instances associated to a device, updating a
  no_pm_callbacks flag at the times when that could change.

Tomeu Vizoso (2):
  PM / sleep: Go direct_complete if driver has no callbacks
  USB / PM: Allow USB devices to remain runtime-suspended when sleeping

 drivers/base/dd.c           |  3 +++
 drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
 drivers/base/power/domain.c |  5 +++++
 drivers/base/power/main.c   |  8 ++++++++
 drivers/base/power/power.h  |  6 ++++++
 drivers/usb/core/port.c     |  6 ++++++
 drivers/usb/core/usb.c      | 11 ++++++++++-
 include/linux/pm.h          |  1 +
 8 files changed, 66 insertions(+), 1 deletion(-)

-- 
2.4.3

[PATCH v7 0/2] Allow USB devices to remain runtime-suspended when sleeping

[Tomeu Vizoso]

Hi,

this is v7 of an attempt to make it easier for devices to remain in
runtime PM when the system goes to sleep, mainly to reduce the time
spent resuming devices.

For this, we interpret the absence of all PM callback implementations as
it being safe to do direct_complete, so their ancestors aren't prevented
from remaining runtime-suspended.

Additionally, the prepare() callback of USB devices will return 1 if
runtime PM is enabled and the current wakeup settings are correct.

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

Changes in v7:
- Reduce indentation by adding a label in device_prepare()

Changes in v6:
- Add stub for !CONFIG_PM.
- Move implementation of device_check_pm_callbacks to power/main.c as it
  doesn't belong to CONFIG_PM_SLEEP.
- Take dev->power.lock before modifying flag.

Changes in v5:
- Check for all dev_pm_ops instances associated to a device, updating a
  no_pm_callbacks flag at the times when that could change.

Tomeu Vizoso (2):
  PM / sleep: Go direct_complete if driver has no callbacks
  USB / PM: Allow USB devices to remain runtime-suspended when sleeping

 drivers/base/dd.c           |  3 +++
 drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
 drivers/base/power/domain.c |  5 +++++
 drivers/base/power/main.c   |  8 ++++++++
 drivers/base/power/power.h  |  6 ++++++
 drivers/usb/core/port.c     |  6 ++++++
 drivers/usb/core/usb.c      | 11 ++++++++++-
 include/linux/pm.h          |  1 +
 8 files changed, 66 insertions(+), 1 deletion(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Tomeu Vizoso]

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 <tomeu.vizoso@collabora.com>
---

Changes in v7:
- Reduce indentation by adding a label in device_prepare()

Changes in v6:
- Add stub for !CONFIG_PM.
- Move implementation of device_check_pm_callbacks to power/main.c as it
  doesn't belong to CONFIG_PM_SLEEP.
- Take dev->power.lock before modifying flag.

Changes in v5:
- Check for all dev_pm_ops instances associated to a device, updating a
  no_pm_callbacks flag at the times when that could change.

 drivers/base/dd.c           |  3 +++
 drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
 drivers/base/power/domain.c |  5 +++++
 drivers/base/power/main.c   |  8 ++++++++
 drivers/base/power/power.h  |  6 ++++++
 include/linux/pm.h          |  1 +
 6 files changed, 50 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..fe0e9cb684b8 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -205,6 +205,8 @@ static void driver_bound(struct device *dev)
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
 
+	device_check_pm_callbacks(dev);
+
 	/*
 	 * Make sure the device is no longer in one of the deferred lists and
 	 * kick off retrying all pending devices
@@ -695,6 +697,7 @@ static void __device_release_driver(struct device *dev)
 			dev->pm_domain->dismiss(dev);
 
 		klist_remove(&dev->p->knode_driver);
+		device_check_pm_callbacks(dev);
 		if (dev->bus)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBOUND_DRIVER,
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index f32b802b98f4..1bba85f8bf8a 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -128,3 +128,30 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+static bool pm_ops_is_empty(const struct dev_pm_ops *ops)
+{
+	if (!ops)
+		return true;
+
+	return !ops->prepare &&
+	       !ops->suspend &&
+	       !ops->suspend_late &&
+	       !ops->suspend_noirq &&
+	       !ops->resume_noirq &&
+	       !ops->resume_early &&
+	       !ops->resume &&
+	       !ops->complete;
+}
+
+void device_check_pm_callbacks(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.no_pm_callbacks =
+		(!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
+		(!dev->class || pm_ops_is_empty(dev->class->pm)) &&
+		(!dev->type || pm_ops_is_empty(dev->type->pm)) &&
+		(!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
+		(!dev->driver || pm_ops_is_empty(dev->driver->pm));
+	spin_unlock_irq(&dev->power.lock);
+}
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 16550c63d611..3cae1aa1885b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -20,6 +20,8 @@
 #include <linux/suspend.h>
 #include <linux/export.h>
 
+#include "power.h"
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -1305,6 +1307,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
+	device_check_pm_callbacks(dev);
  out:
 	mutex_unlock(&genpd->lock);
 
@@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	genpd_free_dev_data(dev, gpd_data);
 
+	device_check_pm_callbacks(dev);
+
 	return 0;
 
  out:
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26ba097..5af45d20b677 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -131,6 +131,8 @@ void device_pm_add(struct device *dev)
 			dev_name(dev->parent));
 	list_add_tail(&dev->power.entry, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
+
+	device_check_pm_callbacks(dev);
 }
 
 /**
@@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
 
 	dev->power.wakeup_path = device_may_wakeup(dev);
 
+	if (dev->power.no_pm_callbacks) {
+		ret = 1;	/* Let device go direct_complete */
+		goto unlock;
+	}
+
 	if (dev->pm_domain) {
 		info = "preparing power domain ";
 		callback = dev->pm_domain->ops.prepare;
@@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	if (callback)
 		ret = callback(dev);
 
+unlock:
 	device_unlock(dev);
 
 	if (ret < 0) {
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 998fa6b23084..f665a7850016 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -29,6 +29,8 @@ struct wake_irq {
 extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
 extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
 
+extern void device_check_pm_callbacks(struct device *dev);
+
 #ifdef CONFIG_PM_SLEEP
 
 extern int device_wakeup_attach_irq(struct device *dev,
@@ -102,6 +104,10 @@ static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
 {
 }
 
+static inline void device_check_pm_callbacks(struct device *dev)
+{
+}
+
 #endif
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 35d599e7250d..e334b9b8cd46 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -566,6 +566,7 @@ struct dev_pm_info {
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
 	bool			direct_complete:1;	/* Owned by the PM core */
+	bool			no_pm_callbacks:1;	/* Owned by the PM core */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
-- 
2.4.3

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

[Tomeu Vizoso]

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 <tomeu.vizoso@collabora.com>
---


 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

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Ulf Hansson]

On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> 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 <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v7:
> - Reduce indentation by adding a label in device_prepare()
>
> Changes in v6:
> - Add stub for !CONFIG_PM.
> - Move implementation of device_check_pm_callbacks to power/main.c as it
>   doesn't belong to CONFIG_PM_SLEEP.
> - Take dev->power.lock before modifying flag.
>
> Changes in v5:
> - Check for all dev_pm_ops instances associated to a device, updating a
>   no_pm_callbacks flag at the times when that could change.
>
>  drivers/base/dd.c           |  3 +++
>  drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
>  drivers/base/power/domain.c |  5 +++++
>  drivers/base/power/main.c   |  8 ++++++++
>  drivers/base/power/power.h  |  6 ++++++
>  include/linux/pm.h          |  1 +
>  6 files changed, 50 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb4639128..fe0e9cb684b8 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -205,6 +205,8 @@ static void driver_bound(struct device *dev)
>
>         klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
>
> +       device_check_pm_callbacks(dev);
> +
>         /*
>          * Make sure the device is no longer in one of the deferred lists and
>          * kick off retrying all pending devices
> @@ -695,6 +697,7 @@ static void __device_release_driver(struct device *dev)
>                         dev->pm_domain->dismiss(dev);
>
>                 klist_remove(&dev->p->knode_driver);
> +               device_check_pm_callbacks(dev);
>                 if (dev->bus)
>                         blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>                                                      BUS_NOTIFY_UNBOUND_DRIVER,
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index f32b802b98f4..1bba85f8bf8a 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -128,3 +128,30 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>                 dev->pm_domain->detach(dev, power_off);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> +
> +static bool pm_ops_is_empty(const struct dev_pm_ops *ops)
> +{
> +       if (!ops)
> +               return true;
> +
> +       return !ops->prepare &&
> +              !ops->suspend &&
> +              !ops->suspend_late &&
> +              !ops->suspend_noirq &&
> +              !ops->resume_noirq &&
> +              !ops->resume_early &&
> +              !ops->resume &&
> +              !ops->complete;
> +}
> +
> +void device_check_pm_callbacks(struct device *dev)
> +{
> +       spin_lock_irq(&dev->power.lock);
> +       dev->power.no_pm_callbacks =
> +               (!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
> +               (!dev->class || pm_ops_is_empty(dev->class->pm)) &&
> +               (!dev->type || pm_ops_is_empty(dev->type->pm)) &&
> +               (!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
> +               (!dev->driver || pm_ops_is_empty(dev->driver->pm));
> +       spin_unlock_irq(&dev->power.lock);
> +}
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 16550c63d611..3cae1aa1885b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -20,6 +20,8 @@
>  #include <linux/suspend.h>
>  #include <linux/export.h>
>
> +#include "power.h"
> +
>  #define GENPD_RETRY_MAX_MS     250             /* Approximate */
>
>  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
> @@ -1305,6 +1307,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>
>         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>
> +       device_check_pm_callbacks(dev);

Don't do this while the genpd mutex is held as there is no need to.
Please move it outside the lock and only do it for non-error case.

>   out:
>         mutex_unlock(&genpd->lock);
>
> @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
>         genpd_free_dev_data(dev, gpd_data);
>
> +       device_check_pm_callbacks(dev);
> +
>         return 0;

I can't tell whether this is an interesting feature to use for devices
that gets attached to the ACPI PM domain. Although, you currently
doesn't deal with that case, and too me I think this looks a bit
weird/unsymmetrical.

>
>   out:
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 1710c26ba097..5af45d20b677 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -131,6 +131,8 @@ void device_pm_add(struct device *dev)
>                         dev_name(dev->parent));
>         list_add_tail(&dev->power.entry, &dpm_list);
>         mutex_unlock(&dpm_list_mtx);
> +
> +       device_check_pm_callbacks(dev);
>  }
>
>  /**
> @@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
>
>         dev->power.wakeup_path = device_may_wakeup(dev);
>
> +       if (dev->power.no_pm_callbacks) {
> +               ret = 1;        /* Let device go direct_complete */
> +               goto unlock;
> +       }
> +
>         if (dev->pm_domain) {
>                 info = "preparing power domain ";
>                 callback = dev->pm_domain->ops.prepare;
> @@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
>         if (callback)
>                 ret = callback(dev);
>
> +unlock:
>         device_unlock(dev);
>
>         if (ret < 0) {
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 998fa6b23084..f665a7850016 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -29,6 +29,8 @@ struct wake_irq {
>  extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
>  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
>
> +extern void device_check_pm_callbacks(struct device *dev);
> +
>  #ifdef CONFIG_PM_SLEEP
>
>  extern int device_wakeup_attach_irq(struct device *dev,
> @@ -102,6 +104,10 @@ static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
>  {
>  }
>
> +static inline void device_check_pm_callbacks(struct device *dev)
> +{
> +}
> +
>  #endif
>
>  #ifdef CONFIG_PM_SLEEP
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 35d599e7250d..e334b9b8cd46 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -566,6 +566,7 @@ struct dev_pm_info {
>         bool                    ignore_children:1;
>         bool                    early_init:1;   /* Owned by the PM core */
>         bool                    direct_complete:1;      /* Owned by the PM core */
> +       bool                    no_pm_callbacks:1;      /* Owned by the PM core */

This flag is only being used while CONFIG_PM is set. I suggest we move
within that ifdef.

>         spinlock_t              lock;
>  #ifdef CONFIG_PM_SLEEP
>         struct list_head        entry;
> --
> 2.4.3
>

Kind regards
Uffe

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> 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 <tomeu.vizoso@collabora.com>
> > ---
> >
> > Changes in v7:
> > - Reduce indentation by adding a label in device_prepare()
> >
> > Changes in v6:
> > - Add stub for !CONFIG_PM.
> > - Move implementation of device_check_pm_callbacks to power/main.c as it
> >   doesn't belong to CONFIG_PM_SLEEP.
> > - Take dev->power.lock before modifying flag.
> >
> > Changes in v5:
> > - Check for all dev_pm_ops instances associated to a device, updating a
> >   no_pm_callbacks flag at the times when that could change.
> >
> >  drivers/base/dd.c           |  3 +++
> >  drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
> >  drivers/base/power/domain.c |  5 +++++
> >  drivers/base/power/main.c   |  8 ++++++++
> >  drivers/base/power/power.h  |  6 ++++++
> >  include/linux/pm.h          |  1 +
> >  6 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index be0eb4639128..fe0e9cb684b8 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -205,6 +205,8 @@ static void driver_bound(struct device *dev)
> >
> >         klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
> >
> > +       device_check_pm_callbacks(dev);
> > +
> >         /*
> >          * Make sure the device is no longer in one of the deferred lists and
> >          * kick off retrying all pending devices
> > @@ -695,6 +697,7 @@ static void __device_release_driver(struct device *dev)
> >                         dev->pm_domain->dismiss(dev);
> >
> >                 klist_remove(&dev->p->knode_driver);
> > +               device_check_pm_callbacks(dev);
> >                 if (dev->bus)
> >                         blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> >                                                      BUS_NOTIFY_UNBOUND_DRIVER,
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index f32b802b98f4..1bba85f8bf8a 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -128,3 +128,30 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
> >                 dev->pm_domain->detach(dev, power_off);
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> > +
> > +static bool pm_ops_is_empty(const struct dev_pm_ops *ops)
> > +{
> > +       if (!ops)
> > +               return true;
> > +
> > +       return !ops->prepare &&
> > +              !ops->suspend &&
> > +              !ops->suspend_late &&
> > +              !ops->suspend_noirq &&
> > +              !ops->resume_noirq &&
> > +              !ops->resume_early &&
> > +              !ops->resume &&
> > +              !ops->complete;
> > +}
> > +
> > +void device_check_pm_callbacks(struct device *dev)
> > +{
> > +       spin_lock_irq(&dev->power.lock);
> > +       dev->power.no_pm_callbacks =
> > +               (!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
> > +               (!dev->class || pm_ops_is_empty(dev->class->pm)) &&
> > +               (!dev->type || pm_ops_is_empty(dev->type->pm)) &&
> > +               (!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
> > +               (!dev->driver || pm_ops_is_empty(dev->driver->pm));
> > +       spin_unlock_irq(&dev->power.lock);
> > +}
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 16550c63d611..3cae1aa1885b 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -20,6 +20,8 @@
> >  #include <linux/suspend.h>
> >  #include <linux/export.h>
> >
> > +#include "power.h"
> > +
> >  #define GENPD_RETRY_MAX_MS     250             /* Approximate */
> >
> >  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
> > @@ -1305,6 +1307,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> >
> >         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> >
> > +       device_check_pm_callbacks(dev);
> 
> Don't do this while the genpd mutex is held as there is no need to.
> Please move it outside the lock and only do it for non-error case.
> 
> >   out:
> >         mutex_unlock(&genpd->lock);
> >
> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >
> >         genpd_free_dev_data(dev, gpd_data);
> >
> > +       device_check_pm_callbacks(dev);
> > +
> >         return 0;
> 
> I can't tell whether this is an interesting feature to use for devices
> that gets attached to the ACPI PM domain. Although, you currently
> doesn't deal with that case, and too me I think this looks a bit
> weird/unsymmetrical.

Good point.

It needs to be done in every situation where a PM domain is or can be used.

I guess we might require all PM domains to be attached to devices after
a successful probe at the latest (no PM domains should be attached/detached
after probe succeeds IOW), in which case it should be sufficient to do the
device_check_pm_callbacks() thing each time after probe successds.

Thoughts?

> >   out:
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 1710c26ba097..5af45d20b677 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -131,6 +131,8 @@ void device_pm_add(struct device *dev)
> >                         dev_name(dev->parent));
> >         list_add_tail(&dev->power.entry, &dpm_list);
> >         mutex_unlock(&dpm_list_mtx);
> > +
> > +       device_check_pm_callbacks(dev);
> >  }
> >
> >  /**
> > @@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
> >
> >         dev->power.wakeup_path = device_may_wakeup(dev);
> >
> > +       if (dev->power.no_pm_callbacks) {
> > +               ret = 1;        /* Let device go direct_complete */
> > +               goto unlock;
> > +       }
> > +
> >         if (dev->pm_domain) {
> >                 info = "preparing power domain ";
> >                 callback = dev->pm_domain->ops.prepare;
> > @@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
> >         if (callback)
> >                 ret = callback(dev);
> >
> > +unlock:
> >         device_unlock(dev);
> >
> >         if (ret < 0) {
> > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> > index 998fa6b23084..f665a7850016 100644
> > --- a/drivers/base/power/power.h
> > +++ b/drivers/base/power/power.h
> > @@ -29,6 +29,8 @@ struct wake_irq {
> >  extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
> >  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
> >
> > +extern void device_check_pm_callbacks(struct device *dev);
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >
> >  extern int device_wakeup_attach_irq(struct device *dev,
> > @@ -102,6 +104,10 @@ static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> >  {
> >  }
> >
> > +static inline void device_check_pm_callbacks(struct device *dev)
> > +{
> > +}
> > +
> >  #endif
> >
> >  #ifdef CONFIG_PM_SLEEP
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index 35d599e7250d..e334b9b8cd46 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -566,6 +566,7 @@ struct dev_pm_info {
> >         bool                    ignore_children:1;
> >         bool                    early_init:1;   /* Owned by the PM core */
> >         bool                    direct_complete:1;      /* Owned by the PM core */
> > +       bool                    no_pm_callbacks:1;      /* Owned by the PM core */
> 
> This flag is only being used while CONFIG_PM is set. I suggest we move
> within that ifdef.

I guess you mean CONFIG_PM_SLEEP?

> >         spinlock_t              lock;
> >  #ifdef CONFIG_PM_SLEEP
> >         struct list_head        entry;

Thanks,
Rafael

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Ulf Hansson]

[...]

>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 16550c63d611..3cae1aa1885b 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -20,6 +20,8 @@
>> >  #include <linux/suspend.h>
>> >  #include <linux/export.h>
>> >
>> > +#include "power.h"
>> > +
>> >  #define GENPD_RETRY_MAX_MS     250             /* Approximate */
>> >
>> >  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
>> > @@ -1305,6 +1307,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>> >
>> >         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>> >
>> > +       device_check_pm_callbacks(dev);
>>
>> Don't do this while the genpd mutex is held as there is no need to.
>> Please move it outside the lock and only do it for non-error case.
>>
>> >   out:
>> >         mutex_unlock(&genpd->lock);
>> >
>> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>> >
>> >         genpd_free_dev_data(dev, gpd_data);
>> >
>> > +       device_check_pm_callbacks(dev);
>> > +
>> >         return 0;
>>
>> I can't tell whether this is an interesting feature to use for devices
>> that gets attached to the ACPI PM domain. Although, you currently
>> doesn't deal with that case, and too me I think this looks a bit
>> weird/unsymmetrical.
>
> Good point.
>
> It needs to be done in every situation where a PM domain is or can be used.
>
> I guess we might require all PM domains to be attached to devices after
> a successful probe at the latest (no PM domains should be attached/detached
> after probe succeeds IOW), in which case it should be sufficient to do the
> device_check_pm_callbacks() thing each time after probe successds.
>
> Thoughts?

Ohh, I just realize that my comment was wrong. Sorry for the noise.

Why genpd needs special treatment is to take care off the
"non-probing" case. In other words when devices gets added to genpd
directly by using the pm_genpd_add_device() API. Perhaps that deserves
a comment though.

>
>> >   out:
>> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> > index 1710c26ba097..5af45d20b677 100644
>> > --- a/drivers/base/power/main.c
>> > +++ b/drivers/base/power/main.c
>> > @@ -131,6 +131,8 @@ void device_pm_add(struct device *dev)
>> >                         dev_name(dev->parent));
>> >         list_add_tail(&dev->power.entry, &dpm_list);
>> >         mutex_unlock(&dpm_list_mtx);
>> > +
>> > +       device_check_pm_callbacks(dev);
>> >  }
>> >
>> >  /**
>> > @@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
>> >
>> >         dev->power.wakeup_path = device_may_wakeup(dev);
>> >
>> > +       if (dev->power.no_pm_callbacks) {
>> > +               ret = 1;        /* Let device go direct_complete */
>> > +               goto unlock;
>> > +       }
>> > +
>> >         if (dev->pm_domain) {
>> >                 info = "preparing power domain ";
>> >                 callback = dev->pm_domain->ops.prepare;
>> > @@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
>> >         if (callback)
>> >                 ret = callback(dev);
>> >
>> > +unlock:
>> >         device_unlock(dev);
>> >
>> >         if (ret < 0) {
>> > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
>> > index 998fa6b23084..f665a7850016 100644
>> > --- a/drivers/base/power/power.h
>> > +++ b/drivers/base/power/power.h
>> > @@ -29,6 +29,8 @@ struct wake_irq {
>> >  extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
>> >  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
>> >
>> > +extern void device_check_pm_callbacks(struct device *dev);
>> > +
>> >  #ifdef CONFIG_PM_SLEEP
>> >
>> >  extern int device_wakeup_attach_irq(struct device *dev,
>> > @@ -102,6 +104,10 @@ static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
>> >  {
>> >  }
>> >
>> > +static inline void device_check_pm_callbacks(struct device *dev)
>> > +{
>> > +}
>> > +
>> >  #endif
>> >
>> >  #ifdef CONFIG_PM_SLEEP
>> > diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > index 35d599e7250d..e334b9b8cd46 100644
>> > --- a/include/linux/pm.h
>> > +++ b/include/linux/pm.h
>> > @@ -566,6 +566,7 @@ struct dev_pm_info {
>> >         bool                    ignore_children:1;
>> >         bool                    early_init:1;   /* Owned by the PM core */
>> >         bool                    direct_complete:1;      /* Owned by the PM core */
>> > +       bool                    no_pm_callbacks:1;      /* Owned by the PM core */
>>
>> This flag is only being used while CONFIG_PM is set. I suggest we move
>> within that ifdef.
>
> I guess you mean CONFIG_PM_SLEEP?

Yes!

Kind regards
Uffe

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Tomeu Vizoso]

On 30 September 2015 at 15:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
>> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>> >
>> >         genpd_free_dev_data(dev, gpd_data);
>> >
>> > +       device_check_pm_callbacks(dev);
>> > +
>> >         return 0;
>>
>> I can't tell whether this is an interesting feature to use for devices
>> that gets attached to the ACPI PM domain. Although, you currently
>> doesn't deal with that case, and too me I think this looks a bit
>> weird/unsymmetrical.
>
> Good point.
>
> It needs to be done in every situation where a PM domain is or can be used.
>
> I guess we might require all PM domains to be attached to devices after
> a successful probe at the latest (no PM domains should be attached/detached
> after probe succeeds IOW), in which case it should be sufficient to do the
> device_check_pm_callbacks() thing each time after probe successds.
>
> Thoughts?

Sound good to me. How were you thinking of doing that? Manually
checking that that's currently the case and adding a WARN() if a
pm_domain is attached to a device that has been probed already?

Thanks,

Tomeu

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 03:59:48 PM Tomeu Vizoso wrote:
> On 30 September 2015 at 15:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
> >> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> >> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >> >
> >> >         genpd_free_dev_data(dev, gpd_data);
> >> >
> >> > +       device_check_pm_callbacks(dev);
> >> > +
> >> >         return 0;
> >>
> >> I can't tell whether this is an interesting feature to use for devices
> >> that gets attached to the ACPI PM domain. Although, you currently
> >> doesn't deal with that case, and too me I think this looks a bit
> >> weird/unsymmetrical.
> >
> > Good point.
> >
> > It needs to be done in every situation where a PM domain is or can be used.
> >
> > I guess we might require all PM domains to be attached to devices after
> > a successful probe at the latest (no PM domains should be attached/detached
> > after probe succeeds IOW), in which case it should be sufficient to do the
> > device_check_pm_callbacks() thing each time after probe successds.
> >
> > Thoughts?
> 
> Sound good to me. How were you thinking of doing that? Manually
> checking that that's currently the case and adding a WARN() if a
> pm_domain is attached to a device that has been probed already?

I guess we need a function for setting/clearing the pm_domain pointer that
will check whether or not the device has been probed and WARN() in that case.

Then, convert all users of PM domains to use that function instead of
manipulating the pointer directly.

Thanks,
Rafael

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Ulf Hansson]

On 30 September 2015 at 23:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 30, 2015 03:59:48 PM Tomeu Vizoso wrote:
>> On 30 September 2015 at 15:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
>> >> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> >> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>> >> >
>> >> >         genpd_free_dev_data(dev, gpd_data);
>> >> >
>> >> > +       device_check_pm_callbacks(dev);
>> >> > +
>> >> >         return 0;
>> >>
>> >> I can't tell whether this is an interesting feature to use for devices
>> >> that gets attached to the ACPI PM domain. Although, you currently
>> >> doesn't deal with that case, and too me I think this looks a bit
>> >> weird/unsymmetrical.
>> >
>> > Good point.
>> >
>> > It needs to be done in every situation where a PM domain is or can be used.
>> >
>> > I guess we might require all PM domains to be attached to devices after
>> > a successful probe at the latest (no PM domains should be attached/detached
>> > after probe succeeds IOW), in which case it should be sufficient to do the
>> > device_check_pm_callbacks() thing each time after probe successds.
>> >
>> > Thoughts?
>>
>> Sound good to me. How were you thinking of doing that? Manually
>> checking that that's currently the case and adding a WARN() if a
>> pm_domain is attached to a device that has been probed already?
>
> I guess we need a function for setting/clearing the pm_domain pointer that
> will check whether or not the device has been probed and WARN() in that case.
>
> Then, convert all users of PM domains to use that function instead of
> manipulating the pointer directly.

Why would ever a device be fully probed without first having its PM
domain pointer assigned?

To me that wouldn't even work, as the device would rather depend on
that the PM domain is power up to succeed being probed (at least in
general case), and unless the PM domain pointer is set we can't
control that.

Kind regards
Uffe

Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Rafael J. Wysocki]

On Thursday, October 01, 2015 11:04:17 AM Ulf Hansson wrote:
> On 30 September 2015 at 23:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, September 30, 2015 03:59:48 PM Tomeu Vizoso wrote:
> >> On 30 September 2015 at 15:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
> >> >> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> >> >> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >> >> >
> >> >> >         genpd_free_dev_data(dev, gpd_data);
> >> >> >
> >> >> > +       device_check_pm_callbacks(dev);
> >> >> > +
> >> >> >         return 0;
> >> >>
> >> >> I can't tell whether this is an interesting feature to use for devices
> >> >> that gets attached to the ACPI PM domain. Although, you currently
> >> >> doesn't deal with that case, and too me I think this looks a bit
> >> >> weird/unsymmetrical.
> >> >
> >> > Good point.
> >> >
> >> > It needs to be done in every situation where a PM domain is or can be used.
> >> >
> >> > I guess we might require all PM domains to be attached to devices after
> >> > a successful probe at the latest (no PM domains should be attached/detached
> >> > after probe succeeds IOW), in which case it should be sufficient to do the
> >> > device_check_pm_callbacks() thing each time after probe successds.
> >> >
> >> > Thoughts?
> >>
> >> Sound good to me. How were you thinking of doing that? Manually
> >> checking that that's currently the case and adding a WARN() if a
> >> pm_domain is attached to a device that has been probed already?
> >
> > I guess we need a function for setting/clearing the pm_domain pointer that
> > will check whether or not the device has been probed and WARN() in that case.
> >
> > Then, convert all users of PM domains to use that function instead of
> > manipulating the pointer directly.
> 
> Why would ever a device be fully probed without first having its PM
> domain pointer assigned?
> 
> To me that wouldn't even work, as the device would rather depend on
> that the PM domain is power up to succeed being probed (at least in
> general case), and unless the PM domain pointer is set we can't
> control that.

That generally is the case, but then I can imagine a situation in which setting
the pm_domain pointer after probe may work for somebody and my point is that
we should enforce things that we are going to rely on for correctness rather
then hope that everybody will do them just because they make sense.

Thanks,
Rafael