Re: [PATCH] x86: clear HPET configuration registers on startup

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, 25 May 2012, Jan Beulich wrote:
> >>> On 25.05.12 at 00:06, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 2 Apr 2012, Jan Beulich wrote:
> > 
> > Sorry for ignoring this for so long.
> > 
> >> +	cfg = hpet_readl(HPET_CFG);
> >> +	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
> >> +				GFP_KERNEL);
> >> +	if (hpet_boot_cfg)
> >> +		*hpet_boot_cfg = cfg;
> >> +	else
> >> +		pr_warn("HPET initial state will not be saved\n");
> >> +	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
> >> +	hpet_writel(cfg, HPET_Tn_CFG(i));
> > 
> > This wants to be  
> > 
> >> +	hpet_writel(cfg, HPET_CFG);
> > 
> > Right ?
> 
> Oh yes, absolutely.
> 
> >> @@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
> >>  void hpet_disable(void)
> >>  {
> >>  	if (is_hpet_capable() && hpet_virt_address) {
> >> -		unsigned int cfg = hpet_readl(HPET_CFG);
> >> +		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
> >>  
> >> -		if (hpet_legacy_int_enabled) {
> >> +		if (hpet_boot_cfg)
> >> +			cfg = *hpet_boot_cfg;
> > 
> > That restores the setting which you recorded at init time. Why do you
> > want to do that? There is no point to restore to an eventually borked
> > state. If we shut down the thing, then we better leave it in a
> > consistent state rather than something dubious, really.
> 
> The problem is that we can't - forward compatibly - say what
> is "borked" and what is merely beyond the knowledge of the
> kernel. Given the system was able to boot with the original
> settings, restoring them seems the safest approach to me.
> 
> Besides that it's not the purpose of the patch to get around
> firmware bugs, but instead to get the hardware back into
> boot-time like state. So I'd really like to merely correct the
> error above that you pointed out (which also would seem to
> be the most appropriate route given that Linus already
> merged the patch), and leave a decision whether you agree
> with my position here (or whether you want to further
> tweak that code) to you.

I can see the point, but what I really don't like is restoring to an
eventually enabled state instead of doing it proper and keep the thing
shut down.

Thanks,

	tglx