Re: [PATCH 1/3] time/sched_clock: Export sched_clock_register()

[Sudeep Holla]

On Wed, Feb 08, 2023 at 08:45:15PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 20:41, Matthias Brugger wrote:
> > On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
> >> On 08/02/2023 10:48, walter.chang@xxxxxxxxxxxx wrote:
> >>> From: Chun-Hung Wu <chun-hung.wu@xxxxxxxxxxxx>
> >>>
> >>> clocksource driver may use sched_clock_register()
> >>> to resigter itself as a sched_clock source.
> >>> Export it to support building such driver
> >>> as module, like timer-mediatek.c
> >>>
> >>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@xxxxxxxxxxxx>
> >>> ---
> >>>   kernel/time/sched_clock.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> >>> index 8464c5acc913..8e49e87d1221 100644
> >>> --- a/kernel/time/sched_clock.c
> >>> +++ b/kernel/time/sched_clock.c
> >>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
> >>>   	return HRTIMER_RESTART;
> >>>   }
> >>>   
> >>> -void __init
> >>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>
> >> Is there a non-init caller?
> >>
> >>>   {
> >>>   	u64 res, wrap, new_mask, new_epoch, cyc, ns;
> >>>   	u32 new_mult, new_shift;
> >>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>>   
> >>>   	pr_debug("Registered %pS as sched_clock source\n", read);
> >>>   }
> >>> +EXPORT_SYMBOL_GPL(sched_clock_register);
> >>
> >> Where is the module using it?
> >>
> >> You need to bring users of these two changes, not just prepare something
> >> for your out of tree patches.
> >>
> > 
> > I'd propose to add at least one driver that will need these changes, to make it 
> > clear why you need that.
> 
> ... and actually test if the system works fine when booted from such
> clocksource as a module. I have doubts that and unfortunately folks
> working on GKI like to put whatever stuff from mainline into modules
> even if it does not make sense for us (see long time ago discussion
> about pinctrl drivers).

Just to add, system boot test with these as modules are not sufficient
based on the usage of such timers. I am aware of systems where arm
architected timer are functional in normal running state of the CPU.
They may have bugs/errata where these arch timers(mmio access) are not
available when CPUs enter low power idle states. Generally, CPUs enter
idle states on boot, but one CPU may not as it will act as broadcast timer.
With this added timer, they may achieve all CPUs entering idle states
properly.

In short the system might be bootable w/o such timers but not power
efficient. We need to tests that involve loading and unloading of such
modules to see if the transition between this timer as broadcast and one
CPU itself as broadcast happens correctly and system survives across such
loading and unloading of the modules.

Note I am not for/against allowing the such timer as module, but just
adding possible testcase to pass to consider if we are allowing it.

--
Regards,
Sudeep