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

[AngeloGioacchino Del Regno]

Il 08/02/23 23:22, Matthias Brugger ha scritto:
> On 08/02/2023 20:45, 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).
>
> Yes thinking about it twice, it makes only sense if  the Arm architecture timer is 
> running. Otherwise the system will hang on boot. I know that older MediaTek devices 
> had problems with that...

I think also some very old Qualcomm SoCs have the same timer "issue" and I bet that
some others as well do.
Now, I won't argue about the benefits or drawbacks of having X, Y or Z as a module
because it's probably not the right time/place to... but!

I was trying to get my brain ticking on this one and I immediately didn't like it:
as a matter of fact, this kind of clocksources (especially the arch timer[s]) is
boot critical and that's not limited to ARM, anyway... this means that a such a
change can't be *that* easy, at all.

If you really want to register such a clocksource driver, I would suggest instead
to make an addition that allows you to do so, while *not* touching common code
paths that are called by multiple architectures and that may need those to stay
as they are for one reason or another.

*If* this kind of modularization will ever happen, it's something that must be
done slowly and again, not in one shot, especially not with one series "taking
care of 'em all". Please be careful when touching *core* code.

That was just an opinion on something that I can envision to be eventually going
wrong in many, many ways...

Regards,
Angelo