linux-kernel - Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb279cf1-0605-3b87-5cb6-241a91977455@linaro.org>
Date:   Wed, 2 Aug 2023 14:34:04 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Michal Wilczynski <michal.wilczynski@...el.com>,
        Zhang Rui <rui.zhang@...el.com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Subject: Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips
 with driver data


Hi Rafael,

On 01/08/2023 21:02, Rafael J. Wysocki wrote:
> On Tue, Aug 1, 2023 at 8:29 PM Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
>>
>> On 25/07/2023 14:04, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>>
>>> Some drivers need to update trip point data (temperature and/or
>>> hysteresis) upon notifications from the platform firmware or they
>>> may need to reprogram hardware when trip point parameters are changed
>>> via sysfs.  For those purposes, they need to connect struct thermal_trip
>>> to a private data set associated with the trip or the other way around
>>> and using a trip point index for that may not always work, because the
>>> core may need to reorder the trips during thermal zone registration (in
>>> particular, they may need to be sorted).
>>>
>>> To allow that to be done without using a trip point index, introduce
>>> a new field in struct thermal_trip that can be pointed by the driver
>>> to its own data structure containing a trip pointer to be initialized
>>> by the core during thermal zone registration.  That pointer will then
>>> have to be updated by the core every time the location of the given
>>> trip point object in memory changes.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>> ---
>>>
>>> v2 -> v3: No changes.
>>>
>>> v1 -> v2: No changes.
>>>
>>> ---
>>>    drivers/thermal/thermal_core.c |   20 +++++++++++++++++---
>>>    include/linux/thermal.h        |   13 +++++++++++++
>>>    2 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> Index: linux-pm/include/linux/thermal.h
>>> ===================================================================
>>> --- linux-pm.orig/include/linux/thermal.h
>>> +++ linux-pm/include/linux/thermal.h
>>> @@ -76,16 +76,29 @@ struct thermal_zone_device_ops {
>>>        void (*critical)(struct thermal_zone_device *);
>>>    };
>>>
>>> +struct thermal_trip_ref {
>>> +     struct thermal_trip *trip;
>>> +};
>>
>> That introduces a circular dependency. That should be avoided.
> 
> Sorry, but this is an empty statement without any substance.

I'm just pointing that we have a struct A pointing to struct B and 
struct B pointing to struct A.

[ ... ]

>>>    struct thermal_cooling_device_ops {
>>> Index: linux-pm/drivers/thermal/thermal_core.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/thermal/thermal_core.c
>>> +++ linux-pm/drivers/thermal/thermal_core.c
>>> @@ -1306,14 +1306,28 @@ thermal_zone_device_register_with_trips(
>>>        if (result)
>>>                goto release_device;
>>>
>>> +     mutex_lock(&tz->lock);
>>> +
>>>        for (count = 0; count < num_trips; count++) {
>>> -             struct thermal_trip trip;
>>> +             int temperature = 0;
>>> +
>>> +             if (trips) {
>>> +                     temperature = trips[count].temperature;
>>> +                     if (trips[count].driver_ref)
>>> +                             trips[count].driver_ref->trip = &trips[count];
>>> +             } else {
>>> +                     struct thermal_trip trip;
>>
>> As mentioned above, that should not appear in the thermal core code.
> 
> Well, this is a matter of opinion to me.  Clearly, I disagree with it.

Why? It is not an opinion. The thermal core code has been very very tied 
with the ACPI implementation (which is logical given the history of the 
changes). All the efforts have been made to cut these frictions and make 
the thermal core code driver agnostic.

The changes put in place a mechanism for the ACPI driver.

The thermal zone lock wrapper is put in place for the ACPI driver.

> Anyway, I want to be productive, so here's the thing: either something
> like this is done, or drivers need to be allowed to walk the trips
> table.
> 
> Which one is better?

None of them. I think we can find a third solution where the changes are 
self contained in the ACPI driver. What do you think?



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