On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@xxxxxxxxxxxxxx wrote:
> From: Jean Pihet <j-pihet@xxxxxx>
>
> 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@xxxxxx>
>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
