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

Daniel Lezcano
2023-08-02T15:52:25Z

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 |