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

[Rafael J. Wysocki]

On Wed, Aug 2, 2023 at 5:50 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> 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.

It doesn't need to be on top of my series, so if you have an idea,
please just let me know what it is.

It can't be entirely in the ACPI driver AFAICS, though, because
trips[i] need to be modified on updates and they belong to the core.
Hence, the driver needs some help from the core to get to them.  It
can be something like "this is my trip tag and please give me the
address of the trip matching it" or similar, but it is needed, because
the driver has to assume that the trip indices used by it initially
may change.