From: Rafael J. Wysocki <[email protected]>
Both trip_point_temp_store() and trip_point_hyst_store() use
thermal_zone_set_trip() to update a given trip point, but none of them
actually needs to change more than one field in struct thermal_trip
representing it. However, each of them effectively calls
__thermal_zone_get_trip() twice in a row for the same trip index value,
once directly and once via thermal_zone_set_trip(), which is not
particularly efficient, and the way in which thermal_zone_set_trip()
carries out the update is not particularly straightforward.
Moreover, some checks done by them both need not go under the thermal
zone lock and code duplication between them can be reduced quilte a bit
by moving the majority of logic into thermal_zone_set_trip().
Rework all of the above funtcions to address the above.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/thermal_core.h | 9 +++++
drivers/thermal/thermal_sysfs.c | 52 +++++++---------------------------
drivers/thermal/thermal_trip.c | 61 ++++++++++++++++++++++++++--------------
include/linux/thermal.h | 3 -
4 files changed, 61 insertions(+), 64 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -122,6 +122,15 @@ void __thermal_zone_device_update(struct
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);
+
+enum thermal_set_trip_target {
+ THERMAL_TRIP_SET_TEMP,
+ THERMAL_TRIP_SET_HYST,
+};
+
+int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
+ enum thermal_set_trip_target what, const char *buf);
+
int thermal_zone_trip_id(struct thermal_zone_device *tz,
const struct thermal_trip *trip);
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -120,31 +120,17 @@ trip_point_temp_store(struct device *dev
const char *buf, size_t count)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- struct thermal_trip trip;
- int trip_id, ret;
+ int trip_id;
+ int ret;
+
+ if (!device_is_registered(dev))
+ return -ENODEV;
if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
return -EINVAL;
- mutex_lock(&tz->lock);
-
- if (!device_is_registered(dev)) {
- ret = -ENODEV;
- goto unlock;
- }
-
- ret = __thermal_zone_get_trip(tz, trip_id, &trip);
- if (ret)
- goto unlock;
-
- ret = kstrtoint(buf, 10, &trip.temperature);
- if (ret)
- goto unlock;
+ ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_TEMP, buf);
- ret = thermal_zone_set_trip(tz, trip_id, &trip);
-unlock:
- mutex_unlock(&tz->lock);
-
return ret ? ret : count;
}
@@ -179,30 +165,16 @@ trip_point_hyst_store(struct device *dev
const char *buf, size_t count)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- struct thermal_trip trip;
- int trip_id, ret;
+ int trip_id;
+ int ret;
+
+ if (!device_is_registered(dev))
+ return -ENODEV;
if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
return -EINVAL;
- mutex_lock(&tz->lock);
-
- if (!device_is_registered(dev)) {
- ret = -ENODEV;
- goto unlock;
- }
-
- ret = __thermal_zone_get_trip(tz, trip_id, &trip);
- if (ret)
- goto unlock;
-
- ret = kstrtoint(buf, 10, &trip.hysteresis);
- if (ret)
- goto unlock;
-
- ret = thermal_zone_set_trip(tz, trip_id, &trip);
-unlock:
- mutex_unlock(&tz->lock);
+ ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_HYST, buf);
return ret ? ret : count;
}
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -148,42 +148,61 @@ int thermal_zone_get_trip(struct thermal
EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
- const struct thermal_trip *trip)
+ enum thermal_set_trip_target what, const char *buf)
{
- struct thermal_trip t;
- int ret;
+ struct thermal_trip *trip;
+ int val, ret = 0;
- if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
- return -EINVAL;
+ if (trip_id < 0 || trip_id >= tz->num_trips)
+ ret = -EINVAL;
- ret = __thermal_zone_get_trip(tz, trip_id, &t);
+ ret = kstrtoint(buf, 10, &val);
if (ret)
return ret;
- if (t.type != trip->type)
- return -EINVAL;
+ mutex_lock(&tz->lock);
- if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
- ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
- if (ret)
- return ret;
- }
+ trip = &tz->trips[trip_id];
- if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
- ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
- if (ret)
- return ret;
+ switch (what) {
+ case THERMAL_TRIP_SET_TEMP:
+ if (val == trip->temperature)
+ goto unlock;
+
+ if (tz->ops->set_trip_temp) {
+ ret = tz->ops->set_trip_temp(tz, trip_id, val);
+ if (ret)
+ goto unlock;
+ }
+ trip->temperature = val;
+ break;
+
+ case THERMAL_TRIP_SET_HYST:
+ if (val == trip->hysteresis)
+ goto unlock;
+
+ if (tz->ops->set_trip_hyst) {
+ ret = tz->ops->set_trip_hyst(tz, trip_id, val);
+ if (ret)
+ goto unlock;
+ }
+ trip->hysteresis = val;
+ break;
+
+ default:
+ ret = -EINVAL;
+ goto unlock;
}
- if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
- tz->trips[trip_id] = *trip;
-
thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
trip->temperature, trip->hysteresis);
__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
- return 0;
+unlock:
+ mutex_unlock(&tz->lock);
+
+ return ret;
}
int thermal_zone_trip_id(struct thermal_zone_device *tz,
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -283,9 +283,6 @@ int __thermal_zone_get_trip(struct therm
int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
struct thermal_trip *trip);
-int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
- const struct thermal_trip *trip);
-
int for_each_thermal_trip(struct thermal_zone_device *tz,
int (*cb)(struct thermal_trip *, void *),
void *data);
Hi Rafael,
On 11/27/23 19:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Both trip_point_temp_store() and trip_point_hyst_store() use
> thermal_zone_set_trip() to update a given trip point, but none of them
> actually needs to change more than one field in struct thermal_trip
> representing it. However, each of them effectively calls
> __thermal_zone_get_trip() twice in a row for the same trip index value,
> once directly and once via thermal_zone_set_trip(), which is not
> particularly efficient, and the way in which thermal_zone_set_trip()
> carries out the update is not particularly straightforward.
>
> Moreover, some checks done by them both need not go under the thermal
> zone lock and code duplication between them can be reduced quilte a bit
s/quilte/quite/
> by moving the majority of logic into thermal_zone_set_trip().
>
> Rework all of the above funtcions to address the above.
s/funtcions/functions/
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/thermal_core.h | 9 +++++
> drivers/thermal/thermal_sysfs.c | 52 +++++++---------------------------
> drivers/thermal/thermal_trip.c | 61 ++++++++++++++++++++++++++--------------
> include/linux/thermal.h | 3 -
> 4 files changed, 61 insertions(+), 64 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -122,6 +122,15 @@ void __thermal_zone_device_update(struct
> 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);
> +
> +enum thermal_set_trip_target {
> + THERMAL_TRIP_SET_TEMP,
> + THERMAL_TRIP_SET_HYST,
> +};
> +
> +int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> + enum thermal_set_trip_target what, const char *buf);
> +
> int thermal_zone_trip_id(struct thermal_zone_device *tz,
> const struct thermal_trip *trip);
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -120,31 +120,17 @@ trip_point_temp_store(struct device *dev
> const char *buf, size_t count)
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> - struct thermal_trip trip;
> - int trip_id, ret;
> + int trip_id;
> + int ret;
> +
> + if (!device_is_registered(dev))
> + return -ENODEV;
>
> if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
> return -EINVAL;
>
> - mutex_lock(&tz->lock);
> -
> - if (!device_is_registered(dev)) {
> - ret = -ENODEV;
> - goto unlock;
> - }
> -
> - ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> - if (ret)
> - goto unlock;
> -
> - ret = kstrtoint(buf, 10, &trip.temperature);
> - if (ret)
> - goto unlock;
> + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_TEMP, buf);
>
> - ret = thermal_zone_set_trip(tz, trip_id, &trip);
> -unlock:
> - mutex_unlock(&tz->lock);
> -
> return ret ? ret : count;
> }
>
> @@ -179,30 +165,16 @@ trip_point_hyst_store(struct device *dev
> const char *buf, size_t count)
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> - struct thermal_trip trip;
> - int trip_id, ret;
> + int trip_id;
> + int ret;
> +
> + if (!device_is_registered(dev))
> + return -ENODEV;
>
> if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
> return -EINVAL;
>
> - mutex_lock(&tz->lock);
> -
> - if (!device_is_registered(dev)) {
> - ret = -ENODEV;
> - goto unlock;
> - }
> -
> - ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> - if (ret)
> - goto unlock;
> -
> - ret = kstrtoint(buf, 10, &trip.hysteresis);
> - if (ret)
> - goto unlock;
> -
> - ret = thermal_zone_set_trip(tz, trip_id, &trip);
> -unlock:
> - mutex_unlock(&tz->lock);
> + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_HYST, buf);
>
> return ret ? ret : count;
> }
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -148,42 +148,61 @@ int thermal_zone_get_trip(struct thermal
> EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
>
> int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> - const struct thermal_trip *trip)
> + enum thermal_set_trip_target what, const char *buf)
> {
> - struct thermal_trip t;
> - int ret;
> + struct thermal_trip *trip;
> + int val, ret = 0;
>
> - if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
> - return -EINVAL;
Here we could bail out when there are no callbacks.
> + if (trip_id < 0 || trip_id >= tz->num_trips)
> + ret = -EINVAL;
>
> - ret = __thermal_zone_get_trip(tz, trip_id, &t);
> + ret = kstrtoint(buf, 10, &val);
> if (ret)
> return ret;
>
> - if (t.type != trip->type)
> - return -EINVAL;
> + mutex_lock(&tz->lock);
>
> - if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
> - ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
> - if (ret)
> - return ret;
> - }
> + trip = &tz->trips[trip_id];
>
> - if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
> - ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
> - if (ret)
> - return ret;
> + switch (what) {
> + case THERMAL_TRIP_SET_TEMP:
> + if (val == trip->temperature)
> + goto unlock;
> +
> + if (tz->ops->set_trip_temp) {
> + ret = tz->ops->set_trip_temp(tz, trip_id, val);
> + if (ret)
> + goto unlock;
> + }
But here we don't bail out and go line below
> + trip->temperature = val;
where we modify the actual '&tz->trips[trip_id]'.
Shouldn't be something like the code flow below?
--------------------------------------------8<-----------------------------
if (!tz->ops->set_trip_temp)
goto unlock;
ret = tz->ops->set_trip_temp(tz, trip_id, val);
if (ret)
goto unlock;
trip->temperature = val;
break
----------------------------------->8--------------------------------------
> + break;
> +
> + case THERMAL_TRIP_SET_HYST:
> + if (val == trip->hysteresis)
> + goto unlock;
> +
> + if (tz->ops->set_trip_hyst) {
> + ret = tz->ops->set_trip_hyst(tz, trip_id, val);
> + if (ret)
> + goto unlock;
> + }
> + trip->hysteresis = val;
Similar question here.
> + break;
> +
> + default:
> + ret = -EINVAL;
> + goto unlock;
> }
>
> - if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> - tz->trips[trip_id] = *trip;
> -
> thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> trip->temperature, trip->hysteresis);
>
> __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
>
> - return 0;
> +unlock:
> + mutex_unlock(&tz->lock);
> +
> + return ret;
> }
>
> int thermal_zone_trip_id(struct thermal_zone_device *tz,
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -283,9 +283,6 @@ int __thermal_zone_get_trip(struct therm
> int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
> struct thermal_trip *trip);
>
> -int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> - const struct thermal_trip *trip);
> -
Surprisingly, nothing outside thermal fwk uses it...
Maybe it's worth to check other functions there.
Other than that, it looks like a good idea.
Regards,
Lukasz
Hi Lukasz,
On Tue, Nov 28, 2023 at 9:16 AM Lukasz Luba <[email protected]> wrote:
>
> Hi Rafael,
>
> On 11/27/23 19:59, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Both trip_point_temp_store() and trip_point_hyst_store() use
> > thermal_zone_set_trip() to update a given trip point, but none of them
> > actually needs to change more than one field in struct thermal_trip
> > representing it. However, each of them effectively calls
> > __thermal_zone_get_trip() twice in a row for the same trip index value,
> > once directly and once via thermal_zone_set_trip(), which is not
> > particularly efficient, and the way in which thermal_zone_set_trip()
> > carries out the update is not particularly straightforward.
> >
> > Moreover, some checks done by them both need not go under the thermal
> > zone lock and code duplication between them can be reduced quilte a bit
>
> s/quilte/quite/
>
> > by moving the majority of logic into thermal_zone_set_trip().
> >
> > Rework all of the above funtcions to address the above.
>
> s/funtcions/functions/
Thanks for spotting the typos!
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/thermal/thermal_core.h | 9 +++++
> > drivers/thermal/thermal_sysfs.c | 52 +++++++---------------------------
> > drivers/thermal/thermal_trip.c | 61 ++++++++++++++++++++++++++--------------
> > include/linux/thermal.h | 3 -
> > 4 files changed, 61 insertions(+), 64 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.h
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > +++ linux-pm/drivers/thermal/thermal_core.h
> > @@ -122,6 +122,15 @@ void __thermal_zone_device_update(struct
> > 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);
> > +
> > +enum thermal_set_trip_target {
> > + THERMAL_TRIP_SET_TEMP,
> > + THERMAL_TRIP_SET_HYST,
> > +};
> > +
> > +int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > + enum thermal_set_trip_target what, const char *buf);
> > +
> > int thermal_zone_trip_id(struct thermal_zone_device *tz,
> > const struct thermal_trip *trip);
> > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > @@ -120,31 +120,17 @@ trip_point_temp_store(struct device *dev
> > const char *buf, size_t count)
> > {
> > struct thermal_zone_device *tz = to_thermal_zone(dev);
> > - struct thermal_trip trip;
> > - int trip_id, ret;
> > + int trip_id;
> > + int ret;
> > +
> > + if (!device_is_registered(dev))
> > + return -ENODEV;
> >
> > if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
> > return -EINVAL;
> >
> > - mutex_lock(&tz->lock);
> > -
> > - if (!device_is_registered(dev)) {
> > - ret = -ENODEV;
> > - goto unlock;
> > - }
> > -
> > - ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > - if (ret)
> > - goto unlock;
> > -
> > - ret = kstrtoint(buf, 10, &trip.temperature);
> > - if (ret)
> > - goto unlock;
> > + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_TEMP, buf);
> >
> > - ret = thermal_zone_set_trip(tz, trip_id, &trip);
> > -unlock:
> > - mutex_unlock(&tz->lock);
> > -
> > return ret ? ret : count;
> > }
> >
> > @@ -179,30 +165,16 @@ trip_point_hyst_store(struct device *dev
> > const char *buf, size_t count)
> > {
> > struct thermal_zone_device *tz = to_thermal_zone(dev);
> > - struct thermal_trip trip;
> > - int trip_id, ret;
> > + int trip_id;
> > + int ret;
> > +
> > + if (!device_is_registered(dev))
> > + return -ENODEV;
> >
> > if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
> > return -EINVAL;
> >
> > - mutex_lock(&tz->lock);
> > -
> > - if (!device_is_registered(dev)) {
> > - ret = -ENODEV;
> > - goto unlock;
> > - }
> > -
> > - ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > - if (ret)
> > - goto unlock;
> > -
> > - ret = kstrtoint(buf, 10, &trip.hysteresis);
> > - if (ret)
> > - goto unlock;
> > -
> > - ret = thermal_zone_set_trip(tz, trip_id, &trip);
> > -unlock:
> > - mutex_unlock(&tz->lock);
> > + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_HYST, buf);
> >
> > return ret ? ret : count;
> > }
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -148,42 +148,61 @@ int thermal_zone_get_trip(struct thermal
> > EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> >
> > int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > - const struct thermal_trip *trip)
> > + enum thermal_set_trip_target what, const char *buf)
> > {
> > - struct thermal_trip t;
> > - int ret;
> > + struct thermal_trip *trip;
> > + int val, ret = 0;
> >
> > - if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
> > - return -EINVAL;
>
> Here we could bail out when there are no callbacks.
Not really, because the trip is updated regardless.
>
> > + if (trip_id < 0 || trip_id >= tz->num_trips)
> > + ret = -EINVAL;
> >
> > - ret = __thermal_zone_get_trip(tz, trip_id, &t);
> > + ret = kstrtoint(buf, 10, &val);
> > if (ret)
> > return ret;
> >
> > - if (t.type != trip->type)
> > - return -EINVAL;
> > + mutex_lock(&tz->lock);
> >
> > - if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
> > - ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
> > - if (ret)
> > - return ret;
> > - }
> > + trip = &tz->trips[trip_id];
> >
> > - if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
> > - ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
> > - if (ret)
> > - return ret;
> > + switch (what) {
> > + case THERMAL_TRIP_SET_TEMP:
> > + if (val == trip->temperature)
> > + goto unlock;
> > +
> > + if (tz->ops->set_trip_temp) {
> > + ret = tz->ops->set_trip_temp(tz, trip_id, val);
> > + if (ret)
> > + goto unlock;
> > + }
>
> But here we don't bail out and go line below
>
> > + trip->temperature = val;
>
> where we modify the actual '&tz->trips[trip_id]'.
Right, the trip is updated regardless unless the callback invocation
fails, in which case it is better to retain the existing configuration
for consistency.
The current logic is exactly this AFAICS except that it is hard to untangle.
> Shouldn't be something like the code flow below?
> --------------------------------------------8<-----------------------------
> if (!tz->ops->set_trip_temp)
> goto unlock;
>
> ret = tz->ops->set_trip_temp(tz, trip_id, val);
> if (ret)
> goto unlock;
>
> trip->temperature = val;
> break
> ----------------------------------->8--------------------------------------
Not really.
>
>
>
>
> > + break;
> > +
> > + case THERMAL_TRIP_SET_HYST:
> > + if (val == trip->hysteresis)
> > + goto unlock;
> > +
> > + if (tz->ops->set_trip_hyst) {
> > + ret = tz->ops->set_trip_hyst(tz, trip_id, val);
> > + if (ret)
> > + goto unlock;
> > + }
> > + trip->hysteresis = val;
>
> Similar question here.
>
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> > + goto unlock;
> > }
> >
> > - if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> > - tz->trips[trip_id] = *trip;
> > -
> > thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> > trip->temperature, trip->hysteresis);
> >
> > __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> >
> > - return 0;
> > +unlock:
> > + mutex_unlock(&tz->lock);
> > +
> > + return ret;
> > }
> >
> > int thermal_zone_trip_id(struct thermal_zone_device *tz,
> > Index: linux-pm/include/linux/thermal.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/thermal.h
> > +++ linux-pm/include/linux/thermal.h
> > @@ -283,9 +283,6 @@ int __thermal_zone_get_trip(struct therm
> > int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
> > struct thermal_trip *trip);
> >
> > -int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > - const struct thermal_trip *trip);
> > -
>
> Surprisingly, nothing outside thermal fwk uses it...
> Maybe it's worth to check other functions there.
Fair enough, but that's outside this patch IMO.
> Other than that, it looks like a good idea.
Thanks for the review!
On Tue, Nov 28, 2023 at 1:53 PM Rafael J. Wysocki <[email protected]> wrote:
>
> Hi Lukasz,
>
> On Tue, Nov 28, 2023 at 9:16 AM Lukasz Luba <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > On 11/27/23 19:59, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
[cut]
> > > Index: linux-pm/drivers/thermal/thermal_trip.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > > +++ linux-pm/drivers/thermal/thermal_trip.c
> > > @@ -148,42 +148,61 @@ int thermal_zone_get_trip(struct thermal
> > > EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> > >
> > > int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > > - const struct thermal_trip *trip)
> > > + enum thermal_set_trip_target what, const char *buf)
> > > {
> > > - struct thermal_trip t;
> > > - int ret;
> > > + struct thermal_trip *trip;
> > > + int val, ret = 0;
> > >
> > > - if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
> > > - return -EINVAL;
> >
> > Here we could bail out when there are no callbacks.
>
> Not really, because the trip is updated regardless.
Actually, the condition above is always false after recent changes,
because tz->trips[] is always present, so the if () statement is
redundant.
On 11/28/23 12:57, Rafael J. Wysocki wrote:
> On Tue, Nov 28, 2023 at 1:53 PM Rafael J. Wysocki <[email protected]> wrote:
>>
>> Hi Lukasz,
>>
>> On Tue, Nov 28, 2023 at 9:16 AM Lukasz Luba <[email protected]> wrote:
>>>
>>> Hi Rafael,
>>>
>>> On 11/27/23 19:59, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <[email protected]>
>>>>
>
> [cut]
>
>>>> Index: linux-pm/drivers/thermal/thermal_trip.c
>>>> ===================================================================
>>>> --- linux-pm.orig/drivers/thermal/thermal_trip.c
>>>> +++ linux-pm/drivers/thermal/thermal_trip.c
>>>> @@ -148,42 +148,61 @@ int thermal_zone_get_trip(struct thermal
>>>> EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
>>>>
>>>> int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>>>> - const struct thermal_trip *trip)
>>>> + enum thermal_set_trip_target what, const char *buf)
>>>> {
>>>> - struct thermal_trip t;
>>>> - int ret;
>>>> + struct thermal_trip *trip;
>>>> + int val, ret = 0;
>>>>
>>>> - if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
>>>> - return -EINVAL;
>>>
>>> Here we could bail out when there are no callbacks.
>>
>> Not really, because the trip is updated regardless.
>
> Actually, the condition above is always false after recent changes,
> because tz->trips[] is always present, so the if () statement is
> redundant.
Hmm, yes you're right. This is yet another sign to refactor the old
code.
For the rest of your comments in the earlier message - I agree.
When you post the v2 I can give it a try later today.
Regards,
Lukasz