| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHYJL4rKE+2TiYLEYUE3VO7Ws+uPazD0Fd-9EqZ1pGB_eU2Lvw@mail.gmail.com>
Date: Wed, 21 Jun 2023 12:51:00 +0800
From: Di Shen <cindygm567@...il.com>
To: Lukasz Luba <lukasz.luba@....com>
Cc: Di Shen <di.shen@...soc.com>, amitk@...nel.org,
linux-pm@...r.kernel.org, rui.zhang@...el.com,
daniel.lezcano@...aro.org, linux-kernel@...r.kernel.org,
xuewen.yan@...soc.com, jeson.gao@...soc.com, zhanglyra@...il.com,
orsonzhai@...il.com, rafael@...nel.org
Subject: Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor
when trip point is changed
On Tue, Jun 20, 2023 at 6:39 PM Lukasz Luba <lukasz.luba@....com> wrote:
>
> Hi Di,
>
> I have missed your v4 because it landed below your v3 thread.
>
> On 6/19/23 07:35, Di Shen wrote:
> > When the thermal trip point is changed, the governor should
> > be reset so that the policy algorithm be updated to adapt to the
> > new trip point.
> >
> > This patch adds an ops for thermal the governor structure to reset
>
> s/ops/callback
>
> > the governor. The ops is called when the trip point is changed.
> > For power allocator, the parameters of pid controller and the states
> > of power cooling devices can be reset when the passive trip point
> > is changed.
> >
> > Signed-off-by: Di Shen <di.shen@...soc.com>
> >
> > ---
> > V4:
> > - Compared to V3, handle it in thermal core instead of in governor.
> >
> > - Add an ops to the governor structure, and call it when a trip
> > point is changed.
> >
> > - Define reset ops for power allocator.
> >
> > V3:
> > - Add fix tag.
> >
> > V2:
> > - Compared to v1, do not revert.
> >
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> > to record the last switch_on_temp value.
> >
> > - Adds a function to renew the update flag and update the
> > last_switch_on_temp when thermal trips are writable.
> >
> > V1:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> > drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> > drivers/thermal/thermal_trip.c | 6 ++++++
> > include/linux/thermal.h | 1 +
> > 3 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 8642f1096b91..41d155adc616 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> > return allocate_power(tz, trip.temperature);
> > }
> >
> > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > +{
> > + int ret = 0;
> > + struct thermal_trip trip;
> > + struct power_allocator_params *params = tz->governor_data;
> > +
> > + ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > + if (ret)
> > + return ret;
> > +
> > + /* Only need reset for passive trips */
> > + if (trip.type != THERMAL_TRIP_PASSIVE)
> > + return -EINVAL;
> > +
> > + reset_pid_controller(params);
> > + allow_maximum_power(tz, true);
> > +
> > + return ret;
> > +}
> > +
> > static struct thermal_governor thermal_gov_power_allocator = {
> > .name = "power_allocator",
> > .bind_to_tz = power_allocator_bind,
> > .unbind_from_tz = power_allocator_unbind,
> > .throttle = power_allocator_throttle,
> > + .reset = power_allocator_reset,
> > };
> > THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > index 907f3a4d7bc8..52eb768fada8 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> > tz->trips[trip_id] = *trip;
> >
> > + if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > + ret = tz->governor->reset(tz, trip_id);
> > + if (ret)
> > + pr_warn_once("Failed to reset thermal governor\n");
> > + }
>
> I agree with Rafael. Maybe change that to debug print, so that can be
> checked during the product testing. We cannot do much if that happens.
>
Right.
> > +
> > thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> > trip->temperature, trip->hysteresis);
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 87837094d549..155ce2291fa5 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -204,6 +204,7 @@ struct thermal_governor {
> > int (*bind_to_tz)(struct thermal_zone_device *tz);
> > void (*unbind_from_tz)(struct thermal_zone_device *tz);
> > int (*throttle)(struct thermal_zone_device *tz, int trip);
> > + int (*reset)(struct thermal_zone_device *tz, int trip);
> > struct list_head governor_list;
> > };
> >
>
> That thermal_governor::reset() callback is what I had im mind while
> giving you the feedback for the v1. Now it's much cleaner what is going
> on and why.
>
Yes, it is necessary to do something for the governor if the trip point
is changed, especially for the governors that their trips are strongly
related to
the policy.
> Apart from some small bits, LGTM. Please adjust the comment in the patch
> header and this debug print and you can add:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@....com>
>
> Please send the next version as separate new thread.
>
> Regards,
> Lukasz
Thank you Lukasz !
I couldn't agree with you more about your comments. What you have said is what
I want to express.
I'd love to send the next version. Thanks again.
Best regards,
Di
Powered by blists - more mailing lists