2023-10-06 17:52:19

by Rafael J. Wysocki

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

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>
---
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;
}

2023-10-12 14:28:08

by Daniel Lezcano

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

On 06/10/2023 19:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2023-10-12 16:22:15

by Rafael J. Wysocki

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

On Thu, Oct 12, 2023 at 4:27 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 06/10/2023 19:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.

2023-10-20 16:58:28

by Lukasz Luba

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

On 10/6/23 18:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Tested-by: Lukasz Luba <[email protected]>

2023-10-20 17:04:41

by Rafael J. Wysocki

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

On Fri, Oct 20, 2023 at 6:58 PM Lukasz Luba <[email protected]> wrote:
>
>
>
> On 10/6/23 18:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> Tested-by: Lukasz Luba <[email protected]>

Thanks!