Re: [PATCH v1 00/11] Generic trip points for ACPI

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: rjw@rjwysocki.net, linux-acpi@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v1 00/11] Generic trip points for ACPI
Date: Mon, 3 Apr 2023 12:28:18 +0200	[thread overview]
Message-ID: <38a5a6e0-2af8-8365-b20e-8494a4efcb0c@linaro.org> (raw)
In-Reply-To: <CAJZ5v0hWE_gRHj7_zZmu=firwTOnF1X4j59hrV_iy045R8GeeQ@mail.gmail.com>

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.

-- 
<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