lkml.org 
[lkml]   [2012]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-05-25 13:01    [W:0.060 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site