Hi Everyone,
This patch series makes the ACPI thermal driver register thermal zones
with the help of thermal_zone_device_register_with_trips(), so it
doesn't need to use the thermal zone callbacks related to trip points
any more (and they are dropped in the last patch).
The approach presented here is quite radically different from the
previous attempts, as it doesn't really rearrange the driver's
internal data structures, but adds the trip table support on top of
them. For this purpose, it uses an additional field in struct thermal_trip
introduced in the first patch.
I have run it on my test-bed systems, but this is not too representative,
because they each have only one ACPI thermal zone with only one (critical)
trip point in it.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
Rework the ACPI thermal driver's .get_trend() callback function,
thermal_get_trend(), to use trip point data stored in the generic
trip structures instead of calling thermal_get_trip_type() and
thermal_get_trip_temp() and make it hold thermal_check_lock to
protect against possible races against trip point updates.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 107 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 78 insertions(+), 29 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -572,47 +572,96 @@ static int thermal_get_crit_temp(struct
return -EINVAL;
}
+static struct thermal_trip *acpi_thermal_get_trip(struct acpi_thermal *tz,
+ int trip_index)
+{
+ struct thermal_trip *trip;
+ int i;
+
+ if (!tz || trip_index < 0)
+ return NULL;
+
+ trip = tz->trips.critical.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+
+ trip = tz->trips.hot.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+
+ trip = tz->trips.passive.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ trip = tz->trips.active[i].trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+ }
+
+ return NULL;
+}
+
static int thermal_get_trend(struct thermal_zone_device *thermal,
- int trip, enum thermal_trend *trend)
+ int trip_index, enum thermal_trend *trend)
{
struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- enum thermal_trip_type type;
- int i;
+ struct thermal_trip *trip;
+ int ret = 0;
- if (thermal_get_trip_type(thermal, trip, &type))
- return -EINVAL;
+ mutex_lock(&tz->thermal_check_lock);
- if (type == THERMAL_TRIP_ACTIVE) {
- int trip_temp;
+ trip = acpi_thermal_get_trip(tz, trip_index);
+ if (!trip) {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (trip->type == THERMAL_TRIP_ACTIVE) {
int temp = deci_kelvin_to_millicelsius_with_offset(
tz->temperature, tz->kelvin_offset);
- if (thermal_get_trip_temp(thermal, trip, &trip_temp))
- return -EINVAL;
- if (temp > trip_temp) {
+ if (temp > trip->temperature)
*trend = THERMAL_TREND_RAISING;
- return 0;
- } else {
- /* Fall back on default trend */
- return -EINVAL;
- }
+ else /* Fall back on default trend */
+ ret = -EINVAL;
+ } else {
+ /*
+ * tz->temperature has already been updated by generic thermal
+ * layer, before this callback being invoked.
+ */
+ int i = tz->trips.passive.tc1 * (tz->temperature -
+ tz->last_temperature) +
+ tz->trips.passive.tc2 * (tz->temperature -
+ tz->trips.passive.temperature);
+
+ if (i > 0)
+ *trend = THERMAL_TREND_RAISING;
+ else if (i < 0)
+ *trend = THERMAL_TREND_DROPPING;
+ else
+ *trend = THERMAL_TREND_STABLE;
}
- /*
- * tz->temperature has already been updated by generic thermal layer,
- * before this callback being invoked
- */
- i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
- tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
-
- if (i > 0)
- *trend = THERMAL_TREND_RAISING;
- else if (i < 0)
- *trend = THERMAL_TREND_DROPPING;
- else
- *trend = THERMAL_TREND_STABLE;
+out:
+ mutex_unlock(&tz->thermal_check_lock);
- return 0;
+ return ret;
}
static void acpi_thermal_zone_device_hot(struct thermal_zone_device *thermal)
From: Rafael J. Wysocki <[email protected]>
Trip points with temperature set to THERMAL_TEMP_INVALID are as good as
disabled, so make handle_thermal_trip() ignore them.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/thermal_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -348,7 +348,8 @@ static void handle_thermal_trip(struct t
struct thermal_trip trip;
/* Ignore disabled trip points */
- if (test_bit(trip_id, &tz->trips_disabled))
+ if (test_bit(trip_id, &tz->trips_disabled) ||
+ trip.temperature == THERMAL_TEMP_INVALID)
return;
__thermal_zone_get_trip(tz, trip_id, &trip);
From: Rafael J. Wysocki <[email protected]>
Some drivers need to update trip point data (temperature and/or
hysteresis) upon notifications from the platform firmware or they
may need to reprogram hardware when trip point parameters are changed
via sysfs. For those purposes, they need to connect struct thermal_trip
to a private data set associated with the trip or the other way around
and using a trip point index for that may not always work, because the
core may need to reorder the trips during thermal zone registration (in
particular, they may need to be sorted).
To allow that to be done without using a trip point index, introduce
a new field in struct thermal_trip that can be pointed by the driver
to its own data structure containing a trip pointer to be initialized
by the core during thermal zone registration. That pointer will then
have to be updated by the core every time the location of the given
trip point object in memory changes.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/thermal_core.c | 20 +++++++++++++++++---
include/linux/thermal.h | 13 +++++++++++++
2 files changed, 30 insertions(+), 3 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -76,16 +76,29 @@ struct thermal_zone_device_ops {
void (*critical)(struct thermal_zone_device *);
};
+struct thermal_trip_ref {
+ struct thermal_trip *trip;
+};
+
/**
* struct thermal_trip - representation of a point in temperature domain
* @temperature: temperature value in miliCelsius
* @hysteresis: relative hysteresis in miliCelsius
* @type: trip point type
+ * @driver_ref: driver's reference to this trip point
+ *
+ * If @driver_ref is not NULL, the trip pointer in the object pointed to by it
+ * will be initialized by the core during thermal zone registration and updated
+ * whenever the location of the given trip object changes. This allows the
+ * driver to access the trip point data without knowing the relative ordering
+ * of trips within the trip table used by the core and, given a trip pointer,
+ * to get back to its private data associated with the given trip.
*/
struct thermal_trip {
int temperature;
int hysteresis;
enum thermal_trip_type type;
+ struct thermal_trip_ref *driver_ref;
};
struct thermal_cooling_device_ops {
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1306,14 +1306,28 @@ thermal_zone_device_register_with_trips(
if (result)
goto release_device;
+ mutex_lock(&tz->lock);
+
for (count = 0; count < num_trips; count++) {
- struct thermal_trip trip;
+ int temperature = 0;
+
+ if (trips) {
+ temperature = trips[count].temperature;
+ if (trips[count].driver_ref)
+ trips[count].driver_ref->trip = &trips[count];
+ } else {
+ struct thermal_trip trip;
- result = thermal_zone_get_trip(tz, count, &trip);
- if (result || !trip.temperature)
+ result = __thermal_zone_get_trip(tz, count, &trip);
+ if (!result)
+ temperature = trip.temperature;
+ }
+ if (!temperature)
set_bit(count, &tz->trips_disabled);
}
+ mutex_unlock(&tz->lock);
+
/* Update 'this' zone's governor information */
mutex_lock(&thermal_governor_lock);
Hi Rafael,
On 18/07/2023 20:01, Rafael J. Wysocki wrote:
> Hi Everyone,
>
> This patch series makes the ACPI thermal driver register thermal zones
> with the help of thermal_zone_device_register_with_trips(), so it
> doesn't need to use the thermal zone callbacks related to trip points
> any more (and they are dropped in the last patch).
Yay!
> The approach presented here is quite radically different from the
> previous attempts, as it doesn't really rearrange the driver's
> internal data structures, but adds the trip table support on top of
> them. For this purpose, it uses an additional field in struct thermal_trip
> introduced in the first patch.
>
> I have run it on my test-bed systems, but this is not too representative,
> because they each have only one ACPI thermal zone with only one (critical)
> trip point in it.
Rui created some ACPI fake tables I was able to run them in a KVM
machine with fake thermal zones.
I can share the setup if you are interested in
--
<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
On Wed, Jul 19, 2023 at 12:46 AM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Rafael,
>
> On 18/07/2023 20:01, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > This patch series makes the ACPI thermal driver register thermal zones
> > with the help of thermal_zone_device_register_with_trips(), so it
> > doesn't need to use the thermal zone callbacks related to trip points
> > any more (and they are dropped in the last patch).
>
> Yay!
>
> > The approach presented here is quite radically different from the
> > previous attempts, as it doesn't really rearrange the driver's
> > internal data structures, but adds the trip table support on top of
> > them. For this purpose, it uses an additional field in struct thermal_trip
> > introduced in the first patch.
> >
> > I have run it on my test-bed systems, but this is not too representative,
> > because they each have only one ACPI thermal zone with only one (critical)
> > trip point in it.
>
> Rui created some ACPI fake tables I was able to run them in a KVM
> machine with fake thermal zones.
>
> I can share the setup if you are interested in
Yes, please!
On Tue, Jul 18, 2023 at 8:21 PM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> Rework the ACPI thermal driver's .get_trend() callback function,
> thermal_get_trend(), to use trip point data stored in the generic
> trip structures instead of calling thermal_get_trip_type() and
> thermal_get_trip_temp() and make it hold thermal_check_lock to
> protect against possible races against trip point updates.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/thermal.c | 107 +++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 78 insertions(+), 29 deletions(-)
>
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -572,47 +572,96 @@ static int thermal_get_crit_temp(struct
> return -EINVAL;
> }
>
> +static struct thermal_trip *acpi_thermal_get_trip(struct acpi_thermal *tz,
> + int trip_index)
> +{
> + struct thermal_trip *trip;
> + int i;
> +
> + if (!tz || trip_index < 0)
> + return NULL;
> +
> + trip = tz->trips.critical.trip_ref.trip;
> + if (trip) {
> + if (!trip_index)
> + return trip;
> +
> + trip_index--;
> + }
> +
> + trip = tz->trips.hot.trip_ref.trip;
> + if (trip) {
> + if (!trip_index)
> + return trip;
> +
> + trip_index--;
> + }
> +
> + trip = tz->trips.passive.trip_ref.trip;
> + if (trip) {
> + if (!trip_index)
> + return trip;
> +
> + trip_index--;
> + }
> +
> + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
> + trip = tz->trips.active[i].trip_ref.trip;
> + if (trip) {
> + if (!trip_index)
> + return trip;
> +
> + trip_index--;
> + }
> + }
> +
> + return NULL;
> +}
> +
> static int thermal_get_trend(struct thermal_zone_device *thermal,
> - int trip, enum thermal_trend *trend)
> + int trip_index, enum thermal_trend *trend)
> {
> struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
> - enum thermal_trip_type type;
> - int i;
> + struct thermal_trip *trip;
> + int ret = 0;
>
> - if (thermal_get_trip_type(thermal, trip, &type))
> - return -EINVAL;
> + mutex_lock(&tz->thermal_check_lock);
>
> - if (type == THERMAL_TRIP_ACTIVE) {
> - int trip_temp;
> + trip = acpi_thermal_get_trip(tz, trip_index);
> + if (!trip) {
This should also return an error for trips with invalid temperature.
Moreover, an error should be returned for the critical and hot trips,
because it doesn't make sense to deal with them here.
It looks like a new version of this patch is needed.
> + ret = -EINVAL;
> + goto out;
> + }
> + if (trip->type == THERMAL_TRIP_ACTIVE) {
> int temp = deci_kelvin_to_millicelsius_with_offset(
> tz->temperature, tz->kelvin_offset);
> - if (thermal_get_trip_temp(thermal, trip, &trip_temp))
> - return -EINVAL;
>
> - if (temp > trip_temp) {
> + if (temp > trip->temperature)
> *trend = THERMAL_TREND_RAISING;
> - return 0;
> - } else {
> - /* Fall back on default trend */
> - return -EINVAL;
> - }
> + else /* Fall back on default trend */
> + ret = -EINVAL;
> + } else {
> + /*
> + * tz->temperature has already been updated by generic thermal
> + * layer, before this callback being invoked.
> + */
> + int i = tz->trips.passive.tc1 * (tz->temperature -
> + tz->last_temperature) +
> + tz->trips.passive.tc2 * (tz->temperature -
> + tz->trips.passive.temperature);
> +
> + if (i > 0)
> + *trend = THERMAL_TREND_RAISING;
> + else if (i < 0)
> + *trend = THERMAL_TREND_DROPPING;
> + else
> + *trend = THERMAL_TREND_STABLE;
> }
>
> - /*
> - * tz->temperature has already been updated by generic thermal layer,
> - * before this callback being invoked
> - */
> - i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
> - tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
> -
> - if (i > 0)
> - *trend = THERMAL_TREND_RAISING;
> - else if (i < 0)
> - *trend = THERMAL_TREND_DROPPING;
> - else
> - *trend = THERMAL_TREND_STABLE;
> +out:
> + mutex_unlock(&tz->thermal_check_lock);
>
> - return 0;
> + return ret;
> }
>
> static void acpi_thermal_zone_device_hot(struct thermal_zone_device *thermal)
>
>
>
Hi Everyone,
This is the second iteration of the $subject patch series and its original
description below is still applicable
On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>
> This patch series makes the ACPI thermal driver register thermal zones
> with the help of thermal_zone_device_register_with_trips(), so it
> doesn't need to use the thermal zone callbacks related to trip points
> any more (and they are dropped in the last patch).
>
> The approach presented here is quite radically different from the
> previous attempts, as it doesn't really rearrange the driver's
> internal data structures, but adds the trip table support on top of
> them. For this purpose, it uses an additional field in struct thermal_trip
> introduced in the first patch.
In the meantime I have updated the patches and tested them on a system with
a couple of ACPI thermal zones. No differences in functionality after applying
the patches have been observed.
The update is mostly related to adding extra locking around trip point
temperature updates and some hardening of the .get_trend() callback routine
against invalid trip point indices.
Please see patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Add thermal_zone_device_lock() and thermal_zone_device_unlock() for
acquiring and releasing the thermal zone lock, respectively.
They will be used by the ACPI thermal driver to protect trip point
temperature updates against races with accesses from elsewhere.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2: New patch.
---
drivers/thermal/thermal_core.c | 13 +++++++++++++
include/linux/thermal.h | 2 ++
2 files changed, 15 insertions(+)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -497,6 +498,18 @@ void thermal_zone_device_update(struct t
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
+void thermal_zone_device_lock(struct thermal_zone_device *tz)
+{
+ mutex_lock(&tz->lock);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device_lock);
+
+void thermal_zone_device_unlock(struct thermal_zone_device *tz)
+{
+ mutex_unlock(&tz->lock);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device_unlock);
+
static void thermal_zone_device_check(struct work_struct *work)
{
struct thermal_zone_device *tz = container_of(work, struct
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -336,6 +336,8 @@ int thermal_zone_unbind_cooling_device(s
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *,
enum thermal_notify_event);
+void thermal_zone_device_lock(struct thermal_zone_device *tz);
+void thermal_zone_device_unlock(struct thermal_zone_device *tz);
struct thermal_cooling_device *thermal_cooling_device_register(const char *,
void *, const struct thermal_cooling_device_ops *);
From: Rafael J. Wysocki <[email protected]>
Some drivers need to update trip point data (temperature and/or
hysteresis) upon notifications from the platform firmware or they
may need to reprogram hardware when trip point parameters are changed
via sysfs. For those purposes, they need to connect struct thermal_trip
to a private data set associated with the trip or the other way around
and using a trip point index for that may not always work, because the
core may need to reorder the trips during thermal zone registration (in
particular, they may need to be sorted).
To allow that to be done without using a trip point index, introduce
a new field in struct thermal_trip that can be pointed by the driver
to its own data structure containing a trip pointer to be initialized
by the core during thermal zone registration. That pointer will then
have to be updated by the core every time the location of the given
trip point object in memory changes.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2: No changes.
---
drivers/thermal/thermal_core.c | 20 +++++++++++++++++---
include/linux/thermal.h | 13 +++++++++++++
2 files changed, 30 insertions(+), 3 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -76,16 +76,29 @@ struct thermal_zone_device_ops {
void (*critical)(struct thermal_zone_device *);
};
+struct thermal_trip_ref {
+ struct thermal_trip *trip;
+};
+
/**
* struct thermal_trip - representation of a point in temperature domain
* @temperature: temperature value in miliCelsius
* @hysteresis: relative hysteresis in miliCelsius
* @type: trip point type
+ * @driver_ref: driver's reference to this trip point
+ *
+ * If @driver_ref is not NULL, the trip pointer in the object pointed to by it
+ * will be initialized by the core during thermal zone registration and updated
+ * whenever the location of the given trip object changes. This allows the
+ * driver to access the trip point data without knowing the relative ordering
+ * of trips within the trip table used by the core and, given a trip pointer,
+ * to get back to its private data associated with the given trip.
*/
struct thermal_trip {
int temperature;
int hysteresis;
enum thermal_trip_type type;
+ struct thermal_trip_ref *driver_ref;
};
struct thermal_cooling_device_ops {
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1306,14 +1306,28 @@ thermal_zone_device_register_with_trips(
if (result)
goto release_device;
+ mutex_lock(&tz->lock);
+
for (count = 0; count < num_trips; count++) {
- struct thermal_trip trip;
+ int temperature = 0;
+
+ if (trips) {
+ temperature = trips[count].temperature;
+ if (trips[count].driver_ref)
+ trips[count].driver_ref->trip = &trips[count];
+ } else {
+ struct thermal_trip trip;
- result = thermal_zone_get_trip(tz, count, &trip);
- if (result || !trip.temperature)
+ result = __thermal_zone_get_trip(tz, count, &trip);
+ if (!result)
+ temperature = trip.temperature;
+ }
+ if (!temperature)
set_bit(count, &tz->trips_disabled);
}
+ mutex_unlock(&tz->lock);
+
/* Update 'this' zone's governor information */
mutex_lock(&thermal_governor_lock);
From: Rafael J. Wysocki <[email protected]>
Make the ACPI thermal driver use thermal_zone_device_register_with_trips()
to register its thermal zones.
For this purpose, make it create a trip point table and pass it to
thermal_zone_device_register_with_trips() as an argument and use the
struct thermal_trip_ref introduced previously to connect the generic
thermal trip structures to the internal data structures representing
trip points in the driver.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 102 ++++++++++++++++++++++++++++++++++++++---
drivers/thermal/thermal_core.c | 1
include/linux/thermal.h | 2
3 files changed, 100 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -97,16 +97,19 @@ MODULE_PARM_DESC(psv, "Disable or overri
static struct workqueue_struct *acpi_thermal_pm_queue;
struct acpi_thermal_critical {
+ struct thermal_trip_ref trip_ref;
unsigned long temperature;
bool valid;
};
struct acpi_thermal_hot {
+ struct thermal_trip_ref trip_ref;
unsigned long temperature;
bool valid;
};
struct acpi_thermal_passive {
+ struct thermal_trip_ref trip_ref;
struct acpi_handle_list devices;
unsigned long temperature;
unsigned long tc1;
@@ -116,6 +119,7 @@ struct acpi_thermal_passive {
};
struct acpi_thermal_active {
+ struct thermal_trip_ref trip_ref;
struct acpi_handle_list devices;
unsigned long temperature;
bool valid;
@@ -137,6 +141,7 @@ struct acpi_thermal {
unsigned long polling_frequency;
volatile u8 zombie;
struct acpi_thermal_trips trips;
+ struct thermal_trip *trip_table;
struct acpi_handle_list devices;
struct thermal_zone_device *thermal_zone;
int kelvin_offset; /* in millidegrees */
@@ -190,6 +195,14 @@ static int acpi_thermal_get_polling_freq
return 0;
}
+static void acpi_thermal_trip_update_temp(struct acpi_thermal *tz,
+ struct thermal_trip *trip,
+ long temperature)
+{
+ trip->temperature = deci_kelvin_to_millicelsius_with_offset(temperature,
+ tz->kelvin_offset);
+}
+
static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
{
acpi_status status;
@@ -756,6 +769,7 @@ static void acpi_thermal_zone_sysfs_remo
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
+ struct thermal_trip *trip;
int passive_delay = 0;
int trip_count = 0;
int result;
@@ -776,10 +790,52 @@ static int acpi_thermal_register_thermal
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++)
trip_count++;
- tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
- tz, &acpi_thermal_zone_ops,
- NULL, passive_delay,
- tz->polling_frequency * 100);
+ tz->trip_table = kcalloc(trip_count, sizeof(*tz->trip_table), GFP_KERNEL);
+ if (!tz->trip_table)
+ return -ENOMEM;
+
+ trip = tz->trip_table;
+
+ if (tz->trips.critical.valid) {
+ trip->type = THERMAL_TRIP_CRITICAL;
+ acpi_thermal_trip_update_temp(tz, trip,
+ tz->trips.critical.temperature);
+ trip->driver_ref = &tz->trips.critical.trip_ref;
+ trip++;
+ }
+
+ if (tz->trips.hot.valid) {
+ trip->type = THERMAL_TRIP_HOT;
+ acpi_thermal_trip_update_temp(tz, trip,
+ tz->trips.hot.temperature);
+ trip->driver_ref = &tz->trips.hot.trip_ref;
+ trip++;
+ }
+
+ if (tz->trips.passive.valid) {
+ trip->type = THERMAL_TRIP_PASSIVE;
+ acpi_thermal_trip_update_temp(tz, trip,
+ tz->trips.passive.temperature);
+ trip->driver_ref = &tz->trips.passive.trip_ref;
+ trip++;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++) {
+ trip->type = THERMAL_TRIP_ACTIVE;
+ acpi_thermal_trip_update_temp(tz, trip,
+ tz->trips.active[i].temperature);
+ trip->driver_ref = &tz->trips.active[i].trip_ref;
+ trip++;
+ }
+
+ tz->thermal_zone = thermal_zone_device_register_with_trips("acpitz",
+ tz->trip_table,
+ trip_count,
+ 0, tz,
+ &acpi_thermal_zone_ops,
+ NULL,
+ passive_delay,
+ tz->polling_frequency * 100);
if (IS_ERR(tz->thermal_zone))
return -ENODEV;
@@ -817,6 +873,7 @@ static void acpi_thermal_unregister_ther
{
acpi_thermal_zone_sysfs_remove(tz);
thermal_zone_device_unregister(tz->thermal_zone);
+ kfree(tz->trip_table);
tz->thermal_zone = NULL;
acpi_bus_detach_private_data(tz->device->handle);
}
@@ -950,6 +1007,9 @@ static void acpi_thermal_check_fn(struct
{
struct acpi_thermal *tz = container_of(work, struct acpi_thermal,
thermal_check_work);
+ struct thermal_trip *trip;
+ long temperature;
+ int i;
/*
* In general, it is not sufficient to check the pending bit, because
@@ -964,7 +1024,39 @@ static void acpi_thermal_check_fn(struct
mutex_lock(&tz->thermal_check_lock);
- thermal_zone_device_update(tz->thermal_zone, THERMAL_EVENT_UNSPECIFIED);
+ thermal_zone_device_lock(tz->thermal_zone);
+
+ trip = tz->trips.passive.trip_ref.trip;
+ if (trip) {
+ /*
+ * This means that the passive trip was valid initially, so
+ * update its temperature in case it has changed or the trip
+ * has become invalid.
+ */
+ temperature = tz->trips.passive.valid ?
+ tz->trips.passive.temperature :
+ THERMAL_TEMP_INVALID;
+ acpi_thermal_trip_update_temp(tz, trip, temperature);
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ trip = tz->trips.active[i].trip_ref.trip;
+ if (trip) {
+ /*
+ * This means that the active trip #i was valid
+ * initially, so update its temperature in case it has
+ * changed or the trip has become invalid.
+ */
+ temperature = tz->trips.active[i].valid ?
+ tz->trips.active[i].temperature :
+ THERMAL_TEMP_INVALID;
+ acpi_thermal_trip_update_temp(tz, trip, temperature);
+ }
+ }
+
+ __thermal_zone_device_update(tz->thermal_zone, THERMAL_EVENT_UNSPECIFIED);
+
+ thermal_zone_device_unlock(tz->thermal_zone);
refcount_inc(&tz->thermal_check_count);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -429,6 +429,7 @@ void __thermal_zone_device_update(struct
monitor_thermal_zone(tz);
}
+EXPORT_SYMBOL_GPL(__thermal_zone_device_update);
static int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
enum thermal_device_mode mode)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -334,6 +334,8 @@ int thermal_zone_bind_cooling_device(str
unsigned int);
int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
struct thermal_cooling_device *);
+void __thermal_zone_device_update(struct thermal_zone_device *,
+ enum thermal_notify_event);
void thermal_zone_device_update(struct thermal_zone_device *,
enum thermal_notify_event);
void thermal_zone_device_lock(struct thermal_zone_device *tz);
From: Rafael J. Wysocki <[email protected]>
Drop the .get_trip_type(), .get_trip_temp() and .get_crit_temp() thermal
zone callbacks that are not necessary any more from the ACPI thermal
driver along with the corresponding callback functions.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 115 -------------------------------------------------
1 file changed, 115 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -465,118 +465,6 @@ static int thermal_get_temp(struct therm
return 0;
}
-static int thermal_get_trip_type(struct thermal_zone_device *thermal,
- int trip, enum thermal_trip_type *type)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- int i;
-
- if (!tz || trip < 0)
- return -EINVAL;
-
- if (tz->trips.critical.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_CRITICAL;
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.hot.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_HOT;
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.passive.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_PASSIVE;
- return 0;
- }
- trip--;
- }
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++) {
- if (!trip) {
- *type = THERMAL_TRIP_ACTIVE;
- return 0;
- }
- trip--;
- }
-
- return -EINVAL;
-}
-
-static int thermal_get_trip_temp(struct thermal_zone_device *thermal,
- int trip, int *temp)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- int i;
-
- if (!tz || trip < 0)
- return -EINVAL;
-
- if (tz->trips.critical.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.critical.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.hot.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.hot.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.passive.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.passive.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE &&
- tz->trips.active[i].valid; i++) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.active[i].temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- return -EINVAL;
-}
-
-static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
- int *temperature)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
-
- if (tz->trips.critical.valid) {
- *temperature = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.critical.temperature,
- tz->kelvin_offset);
- return 0;
- }
-
- return -EINVAL;
-}
-
static struct thermal_trip *acpi_thermal_get_trip(struct acpi_thermal *tz,
int trip_index)
{
@@ -781,9 +669,6 @@ static struct thermal_zone_device_ops ac
.bind = acpi_thermal_bind_cooling_device,
.unbind = acpi_thermal_unbind_cooling_device,
.get_temp = thermal_get_temp,
- .get_trip_type = thermal_get_trip_type,
- .get_trip_temp = thermal_get_trip_temp,
- .get_crit_temp = thermal_get_crit_temp,
.get_trend = thermal_get_trend,
.hot = acpi_thermal_zone_device_hot,
.critical = acpi_thermal_zone_device_critical,
From: Rafael J. Wysocki <[email protected]>
Rename the trips variable in acpi_thermal_register_thermal_zone() to
trip_count so its name better reflects the purpose, rearrange white
space in the loop over active trips for clarity and reduce code
duplication related to calling thermal_zone_device_register() by
using an extra local variable to store the passive delay value.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2: No changes.
---
drivers/acpi/thermal.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -745,34 +745,30 @@ static void acpi_thermal_zone_sysfs_remo
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
- int trips = 0;
+ int passive_delay = 0;
+ int trip_count = 0;
int result;
acpi_status status;
int i;
if (tz->trips.critical.valid)
- trips++;
+ trip_count++;
if (tz->trips.hot.valid)
- trips++;
-
- if (tz->trips.passive.valid)
- trips++;
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid;
- i++, trips++);
-
- if (tz->trips.passive.valid)
- tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz,
- &acpi_thermal_zone_ops, NULL,
- tz->trips.passive.tsp * 100,
- tz->polling_frequency * 100);
- else
- tz->thermal_zone =
- thermal_zone_device_register("acpitz", trips, 0, tz,
- &acpi_thermal_zone_ops, NULL,
- 0, tz->polling_frequency * 100);
+ trip_count++;
+ if (tz->trips.passive.valid) {
+ trip_count++;
+ passive_delay = tz->trips.passive.tsp * 100;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++)
+ trip_count++;
+
+ tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
+ tz, &acpi_thermal_zone_ops,
+ NULL, passive_delay,
+ tz->polling_frequency * 100);
if (IS_ERR(tz->thermal_zone))
return -ENODEV;
From: Rafael J. Wysocki <[email protected]>
There is a race condition between acpi_thermal_trips_update() and
acpi_thermal_check_fn(), because the trip points may get updated while
the latter is running which in theory may lead to inconsistent results.
For example, if two trips are updated together, using the temperature
value of one of them from before the update and the temperature value
of the other one from after the update may not lead to the expected
outcome.
To address this, make acpi_thermal_trips_update() hold the thermal zone
lock across the entire update of trip points.
While at it, change the acpi_thermal_trips_update() return data type
to void as that function always returns 0 anyway.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2:
* Hold the thermal zone lock instead of thermal_check_lock around trip
point updates (this also helps to protect thermal_get_trend() from using
stale trip temperatures).
* Add a comment documenting the purpose of the locking.
* Make acpi_thermal_trips_update() void.
---
drivers/acpi/thermal.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -190,7 +190,7 @@ static int acpi_thermal_get_polling_freq
return 0;
}
-static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
+static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
{
acpi_status status;
unsigned long long tmp;
@@ -398,17 +398,28 @@ static int acpi_thermal_trips_update(str
ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device");
}
}
+}
- return 0;
+static void acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
+{
+ /*
+ * The locking is needed here to protect thermal_get_trend() from using
+ * a stale passive trip temperature and to synchronize with the trip
+ * temperature updates in acpi_thermal_check_fn().
+ */
+ thermal_zone_device_lock(tz->thermal_zone);
+
+ __acpi_thermal_trips_update(tz, flag);
+
+ thermal_zone_device_unlock(tz->thermal_zone);
}
static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
{
- int i, ret = acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
bool valid;
+ int i;
- if (ret)
- return ret;
+ __acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
valid = tz->trips.critical.valid |
tz->trips.hot.valid |
From: Rafael J. Wysocki <[email protected]>
Rework the ACPI thermal driver's .get_trend() callback function,
thermal_get_trend(), to use trip point data stored in the generic
trip structures instead of calling thermal_get_trip_type() and
thermal_get_trip_temp().
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2:
* Do not acquire thermal_check_lock in thermal_get_trend() (lockdep
would complain about this, because it is hold around thermal zone
locking and .get_trend() runs under the thermal zone lock). The
thermal zone locking added in the previous patches is sufficient
to protect this code.
* Check trips against invalid temperature values.
* Return an error for trips other than passive and active.
---
drivers/acpi/thermal.c | 106 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 78 insertions(+), 28 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -577,47 +577,97 @@ static int thermal_get_crit_temp(struct
return -EINVAL;
}
-static int thermal_get_trend(struct thermal_zone_device *thermal,
- int trip, enum thermal_trend *trend)
+static struct thermal_trip *acpi_thermal_get_trip(struct acpi_thermal *tz,
+ int trip_index)
{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- enum thermal_trip_type type;
+ struct thermal_trip *trip;
int i;
- if (thermal_get_trip_type(thermal, trip, &type))
- return -EINVAL;
+ if (!tz || trip_index < 0)
+ return NULL;
- if (type == THERMAL_TRIP_ACTIVE) {
- int trip_temp;
- int temp = deci_kelvin_to_millicelsius_with_offset(
- tz->temperature, tz->kelvin_offset);
- if (thermal_get_trip_temp(thermal, trip, &trip_temp))
- return -EINVAL;
+ trip = tz->trips.critical.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
- if (temp > trip_temp) {
- *trend = THERMAL_TREND_RAISING;
- return 0;
- } else {
- /* Fall back on default trend */
- return -EINVAL;
+ trip_index--;
+ }
+
+ trip = tz->trips.hot.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+
+ trip = tz->trips.passive.trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ trip = tz->trips.active[i].trip_ref.trip;
+ if (trip) {
+ if (!trip_index)
+ return trip;
+
+ trip_index--;
}
}
+ return NULL;
+}
+
+static int thermal_get_trend(struct thermal_zone_device *thermal,
+ int trip_index, enum thermal_trend *trend)
+{
+ struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
+ struct thermal_trip *trip;
+ long t;
+
+ trip = acpi_thermal_get_trip(tz, trip_index);
+ if (!trip || trip->temperature == THERMAL_TEMP_INVALID)
+ return -EINVAL;
+
/*
* tz->temperature has already been updated by generic thermal layer,
- * before this callback being invoked
+ * before this callback being invoked.
*/
- i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
- tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
+ switch (trip->type) {
+ case THERMAL_TRIP_PASSIVE:
+ t = tz->trips.passive.tc1 * (tz->temperature -
+ tz->last_temperature) +
+ tz->trips.passive.tc2 * (tz->temperature -
+ tz->trips.passive.temperature);
+ if (t > 0)
+ *trend = THERMAL_TREND_RAISING;
+ else if (t < 0)
+ *trend = THERMAL_TREND_DROPPING;
+ else
+ *trend = THERMAL_TREND_STABLE;
+
+ return 0;
+
+ case THERMAL_TRIP_ACTIVE:
+ t = deci_kelvin_to_millicelsius_with_offset(tz->temperature,
+ tz->kelvin_offset);
+ if (t > trip->temperature) {
+ *trend = THERMAL_TREND_RAISING;
+ return 0;
+ }
- if (i > 0)
- *trend = THERMAL_TREND_RAISING;
- else if (i < 0)
- *trend = THERMAL_TREND_DROPPING;
- else
- *trend = THERMAL_TREND_STABLE;
+ fallthrough;
- return 0;
+ default:
+ break;
+ }
+
+ return -EINVAL;
}
static void acpi_thermal_zone_device_hot(struct thermal_zone_device *thermal)
Hi Rafael,
could you wait before applying I would like to review the series but I'm
OoO ATM, coming back next week?
On 21/07/2023 14:44, Rafael J. Wysocki wrote:
> Hi Everyone,
>
> This is the second iteration of the $subject patch series and its original
> description below is still applicable
>
> On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>>
>> This patch series makes the ACPI thermal driver register thermal zones
>> with the help of thermal_zone_device_register_with_trips(), so it
>> doesn't need to use the thermal zone callbacks related to trip points
>> any more (and they are dropped in the last patch).
>>
>> The approach presented here is quite radically different from the
>> previous attempts, as it doesn't really rearrange the driver's
>> internal data structures, but adds the trip table support on top of
>> them. For this purpose, it uses an additional field in struct thermal_trip
>> introduced in the first patch.
>
> In the meantime I have updated the patches and tested them on a system with
> a couple of ACPI thermal zones. No differences in functionality after applying
> the patches have been observed.
>
> The update is mostly related to adding extra locking around trip point
> temperature updates and some hardening of the .get_trend() callback routine
> against invalid trip point indices.
>
> Please see patch changelogs for details.
>
> Thanks!
>
>
>
--
<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 Sun, Jul 23, 2023 at 12:19 PM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Rafael,
>
> could you wait before applying I would like to review the series but I'm
> OoO ATM, coming back next week?
Yes, I can wait.
Thanks!
Hi Everyone,
This is the second iteration of the $subject patch series and its original
description below is still applicable
On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>
> This patch series makes the ACPI thermal driver register thermal zones
> with the help of thermal_zone_device_register_with_trips(), so it
> doesn't need to use the thermal zone callbacks related to trip points
> any more (and they are dropped in the last patch).
>
> The approach presented here is quite radically different from the
> previous attempts, as it doesn't really rearrange the driver's
> internal data structures, but adds the trip table support on top of
> them. For this purpose, it uses an additional field in struct thermal_trip
> introduced in the first patch.
This update is mostly related to the observation that the critical and hot trip
points never change after initialization, so they don't really need to be
connected back to the corresponding thermal_trip structures. It also fixes
an error code path memory leak in patch [5/8].
Thanks!
On Tue, Aug 1, 2023 at 8:27 PM Daniel Lezcano <[email protected]> wrote:
>
>
> Hi Rafael,
>
> On 25/07/2023 14:02, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > This is the second iteration of the $subject patch series and its original
> > description below is still applicable
> >
> > On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
> >>
> >> This patch series makes the ACPI thermal driver register thermal zones
> >> with the help of thermal_zone_device_register_with_trips(), so it
> >> doesn't need to use the thermal zone callbacks related to trip points
> >> any more (and they are dropped in the last patch).
> >>
> >> The approach presented here is quite radically different from the
> >> previous attempts, as it doesn't really rearrange the driver's
> >> internal data structures, but adds the trip table support on top of
> >> them. For this purpose, it uses an additional field in struct thermal_trip
> >> introduced in the first patch.
> >
> > This update is mostly related to the observation that the critical and hot trip
> > points never change after initialization, so they don't really need to be
> > connected back to the corresponding thermal_trip structures. It also fixes
> > an error code path memory leak in patch [5/8].
>
> I've been through the series. It is really cool that we can get rid of
> the ops usage at the end of the series.
>
> However, the series introduces a wrapper to the thermal zone lock and
> exports that in the public header. That goes in the opposite direction
> of the recent cleanups and obviously will give the opportunity to
> drivers to do silly things [again].
Because it is necessary to update the trip points in the table under
the zone lock, the driver needs to somehow make that happen.
I suppose I can pass a callback to thermal_zone_device_update() or
similar, but I'm not sure if that's better.
> On the other side, the structure thermal_trip introduces a circular
> reference, which is usually something to avoid.
What do you mean by "a circular reference"?
> Apart those two points, the ACPI changes look ok.
>
> Comments in the different patches will follow
Thanks!
Hi Rafael,
On 25/07/2023 14:02, Rafael J. Wysocki wrote:
> Hi Everyone,
>
> This is the second iteration of the $subject patch series and its original
> description below is still applicable
>
> On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>>
>> This patch series makes the ACPI thermal driver register thermal zones
>> with the help of thermal_zone_device_register_with_trips(), so it
>> doesn't need to use the thermal zone callbacks related to trip points
>> any more (and they are dropped in the last patch).
>>
>> The approach presented here is quite radically different from the
>> previous attempts, as it doesn't really rearrange the driver's
>> internal data structures, but adds the trip table support on top of
>> them. For this purpose, it uses an additional field in struct thermal_trip
>> introduced in the first patch.
>
> This update is mostly related to the observation that the critical and hot trip
> points never change after initialization, so they don't really need to be
> connected back to the corresponding thermal_trip structures. It also fixes
> an error code path memory leak in patch [5/8].
I've been through the series. It is really cool that we can get rid of
the ops usage at the end of the series.
However, the series introduces a wrapper to the thermal zone lock and
exports that in the public header. That goes in the opposite direction
of the recent cleanups and obviously will give the opportunity to
drivers to do silly things [again].
On the other side, the structure thermal_trip introduces a circular
reference, which is usually something to avoid.
Apart those two points, the ACPI changes look ok.
Comments in the different patches will follow
Thanks
--
<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 Everyone,
This is the 4th iteration of the $subject patch series and its original
description below is still applicable
On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>
> This patch series makes the ACPI thermal driver register thermal zones
> with the help of thermal_zone_device_register_with_trips(), so it
> doesn't need to use the thermal zone callbacks related to trip points
> any more (and they are dropped in the last patch).
>
> The approach presented here is quite radically different from the
> previous attempts, as it doesn't really rearrange the driver's
> internal data structures, but adds the trip table support on top of
> them. For this purpose, it uses an additional field in struct thermal_trip
This update uses a different approach to mutual exclusion between trip point
updates in the ACPI thermal driver and the thermal core (roughly speaking, it
adds a "thermal zone update" callback and a helper to invoke that callback
under the zone lock) and a different mechanism to get from the local trip
point representation in the driver to the corresponding trips[i] in the
thermal zone structure.
As a result, some have been dropped, some new patches have been added and
the majority of patches in the series have been reworked from scratch.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Rework the ACPI thermal driver's .get_trend() callback function,
thermal_get_trend(), so that it does not call thermal_get_trip_type()
and thermal_get_trip_temp() which are going to be dropped.
This reduces the overhead of the function too, because it will always
carry out a trip point lookup once after the change.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v3 -> v4:
* Adjust for the lack of a direct way to get from the local trip point
representations to trips[i].
v2 -> v3: Rebase on top of the v2 of the previous patch.
v1 -> v2:
* Do not acquire thermal_check_lock in thermal_get_trend() (lockdep
would complain about this, because it is hold around thermal zone
locking and .get_trend() runs under the thermal zone lock). The
thermal zone locking added in the previous patches is sufficient
to protect this code.
* Check trips against invalid temperature values.
* Return an error for trips other than passive and active.
---
drivers/acpi/thermal.c | 68 +++++++++++++++++++++++++++----------------------
1 file changed, 38 insertions(+), 30 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -616,46 +616,54 @@ static int thermal_get_crit_temp(struct
}
static int thermal_get_trend(struct thermal_zone_device *thermal,
- int trip, enum thermal_trend *trend)
+ int trip_index, enum thermal_trend *trend)
{
struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- enum thermal_trip_type type;
- int i;
+ int t, i;
- if (thermal_get_trip_type(thermal, trip, &type))
+ if (!tz || trip_index < 0)
return -EINVAL;
- if (type == THERMAL_TRIP_ACTIVE) {
- int trip_temp;
- int temp = deci_kelvin_to_millicelsius_with_offset(
- tz->temperature, tz->kelvin_offset);
- if (thermal_get_trip_temp(thermal, trip, &trip_temp))
- return -EINVAL;
+ if (tz->trips.critical.valid)
+ trip_index--;
- if (temp > trip_temp) {
+ if (tz->trips.hot.valid)
+ trip_index--;
+
+ if (trip_index < 0)
+ return -EINVAL;
+
+ if (tz->trips.passive.valid && !trip_index--) {
+ t = tz->trips.passive.tc1 * (tz->temperature -
+ tz->last_temperature) +
+ tz->trips.passive.tc2 * (tz->temperature -
+ tz->trips.passive.temperature);
+ if (t > 0)
*trend = THERMAL_TREND_RAISING;
- return 0;
- } else {
- /* Fall back on default trend */
- return -EINVAL;
- }
+ else if (t < 0)
+ *trend = THERMAL_TREND_DROPPING;
+ else
+ *trend = THERMAL_TREND_STABLE;
+
+ return 0;
}
- /*
- * tz->temperature has already been updated by generic thermal layer,
- * before this callback being invoked
- */
- i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
- tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
-
- if (i > 0)
- *trend = THERMAL_TREND_RAISING;
- else if (i < 0)
- *trend = THERMAL_TREND_DROPPING;
- else
- *trend = THERMAL_TREND_STABLE;
+ t = acpi_thermal_temp(tz, tz->temperature);
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ if (tz->trips.active[i].valid && !trip_index--) {
+ int trip_temp;
+
+ trip_temp = acpi_thermal_temp(tz, tz->trips.active[i].temperature);
+ if (t > trip_temp) {
+ *trend = THERMAL_TREND_RAISING;
+ return 0;
+ }
+ break;
+ }
+ }
- return 0;
+ return -EINVAL;
}
static void acpi_thermal_zone_device_hot(struct thermal_zone_device *thermal)
From: Rafael J. Wysocki <[email protected]>
Introduce a helper routine called thermal_zone_update_trip_temp() that
can be used to update a trip point's temperature with the help of a
pointer to local data associated with that trip point provided by
the thermal driver that created it.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
New patch in v4.
---
drivers/thermal/thermal_trip.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/thermal.h | 4 ++++
2 files changed, 41 insertions(+)
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
return 0;
}
+
+/**
+ * thermal_zone_update_trip_temp - Update the trip point temperature.
+ * @tz: Thermal zone.
+ * @trip_priv: Trip tag.
+ * @temp: New trip temperature.
+ *
+ * This only works for thermal zones using trip tables and its caller must
+ * ensure that the zone lock is held before using it.
+ *
+ * @trip_priv is expected to be the value that has been stored by the driver
+ * in the struct thermal_trip representing the trip point in question, so it
+ * can be matched against the value of the priv field in that structure.
+ *
+ * If @trip_priv does not match any trip point in the trip table of @tz,
+ * nothing happens.
+ */
+void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
+ void *trip_priv, int temperature)
+{
+ int i;
+
+ lockdep_assert_held(&tz->lock);
+
+ if (!tz->trips || !trip_priv)
+ return;
+
+ for (i = 0; i < tz->num_trips; i++) {
+ struct thermal_trip *trip = &tz->trips[i];
+
+ if (trip->priv == trip_priv) {
+ trip->temperature = temperature;
+ return;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -286,9 +286,13 @@ 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);
+void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
+ void *trip_priv, int temperature);
int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
const struct thermal_trip *trip);
+void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
+ void *trip_priv, int temperature);
int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
From: Rafael J. Wysocki <[email protected]>
Introduce a new thermal zone device operation called .update() for
modifying thermal zone components such as trip point and a new helper
function, thermal_zone_device_adjust(), that can be used by drivers
providing the new thermal zone device operation to invoke it under
the zone lock.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
New patch in v4.
---
drivers/thermal/thermal_core.c | 20 ++++++++++++++++++++
include/linux/thermal.h | 2 ++
2 files changed, 22 insertions(+)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -74,6 +74,7 @@ struct thermal_zone_device_ops {
enum thermal_trend *);
void (*hot)(struct thermal_zone_device *);
void (*critical)(struct thermal_zone_device *);
+ void (*update)(struct thermal_zone_device *, unsigned long);
};
/**
@@ -323,6 +324,7 @@ int thermal_zone_unbind_cooling_device(s
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *,
enum thermal_notify_event);
+void thermal_zone_device_adjust(struct thermal_zone_device *tz, unsigned long data);
struct thermal_cooling_device *thermal_cooling_device_register(const char *,
void *, const struct thermal_cooling_device_ops *);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -497,6 +497,26 @@ void thermal_zone_device_update(struct t
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
+/**
+ * thermal_zone_device_adjust - Adjust a thermal zone.
+ * @tz: Thermal zone.
+ * @data: Data to pass to the zone's .update() callback.
+ *
+ * Modify components of a thermal zone (for example, trip points) via
+ * its .update() callback (for example, after a platform configuration
+ * change).
+ */
+void thermal_zone_device_adjust(struct thermal_zone_device *tz, unsigned long data)
+{
+ mutex_lock(&tz->lock);
+
+ if (device_is_registered(&tz->device) && tz->ops->update)
+ tz->ops->update(tz, data);
+
+ mutex_unlock(&tz->lock);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device_adjust);
+
static void thermal_zone_device_check(struct work_struct *work)
{
struct thermal_zone_device *tz = container_of(work, struct
On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Introduce a helper routine called thermal_zone_update_trip_temp() that
> can be used to update a trip point's temperature with the help of a
> pointer to local data associated with that trip point provided by
> the thermal driver that created it.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> New patch in v4.
>
> ---
> drivers/thermal/thermal_trip.c | 37 +++++++++++++++++++++++++++++++++++++
> include/linux/thermal.h | 4 ++++
> 2 files changed, 41 insertions(+)
>
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
>
> return 0;
> }
> +
> +/**
> + * thermal_zone_update_trip_temp - Update the trip point temperature.
> + * @tz: Thermal zone.
> + * @trip_priv: Trip tag.
> + * @temp: New trip temperature.
> + *
> + * This only works for thermal zones using trip tables and its caller must
> + * ensure that the zone lock is held before using it.
> + *
> + * @trip_priv is expected to be the value that has been stored by the driver
> + * in the struct thermal_trip representing the trip point in question, so it
> + * can be matched against the value of the priv field in that structure.
> + *
> + * If @trip_priv does not match any trip point in the trip table of @tz,
> + * nothing happens.
> + */
> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> + void *trip_priv, int temperature)
> +{
> + int i;
> +
> + lockdep_assert_held(&tz->lock);
> +
> + if (!tz->trips || !trip_priv)
> + return;
> +
> + for (i = 0; i < tz->num_trips; i++) {
> + struct thermal_trip *trip = &tz->trips[i];
> +
> + if (trip->priv == trip_priv) {
> + trip->temperature = temperature;
> + return;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
This function would imply the comparator is always trip->priv but if we
want another comparison eg. trip->priv->id, that won't be possible.
Actually, I think you can reuse an existing function with a simple
change, for_each_thermal_trip() located in thermal_core.h.
The changes would be renaming it without the '__' prefix and moving it
in include/linux/thermal.h.
Then the comparison function and the temperature change can be an ACPI
driver specific callback passed as parameter to for_each_thermal_zone
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -286,9 +286,13 @@ 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);
> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> + void *trip_priv, int temperature);
>
> int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> const struct thermal_trip *trip);
> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> + void *trip_priv, int temperature);
>
> int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
>
>
>
>
--
<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
On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <[email protected]> wrote:
>
> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Introduce a helper routine called thermal_zone_update_trip_temp() that
> > can be used to update a trip point's temperature with the help of a
> > pointer to local data associated with that trip point provided by
> > the thermal driver that created it.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >
> > New patch in v4.
> >
> > ---
> > drivers/thermal/thermal_trip.c | 37 +++++++++++++++++++++++++++++++++++++
> > include/linux/thermal.h | 4 ++++
> > 2 files changed, 41 insertions(+)
> >
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
> >
> > return 0;
> > }
> > +
> > +/**
> > + * thermal_zone_update_trip_temp - Update the trip point temperature.
> > + * @tz: Thermal zone.
> > + * @trip_priv: Trip tag.
> > + * @temp: New trip temperature.
> > + *
> > + * This only works for thermal zones using trip tables and its caller must
> > + * ensure that the zone lock is held before using it.
> > + *
> > + * @trip_priv is expected to be the value that has been stored by the driver
> > + * in the struct thermal_trip representing the trip point in question, so it
> > + * can be matched against the value of the priv field in that structure.
> > + *
> > + * If @trip_priv does not match any trip point in the trip table of @tz,
> > + * nothing happens.
> > + */
> > +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> > + void *trip_priv, int temperature)
> > +{
> > + int i;
> > +
> > + lockdep_assert_held(&tz->lock);
> > +
> > + if (!tz->trips || !trip_priv)
> > + return;
> > +
> > + for (i = 0; i < tz->num_trips; i++) {
> > + struct thermal_trip *trip = &tz->trips[i];
> > +
> > + if (trip->priv == trip_priv) {
> > + trip->temperature = temperature;
> > + return;
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
>
> This function would imply the comparator is always trip->priv but if we
> want another comparison eg. trip->priv->id, that won't be possible.
>
> Actually, I think you can reuse an existing function with a simple
> change, for_each_thermal_trip() located in thermal_core.h.
for_each_thermal_trip() is only defined in tools/lib/thermal/thermal.c
AFAICS, but this one could actually work, so I can copy that
definition to somewhere else.
But I suppose that you mean __for_each_thermal_trip() which won't
work, because it makes a copy of the trip and passes that to the
callback, but the callback would need to update the temperature of the
original trip.
It would work if it passed the original trip to the caller, so I can
add something like that.
> The changes would be renaming it without the '__' prefix and moving it
> in include/linux/thermal.h.
>
> Then the comparison function and the temperature change can be an ACPI
> driver specific callback passed as parameter to for_each_thermal_zone
I guess you mean for_each_thermal_trip().
As per the above, not really, but I can do something along these lines.
On Mon, Aug 7, 2023 at 6:17 PM Daniel Lezcano <[email protected]> wrote:
>
> On 07/08/2023 17:40, Rafael J. Wysocki wrote:
> > On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <[email protected]> wrote:
> >>
> >> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <[email protected]>
> >>>
> >>> Introduce a helper routine called thermal_zone_update_trip_temp() that
> >>> can be used to update a trip point's temperature with the help of a
> >>> pointer to local data associated with that trip point provided by
> >>> the thermal driver that created it.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <[email protected]>
> >>> ---
> >>>
> >>> New patch in v4.
> >>>
> >>> ---
> >>> drivers/thermal/thermal_trip.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> include/linux/thermal.h | 4 ++++
> >>> 2 files changed, 41 insertions(+)
> >>>
> >>> Index: linux-pm/drivers/thermal/thermal_trip.c
> >>> ===================================================================
> >>> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> >>> +++ linux-pm/drivers/thermal/thermal_trip.c
> >>> @@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
> >>>
> >>> return 0;
> >>> }
> >>> +
> >>> +/**
> >>> + * thermal_zone_update_trip_temp - Update the trip point temperature.
> >>> + * @tz: Thermal zone.
> >>> + * @trip_priv: Trip tag.
> >>> + * @temp: New trip temperature.
> >>> + *
> >>> + * This only works for thermal zones using trip tables and its caller must
> >>> + * ensure that the zone lock is held before using it.
> >>> + *
> >>> + * @trip_priv is expected to be the value that has been stored by the driver
> >>> + * in the struct thermal_trip representing the trip point in question, so it
> >>> + * can be matched against the value of the priv field in that structure.
> >>> + *
> >>> + * If @trip_priv does not match any trip point in the trip table of @tz,
> >>> + * nothing happens.
> >>> + */
> >>> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
> >>> + void *trip_priv, int temperature)
> >>> +{
> >>> + int i;
> >>> +
> >>> + lockdep_assert_held(&tz->lock);
> >>> +
> >>> + if (!tz->trips || !trip_priv)
> >>> + return;
> >>> +
> >>> + for (i = 0; i < tz->num_trips; i++) {
> >>> + struct thermal_trip *trip = &tz->trips[i];
> >>> +
> >>> + if (trip->priv == trip_priv) {
> >>> + trip->temperature = temperature;
> >>> + return;
> >>> + }
> >>> + }
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
> >>
> >> This function would imply the comparator is always trip->priv but if we
> >> want another comparison eg. trip->priv->id, that won't be possible.
> >>
> >> Actually, I think you can reuse an existing function with a simple
> >> change, for_each_thermal_trip() located in thermal_core.h.
> >
> > for_each_thermal_trip() is only defined in tools/lib/thermal/thermal.c
> > AFAICS, but this one could actually work, so I can copy that
> > definition to somewhere else.
> >
> > But I suppose that you mean __for_each_thermal_trip() which won't
> > work, because it makes a copy of the trip and passes that to the
> > callback, but the callback would need to update the temperature of the
> > original trip.
> >
> > It would work if it passed the original trip to the caller, so I can
> > add something like that.
>
> As there is no user of this function yet, I think you can change that to
> use the trip array instead of the __thermal_zone_get_trip(). This one
> was used to have a compatibility with thermal zones using get_trip_* ops
> but that is not really needed and with your series only one driver will
> remain before dropping these ops.
Sounds good.
> >> The changes would be renaming it without the '__' prefix and moving it
> >> in include/linux/thermal.h.
> >>
> >> Then the comparison function and the temperature change can be an ACPI
> >> driver specific callback passed as parameter to for_each_thermal_zone
> >
> > I guess you mean for_each_thermal_trip().
>
> Yes, __for_each_thermal_trip()
OK
On 07/08/2023 17:40, Rafael J. Wysocki wrote:
> On Mon, Aug 7, 2023 at 1:34 PM Daniel Lezcano <[email protected]> wrote:
>>
>> On 04/08/2023 23:05, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> Introduce a helper routine called thermal_zone_update_trip_temp() that
>>> can be used to update a trip point's temperature with the help of a
>>> pointer to local data associated with that trip point provided by
>>> the thermal driver that created it.
>>>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> ---
>>>
>>> New patch in v4.
>>>
>>> ---
>>> drivers/thermal/thermal_trip.c | 37 +++++++++++++++++++++++++++++++++++++
>>> include/linux/thermal.h | 4 ++++
>>> 2 files changed, 41 insertions(+)
>>>
>>> Index: linux-pm/drivers/thermal/thermal_trip.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/thermal/thermal_trip.c
>>> +++ linux-pm/drivers/thermal/thermal_trip.c
>>> @@ -180,3 +180,40 @@ int thermal_zone_set_trip(struct thermal
>>>
>>> return 0;
>>> }
>>> +
>>> +/**
>>> + * thermal_zone_update_trip_temp - Update the trip point temperature.
>>> + * @tz: Thermal zone.
>>> + * @trip_priv: Trip tag.
>>> + * @temp: New trip temperature.
>>> + *
>>> + * This only works for thermal zones using trip tables and its caller must
>>> + * ensure that the zone lock is held before using it.
>>> + *
>>> + * @trip_priv is expected to be the value that has been stored by the driver
>>> + * in the struct thermal_trip representing the trip point in question, so it
>>> + * can be matched against the value of the priv field in that structure.
>>> + *
>>> + * If @trip_priv does not match any trip point in the trip table of @tz,
>>> + * nothing happens.
>>> + */
>>> +void thermal_zone_update_trip_temp(struct thermal_zone_device *tz,
>>> + void *trip_priv, int temperature)
>>> +{
>>> + int i;
>>> +
>>> + lockdep_assert_held(&tz->lock);
>>> +
>>> + if (!tz->trips || !trip_priv)
>>> + return;
>>> +
>>> + for (i = 0; i < tz->num_trips; i++) {
>>> + struct thermal_trip *trip = &tz->trips[i];
>>> +
>>> + if (trip->priv == trip_priv) {
>>> + trip->temperature = temperature;
>>> + return;
>>> + }
>>> + }
>>> +}
>>> +EXPORT_SYMBOL_GPL(thermal_zone_update_trip_temp);
>>
>> This function would imply the comparator is always trip->priv but if we
>> want another comparison eg. trip->priv->id, that won't be possible.
>>
>> Actually, I think you can reuse an existing function with a simple
>> change, for_each_thermal_trip() located in thermal_core.h.
>
> for_each_thermal_trip() is only defined in tools/lib/thermal/thermal.c
> AFAICS, but this one could actually work, so I can copy that
> definition to somewhere else.
>
> But I suppose that you mean __for_each_thermal_trip() which won't
> work, because it makes a copy of the trip and passes that to the
> callback, but the callback would need to update the temperature of the
> original trip.
>
> It would work if it passed the original trip to the caller, so I can
> add something like that.
As there is no user of this function yet, I think you can change that to
use the trip array instead of the __thermal_zone_get_trip(). This one
was used to have a compatibility with thermal zones using get_trip_* ops
but that is not really needed and with your series only one driver will
remain before dropping these ops.
>> The changes would be renaming it without the '__' prefix and moving it
>> in include/linux/thermal.h.
>>
>> Then the comparison function and the temperature change can be an ACPI
>> driver specific callback passed as parameter to for_each_thermal_zone
>
> I guess you mean for_each_thermal_trip().
Yes, __for_each_thermal_trip()
> As per the above, not really, but I can do something along these lines.
--
<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 Everyone,
This is the 5th iteration of the $subject patch series and its original
description below is still applicable
On Tuesday, July 18, 2023 8:01:20 PM CEST Rafael J. Wysocki wrote:
>
> This patch series makes the ACPI thermal driver register thermal zones
> with the help of thermal_zone_device_register_with_trips(), so it
> doesn't need to use the thermal zone callbacks related to trip points
> any more (and they are dropped in the last patch).
>
> The approach presented here is quite radically different from the
> previous attempts, as it doesn't really rearrange the driver's
> internal data structures, but adds the trip table support on top of
> them. For this purpose, it uses an additional field in struct thermal_trip
This update follows the suggestion from here:
https://lore.kernel.org/linux-acpi/[email protected]/
and so it drops one patch and adds two new patches (one of them merely
rearranging the ACPI thermal drivers data structures to allow the
subsequent patches to be more straightforward).
Thanks!
From: Rafael J. Wysocki <[email protected]>
Drop the .get_trip_type(), .get_trip_temp() and .get_crit_temp() thermal
zone callbacks that are not necessary any more from the ACPI thermal
driver along with the corresponding callback functions.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5: Rebase.
v3 -> v4: No changes.
v2 -> v3: Rebase on top of the v2 of the previous patch.
v1 -> v2: No changes.
---
drivers/acpi/thermal.c | 115 -------------------------------------------------
1 file changed, 115 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -484,118 +484,6 @@ static int thermal_get_temp(struct therm
return 0;
}
-static int thermal_get_trip_type(struct thermal_zone_device *thermal,
- int trip, enum thermal_trip_type *type)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- int i;
-
- if (!tz || trip < 0)
- return -EINVAL;
-
- if (tz->trips.critical.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_CRITICAL;
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.hot.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_HOT;
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.passive.trip.valid) {
- if (!trip) {
- *type = THERMAL_TRIP_PASSIVE;
- return 0;
- }
- trip--;
- }
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].trip.valid; i++) {
- if (!trip) {
- *type = THERMAL_TRIP_ACTIVE;
- return 0;
- }
- trip--;
- }
-
- return -EINVAL;
-}
-
-static int thermal_get_trip_temp(struct thermal_zone_device *thermal,
- int trip, int *temp)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- int i;
-
- if (!tz || trip < 0)
- return -EINVAL;
-
- if (tz->trips.critical.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.critical.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.hot.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.hot.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- if (tz->trips.passive.trip.valid) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.passive.trip.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE &&
- tz->trips.active[i].trip.valid; i++) {
- if (!trip) {
- *temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.active[i].trip.temperature,
- tz->kelvin_offset);
- return 0;
- }
- trip--;
- }
-
- return -EINVAL;
-}
-
-static int thermal_get_crit_temp(struct thermal_zone_device *thermal,
- int *temperature)
-{
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
-
- if (tz->trips.critical.valid) {
- *temperature = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.critical.temperature,
- tz->kelvin_offset);
- return 0;
- }
-
- return -EINVAL;
-}
-
static int thermal_get_trend(struct thermal_zone_device *thermal,
int trip_index, enum thermal_trend *trend)
{
@@ -758,9 +646,6 @@ static struct thermal_zone_device_ops ac
.bind = acpi_thermal_bind_cooling_device,
.unbind = acpi_thermal_unbind_cooling_device,
.get_temp = thermal_get_temp,
- .get_trip_type = thermal_get_trip_type,
- .get_trip_temp = thermal_get_trip_temp,
- .get_crit_temp = thermal_get_crit_temp,
.get_trend = thermal_get_trend,
.hot = acpi_thermal_zone_device_hot,
.critical = acpi_thermal_zone_device_critical,
From: Rafael J. Wysocki <[email protected]>
Introduce a new thermal zone device operation called .update() for
modifying thermal zone components such as trip point and a new helper
function, thermal_zone_device_adjust(), that can be used by drivers
providing the new thermal zone device operation to invoke it under
the zone lock.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5: No changes.
New patch in v4.
---
drivers/thermal/thermal_core.c | 20 ++++++++++++++++++++
include/linux/thermal.h | 2 ++
2 files changed, 22 insertions(+)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -74,6 +74,7 @@ struct thermal_zone_device_ops {
enum thermal_trend *);
void (*hot)(struct thermal_zone_device *);
void (*critical)(struct thermal_zone_device *);
+ void (*update)(struct thermal_zone_device *, unsigned long);
};
/**
@@ -323,6 +324,7 @@ int thermal_zone_unbind_cooling_device(s
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *,
enum thermal_notify_event);
+void thermal_zone_device_adjust(struct thermal_zone_device *tz, unsigned long data);
struct thermal_cooling_device *thermal_cooling_device_register(const char *,
void *, const struct thermal_cooling_device_ops *);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -497,6 +497,26 @@ void thermal_zone_device_update(struct t
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
+/**
+ * thermal_zone_device_adjust - Adjust a thermal zone.
+ * @tz: Thermal zone.
+ * @data: Data to pass to the zone's .update() callback.
+ *
+ * Modify components of a thermal zone (for example, trip points) via
+ * its .update() callback (for example, after a platform configuration
+ * change).
+ */
+void thermal_zone_device_adjust(struct thermal_zone_device *tz, unsigned long data)
+{
+ mutex_lock(&tz->lock);
+
+ if (device_is_registered(&tz->device) && tz->ops->update)
+ tz->ops->update(tz, data);
+
+ mutex_unlock(&tz->lock);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device_adjust);
+
static void thermal_zone_device_check(struct work_struct *work)
{
struct thermal_zone_device *tz = container_of(work, struct
From: Rafael J. Wysocki <[email protected]>
Move the acpi_bus_generate_netlink_event() invocation into
acpi_thermal_trips_update() which allows the code duplication in
acpi_thermal_notify() to be cleaned up, but for this purpose the
event value needs to be passed to acpi_thermal_trips_update() and
from there to acpi_thermal_adjust_thermal_zone() which has to
determine the flag value for __acpi_thermal_trips_update() by
itself.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5: Rebase.
New patch in v4.
---
drivers/acpi/thermal.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -419,8 +419,10 @@ static void acpi_thermal_adjust_thermal_
unsigned long data)
{
struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
+ int flag = data == ACPI_THERMAL_NOTIFY_THRESHOLDS ?
+ ACPI_TRIPS_THRESHOLDS : ACPI_TRIPS_DEVICES;
- __acpi_thermal_trips_update(tz, data);
+ __acpi_thermal_trips_update(tz, flag);
for_each_thermal_trip(tz->thermal_zone, acpi_thermal_adjust_trip, tz);
}
@@ -431,8 +433,10 @@ static void acpi_queue_thermal_check(str
queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work);
}
-static void acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
+static void acpi_thermal_trips_update(struct acpi_thermal *tz, u32 event)
{
+ struct acpi_device *adev = tz->device;
+
/*
* Use thermal_zone_device_adjust() to carry out the trip points
* update, so as to protect thermal_get_trend() from getting stale
@@ -440,8 +444,10 @@ static void acpi_thermal_trips_update(st
* invoked from acpi_thermal_check_fn() from producing inconsistent
* results.
*/
- thermal_zone_device_adjust(tz->thermal_zone, flag);
+ thermal_zone_device_adjust(tz->thermal_zone, event);
acpi_queue_thermal_check(tz);
+ acpi_bus_generate_netlink_event(adev->pnp.device_class,
+ dev_name(&adev->dev), event, 0);
}
static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
@@ -812,14 +818,8 @@ static void acpi_thermal_notify(acpi_han
acpi_queue_thermal_check(tz);
break;
case ACPI_THERMAL_NOTIFY_THRESHOLDS:
- acpi_thermal_trips_update(tz, ACPI_TRIPS_THRESHOLDS);
- acpi_bus_generate_netlink_event(device->pnp.device_class,
- dev_name(&device->dev), event, 0);
- break;
case ACPI_THERMAL_NOTIFY_DEVICES:
- acpi_thermal_trips_update(tz, ACPI_TRIPS_DEVICES);
- acpi_bus_generate_netlink_event(device->pnp.device_class,
- dev_name(&device->dev), event, 0);
+ acpi_thermal_trips_update(tz, event);
break;
default:
acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
From: Rafael J. Wysocki <[email protected]>
Rename the trips variable in acpi_thermal_register_thermal_zone() to
trip_count so its name better reflects the purpose, rearrange white
space in the loop over active trips for clarity and reduce code
duplication related to calling thermal_zone_device_register() by
using an extra local variable to store the passive delay value.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5: No changes.
v3 -> v4: No changes.
v2 -> v3: No changes.
v1 -> v2: No changes.
---
drivers/acpi/thermal.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -740,34 +740,30 @@ static void acpi_thermal_zone_sysfs_remo
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
- int trips = 0;
+ int passive_delay = 0;
+ int trip_count = 0;
int result;
acpi_status status;
int i;
if (tz->trips.critical.valid)
- trips++;
+ trip_count++;
if (tz->trips.hot.valid)
- trips++;
-
- if (tz->trips.passive.valid)
- trips++;
-
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid;
- i++, trips++);
-
- if (tz->trips.passive.valid)
- tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz,
- &acpi_thermal_zone_ops, NULL,
- tz->trips.passive.tsp * 100,
- tz->polling_frequency * 100);
- else
- tz->thermal_zone =
- thermal_zone_device_register("acpitz", trips, 0, tz,
- &acpi_thermal_zone_ops, NULL,
- 0, tz->polling_frequency * 100);
+ trip_count++;
+ if (tz->trips.passive.valid) {
+ trip_count++;
+ passive_delay = tz->trips.passive.tsp * 100;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++)
+ trip_count++;
+
+ tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
+ tz, &acpi_thermal_zone_ops,
+ NULL, passive_delay,
+ tz->polling_frequency * 100);
if (IS_ERR(tz->thermal_zone))
return -ENODEV;
From: Rafael J. Wysocki <[email protected]>
Make the ACPI thermal driver use thermal_zone_device_register_with_trips()
to register its thermal zones.
For this purpose, make it create a trip point table that will be passed to
thermal_zone_device_register_with_trips() as an argument.
Also use the thermal_zone_update_trip_temp() helper introduced
previously to update temperatures of the passive and active trip
points after a trip points change notification from the platform
firmware.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5:
* Use for_each_thermal_trip() introduced previously to update trip
temperatures with the help of a new trip callback function.
* Drop a function that has no users after the above change.
* Rebase on top of patch [07/11].
v3 -> v4:
* Rework to use thermal_zone_update_trip_temp() for updating trip point
temperatures.
* Rebase on top of the new version of the previous patch.
v2 -> v3:
* Fix error code path memory leak in acpi_thermal_register_thermal_zone().
* Notice that the critical and hot trips never change after initialization,
so don't add struct thermal_trip_ref to any of them.
v1 -> v2:
* Use thermal_zone_device_lock()/thermal_zone_device_unlock() in
acpi_thermal_check_fn() explicitly and call __thermal_zone_device_update()
from there without unlocking the thermal zone.
* Export __thermal_zone_device_update() to modules (so it can be called by
the ACPI thermal code).
---
drivers/acpi/thermal.c | 93 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 86 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -125,6 +125,7 @@ struct acpi_thermal {
unsigned long polling_frequency;
volatile u8 zombie;
struct acpi_thermal_trips trips;
+ struct thermal_trip *trip_table;
struct acpi_handle_list devices;
struct thermal_zone_device *thermal_zone;
int kelvin_offset; /* in millidegrees */
@@ -178,6 +179,15 @@ static int acpi_thermal_get_polling_freq
return 0;
}
+static int acpi_thermal_temp(struct acpi_thermal *tz, int temp_deci_k)
+{
+ if (temp_deci_k == THERMAL_TEMP_INVALID)
+ return THERMAL_TEMP_INVALID;
+
+ return deci_kelvin_to_millicelsius_with_offset(temp_deci_k,
+ tz->kelvin_offset);
+}
+
static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
{
acpi_status status;
@@ -389,10 +399,30 @@ static void __acpi_thermal_trips_update(
}
}
+static int acpi_thermal_adjust_trip(struct thermal_trip *trip, void *data)
+{
+ struct acpi_thermal_trip *acpi_trip = trip->priv;
+ struct acpi_thermal *tz = data;
+
+ if (!acpi_trip)
+ return 0;
+
+ if (acpi_trip->valid)
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ else
+ trip->temperature = THERMAL_TEMP_INVALID;
+
+ return 0;
+}
+
static void acpi_thermal_adjust_thermal_zone(struct thermal_zone_device *thermal,
unsigned long data)
{
- __acpi_thermal_trips_update(thermal_zone_device_priv(thermal), data);
+ struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
+
+ __acpi_thermal_trips_update(tz, data);
+
+ for_each_thermal_trip(tz->thermal_zone, acpi_thermal_adjust_trip, tz);
}
static void acpi_queue_thermal_check(struct acpi_thermal *tz)
@@ -757,6 +787,8 @@ static void acpi_thermal_zone_sysfs_remo
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
+ struct acpi_thermal_trip *acpi_trip;
+ struct thermal_trip *trip;
int passive_delay = 0;
int trip_count = 0;
int result;
@@ -777,12 +809,56 @@ static int acpi_thermal_register_thermal
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].trip.valid; i++)
trip_count++;
- tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
- tz, &acpi_thermal_zone_ops,
- NULL, passive_delay,
- tz->polling_frequency * 100);
- if (IS_ERR(tz->thermal_zone))
- return -ENODEV;
+ trip = kcalloc(trip_count, sizeof(*trip), GFP_KERNEL);
+ if (!trip)
+ return -ENOMEM;
+
+ tz->trip_table = trip;
+
+ if (tz->trips.critical.valid) {
+ trip->type = THERMAL_TRIP_CRITICAL;
+ trip->temperature = acpi_thermal_temp(tz, tz->trips.critical.temperature);
+ trip++;
+ }
+
+ if (tz->trips.hot.valid) {
+ trip->type = THERMAL_TRIP_HOT;
+ trip->temperature = acpi_thermal_temp(tz, tz->trips.hot.temperature);
+ trip++;
+ }
+
+ acpi_trip = &tz->trips.passive.trip;
+ if (acpi_trip->valid) {
+ trip->type = THERMAL_TRIP_PASSIVE;
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ trip->priv = &tz->trips.passive.trip;
+ trip++;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ acpi_trip = &tz->trips.active[i].trip;
+
+ if (!acpi_trip->valid)
+ continue;
+
+ trip->type = THERMAL_TRIP_ACTIVE;
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ trip->priv = &tz->trips.active[i].trip;
+ trip++;
+ }
+
+ tz->thermal_zone = thermal_zone_device_register_with_trips("acpitz",
+ tz->trip_table,
+ trip_count,
+ 0, tz,
+ &acpi_thermal_zone_ops,
+ NULL,
+ passive_delay,
+ tz->polling_frequency * 100);
+ if (IS_ERR(tz->thermal_zone)) {
+ result = PTR_ERR(tz->thermal_zone);
+ goto free_trip_table;
+ }
result = acpi_thermal_zone_sysfs_add(tz);
if (result)
@@ -810,6 +886,8 @@ remove_links:
acpi_thermal_zone_sysfs_remove(tz);
unregister_tzd:
thermal_zone_device_unregister(tz->thermal_zone);
+free_trip_table:
+ kfree(tz->trip_table);
return result;
}
@@ -818,6 +896,7 @@ static void acpi_thermal_unregister_ther
{
acpi_thermal_zone_sysfs_remove(tz);
thermal_zone_device_unregister(tz->thermal_zone);
+ kfree(tz->trip_table);
tz->thermal_zone = NULL;
acpi_bus_detach_private_data(tz->device->handle);
}
From: Rafael J. Wysocki <[email protected]>
There is a race condition between acpi_thermal_trips_update() and
acpi_thermal_check_fn(), because the trip points may get updated while
the latter is running which in theory may lead to inconsistent results.
For example, if two trips are updated together, using the temperature
value of one of them from before the update and the temperature value
of the other one from after the update may not lead to the expected
outcome.
Moreover, if thermal_get_trend() runs when a trip points update is in
progress, it may end up using stale trip point temperatures.
To address this, make acpi_thermal_trips_update() call
thermal_zone_device_adjust() to carry out the trip points update and
provide a new acpi_thermal_adjust_thermal_zone() wrapper around
__acpi_thermal_trips_update() as the callback function for the latter.
While at it, change the acpi_thermal_trips_update() return data type
to void as that function always returns 0 anyway.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v4 -> v5: No changes.
v3 -> v4:
* Rework to use thermal_zone_device_adjust() and the .update() callback
instead of using the (exported) zone lock directly.
* Call acpi_queue_thermal_check() from acpi_thermal_trips_update() which
allows code duplication in acpi_thermal_notify() to be reduced.
v2 -> v3: No changes.
v1 -> v2:
* Hold the thermal zone lock instead of thermal_check_lock around trip
point updates (this also helps to protect thermal_get_trend() from using
stale trip temperatures).
* Add a comment documenting the purpose of the locking.
* Make acpi_thermal_trips_update() void.
---
drivers/acpi/thermal.c | 41 ++++++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 13 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -185,7 +185,7 @@ static int acpi_thermal_get_polling_freq
return 0;
}
-static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
+static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
{
acpi_status status;
unsigned long long tmp;
@@ -393,17 +393,39 @@ static int acpi_thermal_trips_update(str
ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device");
}
}
+}
- return 0;
+static void acpi_thermal_adjust_thermal_zone(struct thermal_zone_device *thermal,
+ unsigned long data)
+{
+ __acpi_thermal_trips_update(thermal_zone_device_priv(thermal), data);
+}
+
+static void acpi_queue_thermal_check(struct acpi_thermal *tz)
+{
+ if (!work_pending(&tz->thermal_check_work))
+ queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work);
+}
+
+static void acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
+{
+ /*
+ * Use thermal_zone_device_adjust() to carry out the trip points
+ * update, so as to protect thermal_get_trend() from getting stale
+ * trip point temperatures and to prevent thermal_zone_device_update()
+ * invoked from acpi_thermal_check_fn() from producing inconsistent
+ * results.
+ */
+ thermal_zone_device_adjust(tz->thermal_zone, flag);
+ acpi_queue_thermal_check(tz);
}
static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
{
- int i, ret = acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
bool valid;
+ int i;
- if (ret)
- return ret;
+ __acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
valid = tz->trips.critical.valid |
tz->trips.hot.valid |
@@ -710,6 +732,7 @@ static struct thermal_zone_device_ops ac
.get_trend = thermal_get_trend,
.hot = acpi_thermal_zone_device_hot,
.critical = acpi_thermal_zone_device_critical,
+ .update = acpi_thermal_adjust_thermal_zone,
};
static int acpi_thermal_zone_sysfs_add(struct acpi_thermal *tz)
@@ -810,12 +833,6 @@ static void acpi_thermal_unregister_ther
Driver Interface
-------------------------------------------------------------------------- */
-static void acpi_queue_thermal_check(struct acpi_thermal *tz)
-{
- if (!work_pending(&tz->thermal_check_work))
- queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work);
-}
-
static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_device *device = data;
@@ -830,13 +847,11 @@ static void acpi_thermal_notify(acpi_han
break;
case ACPI_THERMAL_NOTIFY_THRESHOLDS:
acpi_thermal_trips_update(tz, ACPI_TRIPS_THRESHOLDS);
- acpi_queue_thermal_check(tz);
acpi_bus_generate_netlink_event(device->pnp.device_class,
dev_name(&device->dev), event, 0);
break;
case ACPI_THERMAL_NOTIFY_DEVICES:
acpi_thermal_trips_update(tz, ACPI_TRIPS_DEVICES);
- acpi_queue_thermal_check(tz);
acpi_bus_generate_netlink_event(device->pnp.device_class,
dev_name(&device->dev), event, 0);
break;
From: Rafael J. Wysocki <[email protected]>
Add struct acpi_thermal_trip to contain the temperature and valid flag
of each trip point in the driver's local data structures.
This helps to make the subsequent changes more straightforward.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
New patch in v5.
---
drivers/acpi/thermal.c | 96 ++++++++++++++++++++++---------------------------
1 file changed, 45 insertions(+), 51 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -92,34 +92,27 @@ MODULE_PARM_DESC(psv, "Disable or overri
static struct workqueue_struct *acpi_thermal_pm_queue;
-struct acpi_thermal_critical {
- unsigned long temperature;
- bool valid;
-};
-
-struct acpi_thermal_hot {
+struct acpi_thermal_trip {
unsigned long temperature;
bool valid;
};
struct acpi_thermal_passive {
+ struct acpi_thermal_trip trip;
struct acpi_handle_list devices;
- unsigned long temperature;
unsigned long tc1;
unsigned long tc2;
unsigned long tsp;
- bool valid;
};
struct acpi_thermal_active {
+ struct acpi_thermal_trip trip;
struct acpi_handle_list devices;
- unsigned long temperature;
- bool valid;
};
struct acpi_thermal_trips {
- struct acpi_thermal_critical critical;
- struct acpi_thermal_hot hot;
+ struct acpi_thermal_trip critical;
+ struct acpi_thermal_trip hot;
struct acpi_thermal_passive passive;
struct acpi_thermal_active active[ACPI_THERMAL_MAX_ACTIVE];
};
@@ -250,9 +243,9 @@ static void __acpi_thermal_trips_update(
}
/* Passive (optional) */
- if (((flag & ACPI_TRIPS_PASSIVE) && tz->trips.passive.valid) ||
+ if (((flag & ACPI_TRIPS_PASSIVE) && tz->trips.passive.trip.valid) ||
flag == ACPI_TRIPS_INIT) {
- valid = tz->trips.passive.valid;
+ valid = tz->trips.passive.trip.valid;
if (psv == -1) {
status = AE_SUPPORT;
} else if (psv > 0) {
@@ -264,44 +257,44 @@ static void __acpi_thermal_trips_update(
}
if (ACPI_FAILURE(status)) {
- tz->trips.passive.valid = false;
+ tz->trips.passive.trip.valid = false;
} else {
- tz->trips.passive.temperature = tmp;
- tz->trips.passive.valid = true;
+ tz->trips.passive.trip.temperature = tmp;
+ tz->trips.passive.trip.valid = true;
if (flag == ACPI_TRIPS_INIT) {
status = acpi_evaluate_integer(tz->device->handle,
"_TC1", NULL, &tmp);
if (ACPI_FAILURE(status))
- tz->trips.passive.valid = false;
+ tz->trips.passive.trip.valid = false;
else
tz->trips.passive.tc1 = tmp;
status = acpi_evaluate_integer(tz->device->handle,
"_TC2", NULL, &tmp);
if (ACPI_FAILURE(status))
- tz->trips.passive.valid = false;
+ tz->trips.passive.trip.valid = false;
else
tz->trips.passive.tc2 = tmp;
status = acpi_evaluate_integer(tz->device->handle,
"_TSP", NULL, &tmp);
if (ACPI_FAILURE(status))
- tz->trips.passive.valid = false;
+ tz->trips.passive.trip.valid = false;
else
tz->trips.passive.tsp = tmp;
}
}
}
- if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.passive.valid) {
+ if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.passive.trip.valid) {
memset(&devices, 0, sizeof(struct acpi_handle_list));
status = acpi_evaluate_reference(tz->device->handle, "_PSL",
NULL, &devices);
if (ACPI_FAILURE(status)) {
acpi_handle_info(tz->device->handle,
"Invalid passive threshold\n");
- tz->trips.passive.valid = false;
+ tz->trips.passive.trip.valid = false;
} else {
- tz->trips.passive.valid = true;
+ tz->trips.passive.trip.valid = true;
}
if (memcmp(&tz->trips.passive.devices, &devices,
@@ -312,24 +305,24 @@ static void __acpi_thermal_trips_update(
}
}
if ((flag & ACPI_TRIPS_PASSIVE) || (flag & ACPI_TRIPS_DEVICES)) {
- if (valid != tz->trips.passive.valid)
+ if (valid != tz->trips.passive.trip.valid)
ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state");
}
/* Active (optional) */
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
char name[5] = { '_', 'A', 'C', ('0' + i), '\0' };
- valid = tz->trips.active[i].valid;
+ valid = tz->trips.active[i].trip.valid;
if (act == -1)
break; /* disable all active trip points */
if (flag == ACPI_TRIPS_INIT || ((flag & ACPI_TRIPS_ACTIVE) &&
- tz->trips.active[i].valid)) {
+ tz->trips.active[i].trip.valid)) {
status = acpi_evaluate_integer(tz->device->handle,
name, NULL, &tmp);
if (ACPI_FAILURE(status)) {
- tz->trips.active[i].valid = false;
+ tz->trips.active[i].trip.valid = false;
if (i == 0)
break;
@@ -337,35 +330,36 @@ static void __acpi_thermal_trips_update(
break;
if (i == 1)
- tz->trips.active[0].temperature = celsius_to_deci_kelvin(act);
+ tz->trips.active[0].trip.temperature =
+ celsius_to_deci_kelvin(act);
else
/*
* Don't allow override higher than
* the next higher trip point
*/
- tz->trips.active[i-1].temperature =
+ tz->trips.active[i-1].trip.temperature =
min_t(unsigned long,
- tz->trips.active[i-2].temperature,
+ tz->trips.active[i-2].trip.temperature,
celsius_to_deci_kelvin(act));
break;
} else {
- tz->trips.active[i].temperature = tmp;
- tz->trips.active[i].valid = true;
+ tz->trips.active[i].trip.temperature = tmp;
+ tz->trips.active[i].trip.valid = true;
}
}
name[2] = 'L';
- if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.active[i].valid) {
+ if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.active[i].trip.valid) {
memset(&devices, 0, sizeof(struct acpi_handle_list));
status = acpi_evaluate_reference(tz->device->handle,
name, NULL, &devices);
if (ACPI_FAILURE(status)) {
acpi_handle_info(tz->device->handle,
"Invalid active%d threshold\n", i);
- tz->trips.active[i].valid = false;
+ tz->trips.active[i].trip.valid = false;
} else {
- tz->trips.active[i].valid = true;
+ tz->trips.active[i].trip.valid = true;
}
if (memcmp(&tz->trips.active[i].devices, &devices,
@@ -376,10 +370,10 @@ static void __acpi_thermal_trips_update(
}
}
if ((flag & ACPI_TRIPS_ACTIVE) || (flag & ACPI_TRIPS_DEVICES))
- if (valid != tz->trips.active[i].valid)
+ if (valid != tz->trips.active[i].trip.valid)
ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state");
- if (!tz->trips.active[i].valid)
+ if (!tz->trips.active[i].trip.valid)
break;
}
@@ -429,10 +423,10 @@ static int acpi_thermal_get_trip_points(
valid = tz->trips.critical.valid |
tz->trips.hot.valid |
- tz->trips.passive.valid;
+ tz->trips.passive.trip.valid;
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++)
- valid = valid || tz->trips.active[i].valid;
+ valid = valid || tz->trips.active[i].trip.valid;
if (!valid) {
pr_warn(FW_BUG "No valid trip found\n");
@@ -485,7 +479,7 @@ static int thermal_get_trip_type(struct
trip--;
}
- if (tz->trips.passive.valid) {
+ if (tz->trips.passive.trip.valid) {
if (!trip) {
*type = THERMAL_TRIP_PASSIVE;
return 0;
@@ -493,7 +487,7 @@ static int thermal_get_trip_type(struct
trip--;
}
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++) {
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].trip.valid; i++) {
if (!trip) {
*type = THERMAL_TRIP_ACTIVE;
return 0;
@@ -533,10 +527,10 @@ static int thermal_get_trip_temp(struct
trip--;
}
- if (tz->trips.passive.valid) {
+ if (tz->trips.passive.trip.valid) {
if (!trip) {
*temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.passive.temperature,
+ tz->trips.passive.trip.temperature,
tz->kelvin_offset);
return 0;
}
@@ -544,10 +538,10 @@ static int thermal_get_trip_temp(struct
}
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE &&
- tz->trips.active[i].valid; i++) {
+ tz->trips.active[i].trip.valid; i++) {
if (!trip) {
*temp = deci_kelvin_to_millicelsius_with_offset(
- tz->trips.active[i].temperature,
+ tz->trips.active[i].trip.temperature,
tz->kelvin_offset);
return 0;
}
@@ -603,7 +597,7 @@ static int thermal_get_trend(struct ther
* before this callback being invoked
*/
i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
- tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
+ tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.trip.temperature);
if (i > 0)
*trend = THERMAL_TREND_RAISING;
@@ -654,7 +648,7 @@ static int acpi_thermal_cooling_device_c
if (tz->trips.hot.valid)
trip++;
- if (tz->trips.passive.valid) {
+ if (tz->trips.passive.trip.valid) {
trip++;
for (i = 0; i < tz->trips.passive.devices.count; i++) {
handle = tz->trips.passive.devices.handles[i];
@@ -679,7 +673,7 @@ static int acpi_thermal_cooling_device_c
}
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- if (!tz->trips.active[i].valid)
+ if (!tz->trips.active[i].trip.valid)
break;
trip++;
@@ -775,12 +769,12 @@ static int acpi_thermal_register_thermal
if (tz->trips.hot.valid)
trip_count++;
- if (tz->trips.passive.valid) {
+ if (tz->trips.passive.trip.valid) {
trip_count++;
passive_delay = tz->trips.passive.tsp * 100;
}
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].valid; i++)
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].trip.valid; i++)
trip_count++;
tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
@@ -1060,7 +1054,7 @@ static int acpi_thermal_resume(struct de
return -EINVAL;
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- if (!tz->trips.active[i].valid)
+ if (!tz->trips.active[i].trip.valid)
break;
for (j = 0; j < tz->trips.active[i].devices.count; j++) {
From: Rafael J. Wysocki <[email protected]>
Make the ACPI thermal driver use thermal_zone_device_register_with_trips()
to register its thermal zones.
For this purpose, make it create a trip point table that will be passed to
thermal_zone_device_register_with_trips() as an argument.
Also use the thermal_zone_update_trip_temp() helper introduced
previously to update temperatures of the passive and active trip
points after a trip points change notification from the platform
firmware.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
This is a bug fix update of just this patch that doesn't affect any other
patches in the series, which is why it is sent separately.
v5 -> v5.1:
* Terminate the loop over active trip points in
acpi_thermal_register_thermal_zone() on the first invalid one to
avoid reaching out of array bounds.
* Use acpi_trip instead of computing its value from scratch in two
places.
* Fix up white space.
v4 -> v5:
* Use for_each_thermal_trip() introduced previously to update trip
temperatures with the help of a new trip callback function.
* Drop a function that has no users after the above change.
* Rebase on top of patch [07/11].
v3 -> v4:
* Rework to use thermal_zone_update_trip_temp() for updating trip point
temperatures.
* Rebase on top of the new version of the previous patch.
v2 -> v3:
* Fix error code path memory leak in acpi_thermal_register_thermal_zone().
* Notice that the critical and hot trips never change after initialization,
so don't add struct thermal_trip_ref to any of them.
v1 -> v2:
* Use thermal_zone_device_lock()/thermal_zone_device_unlock() in
acpi_thermal_check_fn() explicitly and call __thermal_zone_device_update()
from there without unlocking the thermal zone.
* Export __thermal_zone_device_update() to modules (so it can be called by
the ACPI thermal code).
---
drivers/acpi/thermal.c | 93 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 86 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -125,6 +125,7 @@ struct acpi_thermal {
unsigned long polling_frequency;
volatile u8 zombie;
struct acpi_thermal_trips trips;
+ struct thermal_trip *trip_table;
struct acpi_handle_list devices;
struct thermal_zone_device *thermal_zone;
int kelvin_offset; /* in millidegrees */
@@ -178,6 +179,15 @@ static int acpi_thermal_get_polling_freq
return 0;
}
+static int acpi_thermal_temp(struct acpi_thermal *tz, int temp_deci_k)
+{
+ if (temp_deci_k == THERMAL_TEMP_INVALID)
+ return THERMAL_TEMP_INVALID;
+
+ return deci_kelvin_to_millicelsius_with_offset(temp_deci_k,
+ tz->kelvin_offset);
+}
+
static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
{
acpi_status status;
@@ -389,10 +399,30 @@ static void __acpi_thermal_trips_update(
}
}
+static int acpi_thermal_adjust_trip(struct thermal_trip *trip, void *data)
+{
+ struct acpi_thermal_trip *acpi_trip = trip->priv;
+ struct acpi_thermal *tz = data;
+
+ if (!acpi_trip)
+ return 0;
+
+ if (acpi_trip->valid)
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ else
+ trip->temperature = THERMAL_TEMP_INVALID;
+
+ return 0;
+}
+
static void acpi_thermal_adjust_thermal_zone(struct thermal_zone_device *thermal,
unsigned long data)
{
- __acpi_thermal_trips_update(thermal_zone_device_priv(thermal), data);
+ struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
+
+ __acpi_thermal_trips_update(tz, data);
+
+ for_each_thermal_trip(tz->thermal_zone, acpi_thermal_adjust_trip, tz);
}
static void acpi_queue_thermal_check(struct acpi_thermal *tz)
@@ -757,6 +787,8 @@ static void acpi_thermal_zone_sysfs_remo
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
+ struct acpi_thermal_trip *acpi_trip;
+ struct thermal_trip *trip;
int passive_delay = 0;
int trip_count = 0;
int result;
@@ -776,12 +808,56 @@ static int acpi_thermal_register_thermal
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE && tz->trips.active[i].trip.valid; i++)
trip_count++;
- tz->thermal_zone = thermal_zone_device_register("acpitz", trip_count, 0,
- tz, &acpi_thermal_zone_ops,
- NULL, passive_delay,
- tz->polling_frequency * 100);
- if (IS_ERR(tz->thermal_zone))
- return -ENODEV;
+ trip = kcalloc(trip_count, sizeof(*trip), GFP_KERNEL);
+ if (!trip)
+ return -ENOMEM;
+
+ tz->trip_table = trip;
+
+ if (tz->trips.critical.valid) {
+ trip->type = THERMAL_TRIP_CRITICAL;
+ trip->temperature = acpi_thermal_temp(tz, tz->trips.critical.temperature);
+ trip++;
+ }
+
+ if (tz->trips.hot.valid) {
+ trip->type = THERMAL_TRIP_HOT;
+ trip->temperature = acpi_thermal_temp(tz, tz->trips.hot.temperature);
+ trip++;
+ }
+
+ acpi_trip = &tz->trips.passive.trip;
+ if (acpi_trip->valid) {
+ trip->type = THERMAL_TRIP_PASSIVE;
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ trip->priv = acpi_trip;
+ trip++;
+ }
+
+ for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
+ acpi_trip = &tz->trips.active[i].trip;
+
+ if (!acpi_trip->valid)
+ break;
+
+ trip->type = THERMAL_TRIP_ACTIVE;
+ trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature);
+ trip->priv = acpi_trip;
+ trip++;
+ }
+
+ tz->thermal_zone = thermal_zone_device_register_with_trips("acpitz",
+ tz->trip_table,
+ trip_count,
+ 0, tz,
+ &acpi_thermal_zone_ops,
+ NULL,
+ passive_delay,
+ tz->polling_frequency * 100);
+ if (IS_ERR(tz->thermal_zone)) {
+ result = PTR_ERR(tz->thermal_zone);
+ goto free_trip_table;
+ }
result = acpi_thermal_zone_sysfs_add(tz);
if (result)
@@ -800,6 +876,8 @@ remove_links:
acpi_thermal_zone_sysfs_remove(tz);
unregister_tzd:
thermal_zone_device_unregister(tz->thermal_zone);
+free_trip_table:
+ kfree(tz->trip_table);
return result;
}
@@ -808,6 +886,7 @@ static void acpi_thermal_unregister_ther
{
acpi_thermal_zone_sysfs_remove(tz);
thermal_zone_device_unregister(tz->thermal_zone);
+ kfree(tz->trip_table);
tz->thermal_zone = NULL;
}
On Wednesday, August 16, 2023 6:25:30 PM CEST Daniel Lezcano wrote:
> On 07/08/2023 20:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > There is a race condition between acpi_thermal_trips_update() and
> > acpi_thermal_check_fn(), because the trip points may get updated while
> > the latter is running which in theory may lead to inconsistent results.
> > For example, if two trips are updated together, using the temperature
> > value of one of them from before the update and the temperature value
> > of the other one from after the update may not lead to the expected
> > outcome.
> >
> > Moreover, if thermal_get_trend() runs when a trip points update is in
> > progress, it may end up using stale trip point temperatures.
> >
> > To address this, make acpi_thermal_trips_update() call
> > thermal_zone_device_adjust() to carry out the trip points update and
> > provide a new acpi_thermal_adjust_thermal_zone() wrapper around
> > __acpi_thermal_trips_update() as the callback function for the latter.
> >
> > While at it, change the acpi_thermal_trips_update() return data type
> > to void as that function always returns 0 anyway.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
>
> [ ... ]
>
> > {
> > - int i, ret = acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
> > bool valid;
> > + int i;
> >
> > - if (ret)
> > - return ret;
> > + __acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
> >
> > valid = tz->trips.critical.valid |
> > tz->trips.hot.valid |
> > @@ -710,6 +732,7 @@ static struct thermal_zone_device_ops ac
> > .get_trend = thermal_get_trend,
> > .hot = acpi_thermal_zone_device_hot,
> > .critical = acpi_thermal_zone_device_critical,
> > + .update = acpi_thermal_adjust_thermal_zone,
>
> It is too bad we have to add a callback in the core code just for this
> driver.
>
> I'm wondering if it is not possible to get rid of it ?
Well, it is possible to pass the callback as an argument to the function running it.
The code is slightly simpler this way, so I think I'm going to do that.
Please see the appended replacement for patch [02/11].
Of course, it also is possible to provide accessors for acquiring and releasing
the zone lock, which would be more straightforward still (as mentioned before),
but I kind of understand the concerns regarding possible abuse of those by
drivers.
> Is it possible to use an internal lock for the ACPI driver to solve the
> race issue above ?
No, it is not, and I have already explained it at least once, but let me do
that once again.
There are three code paths that need to be synchronized, because each of them
can run in parallel with any of the other two.
(a) acpi_thermal_trips_update() called via acpi_thermal_notify() which runs
in the ACPI notify kworker context.
(b) thermal_get_trend(), already called under the zone lock by the core.
(c) acpi_thermal_check_fn() running in a kworker context, which calls
thermal_zone_device_update() which it turn takes the zone lock.
Also the trip points update should not race with any computations using trip
point temperatures in the core or in the governors (they are carried out under
the zone lock as a rule).
(b) means that the local lock would need to be always taken under the zone
lock and then either acpi_thermal_check_fn() would need to be able to take
the local lock under the zone lock (so it would need to be able to acquire
the zone lock more or less directly), or acpi_thermal_trips_update() can
use the zone lock (which happens in the $subject patch via the new helper
function).
Moreover, using a local lock in acpi_thermal_trips_update() does not provide
any protection for the code using trip temperatures that runs under the zone
lock mentioned above.
So as I said, the patch below replaces [02/11] and it avoids adding a new
callback to zone operations. The code gets slightly simpler with [02/11]
replaced with the appended one, so I'm going to use the latter.
It requires the $subject patch and patch [11/11] to be rebased, but that
is so trivial that I'm not even going to send updates of these patches.
The current series is available in the acpi-thermal git branch in
linux-pm.git.
---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] thermal: core: Introduce thermal_zone_device_exec()
Introduce a new helper function, thermal_zone_device_exec(), that can
be used by drivers to run a given callback routine under the zone lock.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/thermal_core.c | 19 +++++++++++++++++++
include/linux/thermal.h | 4 ++++
2 files changed, 23 insertions(+)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -323,6 +323,10 @@ int thermal_zone_unbind_cooling_device(s
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *,
enum thermal_notify_event);
+void thermal_zone_device_exec(struct thermal_zone_device *tz,
+ void (*cb)(struct thermal_zone_device *,
+ unsigned long),
+ unsigned long data);
struct thermal_cooling_device *thermal_cooling_device_register(const char *,
void *, const struct thermal_cooling_device_ops *);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -497,6 +497,25 @@ void thermal_zone_device_update(struct t
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
+/**
+ * thermal_zone_device_exec - Run a callback under the zone lock.
+ * @tz: Thermal zone.
+ * @cb: Callback to run.
+ * @data: Data to pass to the callback.
+ */
+void thermal_zone_device_exec(struct thermal_zone_device *tz,
+ void (*cb)(struct thermal_zone_device *,
+ unsigned long),
+ unsigned long data)
+{
+ mutex_lock(&tz->lock);
+
+ cb(tz, data);
+
+ mutex_unlock(&tz->lock);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device_exec);
+
static void thermal_zone_device_check(struct work_struct *work)
{
struct thermal_zone_device *tz = container_of(work, struct