[PATCH v1 0/6] thermal: core: Pass trip pointers to governor .throttle() callbacks

[PATCH v1 0/6] thermal: core: Pass trip pointers to governor .throttle() callbacks

[Rafael J. Wysocki]

Hi All,

While working on https://lore.kernel.org/linux-pm/4846448.GXAFRqVoOG@kreacher/
I started to change thermal governors so as to reduce the usage of trip
indices in them.  At that time, I was concerned that they could not be
replaced with trip pointers overall, because they were needed in certain
situations (tracing, debug messages, userspace governor) and computing them
whenever they were needed would be extra overhead with relatively weak
justification.  In the meantime, however, I realized that for a given trip
pointer, it is actually trivial to compute the corresponding index: it is
sufficient to subtract the start of the trips[] table in the thermal zone
containing the trip from that trip pointer for this purpose.  Patch [1/6]
modifies thermal_zone_trip_id() in accordance with this observation.

Now that the cost of computing a trip index for a given trip pointer and
thermal zone is not a concern any more, the governors can be generally
switched over to using trip pointers for representing trips.  One of the
things they need to do sometimes, though, is to iterate over trips in a
given thermal zone (the core needs to do that too, of course) and
for_each_thermal_trip() is somewhat inconvenient for this purpose, because
it requires callback functions to be defined and in some cases new data
types need to be introduced just in order to use it.  For this reason,
patch [2/6] adds a macro for iterating over trip points in a given thermal
zone with the help of a trip pointer and changes for_each_thermal_trip() to
use that macro internally.

Patches [3-5/6] change individual governors to prepare them for using trip
pointers everywhere for representing trips, except for the trip argument of
the .throttle() callback and patch [6/6] finally changes the .throttle()
callback definition so that it takes a trip pointer as the argument
representing the trip.

Please refer to the individual patch changelogs for details.

Thanks!

[PATCH v1 1/6] thermal: trip: Simplify computing trip indices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

A trip index can be computed right away as a difference between the
value of a trip pointer pointing to the given trip object and the
start of the trips[] table in the thermal zone containing the trip, so
change thermal_zone_trip_id() accordingly.

No intentional functional impact (except for some speedup).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_trip.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -175,14 +175,11 @@ int thermal_zone_set_trip(struct thermal
 int thermal_zone_trip_id(struct thermal_zone_device *tz,
 			 const struct thermal_trip *trip)
 {
-	int i;
-
 	lockdep_assert_held(&tz->lock);
 
-	for (i = 0; i < tz->num_trips; i++) {
-		if (&tz->trips[i] == trip)
-			return i;
-	}
-
-	return -ENODATA;
+	/*
+	 * Assume the trip to be located within the bounds of the thermal
+	 * zone's trips[] table.
+	 */
+	return trip - tz->trips;
 }

Re: [PATCH v1 1/6] thermal: trip: Simplify computing trip indices

[Daniel Lezcano]

On 06/10/2023 19:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> A trip index can be computed right away as a difference between the
> value of a trip pointer pointing to the given trip object and the
> start of the trips[] table in the thermal zone containing the trip, so
> change thermal_zone_trip_id() accordingly.
> 
> No intentional functional impact (except for some speedup).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_trip.c |   13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -175,14 +175,11 @@ int thermal_zone_set_trip(struct thermal
>   int thermal_zone_trip_id(struct thermal_zone_device *tz,
>   			 const struct thermal_trip *trip)
>   {
> -	int i;
> -
>   	lockdep_assert_held(&tz->lock);
>   
> -	for (i = 0; i < tz->num_trips; i++) {
> -		if (&tz->trips[i] == trip)
> -			return i;
> -	}
> -
> -	return -ENODATA;
> +	/*
> +	 * Assume the trip to be located within the bounds of the thermal
> +	 * zone's trips[] table.
> +	 */
> +	return trip - tz->trips;

Shouldn't be divided by sizeof(*trip) ?

>   }
> 
> 
> 

-- 
<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

Re: [PATCH v1 1/6] thermal: trip: Simplify computing trip indices

[Rafael J. Wysocki]

On Thu, Oct 12, 2023 at 4:27 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 06/10/2023 19:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > A trip index can be computed right away as a difference between the
> > value of a trip pointer pointing to the given trip object and the
> > start of the trips[] table in the thermal zone containing the trip, so
> > change thermal_zone_trip_id() accordingly.
> >
> > No intentional functional impact (except for some speedup).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_trip.c |   13 +++++--------
> >   1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -175,14 +175,11 @@ int thermal_zone_set_trip(struct thermal
> >   int thermal_zone_trip_id(struct thermal_zone_device *tz,
> >                        const struct thermal_trip *trip)
> >   {
> > -     int i;
> > -
> >       lockdep_assert_held(&tz->lock);
> >
> > -     for (i = 0; i < tz->num_trips; i++) {
> > -             if (&tz->trips[i] == trip)
> > -                     return i;
> > -     }
> > -
> > -     return -ENODATA;
> > +     /*
> > +      * Assume the trip to be located within the bounds of the thermal
> > +      * zone's trips[] table.
> > +      */
> > +     return trip - tz->trips;
>
> Shouldn't be divided by sizeof(*trip) ?

No, it's in sizeof(*trip) units already.

Re: [PATCH v1 1/6] thermal: trip: Simplify computing trip indices

[Lukasz Luba]

On 10/6/23 18:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> A trip index can be computed right away as a difference between the
> value of a trip pointer pointing to the given trip object and the
> start of the trips[] table in the thermal zone containing the trip, so
> change thermal_zone_trip_id() accordingly.
> 
> No intentional functional impact (except for some speedup).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_trip.c |   13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -175,14 +175,11 @@ int thermal_zone_set_trip(struct thermal
>   int thermal_zone_trip_id(struct thermal_zone_device *tz,
>   			 const struct thermal_trip *trip)
>   {
> -	int i;
> -
>   	lockdep_assert_held(&tz->lock);
>   
> -	for (i = 0; i < tz->num_trips; i++) {
> -		if (&tz->trips[i] == trip)
> -			return i;
> -	}
> -
> -	return -ENODATA;
> +	/*
> +	 * Assume the trip to be located within the bounds of the thermal
> +	 * zone's trips[] table.
> +	 */
> +	return trip - tz->trips;
>   }
> 
> 
> 

I agree wit hthe comment, we should be safe here, since we control that
array.

I could be a bit picky about this 'int' return in that function on
64bit kernels, were we have also ptrdiff_t set to long IIRC. But this
particular usage should be handled properly in all our cases, so:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 1/6] thermal: trip: Simplify computing trip indices

[Rafael J. Wysocki]

On Fri, Oct 20, 2023 at 6:58 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/6/23 18:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > A trip index can be computed right away as a difference between the
> > value of a trip pointer pointing to the given trip object and the
> > start of the trips[] table in the thermal zone containing the trip, so
> > change thermal_zone_trip_id() accordingly.
> >
> > No intentional functional impact (except for some speedup).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_trip.c |   13 +++++--------
> >   1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -175,14 +175,11 @@ int thermal_zone_set_trip(struct thermal
> >   int thermal_zone_trip_id(struct thermal_zone_device *tz,
> >                        const struct thermal_trip *trip)
> >   {
> > -     int i;
> > -
> >       lockdep_assert_held(&tz->lock);
> >
> > -     for (i = 0; i < tz->num_trips; i++) {
> > -             if (&tz->trips[i] == trip)
> > -                     return i;
> > -     }
> > -
> > -     return -ENODATA;
> > +     /*
> > +      * Assume the trip to be located within the bounds of the thermal
> > +      * zone's trips[] table.
> > +      */
> > +     return trip - tz->trips;
> >   }
> >
> >
> >
>
> I agree wit hthe comment, we should be safe here, since we control that
> array.
>
> I could be a bit picky about this 'int' return in that function on
> 64bit kernels, were we have also ptrdiff_t set to long IIRC. But this
> particular usage should be handled properly in all our cases, so:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks!

[PATCH v1 2/6] thermal: trip: Define for_each_trip() macro

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Define a new macro for_each_trip() to be used by the thermal core code
and thermal governors for walking trips in a given thermal zone.

Modify for_each_thermal_trip() to use this macro instead of an open-
coded loop over trips.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.h |    3 +++
 drivers/thermal/thermal_trip.c |    7 ++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -116,6 +116,9 @@ void __thermal_zone_device_update(struct
 				  enum thermal_notify_event event);
 
 /* Helpers */
+#define for_each_trip(__tz, __trip)	\
+	for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
+
 void __thermal_zone_set_trips(struct thermal_zone_device *tz);
 int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			    struct thermal_trip *trip);
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -13,12 +13,13 @@ int for_each_thermal_trip(struct thermal
 			  int (*cb)(struct thermal_trip *, void *),
 			  void *data)
 {
-	int i, ret;
+	struct thermal_trip *trip;
+	int ret;
 
 	lockdep_assert_held(&tz->lock);
 
-	for (i = 0; i < tz->num_trips; i++) {
-		ret = cb(&tz->trips[i], data);
+	for_each_trip(tz, trip) {
+		ret = cb(trip, data);
 		if (ret)
 			return ret;
 	}

Re: [PATCH v1 2/6] thermal: trip: Define for_each_trip() macro

[Daniel Lezcano]

On 06/10/2023 19:41, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Define a new macro for_each_trip() to be used by the thermal core code
> and thermal governors for walking trips in a given thermal zone.
> 
> Modify for_each_thermal_trip() to use this macro instead of an open-
> coded loop over trips.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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

Re: [PATCH v1 2/6] thermal: trip: Define for_each_trip() macro

[Lukasz Luba]

On 10/6/23 18:41, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Define a new macro for_each_trip() to be used by the thermal core code
> and thermal governors for walking trips in a given thermal zone.
> 
> Modify for_each_thermal_trip() to use this macro instead of an open-
> coded loop over trips.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.h |    3 +++
>   drivers/thermal/thermal_trip.c |    7 ++++---
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -116,6 +116,9 @@ void __thermal_zone_device_update(struct
>   				  enum thermal_notify_event event);
>   
>   /* Helpers */
> +#define for_each_trip(__tz, __trip)	\
> +	for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
> +
>   void __thermal_zone_set_trips(struct thermal_zone_device *tz);
>   int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>   			    struct thermal_trip *trip);
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -13,12 +13,13 @@ int for_each_thermal_trip(struct thermal
>   			  int (*cb)(struct thermal_trip *, void *),
>   			  void *data)
>   {
> -	int i, ret;
> +	struct thermal_trip *trip;
> +	int ret;
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	for (i = 0; i < tz->num_trips; i++) {
> -		ret = cb(&tz->trips[i], data);
> +	for_each_trip(tz, trip) {
> +		ret = cb(trip, data);
>   		if (ret)
>   			return ret;
>   	}
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 2/6] thermal: trip: Define for_each_trip() macro

[Rafael J. Wysocki]

On Fri, Oct 20, 2023 at 7:14 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/6/23 18:41, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Define a new macro for_each_trip() to be used by the thermal core code
> > and thermal governors for walking trips in a given thermal zone.
> >
> > Modify for_each_thermal_trip() to use this macro instead of an open-
> > coded loop over trips.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_core.h |    3 +++
> >   drivers/thermal/thermal_trip.c |    7 ++++---
> >   2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.h
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > +++ linux-pm/drivers/thermal/thermal_core.h
> > @@ -116,6 +116,9 @@ void __thermal_zone_device_update(struct
> >                                 enum thermal_notify_event event);
> >
> >   /* Helpers */
> > +#define for_each_trip(__tz, __trip)  \
> > +     for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
> > +
> >   void __thermal_zone_set_trips(struct thermal_zone_device *tz);
> >   int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
> >                           struct thermal_trip *trip);
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -13,12 +13,13 @@ int for_each_thermal_trip(struct thermal
> >                         int (*cb)(struct thermal_trip *, void *),
> >                         void *data)
> >   {
> > -     int i, ret;
> > +     struct thermal_trip *trip;
> > +     int ret;
> >
> >       lockdep_assert_held(&tz->lock);
> >
> > -     for (i = 0; i < tz->num_trips; i++) {
> > -             ret = cb(&tz->trips[i], data);
> > +     for_each_trip(tz, trip) {
> > +             ret = cb(trip, data);
> >               if (ret)
> >                       return ret;
> >       }
> >
> >
> >
>
> LGTM
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks!

[PATCH v1 3/6] thermal: gov_fair_share: Rearrange get_trip_level()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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,

Re: [PATCH v1 3/6] thermal: gov_fair_share: Rearrange get_trip_level()

[Daniel Lezcano]

On 06/10/2023 19:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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 <rafael.j.wysocki@intel.com>
> ---
>   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

Re: [PATCH v1 3/6] thermal: gov_fair_share: Rearrange get_trip_level()

[Rafael J. Wysocki]

On Thu, Oct 12, 2023 at 5:04 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 06/10/2023 19:42, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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 <rafael.j.wysocki@intel.com>
> > ---
> >   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,
> >
> >
> >
>
> --

[PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers instead of trip indices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the power allocator thermal governor to use trip pointers instead
of trip indices everywhere except for the power_allocator_throttle()
second argument that will be changed subsequently along with the
definition of the .throttle() governor callback.

The general functionality is not expected to be changed.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
 1 file changed, 49 insertions(+), 74 deletions(-)

Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -16,8 +16,6 @@
 
 #include "thermal_core.h"
 
-#define INVALID_TRIP -1
-
 #define FRAC_BITS 10
 #define int_to_frac(x) ((x) << FRAC_BITS)
 #define frac_to_int(x) ((x) >> FRAC_BITS)
@@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
  * @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.
+ * @sustainable_power:	Sustainable power (heat) that this thermal zone can
+ *			dissipate
  * @trip_switch_on:	first passive trip point of the thermal zone.  The
  *			governor switches on when this trip point is crossed.
  *			If the thermal zone only has one passive trip point,
- *			@trip_switch_on should be INVALID_TRIP.
+ *			@trip_switch_on should be NULL.
  * @trip_max_desired_temperature:	last passive trip point of the thermal
  *					zone.  The temperature we are
  *					controlling for.
- * @sustainable_power:	Sustainable power (heat) that this thermal zone can
- *			dissipate
  */
 struct power_allocator_params {
 	bool allocated_tzp;
 	s64 err_integral;
 	s32 prev_err;
-	int trip_switch_on;
-	int trip_max_desired_temperature;
 	u32 sustainable_power;
+	const struct thermal_trip *trip_switch_on;
+	const struct thermal_trip *trip_max_desired_temperature;
 };
 
 /**
@@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
 	u32 sustainable_power = 0;
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
-	const struct thermal_trip *trip_max_desired_temperature =
-			&tz->trips[params->trip_max_desired_temperature];
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		struct thermal_cooling_device *cdev = instance->cdev;
 		u32 min_power;
 
-		if (instance->trip != trip_max_desired_temperature)
+		if (instance->trip != params->trip_max_desired_temperature)
 			continue;
 
 		if (!cdev_is_power_actor(cdev))
@@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
  * estimate_pid_constants() - Estimate the constants for the PID controller
  * @tz:		thermal zone for which to estimate the constants
  * @sustainable_power:	sustainable power for the thermal zone
- * @trip_switch_on:	trip point number for the switch on temperature
+ * @trip_switch_on:	trip point for the switch on temperature
  * @control_temp:	target temperature for the power allocator governor
  *
  * This function is used to update the estimation of the PID
  * controller constants in struct thermal_zone_parameters.
  */
 static void estimate_pid_constants(struct thermal_zone_device *tz,
-				   u32 sustainable_power, int trip_switch_on,
+				   u32 sustainable_power,
+				   const struct thermal_trip *trip_switch_on,
 				   int control_temp)
 {
-	struct thermal_trip trip;
 	u32 temperature_threshold = control_temp;
 	int ret;
 	s32 k_i;
 
-	ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
-	if (!ret)
-		temperature_threshold -= trip.temperature;
+	if (trip_switch_on)
+		temperature_threshold -= trip_switch_on->temperature;
 
 	/*
 	 * estimate_pid_constants() tries to find appropriate default
@@ -386,7 +381,7 @@ static int allocate_power(struct thermal
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
 	const struct thermal_trip *trip_max_desired_temperature =
-			&tz->trips[params->trip_max_desired_temperature];
+					params->trip_max_desired_temperature;
 	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
 	u32 *weighted_req_power;
 	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
@@ -496,7 +491,7 @@ static int allocate_power(struct thermal
 }
 
 /**
- * get_governor_trips() - get the number of the two trip points that are key for this governor
+ * get_governor_trips() - get the two trip points that are key for this governor
  * @tz:	thermal zone to operate on
  * @params:	pointer to private data for this governor
  *
@@ -513,46 +508,36 @@ static int allocate_power(struct thermal
 static void get_governor_trips(struct thermal_zone_device *tz,
 			       struct power_allocator_params *params)
 {
-	int i, last_active, last_passive;
-	bool found_first_passive;
-
-	found_first_passive = false;
-	last_active = INVALID_TRIP;
-	last_passive = INVALID_TRIP;
-
-	for (i = 0; i < tz->num_trips; i++) {
-		struct thermal_trip trip;
-		int ret;
-
-		ret = __thermal_zone_get_trip(tz, i, &trip);
-		if (ret) {
-			dev_warn(&tz->device,
-				 "Failed to get trip point %d type: %d\n", i,
-				 ret);
-			continue;
-		}
-
-		if (trip.type == THERMAL_TRIP_PASSIVE) {
-			if (!found_first_passive) {
-				params->trip_switch_on = i;
-				found_first_passive = true;
-			} else  {
-				last_passive = i;
+	const struct thermal_trip *first_passive = NULL;
+	const struct thermal_trip *last_passive = NULL;
+	const struct thermal_trip *last_active = NULL;
+	const struct thermal_trip *trip;
+
+	for_each_trip(tz, trip) {
+		switch (trip->type) {
+		case THERMAL_TRIP_PASSIVE:
+			if (!first_passive) {
+				first_passive = trip;
+				break;
 			}
-		} else if (trip.type == THERMAL_TRIP_ACTIVE) {
-			last_active = i;
-		} else {
+			last_passive = trip;
+			break;
+		case THERMAL_TRIP_ACTIVE:
+			last_active = trip;
+			break;
+		default:
 			break;
 		}
 	}
 
-	if (last_passive != INVALID_TRIP) {
+	if (last_passive) {
+		params->trip_switch_on = first_passive;
 		params->trip_max_desired_temperature = last_passive;
-	} else if (found_first_passive) {
-		params->trip_max_desired_temperature = params->trip_switch_on;
-		params->trip_switch_on = INVALID_TRIP;
+	} else if (first_passive) {
+		params->trip_switch_on = NULL;
+		params->trip_max_desired_temperature = first_passive;
 	} else {
-		params->trip_switch_on = INVALID_TRIP;
+		params->trip_switch_on = NULL;
 		params->trip_max_desired_temperature = last_active;
 	}
 }
@@ -567,14 +552,12 @@ static void allow_maximum_power(struct t
 {
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
-	const struct thermal_trip *trip_max_desired_temperature =
-			&tz->trips[params->trip_max_desired_temperature];
 	u32 req_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		struct thermal_cooling_device *cdev = instance->cdev;
 
-		if ((instance->trip != trip_max_desired_temperature) ||
+		if (instance->trip != params->trip_max_desired_temperature ||
 		    (!cdev_is_power_actor(instance->cdev)))
 			continue;
 
@@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
 {
 	int ret;
 	struct power_allocator_params *params;
-	struct thermal_trip trip;
 
 	ret = check_power_actors(tz);
 	if (ret)
@@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
 	get_governor_trips(tz, params);
 
 	if (tz->num_trips > 0) {
-		ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
-					      &trip);
-		if (!ret)
+		const struct thermal_trip *trip;
+
+		trip = params->trip_max_desired_temperature;
+		if (trip)
 			estimate_pid_constants(tz, tz->tzp->sustainable_power,
 					       params->trip_switch_on,
-					       trip.temperature);
+					       trip->temperature);
 	}
 
 	reset_pid_controller(params);
@@ -697,11 +680,10 @@ static void power_allocator_unbind(struc
 	tz->governor_data = NULL;
 }
 
-static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
+static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
 {
 	struct power_allocator_params *params = tz->governor_data;
-	struct thermal_trip trip;
-	int ret;
+	const struct thermal_trip *trip = &tz->trips[trip_index];
 	bool update;
 
 	lockdep_assert_held(&tz->lock);
@@ -710,12 +692,12 @@ static int power_allocator_throttle(stru
 	 * We get called for every trip point but we only need to do
 	 * our calculations once
 	 */
-	if (trip_id != params->trip_max_desired_temperature)
+	if (trip != params->trip_max_desired_temperature)
 		return 0;
 
-	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
-	if (!ret && (tz->temperature < trip.temperature)) {
-		update = (tz->last_temperature >= trip.temperature);
+	trip = params->trip_switch_on;
+	if (trip && tz->temperature < trip->temperature) {
+		update = tz->last_temperature >= trip->temperature;
 		tz->passive = 0;
 		reset_pid_controller(params);
 		allow_maximum_power(tz, update);
@@ -724,14 +706,7 @@ static int power_allocator_throttle(stru
 
 	tz->passive = 1;
 
-	ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
-	if (ret) {
-		dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
-			 ret);
-		return ret;
-	}
-
-	return allocate_power(tz, trip.temperature);
+	return allocate_power(tz, params->trip_max_desired_temperature->temperature);
 }
 
 static struct thermal_governor thermal_gov_power_allocator = {

Re: [PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers instead of trip indices

[Daniel Lezcano]

On 06/10/2023 19:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify the power allocator thermal governor to use trip pointers instead
> of trip indices everywhere except for the power_allocator_throttle()
> second argument that will be changed subsequently along with the
> definition of the .throttle() governor callback.
> 
> The general functionality is not expected to be changed.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
>   1 file changed, 49 insertions(+), 74 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -16,8 +16,6 @@
>   
>   #include "thermal_core.h"
>   
> -#define INVALID_TRIP -1
> -
>   #define FRAC_BITS 10
>   #define int_to_frac(x) ((x) << FRAC_BITS)
>   #define frac_to_int(x) ((x) >> FRAC_BITS)
> @@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
>    * @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.
> + * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> + *			dissipate
>    * @trip_switch_on:	first passive trip point of the thermal zone.  The
>    *			governor switches on when this trip point is crossed.
>    *			If the thermal zone only has one passive trip point,
> - *			@trip_switch_on should be INVALID_TRIP.
> + *			@trip_switch_on should be NULL.
>    * @trip_max_desired_temperature:	last passive trip point of the thermal
>    *					zone.  The temperature we are
>    *					controlling for.
> - * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> - *			dissipate
>    */
>   struct power_allocator_params {
>   	bool allocated_tzp;
>   	s64 err_integral;
>   	s32 prev_err;
> -	int trip_switch_on;
> -	int trip_max_desired_temperature;
>   	u32 sustainable_power;
> +	const struct thermal_trip *trip_switch_on;
> +	const struct thermal_trip *trip_max_desired_temperature;
>   };
>   
>   /**
> @@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
>   	u32 sustainable_power = 0;
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
> -	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		struct thermal_cooling_device *cdev = instance->cdev;
>   		u32 min_power;
>   
> -		if (instance->trip != trip_max_desired_temperature)
> +		if (instance->trip != params->trip_max_desired_temperature)
>   			continue;
>   
>   		if (!cdev_is_power_actor(cdev))
> @@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
>    * estimate_pid_constants() - Estimate the constants for the PID controller
>    * @tz:		thermal zone for which to estimate the constants
>    * @sustainable_power:	sustainable power for the thermal zone
> - * @trip_switch_on:	trip point number for the switch on temperature
> + * @trip_switch_on:	trip point for the switch on temperature
>    * @control_temp:	target temperature for the power allocator governor
>    *
>    * This function is used to update the estimation of the PID
>    * controller constants in struct thermal_zone_parameters.
>    */
>   static void estimate_pid_constants(struct thermal_zone_device *tz,
> -				   u32 sustainable_power, int trip_switch_on,
> +				   u32 sustainable_power,
> +				   const struct thermal_trip *trip_switch_on,
>   				   int control_temp)
>   {
> -	struct thermal_trip trip;
>   	u32 temperature_threshold = control_temp;
>   	int ret;
>   	s32 k_i;
>   
> -	ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
> -	if (!ret)
> -		temperature_threshold -= trip.temperature;
> +	if (trip_switch_on)
> +		temperature_threshold -= trip_switch_on->temperature;
>   
>   	/*
>   	 * estimate_pid_constants() tries to find appropriate default
> @@ -386,7 +381,7 @@ static int allocate_power(struct thermal
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
> +					params->trip_max_desired_temperature;
>   	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
>   	u32 *weighted_req_power;
>   	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
> @@ -496,7 +491,7 @@ static int allocate_power(struct thermal
>   }
>   
>   /**
> - * get_governor_trips() - get the number of the two trip points that are key for this governor
> + * get_governor_trips() - get the two trip points that are key for this governor
>    * @tz:	thermal zone to operate on
>    * @params:	pointer to private data for this governor
>    *
> @@ -513,46 +508,36 @@ static int allocate_power(struct thermal
>   static void get_governor_trips(struct thermal_zone_device *tz,
>   			       struct power_allocator_params *params)
>   {
> -	int i, last_active, last_passive;
> -	bool found_first_passive;
> -
> -	found_first_passive = false;
> -	last_active = INVALID_TRIP;
> -	last_passive = INVALID_TRIP;
> -
> -	for (i = 0; i < tz->num_trips; i++) {
> -		struct thermal_trip trip;
> -		int ret;
> -
> -		ret = __thermal_zone_get_trip(tz, i, &trip);
> -		if (ret) {
> -			dev_warn(&tz->device,
> -				 "Failed to get trip point %d type: %d\n", i,
> -				 ret);
> -			continue;
> -		}
> -
> -		if (trip.type == THERMAL_TRIP_PASSIVE) {
> -			if (!found_first_passive) {
> -				params->trip_switch_on = i;
> -				found_first_passive = true;
> -			} else  {
> -				last_passive = i;
> +	const struct thermal_trip *first_passive = NULL;
> +	const struct thermal_trip *last_passive = NULL;
> +	const struct thermal_trip *last_active = NULL;
> +	const struct thermal_trip *trip;
> +
> +	for_each_trip(tz, trip) {
> +		switch (trip->type) {
> +		case THERMAL_TRIP_PASSIVE:
> +			if (!first_passive) {
> +				first_passive = trip;
> +				break;
>   			}
> -		} else if (trip.type == THERMAL_TRIP_ACTIVE) {
> -			last_active = i;
> -		} else {
> +			last_passive = trip;
> +			break;
> +		case THERMAL_TRIP_ACTIVE:
> +			last_active = trip;
> +			break;
> +		default:
>   			break;
>   		}
>   	}
>   
> -	if (last_passive != INVALID_TRIP) {
> +	if (last_passive) {
> +		params->trip_switch_on = first_passive;
>   		params->trip_max_desired_temperature = last_passive;
> -	} else if (found_first_passive) {
> -		params->trip_max_desired_temperature = params->trip_switch_on;
> -		params->trip_switch_on = INVALID_TRIP;
> +	} else if (first_passive) {
> +		params->trip_switch_on = NULL;
> +		params->trip_max_desired_temperature = first_passive;
>   	} else {
> -		params->trip_switch_on = INVALID_TRIP;
> +		params->trip_switch_on = NULL;
>   		params->trip_max_desired_temperature = last_active;
>   	}
>   }
> @@ -567,14 +552,12 @@ static void allow_maximum_power(struct t
>   {
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
> -	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
>   	u32 req_power;
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		struct thermal_cooling_device *cdev = instance->cdev;
>   
> -		if ((instance->trip != trip_max_desired_temperature) ||
> +		if (instance->trip != params->trip_max_desired_temperature ||
>   		    (!cdev_is_power_actor(instance->cdev)))
>   			continue;
>   
> @@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
>   {
>   	int ret;
>   	struct power_allocator_params *params;
> -	struct thermal_trip trip;
>   
>   	ret = check_power_actors(tz);
>   	if (ret)
> @@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
>   	get_governor_trips(tz, params);
>   
>   	if (tz->num_trips > 0) {

Maybe this test can go away because if (trip) is true below, then 
logically (tz->num_trips > 0) ?

> -		ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> -					      &trip);
> -		if (!ret)
> +		const struct thermal_trip *trip;
> +
> +		trip = params->trip_max_desired_temperature;
> +		if (trip)
>   			estimate_pid_constants(tz, tz->tzp->sustainable_power,
>   					       params->trip_switch_on,
> -					       trip.temperature);
> +					       trip->temperature);
>   	}
>   
>   	reset_pid_controller(params);
> @@ -697,11 +680,10 @@ static void power_allocator_unbind(struc
>   	tz->governor_data = NULL;
>   }
>   
> -static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> +static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
>   {
>   	struct power_allocator_params *params = tz->governor_data;
> -	struct thermal_trip trip;
> -	int ret;
> +	const struct thermal_trip *trip = &tz->trips[trip_index];
>   	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
> @@ -710,12 +692,12 @@ static int power_allocator_throttle(stru
>   	 * We get called for every trip point but we only need to do
>   	 * our calculations once
>   	 */
> -	if (trip_id != params->trip_max_desired_temperature)
> +	if (trip != params->trip_max_desired_temperature)
>   		return 0;
>   
> -	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> -	if (!ret && (tz->temperature < trip.temperature)) {
> -		update = (tz->last_temperature >= trip.temperature);
> +	trip = params->trip_switch_on;
> +	if (trip && tz->temperature < trip->temperature) {
> +		update = tz->last_temperature >= trip->temperature;
>   		tz->passive = 0;
>   		reset_pid_controller(params);
>   		allow_maximum_power(tz, update);
> @@ -724,14 +706,7 @@ static int power_allocator_throttle(stru
>   
>   	tz->passive = 1;
>   
> -	ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
> -	if (ret) {
> -		dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
> -			 ret);
> -		return ret;
> -	}
> -
> -	return allocate_power(tz, trip.temperature);
> +	return allocate_power(tz, params->trip_max_desired_temperature->temperature);
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
> 

-- 
<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

Re: [PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers instead of trip indices

[Rafael J. Wysocki]

On Thu, Oct 12, 2023 at 5:19 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 06/10/2023 19:47, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify the power allocator thermal governor to use trip pointers instead
> > of trip indices everywhere except for the power_allocator_throttle()
> > second argument that will be changed subsequently along with the
> > definition of the .throttle() governor callback.
> >
> > The general functionality is not expected to be changed.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
> >   1 file changed, 49 insertions(+), 74 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/gov_power_allocator.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> > +++ linux-pm/drivers/thermal/gov_power_allocator.c
> > @@ -16,8 +16,6 @@
> >
> >   #include "thermal_core.h"
> >
> > -#define INVALID_TRIP -1
> > -
> >   #define FRAC_BITS 10
> >   #define int_to_frac(x) ((x) << FRAC_BITS)
> >   #define frac_to_int(x) ((x) >> FRAC_BITS)
> > @@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
> >    * @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.
> > + * @sustainable_power:       Sustainable power (heat) that this thermal zone can
> > + *                   dissipate
> >    * @trip_switch_on: first passive trip point of the thermal zone.  The
> >    *                  governor switches on when this trip point is crossed.
> >    *                  If the thermal zone only has one passive trip point,
> > - *                   @trip_switch_on should be INVALID_TRIP.
> > + *                   @trip_switch_on should be NULL.
> >    * @trip_max_desired_temperature:   last passive trip point of the thermal
> >    *                                  zone.  The temperature we are
> >    *                                  controlling for.
> > - * @sustainable_power:       Sustainable power (heat) that this thermal zone can
> > - *                   dissipate
> >    */
> >   struct power_allocator_params {
> >       bool allocated_tzp;
> >       s64 err_integral;
> >       s32 prev_err;
> > -     int trip_switch_on;
> > -     int trip_max_desired_temperature;
> >       u32 sustainable_power;
> > +     const struct thermal_trip *trip_switch_on;
> > +     const struct thermal_trip *trip_max_desired_temperature;
> >   };
> >
> >   /**
> > @@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
> >       u32 sustainable_power = 0;
> >       struct thermal_instance *instance;
> >       struct power_allocator_params *params = tz->governor_data;
> > -     const struct thermal_trip *trip_max_desired_temperature =
> > -                     &tz->trips[params->trip_max_desired_temperature];
> >
> >       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> >               struct thermal_cooling_device *cdev = instance->cdev;
> >               u32 min_power;
> >
> > -             if (instance->trip != trip_max_desired_temperature)
> > +             if (instance->trip != params->trip_max_desired_temperature)
> >                       continue;
> >
> >               if (!cdev_is_power_actor(cdev))
> > @@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
> >    * estimate_pid_constants() - Estimate the constants for the PID controller
> >    * @tz:             thermal zone for which to estimate the constants
> >    * @sustainable_power:      sustainable power for the thermal zone
> > - * @trip_switch_on:  trip point number for the switch on temperature
> > + * @trip_switch_on:  trip point for the switch on temperature
> >    * @control_temp:   target temperature for the power allocator governor
> >    *
> >    * This function is used to update the estimation of the PID
> >    * controller constants in struct thermal_zone_parameters.
> >    */
> >   static void estimate_pid_constants(struct thermal_zone_device *tz,
> > -                                u32 sustainable_power, int trip_switch_on,
> > +                                u32 sustainable_power,
> > +                                const struct thermal_trip *trip_switch_on,
> >                                  int control_temp)
> >   {
> > -     struct thermal_trip trip;
> >       u32 temperature_threshold = control_temp;
> >       int ret;
> >       s32 k_i;
> >
> > -     ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
> > -     if (!ret)
> > -             temperature_threshold -= trip.temperature;
> > +     if (trip_switch_on)
> > +             temperature_threshold -= trip_switch_on->temperature;
> >
> >       /*
> >        * estimate_pid_constants() tries to find appropriate default
> > @@ -386,7 +381,7 @@ static int allocate_power(struct thermal
> >       struct thermal_instance *instance;
> >       struct power_allocator_params *params = tz->governor_data;
> >       const struct thermal_trip *trip_max_desired_temperature =
> > -                     &tz->trips[params->trip_max_desired_temperature];
> > +                                     params->trip_max_desired_temperature;
> >       u32 *req_power, *max_power, *granted_power, *extra_actor_power;
> >       u32 *weighted_req_power;
> >       u32 total_req_power, max_allocatable_power, total_weighted_req_power;
> > @@ -496,7 +491,7 @@ static int allocate_power(struct thermal
> >   }
> >
> >   /**
> > - * get_governor_trips() - get the number of the two trip points that are key for this governor
> > + * get_governor_trips() - get the two trip points that are key for this governor
> >    * @tz:     thermal zone to operate on
> >    * @params: pointer to private data for this governor
> >    *
> > @@ -513,46 +508,36 @@ static int allocate_power(struct thermal
> >   static void get_governor_trips(struct thermal_zone_device *tz,
> >                              struct power_allocator_params *params)
> >   {
> > -     int i, last_active, last_passive;
> > -     bool found_first_passive;
> > -
> > -     found_first_passive = false;
> > -     last_active = INVALID_TRIP;
> > -     last_passive = INVALID_TRIP;
> > -
> > -     for (i = 0; i < tz->num_trips; i++) {
> > -             struct thermal_trip trip;
> > -             int ret;
> > -
> > -             ret = __thermal_zone_get_trip(tz, i, &trip);
> > -             if (ret) {
> > -                     dev_warn(&tz->device,
> > -                              "Failed to get trip point %d type: %d\n", i,
> > -                              ret);
> > -                     continue;
> > -             }
> > -
> > -             if (trip.type == THERMAL_TRIP_PASSIVE) {
> > -                     if (!found_first_passive) {
> > -                             params->trip_switch_on = i;
> > -                             found_first_passive = true;
> > -                     } else  {
> > -                             last_passive = i;
> > +     const struct thermal_trip *first_passive = NULL;
> > +     const struct thermal_trip *last_passive = NULL;
> > +     const struct thermal_trip *last_active = NULL;
> > +     const struct thermal_trip *trip;
> > +
> > +     for_each_trip(tz, trip) {
> > +             switch (trip->type) {
> > +             case THERMAL_TRIP_PASSIVE:
> > +                     if (!first_passive) {
> > +                             first_passive = trip;
> > +                             break;
> >                       }
> > -             } else if (trip.type == THERMAL_TRIP_ACTIVE) {
> > -                     last_active = i;
> > -             } else {
> > +                     last_passive = trip;
> > +                     break;
> > +             case THERMAL_TRIP_ACTIVE:
> > +                     last_active = trip;
> > +                     break;
> > +             default:
> >                       break;
> >               }
> >       }
> >
> > -     if (last_passive != INVALID_TRIP) {
> > +     if (last_passive) {
> > +             params->trip_switch_on = first_passive;
> >               params->trip_max_desired_temperature = last_passive;
> > -     } else if (found_first_passive) {
> > -             params->trip_max_desired_temperature = params->trip_switch_on;
> > -             params->trip_switch_on = INVALID_TRIP;
> > +     } else if (first_passive) {
> > +             params->trip_switch_on = NULL;
> > +             params->trip_max_desired_temperature = first_passive;
> >       } else {
> > -             params->trip_switch_on = INVALID_TRIP;
> > +             params->trip_switch_on = NULL;
> >               params->trip_max_desired_temperature = last_active;
> >       }
> >   }
> > @@ -567,14 +552,12 @@ static void allow_maximum_power(struct t
> >   {
> >       struct thermal_instance *instance;
> >       struct power_allocator_params *params = tz->governor_data;
> > -     const struct thermal_trip *trip_max_desired_temperature =
> > -                     &tz->trips[params->trip_max_desired_temperature];
> >       u32 req_power;
> >
> >       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> >               struct thermal_cooling_device *cdev = instance->cdev;
> >
> > -             if ((instance->trip != trip_max_desired_temperature) ||
> > +             if (instance->trip != params->trip_max_desired_temperature ||
> >                   (!cdev_is_power_actor(instance->cdev)))
> >                       continue;
> >
> > @@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
> >   {
> >       int ret;
> >       struct power_allocator_params *params;
> > -     struct thermal_trip trip;
> >
> >       ret = check_power_actors(tz);
> >       if (ret)
> > @@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
> >       get_governor_trips(tz, params);
> >
> >       if (tz->num_trips > 0) {
>
> Maybe this test can go away because if (trip) is true below, then
> logically (tz->num_trips > 0) ?

Yes, it can go away.

> > -             ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> > -                                           &trip);
> > -             if (!ret)
> > +             const struct thermal_trip *trip;
> > +
> > +             trip = params->trip_max_desired_temperature;
> > +             if (trip)
> >                       estimate_pid_constants(tz, tz->tzp->sustainable_power,
> >                                              params->trip_switch_on,
> > -                                            trip.temperature);
> > +                                            trip->temperature);
> >       }
> >
> >       reset_pid_controller(params);
> > @@ -697,11 +680,10 @@ static void power_allocator_unbind(struc
> >       tz->governor_data = NULL;
> >   }
> >
> > -static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> > +static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
> >   {
> >       struct power_allocator_params *params = tz->governor_data;
> > -     struct thermal_trip trip;
> > -     int ret;
> > +     const struct thermal_trip *trip = &tz->trips[trip_index];
> >       bool update;
> >
> >       lockdep_assert_held(&tz->lock);
> > @@ -710,12 +692,12 @@ static int power_allocator_throttle(stru
> >        * We get called for every trip point but we only need to do
> >        * our calculations once
> >        */
> > -     if (trip_id != params->trip_max_desired_temperature)
> > +     if (trip != params->trip_max_desired_temperature)
> >               return 0;
> >
> > -     ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> > -     if (!ret && (tz->temperature < trip.temperature)) {
> > -             update = (tz->last_temperature >= trip.temperature);
> > +     trip = params->trip_switch_on;
> > +     if (trip && tz->temperature < trip->temperature) {
> > +             update = tz->last_temperature >= trip->temperature;
> >               tz->passive = 0;
> >               reset_pid_controller(params);
> >               allow_maximum_power(tz, update);
> > @@ -724,14 +706,7 @@ static int power_allocator_throttle(stru
> >
> >       tz->passive = 1;
> >
> > -     ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
> > -     if (ret) {
> > -             dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
> > -                      ret);
> > -             return ret;
> > -     }
> > -
> > -     return allocate_power(tz, trip.temperature);
> > +     return allocate_power(tz, params->trip_max_desired_temperature->temperature);
> >   }
> >
> >   static struct thermal_governor thermal_gov_power_allocator = {
> >
> >
> >
>
> --
> <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
>

Re: [PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers instead of trip indices

[Lukasz Luba]

On 10/6/23 18:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify the power allocator thermal governor to use trip pointers instead
> of trip indices everywhere except for the power_allocator_throttle()
> second argument that will be changed subsequently along with the
> definition of the .throttle() governor callback.
> 
> The general functionality is not expected to be changed.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
>   1 file changed, 49 insertions(+), 74 deletions(-)
> 

[snip]

> @@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
>   {
>   	int ret;
>   	struct power_allocator_params *params;
> -	struct thermal_trip trip;
>   
>   	ret = check_power_actors(tz);
>   	if (ret)
> @@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
>   	get_governor_trips(tz, params);
>   
>   	if (tz->num_trips > 0) {
> -		ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> -					      &trip);
> -		if (!ret)
> +		const struct thermal_trip *trip;
> +
> +		trip = params->trip_max_desired_temperature;
> +		if (trip)
>   			estimate_pid_constants(tz, tz->tzp->sustainable_power,
>   					       params->trip_switch_on,
> -					       trip.temperature);
> +					       trip->temperature);
>   	}

The code check for the populated pointer (by earlier new
get_governor_trips(tz, params)) :

if (params->trip_max_desired_temperature) {
	int temp = params->trip_max_desired_temperature->temperature;

	estimate_pid_constants(...)
}

looks better (what you have figured out already).

Other than that the patch LGTM:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Tested-by: Lukasz Luba <lukasz.luba@arm.com>

I have also tested this is a few ways and it still works as expected:
1. Power allocation in different conditions
2. Trip points properly recognized with this nice get_governor_trips(),
   here is the test output, with many tricky trip point setups:

use case A:
5 trip points:
- 2 passive for 50, 60 degC
- 1 active 70degC
- 2 passive 70, 85 degC
- 1 critical 120 degC

expected IPA trip points: 50, 85 deg => 35 degC IPA operating range

[   24.578806] Power allocator: IPA: trip->temperature=50000
[   24.578824] Power allocator: IPA: passive trip->temperature=50000
[   24.578838] Power allocator: IPA: fist passive trip->temperature=50000
[   24.578851] Power allocator: IPA: trip->temperature=60000
[   24.578863] Power allocator: IPA: passive trip->temperature=60000
[   24.578875] Power allocator: IPA: trip->temperature=70000
[   24.578888] Power allocator: IPA: active trip->temperature=70000
[   24.578900] Power allocator: IPA: trip->temperature=120000
[   24.578912] Power allocator: IPA: trip->temperature=70000
[   24.578925] Power allocator: IPA: passive trip->temperature=70000
[   24.578937] Power allocator: IPA: trip->temperature=85000
[   24.578950] Power allocator: IPA: passive trip->temperature=85000
[   24.578964] Power allocator: IPA: trip_switch_on->temperature=50000 
control_temp=85000
[   24.578978] Power allocator: IPA: temperature_threshold=35000

-------------------------------------------------------------
use case B:
5 trip points:
- 2 active for 50, 60 degC
- 1 active 70degC
- 2 passive 70, 85 degC
- 1 critical 120 degC

expected IPA trip points: 70, 85 deg => 15 degC IPA operating range

[   27.402474] Power allocator: IPA: trip->temperature=50000
[   27.402492] Power allocator: IPA: active trip->temperature=50000
[   27.402505] Power allocator: IPA: trip->temperature=60000
[   27.402518] Power allocator: IPA: active trip->temperature=60000
[   27.402531] Power allocator: IPA: trip->temperature=70000
[   27.402544] Power allocator: IPA: active trip->temperature=70000
[   27.402557] Power allocator: IPA: trip->temperature=120000
[   27.402570] Power allocator: IPA: trip->temperature=70000
[   27.402582] Power allocator: IPA: passive trip->temperature=70000
[   27.402596] Power allocator: IPA: fist passive trip->temperature=70000
[   27.402608] Power allocator: IPA: trip->temperature=85000
[   27.402622] Power allocator: IPA: passive trip->temperature=85000
[   27.402635] Power allocator: IPA: trip_switch_on->temperature=70000 
control_temp=85000
[   27.402649] Power allocator: IPA: temperature_threshold=15000

--------------------------------------------------------
use case C:
6 trip points:
- 2 active for 50, 60 degC
- 1 active 70degC
- 3 passive 70, 85, 90 degC
- 1 critical 120 degC

expected IPA trip points: 50, 85 deg => 20 degC IPA operating range

[   36.998907] Power allocator: IPA: trip->temperature=50000
[   36.998921] Power allocator: IPA: active trip->temperature=50000
[   36.998935] Power allocator: IPA: trip->temperature=60000
[   36.998948] Power allocator: IPA: active trip->temperature=60000
[   36.998960] Power allocator: IPA: trip->temperature=70000
[   36.998973] Power allocator: IPA: active trip->temperature=70000
[   36.998985] Power allocator: IPA: trip->temperature=120000
[   36.998999] Power allocator: IPA: trip->temperature=70000
[   36.999011] Power allocator: IPA: passive trip->temperature=70000
[   36.999024] Power allocator: IPA: fist passive trip->temperature=70000
[   36.999037] Power allocator: IPA: trip->temperature=85000
[   36.999049] Power allocator: IPA: passive trip->temperature=85000
[   36.999062] Power allocator: IPA: trip->temperature=90000
[   36.999074] Power allocator: IPA: passive trip->temperature=90000
[   36.999087] Power allocator: IPA: trip_switch_on->temperature=70000 
control_temp=90000
[   36.999101] Power allocator: IPA: temperature_threshold=20000

Re: [PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers instead of trip indices

[Rafael J. Wysocki]

On Fri, Oct 20, 2023 at 6:36 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/6/23 18:47, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify the power allocator thermal governor to use trip pointers instead
> > of trip indices everywhere except for the power_allocator_throttle()
> > second argument that will be changed subsequently along with the
> > definition of the .throttle() governor callback.
> >
> > The general functionality is not expected to be changed.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
> >   1 file changed, 49 insertions(+), 74 deletions(-)
> >
>
> [snip]
>
> > @@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
> >   {
> >       int ret;
> >       struct power_allocator_params *params;
> > -     struct thermal_trip trip;
> >
> >       ret = check_power_actors(tz);
> >       if (ret)
> > @@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
> >       get_governor_trips(tz, params);
> >
> >       if (tz->num_trips > 0) {
> > -             ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> > -                                           &trip);
> > -             if (!ret)
> > +             const struct thermal_trip *trip;
> > +
> > +             trip = params->trip_max_desired_temperature;
> > +             if (trip)
> >                       estimate_pid_constants(tz, tz->tzp->sustainable_power,
> >                                              params->trip_switch_on,
> > -                                            trip.temperature);
> > +                                            trip->temperature);
> >       }
>
> The code check for the populated pointer (by earlier new
> get_governor_trips(tz, params)) :
>
> if (params->trip_max_desired_temperature) {
>         int temp = params->trip_max_desired_temperature->temperature;
>
>         estimate_pid_constants(...)
> }
>
> looks better (what you have figured out already).
>
> Other than that the patch LGTM:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks!

> I have also tested this is a few ways and it still works as expected:
> 1. Power allocation in different conditions
> 2. Trip points properly recognized with this nice get_governor_trips(),
>    here is the test output, with many tricky trip point setups:

Much appreciated!

> use case A:
> 5 trip points:
> - 2 passive for 50, 60 degC
> - 1 active 70degC
> - 2 passive 70, 85 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 50, 85 deg => 35 degC IPA operating range
>
> [   24.578806] Power allocator: IPA: trip->temperature=50000
> [   24.578824] Power allocator: IPA: passive trip->temperature=50000
> [   24.578838] Power allocator: IPA: fist passive trip->temperature=50000
> [   24.578851] Power allocator: IPA: trip->temperature=60000
> [   24.578863] Power allocator: IPA: passive trip->temperature=60000
> [   24.578875] Power allocator: IPA: trip->temperature=70000
> [   24.578888] Power allocator: IPA: active trip->temperature=70000
> [   24.578900] Power allocator: IPA: trip->temperature=120000
> [   24.578912] Power allocator: IPA: trip->temperature=70000
> [   24.578925] Power allocator: IPA: passive trip->temperature=70000
> [   24.578937] Power allocator: IPA: trip->temperature=85000
> [   24.578950] Power allocator: IPA: passive trip->temperature=85000
> [   24.578964] Power allocator: IPA: trip_switch_on->temperature=50000
> control_temp=85000
> [   24.578978] Power allocator: IPA: temperature_threshold=35000
>
> -------------------------------------------------------------
> use case B:
> 5 trip points:
> - 2 active for 50, 60 degC
> - 1 active 70degC
> - 2 passive 70, 85 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 70, 85 deg => 15 degC IPA operating range
>
> [   27.402474] Power allocator: IPA: trip->temperature=50000
> [   27.402492] Power allocator: IPA: active trip->temperature=50000
> [   27.402505] Power allocator: IPA: trip->temperature=60000
> [   27.402518] Power allocator: IPA: active trip->temperature=60000
> [   27.402531] Power allocator: IPA: trip->temperature=70000
> [   27.402544] Power allocator: IPA: active trip->temperature=70000
> [   27.402557] Power allocator: IPA: trip->temperature=120000
> [   27.402570] Power allocator: IPA: trip->temperature=70000
> [   27.402582] Power allocator: IPA: passive trip->temperature=70000
> [   27.402596] Power allocator: IPA: fist passive trip->temperature=70000
> [   27.402608] Power allocator: IPA: trip->temperature=85000
> [   27.402622] Power allocator: IPA: passive trip->temperature=85000
> [   27.402635] Power allocator: IPA: trip_switch_on->temperature=70000
> control_temp=85000
> [   27.402649] Power allocator: IPA: temperature_threshold=15000
>
> --------------------------------------------------------
> use case C:
> 6 trip points:
> - 2 active for 50, 60 degC
> - 1 active 70degC
> - 3 passive 70, 85, 90 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 50, 85 deg => 20 degC IPA operating range
>
> [   36.998907] Power allocator: IPA: trip->temperature=50000
> [   36.998921] Power allocator: IPA: active trip->temperature=50000
> [   36.998935] Power allocator: IPA: trip->temperature=60000
> [   36.998948] Power allocator: IPA: active trip->temperature=60000
> [   36.998960] Power allocator: IPA: trip->temperature=70000
> [   36.998973] Power allocator: IPA: active trip->temperature=70000
> [   36.998985] Power allocator: IPA: trip->temperature=120000
> [   36.998999] Power allocator: IPA: trip->temperature=70000
> [   36.999011] Power allocator: IPA: passive trip->temperature=70000
> [   36.999024] Power allocator: IPA: fist passive trip->temperature=70000
> [   36.999037] Power allocator: IPA: trip->temperature=85000
> [   36.999049] Power allocator: IPA: passive trip->temperature=85000
> [   36.999062] Power allocator: IPA: trip->temperature=90000
> [   36.999074] Power allocator: IPA: passive trip->temperature=90000
> [   36.999087] Power allocator: IPA: trip_switch_on->temperature=70000
> control_temp=90000
> [   36.999101] Power allocator: IPA: temperature_threshold=20000
>

[PATCH v1 5/6] thermal: gov_step_wise: Fold update_passive_instance() into its caller

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold update_passive_instance() into thermal_zone_trip_update() that is
its only caller so as to make the code in question easeir to follow.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |   28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -68,17 +68,6 @@ static unsigned long get_target_state(st
 	return next_target;
 }
 
-static void update_passive_instance(struct thermal_zone_device *tz,
-				enum thermal_trip_type type, int value)
-{
-	/*
-	 * If value is +1, activate a passive instance.
-	 * If value is -1, deactivate a passive instance.
-	 */
-	if (type == THERMAL_TRIP_PASSIVE)
-		tz->passive += value;
-}
-
 static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
 {
 	const struct thermal_trip *trip = &tz->trips[trip_id];
@@ -109,14 +98,17 @@ static void thermal_zone_trip_update(str
 		if (instance->initialized && old_target == instance->target)
 			continue;
 
-		/* Activate a passive thermal instance */
 		if (old_target == THERMAL_NO_TARGET &&
-			instance->target != THERMAL_NO_TARGET)
-			update_passive_instance(tz, trip->type, 1);
-		/* Deactivate a passive thermal instance */
-		else if (old_target != THERMAL_NO_TARGET &&
-			instance->target == THERMAL_NO_TARGET)
-			update_passive_instance(tz, trip->type, -1);
+		    instance->target != THERMAL_NO_TARGET) {
+			/* Activate a passive thermal instance */
+			if (trip->type == THERMAL_TRIP_PASSIVE)
+				tz->passive++;
+		} else if (old_target != THERMAL_NO_TARGET &&
+			   instance->target == THERMAL_NO_TARGET) {
+			/* Deactivate a passive thermal instance */
+			if (trip->type == THERMAL_TRIP_PASSIVE)
+				tz->passive--;
+		}
 
 		instance->initialized = true;
 		mutex_lock(&instance->cdev->lock);

Re: [PATCH v1 5/6] thermal: gov_step_wise: Fold update_passive_instance() into its caller

[Daniel Lezcano]

On 06/10/2023 19:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold update_passive_instance() into thermal_zone_trip_update() that is
> its only caller so as to make the code in question easeir to follow.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
<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

Re: [PATCH v1 5/6] thermal: gov_step_wise: Fold update_passive_instance() into its caller

[Lukasz Luba]

On 10/6/23 18:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold update_passive_instance() into thermal_zone_trip_update() that is
> its only caller so as to make the code in question easeir to follow.

s/easeir/easier/

> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |   28 ++++++++++------------------
>   1 file changed, 10 insertions(+), 18 deletions(-)
> 

other than that LGTM:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 5/6] thermal: gov_step_wise: Fold update_passive_instance() into its caller

[Rafael J. Wysocki]

On Fri, Oct 20, 2023 at 6:16 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/6/23 18:49, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Fold update_passive_instance() into thermal_zone_trip_update() that is
> > its only caller so as to make the code in question easeir to follow.
>
> s/easeir/easier/

I'll fix this in the tree.

> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/gov_step_wise.c |   28 ++++++++++------------------
> >   1 file changed, 10 insertions(+), 18 deletions(-)
> >
>
> other than that LGTM:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Thank you!

[PATCH v1 6/6] thermal: core: Pass trip pointer to governor throttle callback

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the governor .throttle() callback definition so that it takes a
trip pointer instead of a trip index as its second argument, adjust the
governors accordingly and update the core code invoking .throttle().

This causes the governors to become independent of the representation
of the list of trips in the thermal zone structure.

This change is not expected to alter the general functionality.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c       |    8 +++--
 drivers/thermal/gov_fair_share.c      |    6 ++--
 drivers/thermal/gov_power_allocator.c |    4 +-
 drivers/thermal/gov_step_wise.c       |   12 ++++----
 drivers/thermal/gov_user_space.c      |    8 +++--
 drivers/thermal/thermal_core.c        |   50 +++++++++++++++++-----------------
 drivers/thermal/thermal_core.h        |    2 -
 drivers/thermal/thermal_helpers.c     |    3 --
 include/linux/thermal.h               |    3 +-
 9 files changed, 51 insertions(+), 45 deletions(-)

Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -199,7 +199,8 @@ struct thermal_governor {
 	char name[THERMAL_NAME_LENGTH];
 	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 (*throttle)(struct thermal_zone_device *tz,
+			const struct thermal_trip *trip);
 	struct list_head	governor_list;
 };
 
Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -13,9 +13,10 @@
 
 #include "thermal_core.h"
 
-static int thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_index)
+static int thermal_zone_trip_update(struct thermal_zone_device *tz,
+				    const struct thermal_trip *trip)
 {
-	const struct thermal_trip *trip = &tz->trips[trip_index];
+	int trip_index = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
 
 	if (!trip->hysteresis)
@@ -89,7 +90,8 @@ static int thermal_zone_trip_update(stru
  *     (trip_temp - hyst) so that the fan gets turned off again.
  *
  */
-static int bang_bang_control(struct thermal_zone_device *tz, int trip)
+static int bang_bang_control(struct thermal_zone_device *tz,
+			     const struct thermal_trip *trip)
 {
 	struct thermal_instance *instance;
 	int ret;
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
@@ -47,7 +47,7 @@ static long get_target_state(struct ther
 /**
  * fair_share_throttle - throttles devices associated with the given zone
  * @tz: thermal_zone_device
- * @trip_index: trip point index
+ * @trip: trip point
  *
  * Throttling Logic: This uses three parameters to calculate the new
  * throttle state of the cooling devices associated with the given zone.
@@ -63,9 +63,9 @@ static long get_target_state(struct ther
  *	(Heavily assumes the trip points are in ascending order)
  * new_state of cooling device = P3 * P2 * P1
  */
-static int fair_share_throttle(struct thermal_zone_device *tz, int trip_index)
+static int fair_share_throttle(struct thermal_zone_device *tz,
+			       const struct thermal_trip *trip)
 {
-	const struct thermal_trip *trip = &tz->trips[trip_index];
 	struct thermal_instance *instance;
 	int total_weight = 0;
 	int total_instance = 0;
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -680,10 +680,10 @@ static void power_allocator_unbind(struc
 	tz->governor_data = NULL;
 }
 
-static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
+static int power_allocator_throttle(struct thermal_zone_device *tz,
+				    const struct thermal_trip *trip)
 {
 	struct power_allocator_params *params = tz->governor_data;
-	const struct thermal_trip *trip = &tz->trips[trip_index];
 	bool update;
 
 	lockdep_assert_held(&tz->lock);
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -68,15 +68,16 @@ static unsigned long get_target_state(st
 	return next_target;
 }
 
-static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
+static void thermal_zone_trip_update(struct thermal_zone_device *tz,
+				     const struct thermal_trip *trip)
 {
-	const struct thermal_trip *trip = &tz->trips[trip_id];
+	int trip_id = thermal_zone_trip_id(tz, trip);
 	enum thermal_trend trend;
 	struct thermal_instance *instance;
 	bool throttle = false;
 	int old_target;
 
-	trend = get_tz_trend(tz, trip_id);
+	trend = get_tz_trend(tz, trip);
 
 	if (tz->temperature >= trip->temperature) {
 		throttle = true;
@@ -120,7 +121,7 @@ static void thermal_zone_trip_update(str
 /**
  * step_wise_throttle - throttles devices associated with the given zone
  * @tz: thermal_zone_device
- * @trip: trip point index
+ * @trip: trip point
  *
  * Throttling Logic: This uses the trend of the thermal zone to throttle.
  * If the thermal zone is 'heating up' this throttles all the cooling
@@ -128,7 +129,8 @@ static void thermal_zone_trip_update(str
  * step. If the zone is 'cooling down' it brings back the performance of
  * the devices by one step.
  */
-static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
+static int step_wise_throttle(struct thermal_zone_device *tz,
+			      const struct thermal_trip *trip)
 {
 	struct thermal_instance *instance;
 
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -22,9 +22,8 @@
 #include "thermal_core.h"
 #include "thermal_trace.h"
 
-int get_tz_trend(struct thermal_zone_device *tz, int trip_index)
+int get_tz_trend(struct thermal_zone_device *tz, const struct thermal_trip *trip)
 {
-	struct thermal_trip *trip = tz->trips ? &tz->trips[trip_index] : NULL;
 	enum thermal_trend trend;
 
 	if (tz->emul_temperature || !tz->ops->get_trend ||
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -70,7 +70,7 @@ static inline bool cdev_is_power_actor(s
 void thermal_cdev_update(struct thermal_cooling_device *);
 void __thermal_cdev_update(struct thermal_cooling_device *cdev);
 
-int get_tz_trend(struct thermal_zone_device *tz, int trip_index);
+int get_tz_trend(struct thermal_zone_device *tz, const struct thermal_trip *trip);
 
 struct thermal_instance *
 get_thermal_instance(struct thermal_zone_device *tz,
Index: linux-pm/drivers/thermal/gov_user_space.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_user_space.c
+++ linux-pm/drivers/thermal/gov_user_space.c
@@ -25,11 +25,12 @@ static int user_space_bind(struct therma
 /**
  * notify_user_space - Notifies user space about thermal events
  * @tz: thermal_zone_device
- * @trip: trip point index
+ * @trip: trip point
  *
  * This function notifies the user space through UEvents.
  */
-static int notify_user_space(struct thermal_zone_device *tz, int trip)
+static int notify_user_space(struct thermal_zone_device *tz,
+			     const struct thermal_trip *trip)
 {
 	char *thermal_prop[5];
 	int i;
@@ -38,7 +39,8 @@ static int notify_user_space(struct ther
 
 	thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
 	thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
-	thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
+	thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d",
+				    thermal_zone_trip_id(tz, trip));
 	thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
 	thermal_prop[4] = NULL;
 	kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -307,7 +307,8 @@ static void monitor_thermal_zone(struct
 		thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
 }
 
-static void handle_non_critical_trips(struct thermal_zone_device *tz, int trip)
+static void handle_non_critical_trips(struct thermal_zone_device *tz,
+				      const struct thermal_trip *trip)
 {
 	tz->governor ? tz->governor->throttle(tz, trip) :
 		       def_governor->throttle(tz, trip);
@@ -329,44 +330,43 @@ void thermal_zone_device_critical(struct
 EXPORT_SYMBOL(thermal_zone_device_critical);
 
 static void handle_critical_trips(struct thermal_zone_device *tz,
-				  int trip, int trip_temp, enum thermal_trip_type trip_type)
+				  const struct thermal_trip *trip)
 {
 	/* If we have not crossed the trip_temp, we do not care. */
-	if (trip_temp <= 0 || tz->temperature < trip_temp)
+	if (trip->temperature <= 0 || tz->temperature < trip->temperature)
 		return;
 
-	trace_thermal_zone_trip(tz, trip, trip_type);
+	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
 
-	if (trip_type == THERMAL_TRIP_HOT && tz->ops->hot)
-		tz->ops->hot(tz);
-	else if (trip_type == THERMAL_TRIP_CRITICAL)
+	if (trip->type == THERMAL_TRIP_CRITICAL)
 		tz->ops->critical(tz);
+	else if (tz->ops->hot)
+		tz->ops->hot(tz);
 }
 
-static void handle_thermal_trip(struct thermal_zone_device *tz, int trip_id)
+static void handle_thermal_trip(struct thermal_zone_device *tz,
+				const struct thermal_trip *trip)
 {
-	struct thermal_trip trip;
-
-	__thermal_zone_get_trip(tz, trip_id, &trip);
-
-	if (trip.temperature == THERMAL_TEMP_INVALID)
+	if (trip->temperature == THERMAL_TEMP_INVALID)
 		return;
 
 	if (tz->last_temperature != THERMAL_TEMP_INVALID) {
-		if (tz->last_temperature < trip.temperature &&
-		    tz->temperature >= trip.temperature)
-			thermal_notify_tz_trip_up(tz->id, trip_id,
+		if (tz->last_temperature < trip->temperature &&
+		    tz->temperature >= trip->temperature)
+			thermal_notify_tz_trip_up(tz->id,
+						  thermal_zone_trip_id(tz, trip),
 						  tz->temperature);
-		if (tz->last_temperature >= trip.temperature &&
-		    tz->temperature < (trip.temperature - trip.hysteresis))
-			thermal_notify_tz_trip_down(tz->id, trip_id,
+		if (tz->last_temperature >= trip->temperature &&
+		    tz->temperature < trip->temperature - trip->hysteresis)
+			thermal_notify_tz_trip_down(tz->id,
+						    thermal_zone_trip_id(tz, trip),
 						    tz->temperature);
 	}
 
-	if (trip.type == THERMAL_TRIP_CRITICAL || trip.type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip_id, trip.temperature, trip.type);
+	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
+		handle_critical_trips(tz, trip);
 	else
-		handle_non_critical_trips(tz, trip_id);
+		handle_non_critical_trips(tz, trip);
 }
 
 static void update_temperature(struct thermal_zone_device *tz)
@@ -403,7 +403,7 @@ static void thermal_zone_device_init(str
 void __thermal_zone_device_update(struct thermal_zone_device *tz,
 				  enum thermal_notify_event event)
 {
-	int count;
+	const struct thermal_trip *trip;
 
 	if (atomic_read(&in_suspend))
 		return;
@@ -422,8 +422,8 @@ void __thermal_zone_device_update(struct
 
 	tz->notify_event = event;
 
-	for (count = 0; count < tz->num_trips; count++)
-		handle_thermal_trip(tz, count);
+	for_each_trip(tz, trip)
+		handle_thermal_trip(tz, trip);
 
 	monitor_thermal_zone(tz);
 }

Re: [PATCH v1 6/6] thermal: core: Pass trip pointer to governor throttle callback

[Daniel Lezcano]

On 06/10/2023 19:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify the governor .throttle() callback definition so that it takes a
> trip pointer instead of a trip index as its second argument, adjust the
> governors accordingly and update the core code invoking .throttle().
> 
> This causes the governors to become independent of the representation
> of the list of trips in the thermal zone structure.
> 
> This change is not expected to alter the general functionality.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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

Re: [PATCH v1 0/6] thermal: core: Pass trip pointers to governor .throttle() callbacks

[Lukasz Luba]

Hi Rafael,


On 10/6/23 18:38, Rafael J. Wysocki wrote:
> Hi All,
> 
> While working on https://lore.kernel.org/linux-pm/4846448.GXAFRqVoOG@kreacher/
> I started to change thermal governors so as to reduce the usage of trip
> indices in them.  At that time, I was concerned that they could not be
> replaced with trip pointers overall, because they were needed in certain
> situations (tracing, debug messages, userspace governor) and computing them
> whenever they were needed would be extra overhead with relatively weak
> justification.  In the meantime, however, I realized that for a given trip
> pointer, it is actually trivial to compute the corresponding index: it is
> sufficient to subtract the start of the trips[] table in the thermal zone
> containing the trip from that trip pointer for this purpose.  Patch [1/6]
> modifies thermal_zone_trip_id() in accordance with this observation.
> 
> Now that the cost of computing a trip index for a given trip pointer and
> thermal zone is not a concern any more, the governors can be generally
> switched over to using trip pointers for representing trips.  One of the
> things they need to do sometimes, though, is to iterate over trips in a
> given thermal zone (the core needs to do that too, of course) and
> for_each_thermal_trip() is somewhat inconvenient for this purpose, because
> it requires callback functions to be defined and in some cases new data
> types need to be introduced just in order to use it.  For this reason,
> patch [2/6] adds a macro for iterating over trip points in a given thermal
> zone with the help of a trip pointer and changes for_each_thermal_trip() to
> use that macro internally.
> 
> Patches [3-5/6] change individual governors to prepare them for using trip
> pointers everywhere for representing trips, except for the trip argument of
> the .throttle() callback and patch [6/6] finally changes the .throttle()
> callback definition so that it takes a trip pointer as the argument
> representing the trip.
> 
> Please refer to the individual patch changelogs for details.
> 
> Thanks!
> 
> 
> 

I have issues to apply this series, could you tell me the best
base branch from your tree?

I will give it a try on my boards and review.

Thanks,
Lukasz

Re: [PATCH v1 0/6] thermal: core: Pass trip pointers to governor .throttle() callbacks

[Rafael J. Wysocki]

Hi Lukasz,

On Fri, Oct 13, 2023 at 10:12 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
>
> On 10/6/23 18:38, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > While working on https://lore.kernel.org/linux-pm/4846448.GXAFRqVoOG@kreacher/
> > I started to change thermal governors so as to reduce the usage of trip
> > indices in them.  At that time, I was concerned that they could not be
> > replaced with trip pointers overall, because they were needed in certain
> > situations (tracing, debug messages, userspace governor) and computing them
> > whenever they were needed would be extra overhead with relatively weak
> > justification.  In the meantime, however, I realized that for a given trip
> > pointer, it is actually trivial to compute the corresponding index: it is
> > sufficient to subtract the start of the trips[] table in the thermal zone
> > containing the trip from that trip pointer for this purpose.  Patch [1/6]
> > modifies thermal_zone_trip_id() in accordance with this observation.
> >
> > Now that the cost of computing a trip index for a given trip pointer and
> > thermal zone is not a concern any more, the governors can be generally
> > switched over to using trip pointers for representing trips.  One of the
> > things they need to do sometimes, though, is to iterate over trips in a
> > given thermal zone (the core needs to do that too, of course) and
> > for_each_thermal_trip() is somewhat inconvenient for this purpose, because
> > it requires callback functions to be defined and in some cases new data
> > types need to be introduced just in order to use it.  For this reason,
> > patch [2/6] adds a macro for iterating over trip points in a given thermal
> > zone with the help of a trip pointer and changes for_each_thermal_trip() to
> > use that macro internally.
> >
> > Patches [3-5/6] change individual governors to prepare them for using trip
> > pointers everywhere for representing trips, except for the trip argument of
> > the .throttle() callback and patch [6/6] finally changes the .throttle()
> > callback definition so that it takes a trip pointer as the argument
> > representing the trip.
> >
> > Please refer to the individual patch changelogs for details.
> >
> > Thanks!
> >
> >
> >
>
> I have issues to apply this series, could you tell me the best
> base branch from your tree?
>
> I will give it a try on my boards and review.

It is there in the thermal-core branch of linux-pm.git.

Thanks!