Re: [PATCH v4 1/1] printk: fix zero-valued printk timestamps in early boot

[Geert Uytterhoeven]

Hi Roberto,

On Wed, 15 Apr 2026 at 11:20, Roberto A. Foglietta
<roberto.foglietta@xxxxxxxxx> wrote:
> On Wed, 15 Apr 2026 at 02:19, Roberto A. Foglietta
> <roberto.foglietta@xxxxxxxxx> wrote:
> >
> > On Wed, 15 Apr 2026 at 00:38, Thomas Gleixner <tglx@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Apr 10 2026 at 14:37, Tim Bird wrote:
> > > > +
> > > > +#include <linux/timekeeping.h>
> > > > +#ifdef CONFIG_ARM64
> > > > +#include <asm/sysreg.h>
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_EARLY_CYCLES_KHZ
> > > > +static inline u64 early_unsafe_cycles(void)
> > > > +{
> > > > +#if defined(CONFIG_X86_64)
> > > > +     /*
> > > > +      * This rdtsc may happen before secure TSC is initialized, and
> > > > +      * it is unordered. So please don't use this value for cryptography
> > > > +      * or after SMP is initialized.
> > > > +      */
> > > > +     return rdtsc();
> > > > +#elif defined(CONFIG_ARM64)
> > > > +     return read_sysreg(cntvct_el0);
> > > > +#elif defined(CONFIG_RISCV_TIMER)
> > > > +     u64 val;
> > > > +
> > > > +     asm volatile("rdtime %0" : "=r"(val));
> > > > +     return val;
> > > > +#else
> > > > +     return 0;
> > > > +#endif
> > > > +}
> > >
> > > No. Generic code and generic headers have no business to implement any
> > > architecture specific code and there is zero justification for
> > > architecture specific #ifdefs in generic code.
> >
> > This translates in practice in early_unsafe_cycles() various
> > instances, one for each architecture supported. Even better a macro
> > for each architecture like this example below (untested code, just for
> > exemplification):
> >
> >   ts_nsec = local_clock();
> > #ifdef CONFIG_EARLY_CYCLES_KHZ
> > #if defined(CONFIG_ARM64)
> > #include <asm/sysreg.h>
> > #define early_unsafe_cycles read_sysreg(cntvct_el0);
> > #elif defined(CONFIG_X86_64)
> > #define early_unsafe_cycles rdtsc()
> > #elif defined(CONFIG_RISCV_TIMER)
> > #define early_unsafe_cycles ({ u64 val; asm volatile("rdtime %0" :
> > "=r"(val)); val; })
> > #else
> > #define early_unsafe_cycles 0
> > #endif
> > + if (unlikely(!ts_nsec))
> > + ts_nsec = early_times_ns();
> >  #endif // CONFIG_EARLY_CYCLES_KHZ
> >   caller_id = printk_caller_id();
> >
> > While keeping architectures separate is for abstraction layer best
> > practice, in very specific cases abstraction can be a meaningless
> > concept or much less important than having a single file for a single
> > feature (the complementary way of organising stuff). Because in Linux
> > the abstraction layer approach applies for architectures, the gap
> > between aesthetic and a specific case like an early boot macros set
> > hack for debugging is to clarify the exception from the general rule
> > in the header itself.
>
> IMHO, this patch (upstreamed or not) is for a few hackers and it is
> not even a tool but a first-outlook or last-resort hack. Therefore the
> single point of change that makes sense against spreading stuff that
> is supposed to be further hacked isn't the correct approach. The
> debate can be about upstreaming this kind of changes, and IMHO this
> one still has a sense to be upstreamed among debug features.
>
> I am attaching the path for just sake of independence storage (due my
> project github repo linked before is naturally subject to arbitrary
> changes by me). It is not supposed to be applied as-is (and probably
> cannot be applied anyway) and the patch has been written by hands on a
> keyboard. Gemini was used for retrieving information (in a smart man 3
> command style) and to collect the ASM code and its fencing.
>
> So the main question is: does this v5 suggested approach bridge the
> 1-single-point-of-change practical needs with the aim of keeping the
> source three? If yes then a V6 is worth being developed to provide the
> functionality and propose the upstream (I do not bet in good at the
> first write but on resolving the contrast/debate THEN delivery).
>
> Best regards, R-

Thanks for your patch!

> --- /dev/null
> +++ kernel/printk/early_times.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _EARLY_TIMES_H
> +#define _EARLY_TIMES_H
> +
> +#include <linux/timekeeping.h>
> +
> +/*
> + * Fencing isn't optional here, otherwise unreliable values displaying
> + */
> +#if defined(CONFIG_ARM64)
> +             #include <asm/sysreg.h>
> +             #define __early_raw_cycles ({ u64 val; \
> +                 asm volatile("isb; mrs %0, cntvct_el0" : "=r"(val)); val; })
> +#elif defined(CONFIG_X86_64)
> +             #define __early_raw_cycles ({ u64 val; \
> +                 asm volatile("lfence; rdtsc; shl $32, %%rdx; or %%rdx, %%rax" \
> +                     : "=a"(val) : : "rdx"); val; })
> +#elif defined(CONFIG_RISCV_TIMER)
> +             #define __early_raw_cycles ({ u64 val; \
> +                 asm volatile("fence; rdtime %0" : "=r"(val)); val; })
> +#else

All of these should be handled in arch/*/include/asm/early_times.h...

> +             #define __early_raw_cycles 0

... while this should be in include/asm-generic/early_times.h.

> +#endif
> +
> +#endif /* _EARLY_TIMES_H */

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2294,6 +2295,8 @@ int vprintk_store(int facility, int level,
>        * timestamp with respect to the caller.
>        */
>       ts_nsec = local_clock();
>
> +#if CONFIG_EARLY_BOOT_TIMINGS
> +#include "early_times.h"
> +             /*
> +              * Very few developers are using this feature and they're expecting to deal
> +              * with it as a single point of change hack to be further customised by them.
> +              * The right shift by 2^10 is a raw extimation to provide 6-figure within the
> +              * first second when the kernel internal cycle/nS calibration isn't ready yet.
> +              */
> +             if (unlikely(!ts_nsec))
> +                     ts_nsec = __early_raw_cycles >> 10;

As there can be lots of variation in the granularity of early timers,
I think the shift should be moved in the arch-specific __early_raw_cycles,
so it is only done when useful.


> +#endif

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds