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