Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

From: Jean Pihet <jean.pihet@newoldbits.com>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: markgross@thegnar.org,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	Linux PM mailing list <linux-pm@lists.linux-foundation.org>,
	linux-omap@vger.kernel.org, Jean Pihet <j-pihet@ti.com>
Subject: Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints
Date: Tue, 16 Aug 2011 11:58:38 +0200	[thread overview]
Message-ID: <CAORVsuUNxCN7+KZnpJT5Urd07cB+raX6e23ah_9Mp8M07SAyLw__39425.1238358722$1313488882$gmane$org@mail.gmail.com> (raw)
In-Reply-To: <201108142350.42260.rjw@sisk.pl>

Hi Rafael,

2011/8/14 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> There is some code duplication in this patch that should better be
> avoided (details below).
>
> On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Add a global notification chain that gets called upon changes to the
>> aggregated constraint value for any device.
>> The notification callbacks are passing the full constraint request data
>> in order for the callees to have access to it. The current use is for the
>> platform low-level code to access the target device of the constraint.
>>
...

>
> The following code:
>
>> +     /*
>> +      * Update constraints list and call the per-device callbacks if needed
>> +      */
>> +     ret = pm_qos_update_target(dev->power.constraints,
>> +                                &req->node, PM_QOS_ADD_REQ, value);
>> +
>> +     if (ret) {
>> +             /* Call the global callbacks if needed */
>> +             curr_value = pm_qos_read_value(req->dev->power.constraints);
>> +             blocking_notifier_call_chain(&dev_pm_notifiers,
>> +                                          (unsigned long)curr_value,
>> +                                          req);
>> +     }
>
> is used in dev_pm_qos_update_request() and dev_pm_qos_remove_request()
> with the only difference being the command given to pm_qos_update_target().
> This asks for a common function, eg. dev_pm_qos_update_target(), containing
> that code that will be called by all of them (and, apparently, by
> dev_pm_qos_constraints_destroy()).
Ok that makes the code cleaner.

>
> ...
>> @@ -250,9 +329,18 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
>>                        * Update constraints list and call the per-device
>>                        * callbacks if needed
>>                        */
>> -                     pm_qos_update_target(req->dev->power.constraints,
>> -                                          &req->node, PM_QOS_REMOVE_REQ,
>> -                                          PM_QOS_DEFAULT_VALUE);
>> +                     ret |= pm_qos_update_target(req->dev->power.constraints,
>> +                                                 &req->node,
>> +                                                 PM_QOS_REMOVE_REQ,
>> +                                                 PM_QOS_DEFAULT_VALUE);
>
> I'm not sure why you're using the binary or operator here.  Shouldn't that be
> a simple assignment?
>
>> +
>> +             if (ret) {
>> +                     /* Call the global callbacks if needed */
>> +                     curr_value = dev->power.constraints->default_value;
>> +                     blocking_notifier_call_chain(&dev_pm_notifiers,
>> +                                                  (unsigned long)curr_value,
>> +                                                  req);
>> +             }
In the sake of optimization I had a single return value that
aggregates the return values of the calls target_value and calls the
global notifier callbacks _only once_.

As you suggested it is better to use the common update code, which
makes the code cleaner but also removes this optimization.

>>
>>               kfree(dev->power.constraints->notifiers);
>>               kfree(dev->power.constraints);
>
> Thanks,
> Rafael
>

Regards,
Jean