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

[Lukasz Luba <lukasz.luba@arm.com>]

On 12/10/20 1:37 PM, Daniel Lezcano wrote:
> 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@canonical.com>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.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.

I see.

> 
> [ ... ]
> 
>>>    #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.
> 

Fair enough. Thus, the patch LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz