Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code

On Saturday, August 13, 2011, mark gross wrote:

On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:

quoted

From: Jean Pihet <redacted>

In preparation for the per-device constratins support:
- rename update_target to pm_qos_update_target
- generalize and export pm_qos_update_target for usage by the upcoming
per-device latency constraints framework:
   . operate on struct pm_qos_constraints for constraints management,
   . introduce an 'action' parameter for constraints add/update/remove,
   . the return value indicates if the aggregated constraint value has
     changed,
- update the internal code to operate on struct pm_qos_constraints
- add a NULL pointer check in the API functions

Signed-off-by: Jean Pihet <redacted>

---
 include/linux/pm_qos.h |   14 ++++++
 kernel/power/qos.c     |  123 ++++++++++++++++++++++++++----------------------
 2 files changed, 81 insertions(+), 56 deletions(-)

diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 9772311..84aa150 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h

@@ -44,7 +44,16 @@ struct pm_qos_constraints {
 	struct blocking_notifier_head *notifiers;
 };
 
+/* Action requested to pm_qos_update_target */
+enum pm_qos_req_action {
+	PM_QOS_ADD_REQ,		/* Add a new request */
+	PM_QOS_UPDATE_REQ,	/* Update an existing request */
+	PM_QOS_REMOVE_REQ	/* Remove an existing request */
+};
+

What do you need this enum for?  The function names *_update_*, *_add_*,
and  *_remove_* seem to be pretty redundant if you have to pass an enum
that could possibly conflict with the function name.

quoted

 #ifdef CONFIG_PM
+int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+			 enum pm_qos_req_action action, int value);

The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or
there is something strange going on....  BTW what shold this function do
if the pm_qos_req_action was *not* the UPDATE one?


pm_qos_update_target should be a static to the C- file along with the
enum pm_qos_req_action.

quoted

 void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
 			s32 value);
 void pm_qos_update_request(struct pm_qos_request *req,

@@ -56,6 +65,11 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_request_active(struct pm_qos_request *req);
 #else
+static inline int pm_qos_update_target(struct pm_qos_constraints *c,
+				       struct plist_node *node,
+				       enum pm_qos_req_action action,
+				       int value)
+			{ return 0; }
 static inline void pm_qos_add_request(struct pm_qos_request *req,
 				      int pm_qos_class, s32 value)
 			{ return; }

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 66e8d6f..fc60f96 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c

@@ -121,17 +121,17 @@ static const struct file_operations pm_qos_power_fops = {
 };
 
 /* unlocked internal variant */
-static inline int pm_qos_get_value(struct pm_qos_object *o)
+static inline int pm_qos_get_value(struct pm_qos_constraints *c)
 {
-	if (plist_head_empty(&o->constraints->list))
-		return o->constraints->default_value;
+	if (plist_head_empty(&c->list))
+		return c->default_value;
 
-	switch (o->constraints->type) {
+	switch (c->type) {
 	case PM_QOS_MIN:
-		return plist_first(&o->constraints->list)->prio;
+		return plist_first(&c->list)->prio;
 
 	case PM_QOS_MAX:
-		return plist_last(&o->constraints->list)->prio;
+		return plist_last(&c->list)->prio;
 
 	default:
 		/* runtime check for not using enum */

@@ -139,47 +139,73 @@ static inline int pm_qos_get_value(struct pm_qos_object *o)
 	}
 }
 
-static inline s32 pm_qos_read_value(struct pm_qos_object *o)
+static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
 {
-	return o->constraints->target_value;
+	return c->target_value;
 }
 
-static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
+static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
 {
-	o->constraints->target_value = value;
+	c->target_value = value;
 }
 
-static void update_target(struct pm_qos_object *o, struct plist_node *node,
-			  int del, int value)
+/**
+ * pm_qos_update_target - manages the constraints list and calls the notifiers
+ *  if needed
+ * @c: constraints data struct
+ * @node: request to add to the list, to update or to remove
+ * @action: action to take on the constraints list
+ * @value: value of the request to add or update
+ *
+ * This function returns 1 if the aggregated constraint value has changed, 0
+ *  otherwise.
+ */
+int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+			 enum pm_qos_req_action action, int value)
 {
 	unsigned long flags;
-	int prev_value, curr_value;
+	int prev_value, curr_value, new_value;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	prev_value = pm_qos_get_value(o);
-	/* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
-	if (value != PM_QOS_DEFAULT_VALUE) {
+	prev_value = pm_qos_get_value(c);
+	if (value == PM_QOS_DEFAULT_VALUE)
+		new_value = c->default_value;
+	else
+		new_value = value;
+
+	switch (action) {
+	case PM_QOS_REMOVE_REQ:

We have a remove request API already.  This overloading of this
interface feels wrong to me.

quoted

+		plist_del(node, &c->list);
+		break;
+	case PM_QOS_UPDATE_REQ:
 		/*
 		 * to change the list, we atomically remove, reinit
 		 * with new value and add, then see if the extremal
 		 * changed
 		 */
-		plist_del(node, &o->constraints->list);
-		plist_node_init(node, value);
-		plist_add(node, &o->constraints->list);
-	} else if (del) {
-		plist_del(node, &o->constraints->list);
-	} else {
-		plist_add(node, &o->constraints->list);
+		plist_del(node, &c->list);
+	case PM_QOS_ADD_REQ:

Don't we have an API for adding a request?  if you want to overload
update like this then either we lose the add API or this shouldn't be
here.

quoted

+		plist_node_init(node, new_value);
+		plist_add(node, &c->list);
+		break;
+	default:
+		/* no action */
+		;
 	}
-	curr_value = pm_qos_get_value(o);
-	pm_qos_set_value(o, curr_value);
+
+	curr_value = pm_qos_get_value(c);
+	pm_qos_set_value(c, curr_value);
+
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
-	if (prev_value != curr_value)
-		blocking_notifier_call_chain(o->constraints->notifiers,
+	if (prev_value != curr_value) {
+		blocking_notifier_call_chain(c->notifiers,
 					     (unsigned long)curr_value,
 					     NULL);
+		return 1;
+	} else {
+		return 0;
+	}
 }
 
 /**

@@ -190,7 +216,7 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
  */
 int pm_qos_request(int pm_qos_class)
 {
-	return pm_qos_read_value(pm_qos_array[pm_qos_class]);
+	return pm_qos_read_value(pm_qos_array[pm_qos_class]->constraints);
 }
 EXPORT_SYMBOL_GPL(pm_qos_request);
 

@@ -216,20 +242,16 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
 void pm_qos_add_request(struct pm_qos_request *req,
 			int pm_qos_class, s32 value)
 {
-	struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
-	int new_value;
+	if (!req) /*guard against callers passing in null */
+		return;
 
 	if (pm_qos_request_active(req)) {
 		WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
 		return;
 	}
-	if (value == PM_QOS_DEFAULT_VALUE)
-		new_value = o->constraints->default_value;
-	else
-		new_value = value;
-	plist_node_init(&req->node, new_value);
 	req->pm_qos_class = pm_qos_class;
-	update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE);
+	pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
+			     &req->node, PM_QOS_ADD_REQ, value);

Ok, using pm_qos_update_target to reduce the LOC is ok but I don't think
this function and the enum should be exported outside of pm_qos.c

They are used by the next patches adding the per-device QoS.

Thanks,
Rafael