Re: [PATCH v7 2/9] reboot: thermal: Export hardware protection shutdown

[Matti Vaittinen]

Hi Daniel,

Thank you for the review. Much appreciated!

On Sat, 2021-04-17 at 07:32 +0200, Daniel Lezcano wrote:
> On 14/04/2021 07:52, Matti Vaittinen wrote:
> > Thermal core contains a logic for safety shutdown. System is
> > attempted to
> > be powered off if temperature exceeds safety limits.
> > 
> > Currently this can be also utilized by regulator subsystem as a
> > final
> > protection measure if PMICs report dangerous over-voltage, over-
> > current or
> > over-temperature and if per regulator counter measures fail or do
> > not
> > exist.
> > 
> > Move this logic to kernel/reboot.c and export the functionality for
> > other
> > subsystems to use. Also replace the mutex with a spinlock to allow
> > using
> > the function from any context.
> > 
> > Also the EMIF bus code has implemented a safety shut-down. EMIF
> > does not
> > attempt orderly_poweroff at all. Thus the EMIF code is not
> > converted to use
> > this new function.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > ---
> > Changelog
> >  v7:
> >   - new patch
> > 
> > Please note - this patch has received only a minimal amount of
> > testing.
> > (The new API call was tested to shut-down my system at driver probe
> > but
> > no odd corner-cases have been tested).
> > 
> > Any testing for thermal shutdown is appreciated.
> > ---
> >  drivers/thermal/thermal_core.c | 63 ++-----------------------
> >  include/linux/reboot.h         |  1 +
> >  kernel/reboot.c                | 86
> > ++++++++++++++++++++++++++++++++++
> 
> Please send a patch implementing the reboot/shutdown and then another
> one replacing the thermal shutdown code by a call to the new API.

I guess your suggestion makes sense. That way if the change causes any
problems in thermal-core it can be reverted without impacting other
potential users of this API. My original thinking was that this was
more of an move of functionality than adding an API. Having the move as
one patch makes sense as it shows where the code came from.

> 
> >  3 files changed, 91 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 996c038f83a4..b1444845af38 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -36,10 +36,8 @@ static LIST_HEAD(thermal_governor_list);
> >  
> > 

...

> > +static bool prot_power_off_triggered;
> > +static DEFINE_SPINLOCK(poweroff_lock);
> > +
> > +/**
> > + * hw_protection_shutdown - Trigger an emergency system poweroff
> > + *
> > + * @reason:		Reason of emergency shutdown to be
> > printed.
> > + * @ms_until_forced:	Time to wait for orderly shutdown
> > before tiggering a
> > + *			forced shudown. Negative value disables the
> > forced
> > + *			shutdown.
> > + *
> > + * Initiate an emergency system shutdown in order to protect
> > hardware from
> > + * further damage. Usage examples include a thermal protection or
> > a voltage or
> > + * current regulator failures.
> > + * NOTE: The request is ignored if protection shutdown is already
> > pending even
> > + * if the previous request has given a large timeout for forced
> > shutdown.
> > + * Can be called from any context.
> > + */
> > +void hw_protection_shutdown(const char *reason, int
> > ms_until_forced)
> > +{
> > +	unsigned long flags;
> > +
> > +	pr_emerg("HARDWARE PROTECTION shutdown (%s)\n", reason);
> > +
> > +	spin_lock_irqsave(&poweroff_lock, flags);
> > +	if (prot_power_off_triggered) {
> > +		spin_unlock(&poweroff_lock);
> 
> Why not spin_unlock_irqrestore() ?
> 

Well spotted It for sure must be spin_unlock_irqrestore. My bad.

> > +		return;
> > +	}
> > +	prot_power_off_triggered = true;
> > +	spin_unlock_irqrestore(&poweroff_lock, flags);
> 
> Why not take the spin_lock definitively for all the procedure ?
> 
> eg.
> 
> {
> 	...
> 
> 	pr_emerg( ... );
> 
> 	if (spin_trylock(&lock))
> 		return;
> 
> 	hw_failure_emergency_poweroff(ms_until_forced);
> 
> 	orderly_poweroff(true);
> }
> 
> No need of prot_power_off_triggered and the spin_lock can be declared
> static inside the function.

I think this makes perfect sense. My thinking just jammed to replacing
the mutex thermal-core used with a spin-lock using similar logic. I
guess this could even be just an atomic cmpxchg (or equivalent, I don't
remember what atomic abstractions we have) just to return if function
has been previously executed. Well, the spin_trylock() should work just
fine as far as I can say. So - thanks.


Best Regards
	Matti Vaittinen