Re: [PATCH v7 12/13] ACPI / init: Invoke early ACPI initialization earlier

[Dou Liyang]

At 08/24/2017 06:21 PM, Baoquan He wrote:
> On 08/24/17 at 05:28pm, Dou Liyang wrote:
> > Hi Baoquan,
> >
> > Thanks for your reply.
> >
> > At 08/24/2017 04:05 PM, Baoquan He wrote:
> > > Hi Liyang,
> > >
> > > On 08/24/17 at 11:54am, Dou Liyang wrote:
> > > > > > Test in my own PC(Lenovo M4340).
> > > > > > Ask help for doing regression testing for the bug said in commit
> > > > > > c4e1acbb35e4
> > > > > > ("ACPI / init: Invoke early ACPI initialization later").
> > > > >
> > > > > Now, I can prove this patch doesn't result in the bug[1] which made the
> > > > > fast TSC calibration using PIT failed in a Thinkpad x121e (AMD E-450
> > > > > APU).
> > > > >
> > > > > The true reason of the bug is enabling ACPI subsystem earlier than
> > > > > using PIT, not the SCI setup. invoking acpi_enable_subsystem() later
> > >
> > > Seems redhat mail server was down earlier, I didn't receive new mail in
> > > this thread.  Just curious, do you know why the fast tsc calibration
> > > using PIT will fail if enabling ACPI subsystem earlier than using PIT?
> > It's related to particular hardware, As you know, I tested in many
> > kinds of PC and laptop and PIT works well no matter before or after
> > enabling ACPI subsystem.
> >
> > In pit_verify_msb(), we use inb(0x42) to read the current MSB,
> >
> > Normally, the value is continuously, like following shows:
> >
> > msb = fe
> > msb = fd
> > msb = fc
> > msb = fb
> > msb = fa
> > msb = f9
> > msb = f8
> > msb = f7
> > msb = f6
> > ...
> >
> > But, if in some particular hardware, you will see like that:
> >
> > msb = fe
> > msb = f0
> > msb = ed
> > msb = e9
> > msb = e0
> > msb = db
> > ...
> >
> > In this case, the count in pit_expect_msb() is always zero.
> > So we will see "Fast TSC calibration failed" in our dmesg log.
>
> Thanks for telling!
>
> It's truly weird that the TSC becomes unstable only if enabling ACPI
> subsystem earlier than using PIT.
>
> Let's see what other people say about this.
>
> Btw, you will resend another round, right? Then I would like to test
> your new post.

Yes, I am waiting ACPI maintainers advice and I prepare to re-base it
next week when rc7 is out.

Thanks,
	dou.

> Thanks
> Baoquan
>
> > For the further deep reason why the hardware failed, I'm sorry
> > I can't answer and don't know how to investigate. For hardware,
> > I usually change a new one directly and know very little.
> >
> > Thanks,
> > 	dou.
> >
> >
> > > Thanks
> > > Baoquan
> > >
> > > > > could fix this bug as Julian tested and said[2].
> > > > >
> > > > > And, I found that Commit b064a8fa77df (" ACPI / init: Switch over
> > > > > platform to the ACPI mode later") split the ACPI early initialization
> > > > > code into acpi_early_init() and acpi_subsystem_init(). executing
> > > > > acpi_enable_subsystem() at the original early ACPI initialization spot.
> > > > >
> > > > > The sequence of them shows below:
> > > > >
> > > > >  start_kernel
> > > > > +---------------+
> > > > > |
> > > > > +--> .......
> > > > > |
> > > > > |    late_time_init()
> > > > > +--> +-------+
> > > > > |
> > > > > +--> .......
> > > > > |
> > > > > |    acpi_early_init()
> > > > > +--> +-------+
> > > > > |
> > > > > +--> .......
> > > > > |
> > > > > |   acpi_subsystem_init()
> > > > > +-> +--------+
> > > > >
> > > > > We make sure the acpi_subsystem_init() is called later than
> > > > > late_time_init(), the bug will be avoided.
> > > > >
> > > > > This patch changes the sequence of late_time_init() and
> > > > > acpi_early_init(), doesn't effect acpi_subsystem_init().
> > > > >
> > > > > So, this patch is OK.
> > > > >
> > > > > Btw, Thanks very much for Borislav Petkov, he will have access to
> > > > > Thinkpad x121e from Mid-August and will test this series.
> > > >
> > > > Almost one month passed, Borislav have tested this series in Thinkpad
> > > > x121e and I also have tested in my box and QEmu again. It is OK.
> > > >
> > > > BTW,
> > > > 1) I found your commit b064a8fa77df (" ACPI / init: Switch over
> > > > platform to the ACPI mode later") split the ACPI early initialization
> > > > code into acpi_early_init() and acpi_subsystem_init(). Actually enabling
> > > > the ACPI subsystem is in acpi_subsystem_init().
> > > >
> > > > 2) As we discussed earlier, invoking acpi_put_table() is not good for
> > > > this situation.
> > > >
> > > > So I do this patch, Is that goot to you? Any comments will be welcome.
> > > >
> > > > If it is OK, As the patches need to be re-based, and I also found
> > > > several spelling mistake, I will send a new version next week.
> > > >
> > > > Thanks,
> > > > 	dou.
> > > >
> > > > > [1] https://lkml.org/lkml/2014/3/10/123
> > > > > [2] https://lkml.org/lkml/2014/3/12/311
> > > > >
> > > > >
> > > > > Thanks
> > > > >     dou.
> > > > >
> > > > > >  init/main.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/init/main.c b/init/main.c
> > > > > > index df58a41..7a09467 100644
> > > > > > --- a/init/main.c
> > > > > > +++ b/init/main.c
> > > > > > @@ -654,12 +654,12 @@ asmlinkage __visible void __init start_kernel(void)
> > > > > >      kmemleak_init();
> > > > > >      setup_per_cpu_pageset();
> > > > > >      numa_policy_init();
> > > > > > +    acpi_early_init();
> > > > > >      if (late_time_init)
> > > > > >          late_time_init();
> > > > > >      calibrate_delay();
> > > > > >      pidmap_init();
> > > > > >      anon_vma_init();
> > > > > > -    acpi_early_init();
> > > > > >  #ifdef CONFIG_X86
> > > > > >      if (efi_enabled(EFI_RUNTIME_SERVICES))
> > > > > >          efi_enter_virtual_mode();