On Fri, Feb 3, 2023 at 10:47 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote: > > On 03/02/2023 19:46, Rafael J. Wysocki wrote: > > On Fri, Feb 3, 2023 at 6:34 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote: > >> > >> This series introduces the generic trip points usage in the thermal ACPI > >> driver. It provides a step by step changes to move the current code the > >> generic trip points. > >> > >> I don't have an ACPI platform, the code is not tested. > > > > What's the purpose of sending this now, then? Should it be an RFC? > > I'm certainly going to treat it this way. > > I did basic testing on a x86 laptop having critical trip points but > nothing else. > > I understand it can be treated as RFC. So I've gone through this series now and I'm not entirely convinced. While I agree with the general direction (using a generically-defined trip point representation instead of a home-grown one is definitely an improvement IMV and I understand the idea of putting all trip points into an array which then will allow the overhead in the core to be reduced), I don't quite like the way the change is carried out in the series. In particular, I'd prefer to avoid introducing "temporary" structures that get removed entirely by subsequent patches in the series - this makes the intermediate steps sort of pointless and doesn't really help to understand what's going on (quite on the contrary even, AFAIAC). I also don't quite like changes like the temperature unit conversion in patch 9. Personally, I would do it differently. I would start with changing the ACPI thermal driver to use the generic trip point definition (with possible extensions) instead of the home-grown one and continue from there. I think that this would be more straightforward, but I guess I just need to try it out myself. Beyond that, there is a question regarding the desired behavior of the whole framework in some cases, like when the trip points get modified by firmware and the kernel gets a notification to re-enumerate them. This may happen in ACPI-enabled systems in general, so the ACPI thermal driver needs to be prepared for it, but not just the driver - the thermal core needs to be prepared for it too. So the question is how it all should work in principle. AFAICS, not only the trip temperature can change, but the ordering of them can change as well such that the passive trip point lies between the active ones, for example. So when this happens, is the core going to tear down all of the trip point representation in sysfs and re-create it from scratch, even if it may be used by user space (that may not be prepared for the re-enumeration) at that point, or is it sufficient to simply make the trip point attributes return different values when the corresponding trip points change?
