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]

Date:   Wed, 2 Aug 2023 17:50:48 +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

On 02/08/2023 15:03, Rafael J. Wysocki wrote:

[ ... ]

>>>>> +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.
> 
> Why is this a problem in general?

Cyclic dependencies are often a sign of a design problem.

> There are cases in which struct A needs to be found given struct B
> (like in the ACPI thermal case, when the driver needs to get to
> trips[i] from its local data) and there are cases in which struct B
> needs to be found given struct A (like when a driver's callback is
> invoked and passed a trip pointer, so the driver needs to get to its
> local data from it - arguably this is not the case right now, but I
> suppose it will be the case in the future).
>
>> [ ... ]
>>
>>>>>     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.
> 
> So what's wrong with it, technically?  What's broken by it?  Why does
> it make the code more difficult to maintain?



>> 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.
> 
> Not really, for all drivers that have local trip data and need to get
> to trips[i] from there and/or the other way around.
> 
>> The thermal zone lock wrapper is put in place for the ACPI driver.
> 
> Yes, it is, because that's the most straightforward way to address the
> use case at hand IMV.
> 
>>> 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?
> 
> The ACPI thermal driver needs to update trip point temperatures at
> times.  For this purpose, it needs to get from its local trip data to
> trip[i] somehow.
> 
> Creating a new trips[] array and handing it over to the core is not an
> option, because it potentially breaks the thermal device binding to
> the zone (in which trip indices are used, mind you).
> 
> So how exactly do you want the driver to do the above?
> 
> It could save a pointer to each trips[i] in its local data structures
> before registering the zone, but then if the core reordered the trips,
> those pointers would become stale.
> 
> So how?

Let me check if I can do something on top of your series to move it in 
the ACPI driver.


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