| Message ID | 1313075212-8366-6-git-send-email-j-pihet@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 >
On Saturday, August 13, 2011, mark gross wrote: > 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 They are used by the next patches adding the per-device QoS. Thanks, Rafael
Hi Rafael, Mark, On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Saturday, August 13, 2011, mark gross wrote: >> 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> ... >> > +/* 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
On Sunday, August 14, 2011, Jean Pihet wrote: > Hi Rafael, Mark, > > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Saturday, August 13, 2011, mark gross wrote: > >> 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> > ... > >> > +/* 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. The enums are fine by me and they allow us to simplify the code quite a bit. Thanks, Rafael
On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote: > On Sunday, August 14, 2011, Jean Pihet wrote: > > Hi Rafael, Mark, > > > > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Saturday, August 13, 2011, mark gross wrote: > > >> 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> > > ... > > >> > +/* 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. > > 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". --mark
On Tue, Aug 16, 2011 at 6:08 AM, mark gross <markgross@thegnar.org> wrote: > On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote: >> On Sunday, August 14, 2011, Jean Pihet wrote: >> > Hi Rafael, Mark, >> > >> > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > > On Saturday, August 13, 2011, mark gross wrote: >> > >> 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> >> > ... >> > >> > +/* 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. >> >> 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. > > --mark > Jean
On Tue, Aug 16, 2011 at 08:44:19AM +0200, Jean Pihet wrote: > On Tue, Aug 16, 2011 at 6:08 AM, mark gross <markgross@thegnar.org> wrote: > > On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote: > >> On Sunday, August 14, 2011, Jean Pihet wrote: > >> > Hi Rafael, Mark, > >> > > >> > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > > On Saturday, August 13, 2011, mark gross wrote: > >> > >> 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> > >> > ... > >> > >> > +/* 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. > >> > >> 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. --mark
On Tuesday, August 16, 2011, mark gross wrote: > On Tue, Aug 16, 2011 at 08:44:19AM +0200, Jean Pihet wrote: > > On Tue, Aug 16, 2011 at 6:08 AM, mark gross <markgross@thegnar.org> wrote: > > > On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote: > > >> On Sunday, August 14, 2011, Jean Pihet wrote: > > >> > Hi Rafael, Mark, > > >> > > > >> > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > >> > > On Saturday, August 13, 2011, mark gross wrote: > > >> > >> 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> > > >> > ... > > >> > >> > +/* 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. > > >> > > >> 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
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 */ +}; + #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); 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: + 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: + 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); } 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));