Re: [PATCH 03/13] PM: QoS: extend the in-kernel API with per-device latency constraints

[Jean Pihet <jean.pihet@newoldbits.com>]

Rafael,

2011/7/30 Rafael J. Wysocki <rjw@sisk.pl>:
> On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Extend the PM QoS kernel API:
>> - add a new PM QoS class PM_QOS_DEV_LATENCY for device wake-up latency
>> constraints
>> - make the pm_qos_add_request API more generic by using a parameter of
>> type struct pm_qos_parameters
>> - minor clean-ups and rename of struct fields:
>>   . rename pm_qos_class to class and pm_qos_req to req in internal code
>>   . consistenly use req and params as the API parameters
>>   . rename struct pm_qos_request_list to struct pm_qos_request
>> - update the in-kernel API callers to the new API
>
> There should be more about the motivation in the changelog.  It says
> what the patch is doing, but it doesn't say a word of _why_ it is done in
> the first place.
Ok will add a comment

>
> Second, you're renaming a lot of things in the same patch that makes
> functional changes.  This is confusing and makes it very difficult to
> understand what's going on.  Please use separate patches to rename
> things, when possible, and avoid renaming things that don't need to be
> renamed.
Sorry about that! I will split up the patches.

...
>>
>> -struct pm_qos_request_list {
>> +struct pm_qos_request {
>
> This renaming should go in a separate patch, really.
Ok

>
>>       struct plist_node list;
>> -     int pm_qos_class;
>> +     int class;
>
> This renaming doesn't seem to be necessary.  Moreover, it's confusing,
> because there already is struct class (for device classes) and a member
> called "class" in struct device and they aren't related to this one.
I will keep pm_qos_class if the rename brings in some confusion. The
intention was to simplify the names of the fields in structs with
explicit names.

>
>> +     struct device *dev;
>>  };
>>
>> -void pm_qos_add_request(struct pm_qos_request_list *l, int pm_qos_class, s32 value);
>> -void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>> -             s32 new_value);
>> -void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
>> +struct pm_qos_parameters {
>> +     int class;
>> +     struct device *dev;
>> +     s32 value;
>> +};
>
> What exactly is the "dev" member needed for?
This is the target device that is passed as parameter to the API. It
is used for constraints of the devices latency class.

...
>> +static BLOCKING_NOTIFIER_HEAD(dev_lat_notifier);
>> +static struct pm_qos_object dev_pm_qos = {
>> +     .requests = PLIST_HEAD_INIT(dev_pm_qos.requests, pm_qos_lock),
>> +     .notifiers = &dev_lat_notifier,
>> +     .name = "dev_latency",
>> +     .target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
>> +     .default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
>> +     .type = PM_QOS_MIN,
>> +};
>> +
>
> You seem to be confusing things here.  Since devices will have their own lists
> of requests now (as per the previous patch), the .requests member above is not
> necessary.  Moreover, it seems to be used incorrectly below.
The idea was to split the patches as requested previously: first the
API changes (this very patch [03/13]) and then the actual
implementation ([04/13]). Is this correct?

...
>> -int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
>> +int pm_qos_add_notifier(int class, struct notifier_block *notifier)
>>  {
>>       int retval;
>>
>>       retval = blocking_notifier_chain_register(
>> -                     pm_qos_array[pm_qos_class]->notifiers, notifier);
>> +                     pm_qos_array[class]->notifiers, notifier);
>
> Now, there's a question if we want to have one notifier chain for all
> devices or if it's better to have a per-device notifier chain.  Both
> approaches have their own advantages and drawbacks, but in the latter
> case this code will have to be reworked.
I really think the need is for a per-class notifier. Cf. comments on
[04/13] about that.

>
> Thanks,
> Rafael

Thanks,
Jean