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

  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(-)
  

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