| 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: <777f7688-34d1-4b1e-9187-6f605fb8e80c@notapiano>
Date: Tue, 22 Aug 2023 09:52:09 -0400
From: Nícolas F. R. A. Prado
<nfraprado@...labora.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, kernel@...labora.com,
Amit Kucheria <amitk@...nel.org>,
Zhang Rui <rui.zhang@...el.com>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH] thermal/core: Don't update trip points inside the
hysteresis range
On Tue, Aug 22, 2023 at 12:57:59PM +0200, Rafael J. Wysocki wrote:
> On Tue, Aug 22, 2023 at 12:25 AM Nícolas F. R. A. Prado
> <nfraprado@...labora.com> wrote:
> >
> > On Mon, Aug 21, 2023 at 11:10:27PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jul 3, 2023 at 7:15 PM Nícolas F. R. A. Prado
> > > <nfraprado@...labora.com> wrote:
> > > >
> > > > When searching for the trip points that need to be set, the nearest trip
> > > > point's temperature is used for the high trip, while the nearest trip
> > > > point's temperature minus the hysteresis is used for the low trip. The
> > > > issue with this logic is that when the current temperature is inside a
> > > > trip point's hysteresis range, both high and low trips will come from
> > > > the same trip point. As a consequence instability can still occur like
> > > > this:
> > > > * the temperature rises slightly and enters the hysteresis range of a
> > > > trip point
> > > > * polling happens and updates the trip points to the hysteresis range
> > > > * the temperature falls slightly, exiting the hysteresis range, crossing
> > > > the trip point and triggering an IRQ, the trip points are updated
> > > > * repeat
> > > >
> > > > So even though the current hysteresis implementation prevents
> > > > instability from happening due to IRQs triggering on the same
> > > > temperature value, both ways, it doesn't prevent it from happening due
> > > > to an IRQ on one way and polling on the other.
> > > >
> > > > To properly implement a hysteresis behavior, when inside the hysteresis
> > > > range, don't update the trip points. This way, the previously set trip
> > > > points will stay in effect, which will in a way remember the previous
> > > > state (if the temperature signal came from above or below the range) and
> > > > therefore have the right trip point already set. The exception is if
> > > > there was no previous trip point set, in which case a previous state
> > > > doesn't exist, and so it's sensible to allow the hysteresis range as
> > > > trip points.
> > > >
> > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > > >
> > > > ---
> > > >
> > > > drivers/thermal/thermal_trip.c | 21 +++++++++++++++++++--
> > > > 1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > > > index 907f3a4d7bc8..c386ac5d8bad 100644
> > > > --- a/drivers/thermal/thermal_trip.c
> > > > +++ b/drivers/thermal/thermal_trip.c
> > > > @@ -57,6 +57,7 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> > > > {
> > > > struct thermal_trip trip;
> > > > int low = -INT_MAX, high = INT_MAX;
> > > > + int low_trip_id = -1, high_trip_id = -2;
> > > > int i, ret;
> > > >
> > > > lockdep_assert_held(&tz->lock);
> > > > @@ -73,18 +74,34 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> > > >
> > > > trip_low = trip.temperature - trip.hysteresis;
> > > >
> > > > - if (trip_low < tz->temperature && trip_low > low)
> > > > + if (trip_low < tz->temperature && trip_low > low) {
> > > > low = trip_low;
> > > > + low_trip_id = i;
> > > > + }
> > > >
> > >
> > > I think I get the idea, but wouldn't a similar effect be achieved by
> > > adding an "else" here?
> >
> > No. That would only fix the problem in one direction, namely, when the
> > temperature entered the hysteresis range from above. But when the temperature
> > entered the range from below, we'd need to check the high threshold first to
> > achieve the same result.
> >
> > The way I've implemented here is the simplest I could think of that works for
> > both directions.
>
> Well, what about the replacement patch below (untested)?
Yes, the patch below should work just as well as the one I've proposed. Do you
want me to send a v2 with this code, or do you want to send this patch yourself?
I'll also give it some testing.
Thanks,
Nícolas
>
> ---
> drivers/thermal/thermal_trip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -55,6 +55,7 @@ void __thermal_zone_set_trips(struct the
> {
> struct thermal_trip trip;
> int low = -INT_MAX, high = INT_MAX;
> + bool same_trip = false;
> int i, ret;
>
> lockdep_assert_held(&tz->lock);
> @@ -63,6 +64,7 @@ void __thermal_zone_set_trips(struct the
> return;
>
> for (i = 0; i < tz->num_trips; i++) {
> + bool low_set = false;
> int trip_low;
>
> ret = __thermal_zone_get_trip(tz, i , &trip);
> @@ -71,18 +73,31 @@ void __thermal_zone_set_trips(struct the
>
> trip_low = trip.temperature - trip.hysteresis;
>
> - if (trip_low < tz->temperature && trip_low > low)
> + if (trip_low < tz->temperature && trip_low > low) {
> low = trip_low;
> + low_set = true;
> + same_trip = false;
> + }
>
> if (trip.temperature > tz->temperature &&
> - trip.temperature < high)
> + trip.temperature < high) {
> high = trip.temperature;
> + same_trip = low_set;
> + }
> }
>
> /* No need to change trip points */
> if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> return;
>
> + /*
> + * If "high" and "low" are the same, skip the change unless this is the
> + * first time.
> + */
> + if (same_trip && (tz->prev_low_trip != -INT_MAX ||
> + tz->prev_high_trip != INT_MAX))
> + return;
> +
> tz->prev_low_trip = low;
> tz->prev_high_trip = high;
Powered by blists - more mailing lists