| Message ID | 20230203173331.3322089-1-daniel.lezcano@linaro.org |
|---|---|
| Headers | show |
| Series | Generic trip points for ACPI | expand |
On 03/02/2023 19:46, Rafael J. Wysocki wrote: > On Fri, Feb 3, 2023 at 6:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> 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. Is there any dummy ACPI tables with thermal available somewhere I can play with in Qemu ?
On Fri, Feb 3, 2023 at 10:47 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 03/02/2023 19:46, Rafael J. Wysocki wrote: > > On Fri, Feb 3, 2023 at 6:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> 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?
Hi Rafael, thanks for taking the time to go through the series. On 31/03/2023 18:04, Rafael J. Wysocki wrote: > On Fri, Feb 3, 2023 at 10:47 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 03/02/2023 19:46, Rafael J. Wysocki wrote: >>> On Fri, Feb 3, 2023 at 6:34 PM Daniel Lezcano <daniel.lezcano@linaro.org> 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'm surprised it is less understandable. The ACPI trip points structure have nested and duplicated descriptions. The resulting code goes through these structures depending on the type. In order to convert the code to deal with the same structure, the first patches are moving the structures into a single one, so can more easily replace one by another. Before doing a full conversion to the trip point changes, they co-exist. > I also don't quite like changes like the > temperature unit conversion in patch 9. Is it the conversion itself or how the changes are done? > 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) Do you mean the adding a 'private' field in the trip point structure: struct thermal_trip { int temperature; int hysteresis; enum thermal_trip_type type; void *data; }; ? > 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. Yes, I think we should introduce a new function: thermal_zone_device_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips, int nr_trips); > 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. I recently sent a patch when passing the thermal zone parameters where the structure is kmemdup'ed and stored in the thermal zone device structure [1]. [1] https://lore.kernel.org/all/20230307133735.90772-11-daniel.lezcano@linaro.org/ That allows to initialize the thermal zone device and let the core code thermal framework to do stuff with it. The caller does not have to keep the thermal zone device parameter structure, it can create it on the stack and the forget it. IMO, we should do the same for the thermal trip array. So the thermal framework kmemdups it, reorders it if needed. The thermal trip array is then just an initialization array and once it is passed to thermal_zone_device_register_with_trips(), the driver should not use it anymore but use the thermal framework API. Having the private data in the thermal trip structure will allow the ACPI to store the trip id, so the trip array can be reordered. > 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? IMO, the function thermal_zone_device_trips_update() can take care of: - delete the sysfs entries - reorder the trip points, - create the sysfs entries - notify THERMAL_ZONE_TRIP_CHANGED - update the thermal zone Probably we can move part of the code from thermal_zone_device_register() into this function The userspace should be aware of the THERMAL_ZONE_TRIP_CHANGED event. However, I'm not sure there is an trip update leading to a change of the number of trip points as well as changing the order.
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. The changes are based on top of linux-pm/linux-next Daniel Lezcano (11): thermal/acpi: Remove the intermediate acpi_thermal_trip structure thermal/acpi: Change to a common acpi_thermal_trip structure thermal/acpi: Convert the acpi thermal trips to an array thermal/acpi: Move the active trip points to the same array thermal/acpi: Optimize get_trip_points() thermal/acpi: Encapsulate in functions the trip initialization thermal/acpi: Simplifify the condition check thermal/acpi: Remove active and enabled flags thermal/acpi: Convert the units to milli Celsuis thermal/acpi: Rewrite the trip point intialization to use the generic thermal trip thermal/acpi: Use the thermal framework ACPI API drivers/acpi/thermal.c | 683 ++++++++++++++++++++++++++--------------- 1 file changed, 439 insertions(+), 244 deletions(-)