diff mbox series

[v3,5/8] ACPI: thermal: Hold thermal zone lock around trip updates

Message ID 7552439.EvYhyI6sBW@kreacher
State New
Headers show
Series [v3,1/8] thermal: core: Add mechanism for connecting trips with driver data | expand

Commit Message

Rafael J. Wysocki July 25, 2023, 12:16 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---

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 |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Daniel Lezcano Aug. 1, 2023, 6:39 p.m. UTC | #1 
Hi Rafael,

On 25/07/2023 14:16, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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.

As commented in patch 3/8, having a driver locking a thermal core 
structure is not right and goes to the opposite direction of the recent 
cleanups.

Don't we have 2 race conditions:

acpi_thermal_trips_update() + thermal_zone_device_check()

acpi_thermal_trips_update() + acpi_thermal_trips_update()

For the former, we can disable the thermal zone, update and then enable

For the latter use a driver lock ?


> 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 <rafael.j.wysocki@intel.com>
> ---
> 
> 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 |   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 |
> 
> 
>
Rafael J. Wysocki Aug. 1, 2023, 6:51 p.m. UTC | #2 
On Tue, Aug 1, 2023 at 8:39 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> On 25/07/2023 14:16, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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.
>
> As commented in patch 3/8, having a driver locking a thermal core
> structure is not right and goes to the opposite direction of the recent
> cleanups.

It already happens though, because thermal_zone_device_update() locks
the zone and it is called by the driver.

> Don't we have 2 race conditions:
>
> acpi_thermal_trips_update() + thermal_zone_device_check()
>
> acpi_thermal_trips_update() + acpi_thermal_trips_update()

I'm not sure what you mean.

First off, acpi_thermal_check_fn() needs to be locked against anything
using the trips in the zone's trips[] table, in particular
thermal_get_trend().

However, thermal_get_trend() also uses the driver's private trips
information, so it needs to be locked against
acpi_thermal_trips_update().

And obviously the latter needs to be locked against acpi_thermal_check_fn().

> For the former, we can disable the thermal zone, update and then enable

Disabling the thermal zone is an idea, but it would be necessary to do
that in both acpi_thermal_check_fn() and acpi_thermal_trips_update().
Also I'm not sure how different that would be from holding the zone
lock across the updates.

Moreover, acpi_thermal_trips_update() would then need to hold the
local lock around the thermal zone disable/enable which would be way
uglier than just using the zone lock directly in it.
diff mbox series

Patch

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 |