Re: [PATCH v2 1/3] powerpc/time: Export boot_tb and log initial timebase at boot

[Aboorva Devarajan <aboorvad@linux.ibm.com>]

On Tue, 2025-02-04 at 11:28 +0100, Christophe Leroy wrote:

Hi Christophe,

> 
> Le 04/02/2025 à 07:39, Kajol Jain a écrit :
> > From: Aboorva Devarajan <aboorvad@linux.ibm.com>
> > 
> > - Export `boot_tb` for external use, this is useful in perf vpa-dtl
> >    interface, where `boot_tb` can be used to convert raw timebase
> >    values to it's relative boot timestamp.
> 
> A modification of boot_tb can be catastrophic. I would not export it as 
> is as it doesn't need to be modified, would be better to provide it 
> through an accessor, for instance a function called get_boot_tb()
> 
> By the way, it is only modified by time_init() which is an __init 
> function, so boot_tb should be marked __ro_after_init instead of 
> __read_mostly, then in that case it would be more acceptable to export it.


Thanks for the review and suggestions. Yes `boot_tb` is just initialized once
and is never updated again, I’ll update the patch to mark it __ro_after_init.
I’ll also use an accessor function (get_boot_tb()) instead of exporting it,
as it will be clean and maintainable.

Will send a v3 with the updates.

Regards,
Aboorva

> 
> > - Log the initial timebase at `time_init` as it is a useful
> >    information which can be referred to as needed.
> > 
> > Signed-off-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
> > Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> > ---
> >   arch/powerpc/include/asm/time.h | 1 +
> >   arch/powerpc/kernel/time.c      | 4 +++-
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> > index 9bdd8080299b..b6fc5df01d53 100644
> > --- a/arch/powerpc/include/asm/time.h
> > +++ b/arch/powerpc/include/asm/time.h
> > @@ -23,6 +23,7 @@ extern u64 decrementer_max;
> >   extern unsigned long tb_ticks_per_jiffy;
> >   extern unsigned long tb_ticks_per_usec;
> >   extern unsigned long tb_ticks_per_sec;
> > +extern u64 boot_tb;
> >   extern struct clock_event_device decrementer_clockevent;
> >   extern u64 decrementer_max;
> >   
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index 0727332ad86f..6e8548f0e48f 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -137,7 +137,8 @@ EXPORT_SYMBOL_GPL(rtc_lock);
> >   
> >   static u64 tb_to_ns_scale __read_mostly;
> >   static unsigned tb_to_ns_shift __read_mostly;
> > -static u64 boot_tb __read_mostly;
> > +u64 boot_tb __read_mostly;
> > +EXPORT_SYMBOL_GPL(boot_tb);
> >   
> >   extern struct timezone sys_tz;
> >   static long timezone_offset;
> > @@ -943,6 +944,7 @@ void __init time_init(void)
> >   	tb_to_ns_shift = shift;
> >   	/* Save the current timebase to pretty up CONFIG_PRINTK_TIME */
> >   	boot_tb = get_tb();
> > +	pr_debug("%s: timebase at boot: %llu\n", __func__, (unsigned long long)boot_tb);
> >   
> >   	/* If platform provided a timezone (pmac), we correct the time */
> >   	if (timezone_offset) {