Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

2011-08-16 Thread Jean Pihet 
Hi Rafael,

2011/8/14 Rafael J. Wysocki r...@sisk.pl:
 Hi,

 There is some code duplication in this patch that should better be
 avoided (details below).

 On Thursday, August 11, 2011, jean.pi...@newoldbits.com wrote:
 From: Jean Pihet j-pi...@ti.com

 Add a global notification chain that gets called upon changes to the
 aggregated constraint value for any device.
 The notification callbacks are passing the full constraint request data
 in order for the callees to have access to it. The current use is for the
 platform low-level code to access the target device of the constraint.

...


 The following code:

 +     /*
 +      * Update constraints list and call the per-device callbacks if needed
 +      */
 +     ret = pm_qos_update_target(dev-power.constraints,
 +                                req-node, PM_QOS_ADD_REQ, value);
 +
 +     if (ret) {
 +             /* Call the global callbacks if needed */
 +             curr_value = pm_qos_read_value(req-dev-power.constraints);
 +             blocking_notifier_call_chain(dev_pm_notifiers,
 +                                          (unsigned long)curr_value,
 +                                          req);
 +     }

 is used in dev_pm_qos_update_request() and dev_pm_qos_remove_request()
 with the only difference being the command given to pm_qos_update_target().
 This asks for a common function, eg. dev_pm_qos_update_target(), containing
 that code that will be called by all of them (and, apparently, by
 dev_pm_qos_constraints_destroy()).
Ok that makes the code cleaner.


 ...
 @@ -250,9 +329,18 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
                        * Update constraints list and call the per-device
                        * callbacks if needed
                        */
 -                     pm_qos_update_target(req-dev-power.constraints,
 -                                          req-node, PM_QOS_REMOVE_REQ,
 -                                          PM_QOS_DEFAULT_VALUE);
 +                     ret |= 
 pm_qos_update_target(req-dev-power.constraints,
 +                                                 req-node,
 +                                                 PM_QOS_REMOVE_REQ,
 +                                                 PM_QOS_DEFAULT_VALUE);

 I'm not sure why you're using the binary or operator here.  Shouldn't that be
 a simple assignment?

 +
 +             if (ret) {
 +                     /* Call the global callbacks if needed */
 +                     curr_value = dev-power.constraints-default_value;
 +                     blocking_notifier_call_chain(dev_pm_notifiers,
 +                                                  (unsigned long)curr_value,
 +                                                  req);
 +             }
In the sake of optimization I had a single return value that
aggregates the return values of the calls target_value and calls the
global notifier callbacks _only once_.

As you suggested it is better to use the common update code, which
makes the code cleaner but also removes this optimization.


               kfree(dev-power.constraints-notifiers);
               kfree(dev-power.constraints);

 Thanks,
 Rafael


Regards,
Jean
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

2011-08-16 Thread jean . pihet 
From: Jean Pihet j-pi...@ti.com

Add a global notification chain that gets called upon changes to the
aggregated constraint value for any device.
The notification callbacks are passing the full constraint request data
in order for the callees to have access to it. The current use is for the
platform low-level code to access the target device of the constraint.

Signed-off-by: Jean Pihet j-pi...@ti.com
---
 drivers/base/power/qos.c |   84 --
 include/linux/pm_qos.h   |   11 ++
 kernel/power/qos.c   |2 +-
 3 files changed, 78 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 304d68d..b52b3e8 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -8,6 +8,12 @@
  *
  * This QoS design is best effort based. Dependents register their QoS needs.
  * Watchers register to keep track of the current QoS needs of the system.
+ * Watchers can register different types of notification callbacks:
+ *  . a per-device notification callback using the dev_pm_qos_*_notifier API.
+ *The notification chain data is stored in the per-device constraint
+ *data struct.
+ *  . a system-wide notification callback using the 
dev_pm_qos_*_global_notifier
+ *API. The notification chain data is stored in a static variable.
  *
  * Note about the per-device constraint data struct allocation:
  * . The per-device constraints data struct ptr is tored into the device
@@ -36,8 +42,32 @@
 
 
 static DEFINE_MUTEX(dev_pm_qos_mtx);
+static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
 static void dev_pm_qos_constraints_allocate(struct device *dev);
 
+/*
+ * Update the constraints list using the PM QoS core code and
+ * if needed call the per-device and the global notification callbacks
+ */
+static int _apply_constraint(struct dev_pm_qos_request *req,
+enum pm_qos_req_action action, int value)
+{
+   int ret, curr_value;
+
+   ret = pm_qos_update_target(req-dev-power.constraints,
+  req-node, action, value);
+
+   if (ret) {
+   /* Call the global callbacks if needed */
+   curr_value = pm_qos_read_value(req-dev-power.constraints);
+   blocking_notifier_call_chain(dev_pm_notifiers,
+(unsigned long)curr_value,
+req);
+   }
+
+   return ret;
+}
+
 /**
  * dev_pm_qos_add_request - inserts new qos request into the list
  * @dev: target device for the constraint
@@ -66,16 +96,13 @@ void dev_pm_qos_add_request(struct device *dev, struct 
dev_pm_qos_request *req,
dev_pm_qos_constraints_allocate(dev);
 
mutex_lock(dev_pm_qos_mtx);
-
req-dev = dev;
 
/* Silently return if the device has been removed */
if (req-dev-power.constraints_state != DEV_PM_QOS_ALLOCATED)
goto out;
 
-   pm_qos_update_target(dev-power.constraints,
-req-node, PM_QOS_ADD_REQ, value);
-
+   _apply_constraint(req, PM_QOS_ADD_REQ, value);
 out:
mutex_unlock(dev_pm_qos_mtx);
 }
@@ -92,7 +119,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
  * Attempts are made to make this code callable on hot code paths.
  */
 void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
-  s32 new_value)
+  s32 new_value)
 {
if (!req) /*guard against callers passing in null */
return;
@@ -110,9 +137,7 @@ void dev_pm_qos_update_request(struct dev_pm_qos_request 
*req,
goto out;
 
if (new_value != req-node.prio)
-   pm_qos_update_target(
-   req-dev-power.constraints,
-   req-node, PM_QOS_UPDATE_REQ, new_value);
+   _apply_constraint(req, PM_QOS_UPDATE_REQ, new_value);
 
 out:
mutex_unlock(dev_pm_qos_mtx);
@@ -143,9 +168,7 @@ void dev_pm_qos_remove_request(struct dev_pm_qos_request 
*req)
if (req-dev-power.constraints_state != DEV_PM_QOS_ALLOCATED)
goto out;
 
-   pm_qos_update_target(req-dev-power.constraints,
-req-node, PM_QOS_REMOVE_REQ,
-PM_QOS_DEFAULT_VALUE);
+   _apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
 
 out:
@@ -214,6 +237,36 @@ out:
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
 
+/**
+ * dev_pm_qos_add_global_notifier - sets notification entry for changes to
+ * target value of the PM QoS constraints for any device
+ *
+ * @notifier: notifier block managed by caller.
+ *
+ * Will register the notifier into a notification chain that gets called
+ * upon changes to the target value for any device.
+ */
+int dev_pm_qos_add_global_notifier(struct notifier_block *notifier)
+{
+   return blocking_notifier_chain_register(dev_pm_notifiers,

Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

2011-08-16 Thread Rafael J. Wysocki 
Hi,

On Tuesday, August 16, 2011, jean.pi...@newoldbits.com wrote:
 From: Jean Pihet j-pi...@ti.com
 
 Add a global notification chain that gets called upon changes to the
 aggregated constraint value for any device.
 The notification callbacks are passing the full constraint request data
 in order for the callees to have access to it. The current use is for the
 platform low-level code to access the target device of the constraint.
 
 Signed-off-by: Jean Pihet j-pi...@ti.com
 ---
  drivers/base/power/qos.c |   84 
 --
  include/linux/pm_qos.h   |   11 ++
  kernel/power/qos.c   |2 +-
  3 files changed, 78 insertions(+), 19 deletions(-)
 
 diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
 index 304d68d..b52b3e8 100644
 --- a/drivers/base/power/qos.c
 +++ b/drivers/base/power/qos.c
 @@ -8,6 +8,12 @@
   *
   * This QoS design is best effort based. Dependents register their QoS needs.
   * Watchers register to keep track of the current QoS needs of the system.
 + * Watchers can register different types of notification callbacks:
 + *  . a per-device notification callback using the dev_pm_qos_*_notifier API.
 + *The notification chain data is stored in the per-device constraint
 + *data struct.
 + *  . a system-wide notification callback using the 
 dev_pm_qos_*_global_notifier
 + *API. The notification chain data is stored in a static variable.
   *
   * Note about the per-device constraint data struct allocation:
   * . The per-device constraints data struct ptr is tored into the device
 @@ -36,8 +42,32 @@
  
  
  static DEFINE_MUTEX(dev_pm_qos_mtx);
 +static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
  static void dev_pm_qos_constraints_allocate(struct device *dev);
  
 +/*
 + * Update the constraints list using the PM QoS core code and
 + * if needed call the per-device and the global notification callbacks
 + */

Well, if you add kerneldoc comments, please make them follow the standard,
even if that's a static function.

 +static int _apply_constraint(struct dev_pm_qos_request *req,

The initial underscore looks odd and is not really necessary.  Please drop it.

The rest of the patch looks fine.

Thanks,
Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

2011-08-14 Thread Rafael J. Wysocki 
Hi,

There is some code duplication in this patch that should better be
avoided (details below).

On Thursday, August 11, 2011, jean.pi...@newoldbits.com wrote:
 From: Jean Pihet j-pi...@ti.com
 
 Add a global notification chain that gets called upon changes to the
 aggregated constraint value for any device.
 The notification callbacks are passing the full constraint request data
 in order for the callees to have access to it. The current use is for the
 platform low-level code to access the target device of the constraint.
 
 Signed-off-by: Jean Pihet j-pi...@ti.com
 ---
  drivers/base/power/qos.c |  114 -
  include/linux/pm_qos.h   |   11 
  kernel/power/qos.c   |2 +-
  3 files changed, 113 insertions(+), 14 deletions(-)
 
 diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
 index 465e419..4b0b316 100644
 --- a/drivers/base/power/qos.c
 +++ b/drivers/base/power/qos.c
 @@ -8,6 +8,12 @@
   *
   * This QoS design is best effort based. Dependents register their QoS needs.
   * Watchers register to keep track of the current QoS needs of the system.
 + * Watchers can register different types of notification callbacks:
 + *  . a per-device notification callback using the dev_pm_qos_*_notifier API.
 + *The notification chain data is stored in the per-device constraint
 + *data struct.
 + *  . a system-wide notification callback using the 
 dev_pm_qos_*_global_notifier
 + *API. The notification chain data is stored in a static variable.
   *
   * Note about the per-device constraint data struct allocation:
   * . The per-device constraints data struct ptr is tored into the device
 @@ -41,6 +47,7 @@
  #include linux/kernel.h
  
  
 +static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
  static void dev_pm_qos_constraints_allocate(struct device *dev);
  
  int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
 @@ -64,6 +71,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
  void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device 
 *dev,
   s32 value)
  {
 + int ret, curr_value;
 +
   if (!req) /*guard against callers passing in null */
   return;
  
 @@ -82,8 +91,19 @@ void dev_pm_qos_add_request(struct dev_pm_qos_request 
 *req, struct device *dev,
   if (req-dev-power.constraints_state != DEV_PM_QOS_ALLOCATED)
   return;
  
 - pm_qos_update_target(dev-power.constraints,
 -  req-node, PM_QOS_ADD_REQ, value);

The following code:

 + /*
 +  * Update constraints list and call the per-device callbacks if needed
 +  */
 + ret = pm_qos_update_target(dev-power.constraints,
 +req-node, PM_QOS_ADD_REQ, value);
 +
 + if (ret) {
 + /* Call the global callbacks if needed */
 + curr_value = pm_qos_read_value(req-dev-power.constraints);
 + blocking_notifier_call_chain(dev_pm_notifiers,
 +  (unsigned long)curr_value,
 +  req);
 + }

is used in dev_pm_qos_update_request() and dev_pm_qos_remove_request()
with the only difference being the command given to pm_qos_update_target().
This asks for a common function, eg. dev_pm_qos_update_target(), containing
that code that will be called by all of them (and, apparently, by
dev_pm_qos_constraints_destroy()).

...
 @@ -250,9 +329,18 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
* Update constraints list and call the per-device
* callbacks if needed
*/
 - pm_qos_update_target(req-dev-power.constraints,
 -  req-node, PM_QOS_REMOVE_REQ,
 -  PM_QOS_DEFAULT_VALUE);
 + ret |= pm_qos_update_target(req-dev-power.constraints,
 + req-node,
 + PM_QOS_REMOVE_REQ,
 + PM_QOS_DEFAULT_VALUE);

I'm not sure why you're using the binary or operator here.  Shouldn't that be
a simple assignment?

 +
 + if (ret) {
 + /* Call the global callbacks if needed */
 + curr_value = dev-power.constraints-default_value;
 + blocking_notifier_call_chain(dev_pm_notifiers,
 +  (unsigned long)curr_value,
 +  req);
 + }
  
   kfree(dev-power.constraints-notifiers);
   kfree(dev-power.constraints);

Thanks,
Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

2011-08-11 Thread jean . pihet 
From: Jean Pihet j-pi...@ti.com

Add a global notification chain that gets called upon changes to the
aggregated constraint value for any device.
The notification callbacks are passing the full constraint request data
in order for the callees to have access to it. The current use is for the
platform low-level code to access the target device of the constraint.

Signed-off-by: Jean Pihet j-pi...@ti.com
---
 drivers/base/power/qos.c |  114 -
 include/linux/pm_qos.h   |   11 
 kernel/power/qos.c   |2 +-
 3 files changed, 113 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 465e419..4b0b316 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -8,6 +8,12 @@
  *
  * This QoS design is best effort based. Dependents register their QoS needs.
  * Watchers register to keep track of the current QoS needs of the system.
+ * Watchers can register different types of notification callbacks:
+ *  . a per-device notification callback using the dev_pm_qos_*_notifier API.
+ *The notification chain data is stored in the per-device constraint
+ *data struct.
+ *  . a system-wide notification callback using the 
dev_pm_qos_*_global_notifier
+ *API. The notification chain data is stored in a static variable.
  *
  * Note about the per-device constraint data struct allocation:
  * . The per-device constraints data struct ptr is tored into the device
@@ -41,6 +47,7 @@
 #include linux/kernel.h
 
 
+static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
 static void dev_pm_qos_constraints_allocate(struct device *dev);
 
 int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
@@ -64,6 +71,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
 void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
s32 value)
 {
+   int ret, curr_value;
+
if (!req) /*guard against callers passing in null */
return;
 
@@ -82,8 +91,19 @@ void dev_pm_qos_add_request(struct dev_pm_qos_request *req, 
struct device *dev,
if (req-dev-power.constraints_state != DEV_PM_QOS_ALLOCATED)
return;
 
-   pm_qos_update_target(dev-power.constraints,
-req-node, PM_QOS_ADD_REQ, value);
+   /*
+* Update constraints list and call the per-device callbacks if needed
+*/
+   ret = pm_qos_update_target(dev-power.constraints,
+  req-node, PM_QOS_ADD_REQ, value);
+
+   if (ret) {
+   /* Call the global callbacks if needed */
+   curr_value = pm_qos_read_value(req-dev-power.constraints);
+   blocking_notifier_call_chain(dev_pm_notifiers,
+(unsigned long)curr_value,
+req);
+   }
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
 
@@ -98,8 +118,10 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
  * Attempts are made to make this code callable on hot code paths.
  */
 void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
-  s32 new_value)
+  s32 new_value)
 {
+   int ret, curr_value;
+
if (!req) /*guard against callers passing in null */
return;
 
@@ -113,10 +135,23 @@ void dev_pm_qos_update_request(struct dev_pm_qos_request 
*req,
if (req-dev-power.constraints_state != DEV_PM_QOS_ALLOCATED)
return;
 
-   if (new_value != req-node.prio)
-   pm_qos_update_target(
-   req-dev-power.constraints,
-   req-node, PM_QOS_UPDATE_REQ, new_value);
+   if (new_value != req-node.prio) {
+   /*
+* Update constraints list and call the per-device callbacks
+* if needed
+*/
+   ret = pm_qos_update_target(req-dev-power.constraints,
+  req-node, PM_QOS_UPDATE_REQ,
+  new_value);
+   if (ret) {
+   /* Call the global callbacks if needed */
+   curr_value = pm_qos_read_value(
+   req-dev-power.constraints);
+   blocking_notifier_call_chain(dev_pm_notifiers,
+(unsigned long)curr_value,
+req);
+   }
+   }
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
 
@@ -129,6 +164,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
  */
 void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 {
+   int ret, curr_value;
+
if (!req) /*guard against callers passing in null */
return;
 
@@ -142,9 +179,20 @@ void dev_pm_qos_remove_request(struct dev_pm_qos_request 
*req)
if