PM: Generic PM domains and device PM QoS

From: Rafael J. Wysocki <[email protected]>

The rpm_suspend() and rpm_resume() routines execute subsystem or PM domain callbacks under power.lock if power.irq_safe is set for the given device.  This is inconsistent with that rpm_idle() does after commit 02b2677 (PM / Runtime: Allow _put_sync() from interrupts-disabled context) and is problematic for subsystems and PM domains wanting to use power.lock for synchronization in their runtime PM callbacks.  For this reason, make runtime PM core functions always release power.lock before invoking subsystem or PM domain

callbacks.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/runtime.c |   50 ++++++++++++++++++++++++-------------------

 1 file changed, 28 insertions(+), 22 deletions(-)

Index: linux/drivers/base/power/runtime.c =================================> --- linux.orig/drivers/base/power/runtime.c +++ linux/drivers/base/power/runtime.c

(quoted diff) +25 -0

@@ -155,6 +155,31 @@ static int rpm_check_suspend_allowed(str
  }
 
  /**
+ * __rpm_callback - Run a given runtime PM callback for a given device.
+ * @cb: Runtime PM callback to run.
+ * @dev: Device to run the callback for.
+ */
+static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)
+{
+       int retval;
+
+       if (dev->power.irq_safe)
+               spin_unlock(&dev->power.lock);
+       else
+               spin_unlock_irq(&dev->power.lock);
+
+       retval = cb(dev);
+
+       if (dev->power.irq_safe)
+               spin_lock(&dev->power.lock);
+       else
+               spin_lock_irq(&dev->power.lock);
+
+       return retval;
+}
+
+/**
  * rpm_idle - Notify device bus type if the device can be suspended.
  * @dev: Device to notify the bus type about.
  * @rpmflags: Flag bits.

@@ -225,19 +250,8 @@ static int rpm_idle(struct device *dev,

       else
               callback = NULL;

-       if (callback) {
-               if (dev->power.irq_safe)
-                       spin_unlock(&dev->power.lock);
-               else
-                       spin_unlock_irq(&dev->power.lock);

(quoted diff) +2 -8

-
-               callback(dev);
-
-               if (dev->power.irq_safe)
-                       spin_lock(&dev->power.lock);
-               else
-                       spin_lock_irq(&dev->power.lock);
-       }
+       if (callback)
+               __rpm_callback(callback, dev);

       dev->power.idle_notification = false;
       wake_up_all(&dev->power.wait_queue);

(quoted diff) +1 -10

@@ -252,22 +266,14 @@ static int rpm_idle(struct device *dev,
  * @dev: Device to run the callback for.
  */
  static int rpm_callback(int (*cb)(struct device *), struct device *dev)
-       __releases(&dev->power.lock) __acquires(&dev->power.lock)
  {
        int retval;
 
        if (!cb)
                return -ENOSYS;
 
-       if (dev->power.irq_safe) {
-               retval = cb(dev);
-       } else {
-               spin_unlock_irq(&dev->power.lock);
-
-               retval = cb(dev);
+       retval = __rpm_callback(cb, dev);
 
-               spin_lock_irq(&dev->power.lock);
-       }
        dev->power.runtime_error = retval;
        return retval != -EACCES ? retval : -EIO;
  }
 
--

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/

Hi,

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The rpm_suspend() and rpm_resume() routines execute subsystem or PM domain callbacks under power.lock if power.irq_safe is set for the given device. This is inconsistent with that rpm_idle() does after commit 02b2677 (PM / Runtime: Allow _put_sync() from interrupts-disabled context) and is problematic for subsystems and PM domains wanting to use power.lock for synchronization in their runtime PM callbacks. For this reason, make runtime PM core functions always release power.lock before invoking subsystem or PM domain

If power.lock is released, the transition states(resuming or suspending) may be observed in rpm_suspend or rpm_resume, then tasks schedule will be produced in these two functions,

so the functions below can't be

pm_runtime_suspend()
pm_runtime_autosuspend()
pm_runtime_resume()
pm_runtime_put_sync_suspend()
pm_runtime_put_sync_autosuspend()

called in irq-off contexts safely even though irq_safe flag is set.

Hi,

Quoted 27 lines · Ming Lei

Hi,

Quoted 19 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The rpm_suspend() and rpm_resume() routines execute subsystem or PM

domain callbacks under power.lock if power.irq_safe is set for the

given device.  This is inconsistent with that rpm_idle() does after

commit 02b2677 (PM / Runtime: Allow _put_sync() from

interrupts-disabled context) and is problematic for subsystems and PM

domains wanting to use power.lock for synchronization in their

runtime PM callbacks.  For this reason, make runtime PM core functions

always release power.lock before invoking subsystem or PM domain

Quoted 5 lines

If power.lock is released, the transition states(resuming or suspending)

may be observed in rpm_suspend or rpm_resume, then tasks schedule

will be produced in these two functions,

I don't think so, because the interrupts are still off.

Quoted 16 lines

so the functions below can't be

pm_runtime_suspend()

pm_runtime_autosuspend()

pm_runtime_resume()

pm_runtime_put_sync_suspend()

pm_runtime_put_sync_autosuspend()

Quoted 1 line

called in irq-off contexts safely even though irq_safe flag is set.

The patch doesn't cause rpm_suspend() and rpm_resume() to turn interrupts

on if irq_safe is set, it only causes them to release the lock (temporarily).

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 31 lines · Ming Lei

Hi,

Quoted 12 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The rpm_suspend() and rpm_resume() routines execute subsystem or PM

domain callbacks under power.lock if power.irq_safe is set for the

given device. This is inconsistent with that rpm_idle() does after

commit 02b2677 (PM / Runtime: Allow _put_sync() from

Quoted 7 lines

interrupts-disabled context) and is problematic for subsystems and PM

domains wanting to use power.lock for synchronization in their

runtime PM callbacks. For this reason, make runtime PM core functions

always release power.lock before invoking subsystem or PM domain

If power.lock is released, the transition states(resuming or suspending)

may be observed in rpm_suspend or rpm_resume, then tasks schedule

Quoted 3 lines

Quoted 1 line

will be produced in these two functions,

I don't think so, because the interrupts are still off.

Yes, the interrupts are still off on local CPU, but the release of spin lock may cause another CPUs to run into rpm_suspend or rpm_resume and produce task schedule inside the two functions.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

If power.lock is released, the transition states(resuming or suspending)

may be observed in rpm_suspend or rpm_resume, then tasks schedule

will be produced in these two functions,

Quoted 5 lines

Quoted 1 line

I don't think so, because the interrupts are still off.

Yes, the interrupts are still off on local CPU, but the release of spin lock may cause another CPUs to run into rpm_suspend or rpm_resume and produce task schedule inside the two functions.

Not for the same device, though.

Also, I'm not quite sure what scenario exactly are you referring to. Could you please give an example?

Hi,

Quoted 6 lines · Rafael J. Wysocki

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

If power.lock is released, the transition states(resuming or suspending)

may be observed in rpm_suspend or rpm_resume, then tasks schedule

will be produced in these two functions,

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

I don't think so, because the interrupts are still off.

Yes, the interrupts are still off on local CPU, but the release of spin lock may cause another CPUs to run into rpm_suspend or rpm_resume and produce task schedule inside the two functions.

Not for the same device, though.

I think it is probable to happen on the same device in theory, see below:

- suppose irq_safe is set before calling two pm_runtime_suspend below
- suppose this patch has been applied

CPU0 CPU1
pm_runtime_suspend
acquired power lock
rpm_suspend
pm_runtime_suspend
spining power lock
...
release power lock
acquired power lock
run .runtime_suspend
found the dev suspending wait for power state and schedule

Quoted 23 lines · Ming Lei

Hi,

Quoted 6 lines · Rafael J. Wysocki

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

If power.lock is released, the transition states(resuming or suspending)

may be observed in rpm_suspend or rpm_resume, then tasks schedule

will be produced in these two functions,

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

I don't think so, because the interrupts are still off.

Yes, the interrupts are still off on local CPU, but the release of spin lock may cause another CPUs to run into rpm_suspend or rpm_resume and produce task schedule inside the two functions.

Not for the same device, though.

I think it is probable to happen on the same device in theory, see below:

- suppose irq_safe is set before calling two pm_runtime_suspend below
- suppose this patch has been applied

Quoted 12 lines

CPU0                                          CPU1
pm_runtime_suspend
      acquired power lock
      rpm_suspend
                                              pm_runtime_suspend
                                                      spining power lock
      ...
      release power lock
                                                      acquired power lock run .runtime_suspend
                                                      found the dev suspending wait for power state and schedule

OK, I see what the problem is.  The second CPU can see the status being RPM_SUSPENDING in the irq_safe case, which isn't possible without the patch.

Good catch!

I think in that case rpm_suspend() should just release the lock, run cpu_relax(), reacquire the lock and go to the "repeat" label.

From: Rafael J. Wysocki <[email protected]>

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that.  For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed.  However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  167 ++++++++++++++++++++++++++-------------------- include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3

 4 files changed, 101 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c =================================> --- linux.orig/drivers/base/power/qos.c +++ linux/drivers/base/power/qos.c

(quoted diff) +0 -0

@@ -30,15 +30,6 @@

(quoted diff) +2 -10

  */
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value)

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

(quoted diff) +10 -0

+               } else {
+                       device_pm_unlock();
+                       /*
+                        * Allocate the constraints data on the first call to
+                        * add_request, i.e. only if the data is not already
+                        * allocated and if the device has not been removed.
+                        */
+                       ret = dev_pm_qos_constraints_allocate(dev);
+               }
+       }
 

Hi Rafael,

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that. For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Ok.

Quoted 8 lines

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed. However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Ok that makes sense.
The constraints_state field can be replaced by a combination of dev->power.constraints and list_empty(&dev->power.entry), which makes the code more compact and less redundant.

Quoted 14 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  167 ++++++++++++++++++++++++++--------------------
 include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3
 4 files changed, 101 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c
===================================================================

drivers/base/power/qos.c +0 -0

@@ -30,15 +30,6 @@

...

Quoted 1 line

drivers/base/power/qos.c void dev_pm_qos_constraints_destroy(stru · line 206

Quoted 8 lines

drivers/base/power/qos.c +2 -2

@@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
 *
 * Returns 1 if the aggregated constraint value has changed,
 * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures.

Why not use -ENODEV in case there is no device?

Quoted 18 lines

drivers/base/power/qos.c +2 -6

 */
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                          s32 value)

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
               return -EINVAL;
       }
 
-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 

Quoted 4 lines

drivers/base/power/qos.c +0 -4

-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

drivers/base/power/qos.c int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

drivers/base/power/qos.c +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Hi,

Quoted 41 lines · Jean Pihet

Hi Rafael,

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that.  For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Ok.

Quoted 8 lines

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed.  However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Ok that makes sense.
The constraints_state field can be replaced by a combination of dev->power.constraints and list_empty(&dev->power.entry), which makes the code more compact and less redundant.

Quoted 13 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  167 ++++++++++++++++++++++++++-------------------- include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3

 4 files changed, 101 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c =================================>> > --- linux.orig/drivers/base/power/qos.c +++ linux/drivers/base/power/qos.c

(quoted diff) +0 -0

@@ -30,15 +30,6 @@

...

Quoted 1 line

Quoted 9 lines

Review void dev_pm_qos_constraints_destroy(stru · line 206

Quoted 8 lines

(quoted diff) +2 -2

@@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
  *
  * Returns 1 if the aggregated constraint value has changed,
  * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures.

Why not use -ENODEV in case there is no device?

I don't think it's useful for the caller.  If the device is gone, the constraing simply doesn't matter, so there's no error to handle.

Quoted 18 lines

Quoted 18 lines

(quoted diff) +2 -6

  */
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value)

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 

Quoted 16 lines

Quoted 4 lines

(quoted diff) +0 -4

-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

Review int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

(quoted diff) +7 -3

-        */
-       if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Pretty much it is.  Is that a problem?

Rafael

Quoted 93 lines · Rafael J. Wysocki

Hi,

Quoted 42 lines · Jean Pihet

Hi Rafael,

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that. For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Ok.

Quoted 8 lines

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed. However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Ok that makes sense.
The constraints_state field can be replaced by a combination of dev->power.constraints and list_empty(&dev->power.entry), which makes the code more compact and less redundant.

Quoted 14 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  167 ++++++++++++++++++++++++++--------------------
 include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3
 4 files changed, 101 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c
===================================================================

drivers/base/power/qos.c +0 -0

@@ -30,15 +30,6 @@

...

Quoted 1 line

Quoted 9 lines

drivers/base/power/qos.c void dev_pm_qos_constraints_destroy(stru · line 206

Quoted 8 lines

drivers/base/power/qos.c +2 -2

@@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
 *
 * Returns 1 if the aggregated constraint value has changed,
 * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures.

Why not use -ENODEV in case there is no device?

I don't think it's useful for the caller. If the device is gone, the constraing simply doesn't matter, so there's no error to handle.

Quoted 34 lines

Quoted 22 lines

drivers/base/power/qos.c +2 -10

 */
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                          s32 value)

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
               return -EINVAL;
       }
 
-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

drivers/base/power/qos.c int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

drivers/base/power/qos.c +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Quoted 1 line

Pretty much it is. Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

Quoted 29 lines

Quoted 29 lines

Quoted 28 lines

Quoted 16 lines

(quoted diff) +2 -10

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

Review

Quoted 10 lines

(quoted diff) +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Quoted 3 lines

Quoted 1 line

Pretty much it is. Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

However, the removal of the device is a special case. What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it? Not much I guess.

Hi,

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

...

Quoted 33 lines

Quoted 32 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;

-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);

-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

Quoted 4 lines

Quoted 2 lines

0 is silently returned in case the device has been removed. Is that the intention?

Pretty much it is.  Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

Quoted 3 lines

However, the removal of the device is a special case.  What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it?  Not much I guess.

Anyway, since returning an error code in that case won't hurt either, below is a v2 of the patch doing that and resetting the dev field in the req structure.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / QoS: Add function dev_pm_qos_read_value() (v2)

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that.  For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed.  However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  170 ++++++++++++++++++++++++++-------------------- include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3

 4 files changed, 104 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c =================================> --- linux.orig/drivers/base/power/qos.c +++ linux/drivers/base/power/qos.c

drivers/base/power/qos.c +86 -52

@@ -30,15 +30,6 @@
  * . To minimize the data usage by the per-device constraints, the data struct
  *   is only allocated at the first call to dev_pm_qos_add_request.
  * . The data is later free'd when the device is removed from the system.
- * . The constraints_state variable from dev_pm_info tracks the data struct
- *    allocation state:
- *    DEV_PM_QOS_NO_DEVICE: No device present or device removed, no data
- *     allocated,
- *    DEV_PM_QOS_DEVICE_PRESENT: Device present, data not allocated and will be
- *     allocated at the first call to dev_pm_qos_add_request,
- *    DEV_PM_QOS_ALLOCATED: Device present, data allocated. The per-device
- *     PM QoS constraints framework is operational and constraints can be
- *     added, updated or removed using the dev_pm_qos_* API.
  *  . A global mutex protects the constraints users from the data being
  *     allocated and free'd.
  */

@@ -51,8 +42,30 @@
 
 
  static DEFINE_MUTEX(dev_pm_qos_mtx);
+
  static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
 
+/**
+ * dev_pm_qos_read_value - Get PM QoS constraint for a given device.
+ * @dev: Device to get the PM QoS constraint value for.
+ */
+s32 dev_pm_qos_read_value(struct device *dev)
+{
+       struct pm_qos_constraints *c;
+       unsigned long flags;
+       s32 ret = 0;
+
+       spin_lock_irqsave(&dev->power.lock, flags);
+
+       c = dev->power.constraints;
+       if (c)
+               ret = pm_qos_read_value(c);
+
+       spin_unlock_irqrestore(&dev->power.lock, flags);
+
+       return ret;
+}
+
  /*
  * apply_constraint
  * @req: constraint request to apply

@@ -105,27 +118,37 @@ static int dev_pm_qos_constraints_alloca
        }
        BLOCKING_INIT_NOTIFIER_HEAD(n);
 
+       plist_head_init(&c->list);
+       c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+       c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+       c->type = PM_QOS_MIN;
+       c->notifiers = n;
+
+       spin_lock_irq(&dev->power.lock);
        dev->power.constraints = c;
-       plist_head_init(&dev->power.constraints->list);
-       dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
-       dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
-       dev->power.constraints->type = PM_QOS_MIN;
-       dev->power.constraints->notifiers = n;
-       dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
+       spin_unlock_irq(&dev->power.lock);
 
        return 0;
  }
 
+static void __dev_pm_qos_constraints_init(struct device *dev)
+{
+       spin_lock_irq(&dev->power.lock);
+       dev->power.constraints = NULL;
+       spin_unlock_irq(&dev->power.lock);
+}
+
  /**
- * dev_pm_qos_constraints_init
+ * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
  * @dev: target device
  *
- * Called from the device PM subsystem at device insertion
+ * Called from the device PM subsystem at device insertion under
+ * device_pm_lock().
  */
  void dev_pm_qos_constraints_init(struct device *dev)
  {
        mutex_lock(&dev_pm_qos_mtx);
-       dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
+       dev->power.constraints = NULL;
        mutex_unlock(&dev_pm_qos_mtx);
  }
 

@@ -133,34 +156,35 @@ void dev_pm_qos_constraints_init(struct
  * dev_pm_qos_constraints_destroy
  * @dev: target device
  *
- * Called from the device PM subsystem at device removal
+ * Called from the device PM subsystem at device removal under device_pm_lock().
  */
  void dev_pm_qos_constraints_destroy(struct device *dev)
  {
        struct dev_pm_qos_request *req, *tmp;
+       struct pm_qos_constraints *c;
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       if (dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
-               /* Flush the constraints list for the device */
-               plist_for_each_entry_safe(req, tmp,
-                                         &dev->power.constraints->list,
-                                         node) {
-                       /*
-                        * Update constraints list and call the notification
-                        * callbacks if needed
-                        */
-                       apply_constraint(req, PM_QOS_REMOVE_REQ,
-                                        PM_QOS_DEFAULT_VALUE);
-                       memset(req, 0, sizeof(*req));
-               }
+       c = dev->power.constraints;
+       if (!c)
+               goto out;
 
-               kfree(dev->power.constraints->notifiers);
-               kfree(dev->power.constraints);
-               dev->power.constraints = NULL;
+       /* Flush the constraints list for the device */
+       plist_for_each_entry_safe(req, tmp, &c->list, node) {
+               /*
+                * Update constraints list and call the notification
+                * callbacks if needed
+                */
+               apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+               memset(req, 0, sizeof(*req));
        }
-       dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
 
+       __dev_pm_qos_constraints_init(dev);
+
+       kfree(c->notifiers);
+       kfree(c);
+
+ out:
        mutex_unlock(&dev_pm_qos_mtx);
  }
 

@@ -178,8 +202,9 @@ void dev_pm_qos_constraints_destroy(stru
  *
  * Returns 1 if the aggregated constraint value has changed,
  * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures, -ENODEV if the device has just been removed
+ * from the system.
  */
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value)

@@ -195,28 +220,37 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       req->dev = NULL;
+                       ret = -ENODEV;
+                       goto out;
+               } else {
+                       device_pm_unlock();
+                       /*
+                        * Allocate the constraints data on the first call to
+                        * add_request, i.e. only if the data is not already
+                        * allocated and if the device has not been removed.
+                        */
+                       ret = dev_pm_qos_constraints_allocate(dev);
+               }
+       }
 
        if (!ret)
                ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
 
-out:
+ out:
        mutex_unlock(&dev_pm_qos_mtx);
+
        return ret;
  }
  EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);

@@ -252,13 +286,13 @@ int dev_pm_qos_update_request(struct dev
 
        mutex_lock(&dev_pm_qos_mtx);

drivers/base/power/qos.c +11 -17

        }
 
        mutex_unlock(&dev_pm_qos_mtx);

@@ -293,7 +327,7 @@ int dev_pm_qos_remove_request(struct dev
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
+       if (req->dev->power.constraints) {
                ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
                                       PM_QOS_DEFAULT_VALUE);
                memset(req, 0, sizeof(*req));

@@ -323,15 +357,12 @@ int dev_pm_qos_add_notifier(struct devic
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       /* Silently return if the device has been removed */
-       if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
-               goto out;
-
-       retval = blocking_notifier_chain_register(
-                       dev->power.constraints->notifiers,
-                       notifier);
+       /* Silently return if the constraints object is not present. */
+       if (dev->power.constraints)
+               retval = blocking_notifier_chain_register(
+                               dev->power.constraints->notifiers,
+                               notifier);
 
-out:
        mutex_unlock(&dev_pm_qos_mtx);
        return retval;
  }

@@ -354,15 +385,12 @@ int dev_pm_qos_remove_notifier(struct de
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       /* Silently return if the device has been removed */
-       if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
-               goto out;
-
-       retval = blocking_notifier_chain_unregister(
-                       dev->power.constraints->notifiers,
-                       notifier);
+       /* Silently return if the constraints object is not present. */
+       if (dev->power.constraints)
+               retval = blocking_notifier_chain_unregister(
+                               dev->power.constraints->notifiers,
+                               notifier);
 
-out:
        mutex_unlock(&dev_pm_qos_mtx);
        return retval;
  }

Index: linux/include/linux/pm_qos.h =================================> --- linux.orig/include/linux/pm_qos.h +++ linux/include/linux/pm_qos.h

include/linux/pm_qos.h +3 -0

@@ -77,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_cl
  int pm_qos_request_active(struct pm_qos_request *req);
  s32 pm_qos_read_value(struct pm_qos_constraints *c);
 
+s32 dev_pm_qos_read_value(struct device *dev);
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value);
  int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);

@@ -117,6 +118,8 @@ static inline int pm_qos_request_active(
  static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
                        { return 0; }
 
+static inline s32 dev_pm_qos_read_value(struct device *dev)
+                       { return 0; }
  static inline int dev_pm_qos_add_request(struct device *dev,
                                         struct dev_pm_qos_request *req,
                                         s32 value)

Index: linux/drivers/base/power/main.c =================================> --- linux.orig/drivers/base/power/main.c +++ linux/drivers/base/power/main.c

drivers/base/power/main.c +2 -2

@@ -98,8 +98,8 @@ void device_pm_add(struct device *dev)
                dev_warn(dev, "parent %s should not be sleeping\n",
                        dev_name(dev->parent));
        list_add_tail(&dev->power.entry, &dpm_list);
-       mutex_unlock(&dpm_list_mtx);
        dev_pm_qos_constraints_init(dev);
+       mutex_unlock(&dpm_list_mtx);
  }
 
  /**

@@ -110,9 +110,9 @@ void device_pm_remove(struct device *dev
  {
        pr_debug("PM: Removing info for %s:%s\n",
                 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
-       dev_pm_qos_constraints_destroy(dev);
        complete_all(&dev->power.completion);
        mutex_lock(&dpm_list_mtx);
+       dev_pm_qos_constraints_destroy(dev);
        list_del_init(&dev->power.entry);
        mutex_unlock(&dpm_list_mtx);
        device_wakeup_disable(dev);

Index: linux/include/linux/pm.h =================================> --- linux.orig/include/linux/pm.h +++ linux/include/linux/pm.h

include/linux/pm.h +0 -7

@@ -421,13 +421,6 @@ enum rpm_request {
        RPM_REQ_RESUME,
  };
 
-/* Per-device PM QoS constraints data struct state */
-enum dev_pm_qos_state {
-       DEV_PM_QOS_NO_DEVICE,           /* No device present */
-       DEV_PM_QOS_DEVICE_PRESENT,      /* Device present, data not allocated */
-       DEV_PM_QOS_ALLOCATED,           /* Device present, data allocated */
-};
-
  struct wakeup_source;
 
  struct pm_domain_data {

@@ -489,7 +482,6 @@ struct dev_pm_info {
  #endif

       struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */ struct pm_qos_constraints *constraints;
-       enum dev_pm_qos_state   constraints_state;
 };

 extern void update_pm_runtime_accounting(struct device *dev);

Hi,

Quoted 39 lines · Rafael J. Wysocki

Hi,

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

...

Quoted 33 lines

Quoted 32 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

(quoted diff) +9 -13

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

Quoted 4 lines

Quoted 2 lines

0 is silently returned in case the device has been removed. Is that the intention?

Pretty much it is. Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

Quoted 7 lines

Quoted 3 lines

However, the removal of the device is a special case. What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it? Not much I guess.

Anyway, since returning an error code in that case won't hurt either, below is a v2 of the patch doing that and resetting the dev field in the req structure.

Ok thanks for the update. The comments are inlined below.

Review

Quoted 10 lines

(quoted diff) +2 -2

-       if (req->dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
+       if (req->dev->power.constraints) {
               if (new_value != req->node.prio)
                       ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
                                              new_value);
       } else {
               /* Return if the device has been removed */
-               ret = -ENODEV;
+               ret = -EINVAL;

The retcode should be -ENODEV.

Is the kerneldoc still OK?

Hi,

Quoted 39 lines · Rafael J. Wysocki

Hi,

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

...

Quoted 33 lines

Quoted 32 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

(quoted diff) +9 -13

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

Quoted 4 lines

Quoted 2 lines

0 is silently returned in case the device has been removed. Is that the intention?

Pretty much it is. Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

Quoted 7 lines

Quoted 3 lines

However, the removal of the device is a special case. What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it? Not much I guess.

Anyway, since returning an error code in that case won't hurt either, below is a v2 of the patch doing that and resetting the dev field in the req structure.

Ok thanks for the update. The comments are inlined below.

Review

Quoted 10 lines

(quoted diff) +2 -2

-       if (req->dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
+       if (req->dev->power.constraints) {
               if (new_value != req->node.prio)
                       ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
                                              new_value);
       } else {
               /* Return if the device has been removed */
-               ret = -ENODEV;
+               ret = -EINVAL;

The retcode should be -ENODEV.

Is the kerneldoc still OK?

Hi,

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

...

Quoted 33 lines

Quoted 32 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;

-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);

-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

Quoted 4 lines

Quoted 2 lines

0 is silently returned in case the device has been removed. Is that the intention?

Pretty much it is.  Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

Quoted 3 lines

However, the removal of the device is a special case.  What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it?  Not much I guess.

Anyway, since returning an error code in that case won't hurt either, below is a v2 of the patch doing that and resetting the dev field in the req structure.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / QoS: Add function dev_pm_qos_read_value() (v2)

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that.  For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed.  However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  170 ++++++++++++++++++++++++++-------------------- include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3

 4 files changed, 104 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c
===================================================================

drivers/base/power/qos.c +86 -52

@@ -30,15 +30,6 @@
  * . To minimize the data usage by the per-device constraints, the data struct
  *   is only allocated at the first call to dev_pm_qos_add_request.
  * . The data is later free'd when the device is removed from the system.
- * . The constraints_state variable from dev_pm_info tracks the data struct
- *    allocation state:
- *    DEV_PM_QOS_NO_DEVICE: No device present or device removed, no data
- *     allocated,
- *    DEV_PM_QOS_DEVICE_PRESENT: Device present, data not allocated and will be
- *     allocated at the first call to dev_pm_qos_add_request,
- *    DEV_PM_QOS_ALLOCATED: Device present, data allocated. The per-device
- *     PM QoS constraints framework is operational and constraints can be
- *     added, updated or removed using the dev_pm_qos_* API.
  *  . A global mutex protects the constraints users from the data being
  *     allocated and free'd.
  */

@@ -51,8 +42,30 @@
 
 
  static DEFINE_MUTEX(dev_pm_qos_mtx);
+
  static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
 
+/**
+ * dev_pm_qos_read_value - Get PM QoS constraint for a given device.
+ * @dev: Device to get the PM QoS constraint value for.
+ */
+s32 dev_pm_qos_read_value(struct device *dev)
+{
+       struct pm_qos_constraints *c;
+       unsigned long flags;
+       s32 ret = 0;
+
+       spin_lock_irqsave(&dev->power.lock, flags);
+
+       c = dev->power.constraints;
+       if (c)
+               ret = pm_qos_read_value(c);
+
+       spin_unlock_irqrestore(&dev->power.lock, flags);
+
+       return ret;
+}
+
  /*
  * apply_constraint
  * @req: constraint request to apply

@@ -105,27 +118,37 @@ static int dev_pm_qos_constraints_alloca
        }
        BLOCKING_INIT_NOTIFIER_HEAD(n);
 
+       plist_head_init(&c->list);
+       c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+       c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+       c->type = PM_QOS_MIN;
+       c->notifiers = n;
+
+       spin_lock_irq(&dev->power.lock);
        dev->power.constraints = c;
-       plist_head_init(&dev->power.constraints->list);
-       dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
-       dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
-       dev->power.constraints->type = PM_QOS_MIN;
-       dev->power.constraints->notifiers = n;
-       dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
+       spin_unlock_irq(&dev->power.lock);
 
        return 0;
  }
 
+static void __dev_pm_qos_constraints_init(struct device *dev)
+{
+       spin_lock_irq(&dev->power.lock);
+       dev->power.constraints = NULL;
+       spin_unlock_irq(&dev->power.lock);
+}
+
  /**
- * dev_pm_qos_constraints_init
+ * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
  * @dev: target device
  *
- * Called from the device PM subsystem at device insertion
+ * Called from the device PM subsystem at device insertion under
+ * device_pm_lock().
  */
  void dev_pm_qos_constraints_init(struct device *dev)
  {
        mutex_lock(&dev_pm_qos_mtx);
-       dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
+       dev->power.constraints = NULL;
        mutex_unlock(&dev_pm_qos_mtx);
  }
 

@@ -133,34 +156,35 @@ void dev_pm_qos_constraints_init(struct
  * dev_pm_qos_constraints_destroy
  * @dev: target device
  *
- * Called from the device PM subsystem at device removal
+ * Called from the device PM subsystem at device removal under device_pm_lock().
  */
  void dev_pm_qos_constraints_destroy(struct device *dev)
  {
        struct dev_pm_qos_request *req, *tmp;
+       struct pm_qos_constraints *c;
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       if (dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
-               /* Flush the constraints list for the device */
-               plist_for_each_entry_safe(req, tmp,
-                                         &dev->power.constraints->list,
-                                         node) {
-                       /*
-                        * Update constraints list and call the notification
-                        * callbacks if needed
-                        */
-                       apply_constraint(req, PM_QOS_REMOVE_REQ,
-                                        PM_QOS_DEFAULT_VALUE);
-                       memset(req, 0, sizeof(*req));
-               }
+       c = dev->power.constraints;
+       if (!c)
+               goto out;
 
-               kfree(dev->power.constraints->notifiers);
-               kfree(dev->power.constraints);
-               dev->power.constraints = NULL;
+       /* Flush the constraints list for the device */
+       plist_for_each_entry_safe(req, tmp, &c->list, node) {
+               /*
+                * Update constraints list and call the notification
+                * callbacks if needed
+                */
+               apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+               memset(req, 0, sizeof(*req));
        }
-       dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
 
+       __dev_pm_qos_constraints_init(dev);
+
+       kfree(c->notifiers);
+       kfree(c);
+
+ out:
        mutex_unlock(&dev_pm_qos_mtx);
  }
 

@@ -178,8 +202,9 @@ void dev_pm_qos_constraints_destroy(stru
  *
  * Returns 1 if the aggregated constraint value has changed,
  * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures, -ENODEV if the device has just been removed
+ * from the system.
  */
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value)

@@ -195,28 +220,37 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed
-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       req->dev = NULL;
+                       ret = -ENODEV;
+                       goto out;
+               } else {
+                       device_pm_unlock();
+                       /*
+                        * Allocate the constraints data on the first call to
+                        * add_request, i.e. only if the data is not already
+                        * allocated and if the device has not been removed.
+                        */
+                       ret = dev_pm_qos_constraints_allocate(dev);
+               }
+       }
 
        if (!ret)
                ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
 
-out:
+ out:
        mutex_unlock(&dev_pm_qos_mtx);
+
        return ret;
  }
  EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);

@@ -252,13 +286,13 @@ int dev_pm_qos_update_request(struct dev
 
        mutex_lock(&dev_pm_qos_mtx);

drivers/base/power/qos.c +11 -17

        }
 
        mutex_unlock(&dev_pm_qos_mtx);

@@ -293,7 +327,7 @@ int dev_pm_qos_remove_request(struct dev
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       if (req->dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
+       if (req->dev->power.constraints) {
                ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
                                       PM_QOS_DEFAULT_VALUE);
                memset(req, 0, sizeof(*req));

@@ -323,15 +357,12 @@ int dev_pm_qos_add_notifier(struct devic
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       /* Silently return if the device has been removed */
-       if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
-               goto out;
-
-       retval = blocking_notifier_chain_register(
-                       dev->power.constraints->notifiers,
-                       notifier);
+       /* Silently return if the constraints object is not present. */
+       if (dev->power.constraints)
+               retval = blocking_notifier_chain_register(
+                               dev->power.constraints->notifiers,
+                               notifier);
 
-out:
        mutex_unlock(&dev_pm_qos_mtx);
        return retval;
  }

@@ -354,15 +385,12 @@ int dev_pm_qos_remove_notifier(struct de
 
        mutex_lock(&dev_pm_qos_mtx);
 
-       /* Silently return if the device has been removed */
-       if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
-               goto out;
-
-       retval = blocking_notifier_chain_unregister(
-                       dev->power.constraints->notifiers,
-                       notifier);
+       /* Silently return if the constraints object is not present. */
+       if (dev->power.constraints)
+               retval = blocking_notifier_chain_unregister(
+                               dev->power.constraints->notifiers,
+                               notifier);
 
-out:
        mutex_unlock(&dev_pm_qos_mtx);
        return retval;
  }

Index: linux/include/linux/pm_qos.h
===================================================================

include/linux/pm_qos.h +3 -0

@@ -77,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_cl
  int pm_qos_request_active(struct pm_qos_request *req);
  s32 pm_qos_read_value(struct pm_qos_constraints *c);
 
+s32 dev_pm_qos_read_value(struct device *dev);
  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                           s32 value);
  int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);

@@ -117,6 +118,8 @@ static inline int pm_qos_request_active(
  static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
                        { return 0; }
 
+static inline s32 dev_pm_qos_read_value(struct device *dev)
+                       { return 0; }
  static inline int dev_pm_qos_add_request(struct device *dev,
                                         struct dev_pm_qos_request *req,
                                         s32 value)

Index: linux/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +2 -2

@@ -98,8 +98,8 @@ void device_pm_add(struct device *dev)
                dev_warn(dev, "parent %s should not be sleeping\n",
                        dev_name(dev->parent));
        list_add_tail(&dev->power.entry, &dpm_list);
-       mutex_unlock(&dpm_list_mtx);
        dev_pm_qos_constraints_init(dev);
+       mutex_unlock(&dpm_list_mtx);
  }
 
  /**

@@ -110,9 +110,9 @@ void device_pm_remove(struct device *dev
  {
        pr_debug("PM: Removing info for %s:%s\n",
                 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
-       dev_pm_qos_constraints_destroy(dev);
        complete_all(&dev->power.completion);
        mutex_lock(&dpm_list_mtx);
+       dev_pm_qos_constraints_destroy(dev);
        list_del_init(&dev->power.entry);
        mutex_unlock(&dpm_list_mtx);
        device_wakeup_disable(dev);

Index: linux/include/linux/pm.h
===================================================================

include/linux/pm.h +0 -7

@@ -421,13 +421,6 @@ enum rpm_request {
        RPM_REQ_RESUME,
  };
 
-/* Per-device PM QoS constraints data struct state */
-enum dev_pm_qos_state {
-       DEV_PM_QOS_NO_DEVICE,           /* No device present */
-       DEV_PM_QOS_DEVICE_PRESENT,      /* Device present, data not allocated */
-       DEV_PM_QOS_ALLOCATED,           /* Device present, data allocated */
-};
-
  struct wakeup_source;
 
  struct pm_domain_data {

@@ -489,7 +482,6 @@ struct dev_pm_info {
  #endif

       struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */ struct pm_qos_constraints *constraints;
-       enum dev_pm_qos_state   constraints_state;
 };

 extern void update_pm_runtime_accounting(struct device *dev);

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

Quoted 29 lines

Quoted 29 lines

Quoted 28 lines

Quoted 16 lines

(quoted diff) +2 -10

-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

Review

Quoted 10 lines

(quoted diff) +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Quoted 3 lines

Quoted 1 line

Pretty much it is. Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

However, the removal of the device is a special case. What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it? Not much I guess.

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

Quoted 35 lines

Quoted 35 lines

Quoted 34 lines

Quoted 22 lines

 */
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req, s32 value)

(quoted diff) +2 -10

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

Review int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

(quoted diff) +7 -3

-        */
-       if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Quoted 3 lines

Quoted 1 line

Pretty much it is.  Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

However, the removal of the device is a special case.  What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it?  Not much I guess.

Thanks,
Rafael

Quoted 1 line · Jean Pihet

On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <[email protected]> wrote:

Quoted 35 lines

Quoted 35 lines

Quoted 34 lines

Quoted 22 lines

 */
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req, s32 value)

(quoted diff) +2 -10

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
                return -EINVAL;
        }
 
-       mutex_lock(&dev_pm_qos_mtx);
        req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

Review int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

(quoted diff) +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?

Quoted 3 lines

Quoted 1 line

Pretty much it is.  Is that a problem?

I think the caller needs to know if the constraint has been applied correctly or not.

However, the removal of the device is a special case.  What would the caller be supposed to do when it learned that the device had been removed while it had been trying to add a QoS constraing for it?  Not much I guess.

Thanks,
Rafael

Quoted 93 lines · Rafael J. Wysocki

Hi,

Quoted 42 lines · Jean Pihet

Hi Rafael,

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

To read the current PM QoS value for a given device we need to make sure that the device's power.constraints object won't be removed while we're doing that. For this reason, put the operation under dev->power.lock and acquire the lock around the initialization and removal of power.constraints.

Ok.

Quoted 8 lines

Moreover, since we're using the value of power.constraints to determine whether or not the object is present, the power.constraints_state field isn't necessary any more and may be removed. However, dev_pm_qos_add_request() needs to check if the device is being removed from the system before allocating a new PM QoS constraints object for it, so it has to use device_pm_lock() and the device PM QoS initialization and destruction should be done under device_pm_lock() as well.

Ok that makes sense.
The constraints_state field can be replaced by a combination of dev->power.constraints and list_empty(&dev->power.entry), which makes the code more compact and less redundant.

Quoted 14 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |    4 -
 drivers/base/power/qos.c  |  167 ++++++++++++++++++++++++++--------------------
 include/linux/pm.h        |    8 --
 include/linux/pm_qos.h    |    3
 4 files changed, 101 insertions(+), 81 deletions(-)

Index: linux/drivers/base/power/qos.c
===================================================================

drivers/base/power/qos.c +0 -0

@@ -30,15 +30,6 @@

...

Quoted 1 line

Quoted 9 lines

drivers/base/power/qos.c void dev_pm_qos_constraints_destroy(stru · line 206

Quoted 8 lines

drivers/base/power/qos.c +2 -2

@@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
 *
 * Returns 1 if the aggregated constraint value has changed,
 * 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures.

Why not use -ENODEV in case there is no device?

I don't think it's useful for the caller. If the device is gone, the constraing simply doesn't matter, so there's no error to handle.

Quoted 34 lines

Quoted 22 lines

drivers/base/power/qos.c +2 -10

 */
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
                          s32 value)

@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
               return -EINVAL;
       }
 
-       mutex_lock(&dev_pm_qos_mtx);
       req->dev = dev;
 
-       /* Return if the device has been removed */
-       if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
-               ret = -ENODEV;
-               goto out;
-       }
+       device_pm_lock();
+       mutex_lock(&dev_pm_qos_mtx);
 
-       /*
-        * Allocate the constraints data on the first call to add_request,
-        * i.e. only if the data is not already allocated and if the device has
-        * not been removed

drivers/base/power/qos.c int dev_pm_qos_add_request(struct device · line 233

Quoted 10 lines

drivers/base/power/qos.c +7 -3

-        */
-       if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
-               ret = dev_pm_qos_constraints_allocate(dev);
+       if (dev->power.constraints) {
+               device_pm_unlock();
+       } else {
+               if (list_empty(&dev->power.entry)) {
+                       /* The device has been removed from the system. */
+                       device_pm_unlock();
+                       goto out;

0 is silently returned in case the device has been removed. Is that the intention?