RE: [PATCH v2] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

On Wednesday, May 17, 2017 8:52 PM Sascha Hauer wrote:
> On Wed, May 17, 2017 at 02:42:24PM +0000, Mirea, Bogdan-Stefan wrote:
> > Hello Sascha,
> >
> > On Wednesday, May 17, 2017 2:30 PM Sascha Hauer wrote:
> > > As John already said, there's the read_boot_clock64() interface which
> > > should be used here.
> > > By using the read_boot_clock64() interface you can make sure that you
> > > only provide the functionality when it's actually supposed to work.
> > The read_boot_clock64() function is called from timekeeping_init().
> > The arm arch timer init callback arch_timer_of_init() function is called
> > from time_init() function.
> > We will have an uninitialized arm arch timer (this is our only timer
> > source) at the read_boot_clock64() function call since both
> > timekeeping_init() and time_init() functions are called from
> > start_kernel() in the following order:
> >     asmlinkage __visible void __init start_kernel(void)
> >     {
> >         /* ... */
> >         timekeeping_init();
> >         time_init();
> >         /* ... */
> >     }
> > So in our case, we cannot use a read_boot_clock64() function to read cyc
> > value.
> >
> >
> > > In your patch you provide a generic option (BOOT_TIME_PRESERVE) which
> > > only works as expected in some special cases. You assume that the
> > > bootloader has started the same timer with the same frequency as Linux
> > > does.
> > Yes, I assume that the bootloader and Linux use and configure the same
> > timer at the same frequency. This is a must since we want to measure the
> > exact boottime. The reason the code is guarded with BOOT_TIME_PRESERVE
> > is because we want to avoid situations like the one you described.
> 
> IMO Kernel options should be safe to enable everytime. They shouldn't
> have side effects on boards that lack the necessary hardware or
> bootloader support. I think we should at least have a commandline option
> or device tree property in which the bootloader can signal that it has
> initialized the timer suitable for calculating the boot time.
> 
> >
> >
> > > Also you assume that the timer startup code doesn't reset the
> > > timer. When these assumptions are not true then you just get bogus
> > > random timing information.
> > If the timer initialization code will reset the timer, the Linux system
> > will work exactly as before, showing boottime 0 in kmsg and 0 in
> > uptime, so no bugs/crashes will occur.
> 
> It may also be that the timer initialization code switches to another
> frequency, but doesn't reset the timer. All kinds of funny things can
> happen when the timer initialization code hasn't been audited to support
> this usecase. So really we shouldn't provide the user an option without
> giving any hint if this can even work on his hardware

I agree, I will add a check for a cmdline parameter passed from bootloader.
I will create a patch v3.

Kind Regards,
Bogdan