Re: [PATCH v5 05/11] ACPI: thermal: Carry out trip point updates under zone lock

[Rafael J. Wysocki]

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 <rafael.j.wysocki@xxxxxxxxx>
> > 
> > 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 <rafael.j.wysocki@xxxxxxxxx>
> > ---
> 
> [ ... ]
> 
> >   {
> > -	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 <rafael.j.wysocki@xxxxxxxxx>
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 <rafael.j.wysocki@xxxxxxxxx>
---
 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