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@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'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@xxxxxxxxxx/
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.
--
<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
