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

[mark gross <markgross@thegnar.org>]

On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
> 
> 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 <j-pihet@ti.com>
> 
> ---
>  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.

>  #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.


>  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.

> +		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.


> +		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

--mark


>  }
>  EXPORT_SYMBOL_GPL(pm_qos_add_request);
>  
> @@ -246,9 +268,6 @@ EXPORT_SYMBOL_GPL(pm_qos_add_request);
>  void pm_qos_update_request(struct pm_qos_request *req,
>  			   s32 new_value)
>  {
> -	s32 temp;
> -	struct pm_qos_object *o;
> -
>  	if (!req) /*guard against callers passing in null */
>  		return;
>  
> @@ -257,15 +276,10 @@ void pm_qos_update_request(struct pm_qos_request *req,
>  		return;
>  	}
>  
> -	o = pm_qos_array[req->pm_qos_class];
> -
> -	if (new_value == PM_QOS_DEFAULT_VALUE)
> -		temp = o->constraints->default_value;
> -	else
> -		temp = new_value;
> -
> -	if (temp != req->node.prio)
> -		update_target(o, &req->node, 0, temp);
> +	if (new_value != req->node.prio)
> +		pm_qos_update_target(
> +			pm_qos_array[req->pm_qos_class]->constraints,
> +			&req->node, PM_QOS_UPDATE_REQ, new_value);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);
>  
> @@ -279,9 +293,7 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
>   */
>  void pm_qos_remove_request(struct pm_qos_request *req)
>  {
> -	struct pm_qos_object *o;
> -
> -	if (req == NULL)
> +	if (!req) /*guard against callers passing in null */
>  		return;
>  		/* silent return to keep pcm code cleaner */
>  
> @@ -290,8 +302,9 @@ void pm_qos_remove_request(struct pm_qos_request *req)
>  		return;
>  	}
>  
> -	o = pm_qos_array[req->pm_qos_class];
> -	update_target(o, &req->node, 1, PM_QOS_DEFAULT_VALUE);
> +	pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
> +			     &req->node, PM_QOS_REMOVE_REQ,
> +			     PM_QOS_DEFAULT_VALUE);
>  	memset(req, 0, sizeof(*req));
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
> @@ -395,7 +408,6 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
>  {
>  	s32 value;
>  	unsigned long flags;
> -	struct pm_qos_object *o;
>  	struct pm_qos_request *req = filp->private_data;
>  
>  	if (!req)
> @@ -403,9 +415,8 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
>  	if (!pm_qos_request_active(req))
>  		return -EINVAL;
>  
> -	o = pm_qos_array[req->pm_qos_class];
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	value = pm_qos_get_value(o);
> +	value = pm_qos_get_value(pm_qos_array[req->pm_qos_class]->constraints);
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  
>  	return simple_read_from_buffer(buf, count, f_pos, &value, sizeof(s32));
> -- 
> 1.7.2.5
>