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

[Lukasz Luba]

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@xxxxxxxxxxxxx>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > ---
> > >    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@xxxxxxx>

Regards,
Lukasz