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

[Rafael J. Wysocki]

Hi Daniel,

On Wed, Aug 2, 2023 at 2:34 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
>
> Hi Rafael,
>
> On 01/08/2023 21:02, Rafael J. Wysocki wrote:
> > On Tue, Aug 1, 2023 at 8:29 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> On 25/07/2023 14:04, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >>>
> >>> 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 <rafael.j.wysocki@xxxxxxxxx>
> >>> ---
> >>>
> >>> v2 -> v3: No changes.
> >>>
> >>> 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;
> >>> +};
> >>
> >> 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?

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?