Messages in this thread | | | Date | Fri, 25 May 2012 12:33:57 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH] x86: clear HPET configuration registers on startup |
| |
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
| |