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

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

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.

Something like a helper function always called:
void _handle_critical( *tz)
{
	if (tz->ops->critical)
		tz->ops->critical(tz);

	thermal_zone_device_critical(tz);
}

and then called:

	else if (trip_type == THERMAL_TRIP_CRITICAL)
		_handle_critical(tz);

>   }
>   
>   static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
> @@ -1331,6 +1338,10 @@ thermal_zone_device_register(const char *type, int trips, int mask,
>   
>   	tz->id = id;
>   	strlcpy(tz->type, type, sizeof(tz->type));
> +
> +	if (!ops->critical)
> +		ops->critical = thermal_zone_device_critical;

Then we could drop this default assignment.

> +
>   	tz->ops = ops;
>   	tz->tzp = tzp;
>   	tz->device.class = &thermal_class;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f23a388ded15..125c8a4d52e6 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -79,6 +79,8 @@ struct thermal_zone_device_ops {
>   			  enum thermal_trend *);
>   	int (*notify) (struct thermal_zone_device *, int,
>   		       enum thermal_trip_type);
> +	void (*hot)(struct thermal_zone_device *);
> +	void (*critical)(struct thermal_zone_device *);
>   };
>   
>   struct thermal_cooling_device_ops {
> @@ -399,6 +401,7 @@ void thermal_cdev_update(struct thermal_cooling_device *);
>   void thermal_notify_framework(struct thermal_zone_device *, int);
>   int thermal_zone_device_enable(struct thermal_zone_device *tz);
>   int thermal_zone_device_disable(struct thermal_zone_device *tz);
> +void thermal_zone_device_critical(struct thermal_zone_device *tz);
>   #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)?

Regards,
Lukasz