| Message ID | 20230208094813.20874-2-walter.chang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support timer drivers as loadable modules | expand |
On 08/02/2023 10:48, walter.chang@mediatek.com wrote: > From: Chun-Hung Wu <chun-hung.wu@mediatek.com> > > 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@mediatek.com> > --- > 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. Best regards, Krzysztof
On 08/02/2023 15:24, Krzysztof Kozlowski wrote: > On 08/02/2023 10:48, walter.chang@mediatek.com wrote: >> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> >> >> 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@mediatek.com> >> --- >> 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. Regards, Matthias
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@mediatek.com wrote: >>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> >>> >>> 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@mediatek.com> >>> --- >>> 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). Best regards, Krzysztof
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@mediatek.com wrote: >>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> >>>> >>>> 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@mediatek.com> >>>> --- >>>> 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...
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@mediatek.com wrote: >>>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> >>>>> >>>>> 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@mediatek.com> >>>>> --- >>>>> 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
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@mediatek.com wrote: >>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> >>>> >>>> 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@mediatek.com> >>>> --- [ ... ] > ... 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). +1 It is not the first time there is a proposal to convert the timers to modules. After asking, nobody came with a real study regarding the impact of the modularization of these drivers vs the time core framework and the benefit. My gut feeling is that is not that simple.
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@mediatek.com wrote: > >>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com> > >>> > >>> 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@mediatek.com> > >>> --- > >>> 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
On Wed, 2023-02-08 at 20:41 +0100, Matthias Brugger wrote: > > On 08/02/2023 15:24, Krzysztof Kozlowski wrote: > > On 08/02/2023 10:48, walter.chang@mediatek.com wrote: > > > From: Chun-Hung Wu <chun-hung.wu@mediatek.com> > > > > > > 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@mediatek.com> > > > --- > > > 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. > > Regards, > Matthias I have uploaded another patch to make timer-mediatek.c driver become loadable module. https://lore.kernel.org/lkml/20230210100058.19861-1-walter.chang@mediatek.com/T/#u This driver registers an always-on timer as tick_broadcast_device on MediaTek SoCs. If the system does not load this module, system would also boot normally since Arm Generic Timer will take over this part. Thanks, Walter Chang
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) { 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); void __init generic_sched_clock_init(void) {