On Wed, 2017-04-12 at 08:19 +0530, Keerthy wrote: > > On Tuesday 11 April 2017 10:59 PM, Eduardo Valentin wrote: > > > > Hey, > > > > On Fri, Mar 31, 2017 at 12:00:20PM +0530, Keerthy wrote: > > > > > > orderly_poweroff is triggered when a graceful shutdown > > > of system is desired. This may be used in many critical states of > > > the > > > kernel such as when subsystems detects conditions such as > > > critical > > > temperature conditions. However, in certain conditions in system > > > boot up sequences like those in the middle of driver probes being > > > initiated, userspace will be unable to power off the system in a > > > clean > > > manner and leaves the system in a critical state. In cases like > > > these, > > > the /sbin/poweroff will return success (having forked off to > > > attempt > > > powering off the system. However, the system overall will fail to > > > completely poweroff (since other modules will be probed) and the > > > system > > > is still functional with no userspace (since that would have shut > > > itself > > > off). > > OK... This seams to me, still a corner case supposed to be fixed at > > orderly_power_off, not at thermal. But.. > > > > > > > > > > > However, there is no clean way of detecting such failure of > > > userspace > > > powering off the system. In such scenarios, it is necessary for a > > > backup > > > workqueue to be able to force a shutdown of the system when > > > orderly > > > shutdown is not successful after a configurable time period. > > > > > Given that system running hot is a thermal issue, I guess we care > > more > > on this matter then.. > Yes! > I just read this thread again https://patchwork.kernel.org/patch/802458 1/ to recall the previous discussion. https://patchwork.kernel.org/patch/8149891/ https://patchwork.kernel.org/patch/8149861/ should be the solution made based on Ingo' suggestion, right? And to me, this sounds like the right direction to go, thermal does not need a back up shutdown solution, it just needs a kernel function call which guarantees the system can be shutdown/reboot immediately. is there any reason that patch 1/2 is not accepted? thanks, rui > > > > > > > > > > Reported-by: Nishanth Menon <nm@xxxxxx> > > > Signed-off-by: Keerthy <j-keerthy@xxxxxx> > > > --- > > > drivers/thermal/Kconfig | 13 +++++++++++++ > > > drivers/thermal/thermal_core.c | 42 > > > ++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 55 insertions(+) > > > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > > index 0a16cf4..4cc55f9 100644 > > > --- a/drivers/thermal/Kconfig > > > +++ b/drivers/thermal/Kconfig > > > @@ -15,6 +15,19 @@ menuconfig THERMAL > > > > > > if THERMAL > > > > > > +config THERMAL_EMERGENCY_POWEROFF_DELAY_MS > > > + int "Emergency poweroff delay in milli-seconds" > > > + depends on THERMAL > > > + default 0 > > > + help > > > + The number of milliseconds to delay before emergency > > > + poweroff kicks in. The delay should be carefully > > > profiled > > > + so as to give adequate time for orderly_poweroff. In > > > case > > > + of failure of an orderly_poweroff the emergency > > > poweroff > > > + kicks in after the delay has elapsed and shuts down > > > the system. > > > + > > > + If set to 0 poweroff will happen immediately. > > > + > > > config THERMAL_HWMON > > > bool > > > prompt "Expose thermal sensors as hwmon device" > > > diff --git a/drivers/thermal/thermal_core.c > > > b/drivers/thermal/thermal_core.c > > > index 11f0675..dc7fdd4 100644 > > > --- a/drivers/thermal/thermal_core.c > > > +++ b/drivers/thermal/thermal_core.c > > > @@ -322,6 +322,47 @@ static void handle_non_critical_trips(struct > > > thermal_zone_device *tz, > > > def_governor->throttle(tz, trip); > > > } > > > > > > +/** > > > + * emergency_poweroff_func - emergency poweroff work after a > > > known delay > > > + * @work: work_struct associated with the emergency poweroff > > > function > > > + * > > > + * This function is called in very critical situations to force > > > + * a kernel poweroff after a configurable timeout value. > > > + */ > > > +static void emergency_poweroff_func(struct work_struct *work) > > > +{ > > > + /** > > > + * We have reached here after the emergency thermal > > > shutdown > > > + * Waiting period has expired. This means > > > orderly_poweroff has > > > + * not been able to shut off the system for some reason. > > > + * Try to shut down the system immediately using > > > pm_power_off > > > + * if populated > > > + */ > > The above is not a kernel doc entry... > I will fix that. > > > > > > > > > > > + pr_warn("Attempting kernel_power_off\n"); > > > + if (pm_power_off) > > > + pm_power_off(); > > Why not calling kernel_power_off() directly instead? That is what > > is called by orderly > > power off in case it fails, which seams to be the missing part > > when > > user land returns success, and therefore we don't call > > kernel_power_off(). That path goes through the machine_power_off(), > > which seams to be the default for pm_power_off() anyway. > > > > kernel_power_off() handles the power off system call too. > Yes. This is after orderly_poweroff fails so i felt why go through > kernel_power_off and directly call pm_power_off which directly pulls > out > the power plug. This is in dire straits situation. Hence preferred to > call the last piece directly. > > > > > > > > > > > + > > > + /** > > not a kernel doc entry... > Okay. > > > > > > > > > > > + * Worst of the worst case trigger emergency restart > > > + */ > > > + pr_warn("kernel_power_off has failed! Attempting > > > emergency_restart\n"); > > > + emergency_restart(); > > > +} > > > + > > > +static DECLARE_DELAYED_WORK(emergency_poweroff_work, > > > emergency_poweroff_func); > > > + > > > +/** > > > + * emergency_poweroff - Trigger an emergency system poweroff > > > + * > > > + * This may be called from any critical situation to trigger a > > > system shutdown > > > + * after a known period of time. By default the delay is 0 > > > millisecond > > > + */ > > > +void thermal_emergency_poweroff(void) > > > +{ > > > + schedule_delayed_work(&emergency_poweroff_work, > > > + msecs_to_jiffies(CONFIG_THERMAL_EM > > > ERGENCY_POWEROFF_DELAY_MS)); > > > +} > > > + > > > static void handle_critical_trips(struct thermal_zone_device > > > *tz, > > > int trip, enum > > > thermal_trip_type trip_type) > > > { > > > @@ -343,6 +384,7 @@ static void handle_critical_trips(struct > > > thermal_zone_device *tz, > > > "critical temperature reached(%d > > > C),shutting down\n", > > > tz->temperature / 1000); > > > orderly_poweroff(true); > > > + thermal_emergency_poweroff(); > > Shouldn't we start count the timeout before calling > > orderly_poweroff? > Okay yes. That makes more sense. Queue the emergency function, start > the > countdown and immediately call the orderly_poweroff. I will fix the > above comments and send a v2. I still want to go with pm_power_off > over > kernel_poweroff as we have already elapsed the time out and the first > thing we want is to shut off the SoC! Let me know. > > > > > > > > > > > > } > > > } > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
