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, input processing need not be done under the thermal zone lock
in any of these functions.
Rework trip_point_temp_store() and trip_point_hyst_store() to address
the above, move the part of thermal_zone_set_trip() that is still
useful to a new function called thermal_zone_trip_updated() and drop
the rest of it.
While at it, make trip_point_hyst_store() reject negative hysteresis
values.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v2 -> v3: No changes
v1 -> v2: Still check device_is_registered() under the zone lock
---
drivers/thermal/thermal_core.h | 2 +
drivers/thermal/thermal_sysfs.c | 75 ++++++++++++++++++++++++++++------------
drivers/thermal/thermal_trip.c | 45 ++++--------------------
include/linux/thermal.h | 4 --
4 files changed, 64 insertions(+), 62 deletions(-)
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
return count;
}
+static int check_thermal_zone_and_trip_id(struct device *dev,
+ struct thermal_zone_device *tz,
+ int trip_id)
+{
+ if (!device_is_registered(dev))
+ return -ENODEV;
+
+ if (trip_id < 0 || trip_id >= tz->num_trips)
+ return -EINVAL;
+
+ return 0;
+}
+
static ssize_t
trip_point_type_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -120,28 +133,37 @@ 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;
+ struct thermal_trip *trip;
int trip_id, ret;
+ int temp;
+
+ ret = kstrtoint(buf, 10, &temp);
+ if (ret)
+ return -EINVAL;
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);
+ ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
if (ret)
goto unlock;
- ret = kstrtoint(buf, 10, &trip.temperature);
- if (ret)
- goto unlock;
+ trip = &tz->trips[trip_id];
+
+ if (temp != trip->temperature) {
+ if (tz->ops->set_trip_temp) {
+ ret = tz->ops->set_trip_temp(tz, trip_id, temp);
+ if (ret)
+ goto unlock;
+ }
+
+ trip->temperature = temp;
+
+ thermal_zone_trip_updated(tz, trip);
+ }
- ret = thermal_zone_set_trip(tz, trip_id, &trip);
unlock:
mutex_unlock(&tz->lock);
@@ -179,28 +201,37 @@ 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;
+ struct thermal_trip *trip;
int trip_id, ret;
+ int hyst;
+
+ ret = kstrtoint(buf, 10, &hyst);
+ if (ret || hyst < 0)
+ return -EINVAL;
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);
+ ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
if (ret)
goto unlock;
- ret = kstrtoint(buf, 10, &trip.hysteresis);
- if (ret)
- goto unlock;
+ trip = &tz->trips[trip_id];
+
+ if (hyst != trip->hysteresis) {
+ if (tz->ops->set_trip_hyst) {
+ ret = tz->ops->set_trip_hyst(tz, trip_id, hyst);
+ if (ret)
+ goto unlock;
+ }
+
+ trip->hysteresis = hyst;
+
+ thermal_zone_trip_updated(tz, trip);
+ }
- ret = thermal_zone_set_trip(tz, trip_id, &trip);
unlock:
mutex_unlock(&tz->lock);
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -124,6 +124,8 @@ int __thermal_zone_get_trip(struct therm
struct thermal_trip *trip);
int thermal_zone_trip_id(struct thermal_zone_device *tz,
const struct thermal_trip *trip);
+void thermal_zone_trip_updated(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip);
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
/* sysfs I/F */
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -147,42 +147,6 @@ 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)
-{
- struct thermal_trip t;
- int ret;
-
- ret = __thermal_zone_get_trip(tz, trip_id, &t);
- if (ret)
- return ret;
-
- if (t.type != trip->type)
- return -EINVAL;
-
- 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;
- }
-
- 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;
- }
-
- 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;
-}
-
int thermal_zone_trip_id(struct thermal_zone_device *tz,
const struct thermal_trip *trip)
{
@@ -192,3 +156,12 @@ int thermal_zone_trip_id(struct thermal_
*/
return trip - tz->trips;
}
+
+void thermal_zone_trip_updated(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip)
+{
+ thermal_notify_tz_trip_change(tz->id, thermal_zone_trip_id(tz, trip),
+ trip->type, trip->temperature,
+ trip->hysteresis);
+ __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
+}
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -282,10 +282,6 @@ int __thermal_zone_get_trip(struct therm
struct thermal_trip *trip);
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 04/12/2023 15:50, 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, input processing need not be done under the thermal zone lock
> in any of these functions.
>
> Rework trip_point_temp_store() and trip_point_hyst_store() to address
> the above, move the part of thermal_zone_set_trip() that is still
> useful to a new function called thermal_zone_trip_updated() and drop
> the rest of it.
>
> While at it, make trip_point_hyst_store() reject negative hysteresis
> values.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> v2 -> v3: No changes
>
> v1 -> v2: Still check device_is_registered() under the zone lock
>
> ---
> drivers/thermal/thermal_core.h | 2 +
> drivers/thermal/thermal_sysfs.c | 75 ++++++++++++++++++++++++++++------------
> drivers/thermal/thermal_trip.c | 45 ++++--------------------
> include/linux/thermal.h | 4 --
> 4 files changed, 64 insertions(+), 62 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
> return count;
> }
>
> +static int check_thermal_zone_and_trip_id(struct device *dev,
> + struct thermal_zone_device *tz,
> + int trip_id)
> +{
> + if (!device_is_registered(dev))
> + return -ENODEV;
> +
> + if (trip_id < 0 || trip_id >= tz->num_trips)
> + return -EINVAL;
I'm not sure if this check is useful. The function is called from
trip_point_*_store() which is providing the trip id from the file name
parsing which is in turn built from an existing trip id. There is no
reason the trip id is going to be wrong.
[ ... ]
--
<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
Hi Daniel,
On Mon, Dec 4, 2023 at 5:55 PM Daniel Lezcano <[email protected]> wrote:
>
>
> Hi Rafael,
>
> On 04/12/2023 15:50, 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, input processing need not be done under the thermal zone lock
> > in any of these functions.
> >
> > Rework trip_point_temp_store() and trip_point_hyst_store() to address
> > the above, move the part of thermal_zone_set_trip() that is still
> > useful to a new function called thermal_zone_trip_updated() and drop
> > the rest of it.
> >
> > While at it, make trip_point_hyst_store() reject negative hysteresis
> > values.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >
> > v2 -> v3: No changes
> >
> > v1 -> v2: Still check device_is_registered() under the zone lock
> >
> > ---
> > drivers/thermal/thermal_core.h | 2 +
> > drivers/thermal/thermal_sysfs.c | 75 ++++++++++++++++++++++++++++------------
> > drivers/thermal/thermal_trip.c | 45 ++++--------------------
> > include/linux/thermal.h | 4 --
> > 4 files changed, 64 insertions(+), 62 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > @@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
> > return count;
> > }
> >
> > +static int check_thermal_zone_and_trip_id(struct device *dev,
> > + struct thermal_zone_device *tz,
> > + int trip_id)
> > +{
> > + if (!device_is_registered(dev))
> > + return -ENODEV;
> > +
> > + if (trip_id < 0 || trip_id >= tz->num_trips)
> > + return -EINVAL;
>
> I'm not sure if this check is useful. The function is called from
> trip_point_*_store() which is providing the trip id from the file name
> parsing which is in turn built from an existing trip id. There is no
> reason the trip id is going to be wrong.
I can drop the check just fine.
Do you have any other comments on this patch?
On 04/12/2023 18:47, Rafael J. Wysocki wrote:
[ ... ]
>>> +static int check_thermal_zone_and_trip_id(struct device *dev,
>>> + struct thermal_zone_device *tz,
>>> + int trip_id)
>>> +{
>>> + if (!device_is_registered(dev))
>>> + return -ENODEV;
>>> +
>>> + if (trip_id < 0 || trip_id >= tz->num_trips)
>>> + return -EINVAL;
>>
>> I'm not sure if this check is useful. The function is called from
>> trip_point_*_store() which is providing the trip id from the file name
>> parsing which is in turn built from an existing trip id. There is no
>> reason the trip id is going to be wrong.
>
> I can drop the check just fine.
>
> Do you have any other comments on this patch?
No, the rest LGTM
--
<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