From: Rafael J. Wysocki <[email protected]>
Make get_trip_level() use for_each_trip() to iterate over trip points
and make it call thermal_zone_trip_id() to obtain the integer ID of a
given trip point so as to avoid relying on the knowledge of struct
thermal_zone_device internals.
The general functionality is not expected to be changed.
This change causes the governor to use trip pointers instead of trip
indices everywhere except for the fair_share_throttle() second argument
that will be modified subsequently along with the definition of the
governor .throttle() callback.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_fair_share.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -15,29 +15,27 @@
#include "thermal_core.h"
-/**
- * get_trip_level: - obtains the current trip level for a zone
- * @tz: thermal zone device
- */
static int get_trip_level(struct thermal_zone_device *tz)
{
- struct thermal_trip trip;
- int count;
+ const struct thermal_trip *trip, *level_trip = NULL;
+ int trip_level;
- for (count = 0; count < tz->num_trips; count++) {
- __thermal_zone_get_trip(tz, count, &trip);
- if (tz->temperature < trip.temperature)
+ for_each_trip(tz, trip) {
+ if (level_trip && trip->temperature >= tz->temperature)
break;
+
+ level_trip = trip;
}
- /*
- * count > 0 only if temperature is greater than first trip
- * point, in which case, trip_point = count - 1
- */
- if (count > 0)
- trace_thermal_zone_trip(tz, count - 1, trip.type);
+ /* Bail out if the temperature is not greater than any trips. */
+ if (level_trip->temperature >= tz->temperature)
+ return 0;
+
+ trip_level = thermal_zone_trip_id(tz, level_trip);
+
+ trace_thermal_zone_trip(tz, trip_level, level_trip->type);
- return count;
+ return trip_level;
}
static long get_target_state(struct thermal_zone_device *tz,
On 06/10/2023 19:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Make get_trip_level() use for_each_trip() to iterate over trip points
> and make it call thermal_zone_trip_id() to obtain the integer ID of a
> given trip point so as to avoid relying on the knowledge of struct
> thermal_zone_device internals.
>
> The general functionality is not expected to be changed.
>
> This change causes the governor to use trip pointers instead of trip
> indices everywhere except for the fair_share_throttle() second argument
> that will be modified subsequently along with the definition of the
> governor .throttle() callback.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/gov_fair_share.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -15,29 +15,27 @@
>
> #include "thermal_core.h"
>
> -/**
> - * get_trip_level: - obtains the current trip level for a zone
> - * @tz: thermal zone device
> - */
> static int get_trip_level(struct thermal_zone_device *tz)
> {
> - struct thermal_trip trip;
> - int count;
> + const struct thermal_trip *trip, *level_trip = NULL;
> + int trip_level;
>
> - for (count = 0; count < tz->num_trips; count++) {
> - __thermal_zone_get_trip(tz, count, &trip);
> - if (tz->temperature < trip.temperature)
> + for_each_trip(tz, trip) {
> + if (level_trip && trip->temperature >= tz->temperature)
> break;
Even if very likely the trip points are ordered by the hardware
enumeration, strictly we don't have yet the guarantee the trips are
ordered (as that is the final goal to correctly detect thresholds
crossing with the generic trip). We should go through all the trip
points, no?
> + level_trip = trip;
> }
>
> - /*
> - * count > 0 only if temperature is greater than first trip
> - * point, in which case, trip_point = count - 1
> - */
> - if (count > 0)
> - trace_thermal_zone_trip(tz, count - 1, trip.type);
> + /* Bail out if the temperature is not greater than any trips. */
> + if (level_trip->temperature >= tz->temperature)
> + return 0;
Isn't simpler to remove the test level_trip != NULL in the loop and then
check here if it is NULL and then return 0.
> + trip_level = thermal_zone_trip_id(tz, level_trip);
> +
> + trace_thermal_zone_trip(tz, trip_level, level_trip->type);
>
> - return count;
> + return trip_level;
> }
>
> static long get_target_state(struct thermal_zone_device *tz,
>
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Thu, Oct 12, 2023 at 5:04 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 06/10/2023 19:42, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Make get_trip_level() use for_each_trip() to iterate over trip points
> > and make it call thermal_zone_trip_id() to obtain the integer ID of a
> > given trip point so as to avoid relying on the knowledge of struct
> > thermal_zone_device internals.
> >
> > The general functionality is not expected to be changed.
> >
> > This change causes the governor to use trip pointers instead of trip
> > indices everywhere except for the fair_share_throttle() second argument
> > that will be modified subsequently along with the definition of the
> > governor .throttle() callback.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/thermal/gov_fair_share.c | 30 ++++++++++++++----------------
> > 1 file changed, 14 insertions(+), 16 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/gov_fair_share.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> > +++ linux-pm/drivers/thermal/gov_fair_share.c
> > @@ -15,29 +15,27 @@
> >
> > #include "thermal_core.h"
> >
> > -/**
> > - * get_trip_level: - obtains the current trip level for a zone
> > - * @tz: thermal zone device
> > - */
> > static int get_trip_level(struct thermal_zone_device *tz)
> > {
> > - struct thermal_trip trip;
> > - int count;
> > + const struct thermal_trip *trip, *level_trip = NULL;
> > + int trip_level;
> >
> > - for (count = 0; count < tz->num_trips; count++) {
> > - __thermal_zone_get_trip(tz, count, &trip);
> > - if (tz->temperature < trip.temperature)
> > + for_each_trip(tz, trip) {
> > + if (level_trip && trip->temperature >= tz->temperature)
> > break;
>
> Even if very likely the trip points are ordered by the hardware
> enumeration, strictly we don't have yet the guarantee the trips are
> ordered (as that is the final goal to correctly detect thresholds
> crossing with the generic trip). We should go through all the trip
> points, no?
Well, I just retained the existing logic, because changing it is not
the purpose of this patch.
Such a change can certainly be considered, but not in this patch and
not in this patch series.
> > + level_trip = trip;
> > }
> >
> > - /*
> > - * count > 0 only if temperature is greater than first trip
> > - * point, in which case, trip_point = count - 1
> > - */
> > - if (count > 0)
> > - trace_thermal_zone_trip(tz, count - 1, trip.type);
> > + /* Bail out if the temperature is not greater than any trips. */
> > + if (level_trip->temperature >= tz->temperature)
> > + return 0;
>
> Isn't simpler to remove the test level_trip != NULL in the loop and then
> check here if it is NULL and then return 0.
Yes, good point.
> > + trip_level = thermal_zone_trip_id(tz, level_trip);
> > +
> > + trace_thermal_zone_trip(tz, trip_level, level_trip->type);
> >
> > - return count;
> > + return trip_level;
> > }
> >
> > static long get_target_state(struct thermal_zone_device *tz,
> >
> >
> >
>
> --