| Message ID | 20201210121514.25760-2-daniel.lezcano@linaro.org |
|---|---|
| State | Accepted |
| Commit | d7203eedf4f68e9909fd489453168a9d26bf0c3d |
| Headers | show |
| Series | [1/5] thermal/core: Emit a warning if the thermal zone is updated without ops | expand |
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
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. [ ... ] >> #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
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
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); } 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; + 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,