| Message ID | 3256881.aeNJFYEL58@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | thermal: core: Pass trip pointers to governor .throttle() callbacks | expand |
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) ? > } > > >
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.
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>
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!
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; }