Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data

From: Daniel Lezcano
Date: Wed Aug 02 2023 - 11:52:31 EST

On 02/08/2023 15:03, Rafael J. Wysocki wrote:

[ ... ]

+struct thermal_trip_ref {
+ struct thermal_trip *trip;
+};

That introduces a circular dependency. That should be avoided.

Sorry, but this is an empty statement without any substance.

I'm just pointing that we have a struct A pointing to struct B and
struct B pointing to struct A.

Why is this a problem in general?

Cyclic dependencies are often a sign of a design problem.

There are cases in which struct A needs to be found given struct B
(like in the ACPI thermal case, when the driver needs to get to
trips[i] from its local data) and there are cases in which struct B
needs to be found given struct A (like when a driver's callback is
invoked and passed a trip pointer, so the driver needs to get to its
local data from it - arguably this is not the case right now, but I
suppose it will be the case in the future).

[ ... ]

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;

As mentioned above, that should not appear in the thermal core code.

Well, this is a matter of opinion to me. Clearly, I disagree with it.

Why? It is not an opinion.

So what's wrong with it, technically? What's broken by it? Why does
it make the code more difficult to maintain?



The thermal core code has been very very tied
with the ACPI implementation (which is logical given the history of the
changes). All the efforts have been made to cut these frictions and make
the thermal core code driver agnostic.

The changes put in place a mechanism for the ACPI driver.

Not really, for all drivers that have local trip data and need to get
to trips[i] from there and/or the other way around.

The thermal zone lock wrapper is put in place for the ACPI driver.

Yes, it is, because that's the most straightforward way to address the
use case at hand IMV.

Anyway, I want to be productive, so here's the thing: either something
like this is done, or drivers need to be allowed to walk the trips
table.

Which one is better?

None of them. I think we can find a third solution where the changes are
self contained in the ACPI driver. What do you think?

The ACPI thermal driver needs to update trip point temperatures at
times. For this purpose, it needs to get from its local trip data to
trip[i] somehow.

Creating a new trips[] array and handing it over to the core is not an
option, because it potentially breaks the thermal device binding to
the zone (in which trip indices are used, mind you).

So how exactly do you want the driver to do the above?

It could save a pointer to each trips[i] in its local data structures
before registering the zone, but then if the core reordered the trips,
those pointers would become stale.

So how?

Let me check if I can do something on top of your series to move it in the ACPI driver.


--
<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