Messages in this thread | | | Date | Wed, 2 Aug 2023 17:50:48 +0200 | Subject | Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data | From | Daniel Lezcano <> |
| |
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
| |