mbox series

[v1,00/11] Generic trip points for ACPI

Message ID 20230203173331.3322089-1-daniel.lezcano@linaro.org
Headers show
Series Generic trip points for ACPI | expand

Message

Daniel Lezcano Feb. 3, 2023, 5:33 p.m. UTC
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(-)

Comments

Daniel Lezcano Feb. 3, 2023, 9:47 p.m. UTC | #1
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 ?
Rafael J. Wysocki March 31, 2023, 4:04 p.m. UTC | #2
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?
Daniel Lezcano April 3, 2023, 10:28 a.m. UTC | #3
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.