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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Rafael, Mark,

On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Saturday, August 13, 2011, mark gross wrote:
>> 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>
...
>> > +/* 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?

The meaning of pm_qos_update_target is 'update the PM QoS target
constraints lists'. As described in the changelog the intention of
this patch is to implement the constraints lists management logic in
update_target and simplify the API functions (add/update/remove). It
is also exported for the upcoming (patch 06/15]) to use it as well.

...

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

>
> Thanks,
> Rafael
>

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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux