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: <fedc35c2-1dc9-48af-f03f-fbb8566284fb@linaro.org>
Date:   Wed, 1 Feb 2023 11:42:17 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     srinivas.pandruvada@...ux.intel.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, rui.zhang@...el.com,
        Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for
 quark_dts

On 31/01/2023 20:11, Rafael J. Wysocki wrote:
> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
> <daniel.lezcano@...aro.org> wrote:
>>
>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
>>> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
>>> <daniel.lezcano@...aro.org> wrote:
>>>>
>>>> The thermal framework gives the possibility to register the trip
>>>> points with the thermal zone. When that is done, no get_trip_* ops are
>>>> needed and they can be removed.
>>>>
>>>> Convert ops content logic into generic trip points and register them with the
>>>> thermal zone.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>>>> ---
>>
>> [ ... ]
>>
>>>> -       aux_entry->tzone = thermal_zone_device_register("quark_dts",
>>>> -                       QRK_MAX_DTS_TRIPS,
>>>> -                       wr_mask,
>>>> -                       aux_entry, &tzone_ops, NULL, 0, polling_delay);
>>>> +       err = get_trip_temp(QRK_DTS_ID_TP_CRITICAL, &temperature);
>>>> +       if (err)
>>>> +               goto err_ret;
>>>> +
>>>> +       aux_entry->trips[QRK_DTS_ID_TP_CRITICAL].temperature = temperature;
>>>> +       aux_entry->trips[QRK_DTS_ID_TP_CRITICAL].type = THERMAL_TRIP_CRITICAL;
>>>> +
>>>> +       err = get_trip_temp(QRK_DTS_ID_TP_HOT, &temperature);
>>>> +       if (err)
>>>> +               goto err_ret;
>>>
>>> If I'm not mistaken, this won't even try to register the thermal zone
>>> if at least one trip cannot be initialized, but previously it was
>>> registered in that case, but the trips that failed to respond were
>>> disabled.
>>>
>>> This is a change in behavior that would at least need to be documented
>>> in the changelog, but it isn't.
>>>
>>> I'm not sure if it is safe to make even, however.
>>
>> Thanks for catching this.
>>
>> Two solutions:
>>
>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
>> THERMAL_TEMP_INVALID
>>
>> 2. Register only the valid trip points.
>>
>> What would be the preferable way ?
> 
> I think that the trip points that are registered currently need to
> still be registered after the change.
> 
> Does registering a trip point with the temperature set to
> THERMAL_TEMP_INVALID cause it to be effectively disabled?

The initial behavior before the changes is:

The function thermal_zone_device_register() will go through all the trip 
points and call thermal_zone_get_trip(), resulting in a call to 
ops->get_trip_temp(). If the call fails, the trip point is tagged as 
disabled and will stay in this state forever, so discarded in the trip 
point crossed detection.

That does not report an error and the trip point is showed in sysfs but 
in a inconsistent state as it is actually disabled. Reading the trip 
point will return an error or not, but it is in any case disabled in the 
thermal framework. The userspace does not have the information about the 
trip point being disabled, so showing it up regardless its state is 
pointless and prone to confusion for the userspace.

IMO, it would be more sane to register the trip points which are 
actually valid, so invalid trip points are not showed up and does 
prevent extra complexity in the thermal core to handle them.





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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ