Date:   Wed, 12 Apr 2017 11:20:48 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     Keerthy <j-keerthy@...com>, Eduardo Valentin <edubezval@...il.com>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, nm@...com, t-kristo@...com
Subject: Re: [PATCH] thermal: core: Add a back up thermal shutdown mechanism

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@...com>
> > > Signed-off-by: Keerthy <j-keerthy@...com>
> > > ---
> > >  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.
> 
> 
> > 
> > 
> > > 
> > >  	}
> > >  }
> > >  

Powered by blists - more mailing lists