From: Daniel Kurtz <djkurtz@chromium.org>
To: Javi Merino <javi.merino@arm.com>
Cc: "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Chung-yih Wang <cywang@chromium.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Punit Agrawal <Punit.Agrawal@arm.com>,
Eduardo Valentin <edubezval@gmail.com>,
Zhang Rui <rui.zhang@intel.com>
Subject: Re: [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone
Date: Mon, 31 Aug 2015 21:14:47 +0800 [thread overview]
Message-ID: <CAGS+omDj0NR3pD-E+L6zQ9_mbCuXPQEBsMWEZXwKx-dor0fjFA@mail.gmail.com> (raw)
In-Reply-To: <20150828162807.GD2759@e104805>
Hi Javi,
On Sat, Aug 29, 2015 at 12:28 AM, Javi Merino <javi.merino@arm.com> wrote:
> On Fri, Aug 28, 2015 at 03:18:20AM +0100, Daniel Kurtz wrote:
>> On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <javi.merino@arm.com> wrote:
>> > Thermal zones created using thermal_zone_device_create() may not have
>> > tzp. As the governor gets its parameters from there, allocate it while
>> > the governor is bound to the thermal zone so that it can operate in it.
>> > In this case, tzp is freed when the thermal zone switches to another
>> > governor.
>> >
>> > Cc: Zhang Rui <rui.zhang@intel.com>
>> > Cc: Eduardo Valentin <edubezval@gmail.com>
>> > Signed-off-by: Javi Merino <javi.merino@arm.com>
>> > ---
>> >
>> > While this would be easier to do by just ignoring the thermal zone if
>> > there was no tzp, I think the approach in this patch provides a better
>> > behavior.
>>
>> Why?
>> Just ignoring the thermal zone seems reasonable and simpler.
>
> From the developer point of view, I agree that it's simpler. What I
> want to avoid is the system integrator getting different behaviors
> based on the presence of tzp when the thermal zone was created. If
> the integrator was to configure this from userspace, they would only
> be able to do so if the thermal zone was created with tzp. I don't
> like this distinction, I prefer the consistency from the user point of
> view that this patch gives.
Ok, thanks for the answer.
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Thanks!
-Dan
>
> Cheers,
> Javi
>
>> > drivers/thermal/power_allocator.c | 32 +++++++++++++++++++++++++++-----
>> > 1 file changed, 27 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
>> > index 2dfb8ade4d1b..85ce0aac9a41 100644
>> > --- a/drivers/thermal/power_allocator.c
>> > +++ b/drivers/thermal/power_allocator.c
>> > @@ -58,6 +58,8 @@ static inline s64 div_frac(s64 x, s64 y)
>> >
>> > /**
>> > * struct power_allocator_params - parameters for the power allocator governor
>> > + * @allocated_tzp: whether we have allocated tzp for this thermal zone and
>> > + * it needs to be freed on unbind
>> > * @err_integral: accumulated error in the PID controller.
>> > * @prev_err: error in the previous iteration of the PID controller.
>> > * Used to calculate the derivative term.
>> > @@ -70,6 +72,7 @@ static inline s64 div_frac(s64 x, s64 y)
>> > * controlling for.
>> > */
>> > struct power_allocator_params {
>> > + bool allocated_tzp;
>> > s64 err_integral;
>> > s32 prev_err;
>> > int trip_switch_on;
>> > @@ -530,8 +533,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
>> > * Initialize the PID controller parameters and bind it to the thermal
>> > * zone.
>> > *
>> > - * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
>> > - * if we ran out of memory.
>> > + * Return: 0 on success, or -ENOMEM if we ran out of memory.
>> > */
>> > static int power_allocator_bind(struct thermal_zone_device *tz)
>> > {
>> > @@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>> > struct power_allocator_params *params;
>> > unsigned long control_temp;
>> >
>> > - if (!tz->tzp)
>> > - return -EINVAL;
>> > -
>> > params = kzalloc(sizeof(*params), GFP_KERNEL);
>> > if (!params)
>> > return -ENOMEM;
>> >
>> > + if (!tz->tzp) {
>> > + tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
>>
>> Why bother to allocate this dummy struct?
>> Can't we just leave tz->tzp as NULL, and do a NULL check where needed?
>>
>> > + if (!tz->tzp) {
>> > + ret = -ENOMEM;
>> > + goto free_params;
>> > + }
>> > +
>> > + params->allocated_tzp = true;
>> > + }
>> > +
>> > if (!tz->tzp->sustainable_power)
>> > dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
>> >
>> > @@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>> > tz->governor_data = params;
>> >
>> > return 0;
>> > +
>> > +free_params:
>> > + kfree(params);
>> > +
>> > + return ret;
>> > }
>> >
>> > static void power_allocator_unbind(struct thermal_zone_device *tz)
>> > {
>> > + struct power_allocator_params *params = tz->governor_data;
>> > +
>> > dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
>> > +
>> > + if (params->allocated_tzp) {
>> > + kfree(tz->tzp);
>> > + tz->tzp = NULL;
>> > + }
>> > +
>> > kfree(tz->governor_data);
>> > tz->governor_data = NULL;
>> > }
>> > --
>> > 1.9.1
>> >
>>
next prev parent reply other threads:[~2015-08-31 13:15 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-04 16:39 [PATCH] thermal: remove power allocator from list of default governors Dmitry Torokhov
2015-08-05 8:37 ` Javi Merino
2015-08-05 16:18 ` Srinivas Pandruvada
2015-08-05 16:35 ` Dmitry Torokhov
2015-08-05 18:49 ` Eduardo Valentin
2015-08-06 8:30 ` Javi Merino
2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino
2015-08-10 16:04 ` [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-08-10 16:04 ` [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-08-10 16:04 ` [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino
2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-08-11 13:42 ` Punit Agrawal
2015-08-11 17:37 ` Javi Merino
2015-08-12 11:05 ` Daniel Kurtz
2015-08-11 10:21 ` [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-08-12 11:13 ` Daniel Kurtz
2015-08-12 16:46 ` Javi Merino
2015-08-11 10:21 ` [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino
2015-08-11 10:21 ` [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino
2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-08-20 22:16 ` Eduardo Valentin
2015-08-24 18:37 ` Javi Merino
2015-08-17 17:36 ` [PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-08-17 17:36 ` [PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino
2015-08-17 17:36 ` [PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino
2015-08-26 13:26 ` [PATCH v4 1/5] thermal: Add a function to get the minimum power Javi Merino
2015-08-26 13:26 ` [PATCH v4 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-08-26 13:26 ` [PATCH v4 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-08-26 13:26 ` [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino
2015-08-28 2:18 ` Daniel Kurtz
2015-08-28 16:28 ` Javi Merino
2015-08-31 13:14 ` Daniel Kurtz [this message]
2015-08-26 13:26 ` [PATCH v4 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-09-02 15:11 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino
2015-09-07 13:19 ` [PATCH v5 " Javi Merino
2015-09-07 13:19 ` [PATCH v5 1/5] thermal: Add a function to get the minimum power Javi Merino
2015-09-07 13:19 ` [PATCH v5 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-09-07 13:19 ` [PATCH v5 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-09-07 13:19 ` [PATCH v5 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino
2015-09-07 13:19 ` [PATCH v5 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-09-08 1:46 ` [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone Daniel Kurtz
2015-09-14 13:23 ` [PATCH v6 " Javi Merino
2015-09-14 13:23 ` [PATCH v6 1/5] thermal: Add a function to get the minimum power Javi Merino
2015-09-14 13:23 ` [PATCH v6 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino
2015-09-14 13:23 ` [PATCH v6 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino
2015-09-14 13:23 ` [PATCH v6 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino
2015-09-14 13:23 ` [PATCH v6 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino
2015-09-14 3:04 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Eduardo Valentin
2015-09-14 13:32 ` Javi Merino
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGS+omDj0NR3pD-E+L6zQ9_mbCuXPQEBsMWEZXwKx-dor0fjFA@mail.gmail.com \
--to=djkurtz@chromium.org \
--cc=Punit.Agrawal@arm.com \
--cc=cywang@chromium.org \
--cc=dmitry.torokhov@gmail.com \
--cc=edubezval@gmail.com \
--cc=javi.merino@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.