linux-kernel - Re: [PATCH 2/5] thermal/core: Add critical and hot ops

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: <565c354e-0850-47f3-ad58-ee28fdedcfb2@linaro.org>
Date:   Thu, 10 Dec 2020 14:37:27 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Lukasz Luba <lukasz.luba@....com>, rui.zhang@...el.com
Cc:     kai.heng.feng@...onical.com, srinivas.pandruvada@...ux.intel.com,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH 2/5] thermal/core: Add critical and hot ops

On 10/12/2020 13:44, Lukasz Luba wrote:
> 
> 
> On 12/10/20 12:15 PM, Daniel Lezcano wrote:
>> Currently there is no way to the sensors to directly call an ops in
>> interrupt mode without calling thermal_zone_device_update assuming all
>> the trip points are defined.
>>
>> A sensor may want to do something special if a trip point is hot or
>> critical.
>>
>> This patch adds the critical and hot ops to the thermal zone device,
>> so a sensor can directly invoke them or let the thermal framework to
>> call the sensor specific ones.
>>
>> Tested-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>> ---
>>   drivers/thermal/thermal_core.c | 43 +++++++++++++++++++++-------------
>>   include/linux/thermal.h        |  3 +++
>>   2 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c
>> b/drivers/thermal/thermal_core.c
>> index e6771e5aeedb..cee0b31b5cd7 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -375,6 +375,25 @@ static void thermal_emergency_poweroff(void)
>>                     msecs_to_jiffies(poweroff_delay_ms));
>>   }
>>   +void thermal_zone_device_critical(struct thermal_zone_device *tz)
>> +{
>> +    dev_emerg(&tz->device, "%s: critical temperature reached, "
>> +          "shutting down\n", tz->type);
>> +
>> +    mutex_lock(&poweroff_lock);
>> +    if (!power_off_triggered) {
>> +        /*
>> +         * Queue a backup emergency shutdown in the event of
>> +         * orderly_poweroff failure
>> +         */
>> +        thermal_emergency_poweroff();
>> +        orderly_poweroff(true);
>> +        power_off_triggered = true;
>> +    }
>> +    mutex_unlock(&poweroff_lock);
>> +}
>> +EXPORT_SYMBOL(thermal_zone_device_critical);
>> +
>>   static void handle_critical_trips(struct thermal_zone_device *tz,
>>                     int trip, enum thermal_trip_type trip_type)
>>   {
>> @@ -391,22 +410,10 @@ static void handle_critical_trips(struct
>> thermal_zone_device *tz,
>>       if (tz->ops->notify)
>>           tz->ops->notify(tz, trip, trip_type);
>>   -    if (trip_type == THERMAL_TRIP_CRITICAL) {
>> -        dev_emerg(&tz->device,
>> -              "critical temperature reached (%d C), shutting down\n",
>> -              tz->temperature / 1000);
>> -        mutex_lock(&poweroff_lock);
>> -        if (!power_off_triggered) {
>> -            /*
>> -             * Queue a backup emergency shutdown in the event of
>> -             * orderly_poweroff failure
>> -             */
>> -            thermal_emergency_poweroff();
>> -            orderly_poweroff(true);
>> -            power_off_triggered = true;
>> -        }
>> -        mutex_unlock(&poweroff_lock);
>> -    }
>> +    if (trip_type == THERMAL_TRIP_HOT && tz->ops->hot)
>> +        tz->ops->hot(tz);
>> +    else if (trip_type == THERMAL_TRIP_CRITICAL)
>> +        tz->ops->critical(tz);
> 
> I can see that in the patch 3/5 there driver .critical() callback
> calls framework thermal_zone_device_critical() at the end.
> I wonder if we could always call this framework function.

It is actually done on purpose, we want to let the driver to handle the
critical routine which may not end up with an emergency shutdown.

[ ... ]

>>   #else
>>   static inline struct thermal_zone_device *thermal_zone_device_register(
>>       const char *type, int trips, int mask, void *devdata,
>>
> 
> I am just concerned about drivers which provide own .critical() callback
> but forgot to call thermal_zone_device_critical() at the end and
> framework could skip it.
> 
> Or we can make sure during the review that it's not an issue (and ignore
> out of tree drivers)?

Yes, the framework guarantees if the critical trip point is crossed we
call the emergency shutdown by default. If the driver choose to override
it, it takes responsibility of the change.



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