Messages in this thread | | | Date | Thu, 3 Aug 2023 18:20:06 +0200 | Subject | Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data | From | Daniel Lezcano <> |
| |
On 03/08/2023 16:15, Rafael J. Wysocki wrote: > On Thu, Aug 3, 2023 at 3:06 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 02/08/2023 18:48, Rafael J. Wysocki wrote: >> >> [ ... ] >> >>>> 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. >> >> May be I'm missing something but driver_ref does not seems to be used >> except when assigning it, no? > > It is used on the other side. That is, the value assigned to the trip > field in it is accessed via trip_ref in the driver. > > The idea is that the driver puts a pointer to its local struct > thermal_trip_ref into a struct thermal_trip and the core stores the > address of that struct thermal_trip in there, which allows the driver > to access the struct thermal_trip via its local struct > thermal_trip_ref going forward. > > Admittedly, this is somewhat convoluted. > > I have an alternative approach in the works, just for illustration > purposes if nothing else, but I have encountered a problem that I > would like to ask you about. > > Namely, zone disabling is not particularly useful for preventing the > zone from being used while the trips are updated, because it has side > effects. First, it triggers __thermal_zone_device_update() and a > netlink message every time the mode changes, which can be kind of > overcome.
Right
> But second, if the mode is "disabled", it does not actually > prevent things like __thermal_zone_get_trip() from running and the > zone lock is the only thing that can be used for that AFAICS. > > So by "disabling" a thermal zone, did you mean changing its mode to > "disabled" or something else?
Yes, that is what I meant.
May be the initial proposal by updating the thermal trips pointer can solve that [1]
IMO we can assume the trip point changes are very rare (if any), so rebuilding a new trip array and update the thermal zone with the pointer may solve the situation.
The routine does a copy of the trips array, so it can reorder it without impacting the array passed as a parameter. And it can take the lock.
We just have to constraint the update function to invalidate arrays with a number of trip points different from the one initially passed when creating the thermal zone.
Alternatively, we can be smarter in the ACPI driver and update the corresponding temperature+hysteresis trip point by using the thermal_zone_set_trip() function.
[1] https://lore.kernel.org/all/20230525140135.3589917-5-daniel.lezcano@linaro.org/
-- <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
| |