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

On Tuesday, August 16, 2011, mark gross wrote:

On Tue, Aug 16, 2011 at 08:44:19AM +0200, Jean Pihet wrote:

quoted

On Tue, Aug 16, 2011 at 6:08 AM, mark gross [off-list ref] wrote:

quoted

On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote:

quoted

On Sunday, August 14, 2011, Jean Pihet wrote:

quoted

Hi Rafael, Mark,

On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

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

quoted

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>

...

quoted

quoted

quoted

+/* 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?

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.

The enums are fine by me and they allow us to simplify the code
quite a bit.

Ok, but they look a bit sloppy to me as we now have an API that says
"add" we can actually pass in an enum that says "remove".

We have an API that says 'update target' that we pass in a parameter
that says 'add request', 'update request' or 'remove request'.
If it is required I could just rename the internal function
update_target, in a later patch.

You are right.  I thought I saw the enum added to the other API's for
some reason.  Still, with this update we have an API overloaded through
that enum parameter providing 2 add, 2 delete and 2 update API's  Each
pair doing about the same thing.

To me it feels like a baby step toward an ioctl type of API that I don't
like.  I'm not going to fight about it any more but I don't like such
API's as they tend to get hard to read and get out of control.

please rethink this a little.

For real _users_, the API is still "add", "update" and "remove",
but _internally_ those functions use the same "worker" routine with an
argument specifying the operation to carry out.  This reduces code
duplication quite a bit and it quite common in the kernel.

Thanks,
Rafael