Re: [PATCH v3 7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate

[Lukasz Luba]

On 2/12/20 10:12 AM, Marc Zyngier wrote:
> On 2020-02-12 10:01, Lukasz Luba wrote:
> > Hi Ionela, Valentin
> >
> > On 2/11/20 6:45 PM, Ionela Voinescu wrote:
> > > From: Valentin Schneider <valentin.schneider@xxxxxxx>
> > >
> > > Using an arch timer with a frequency of less than 1MHz can result in an
> > > incorrect functionality of the system which assumes a reasonable rate.
> > >
> > > One example is the use of activity monitors for frequency invariance
> > > which uses the rate of the arch timer as the known rate of the constant
> > > cycle counter in computing its ratio compared to the maximum frequency
> > > of a CPU. For arch timer frequencies less than 1MHz this ratio could
> > > end up being 0 which is an invalid value for its use.
> > >
> > > Therefore, warn if the arch timer rate is below 1MHz which contravenes
> > > the recommended architecture interval of 1 to 50MHz.
> > >
> > > Signed-off-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > > Â drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++---
> > > Â 1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/arm_arch_timer.c 
> > > b/drivers/clocksource/arm_arch_timer.c
> > > index 9a5464c625b4..4faa930eabf8 100644
> > > --- a/drivers/clocksource/arm_arch_timer.c
> > > +++ b/drivers/clocksource/arm_arch_timer.c
> > > @@ -885,6 +885,17 @@ static int arch_timer_starting_cpu(unsigned int 
> > > cpu)
> > > ÂÂÂÂÂ return 0;
> > > Â }
> > > Â +static int validate_timer_rate(void)
> > > +{
> > > +ÂÂÂ if (!arch_timer_rate)
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +
> > > +ÂÂÂ /* Arch timer frequency < 1MHz can cause trouble */
> > > +ÂÂÂ WARN_ON(arch_timer_rate < 1000000);
> >
> > I don't see a big value of having a patch just to add one extra warning,
> > in a situation which we handle in our code with in 6/7 with:
> >
> > +ÂÂÂ if (!ratio) {
> > +ÂÂÂÂÂÂÂ pr_err("System timer frequency too low.\n");
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +ÂÂÂ }
> >
> > Furthermore, the value '100000' here is because of our code and
> > calculation in there, so it does not belong to arch timer. Someone
> > might ask why it's not 200000 or a define in our header...
> > Or questions asking why do you warn when that arch timer and cpu is not
> > AMU capable...
>
> Because, as the commit message outlines it, such a frequency is terribly
> out of spec?

I don't see in the RM that < 1MHz is terribly out of spec.
'Frequency
Increments at a fixed frequency, typically in the range 1-50MHz.
Can support one or more alternative operating modes in which it 
increments by larger amounts at a
lower frequency, typically for power-saving.'

There is even an example how to operate at 20kHz and increment by 500.

I don't know the code if it's supported, thought.

> > > +
> > > +ÂÂÂ return 0;
> > > +}
> > > +
> > > Â /*
> > > ÂÂ * For historical reasons, when probing with DT we use whichever 
> > > (non-zero)
> > > ÂÂ * rate was probed first, and don't verify that others match. If 
> > > the first node
> > > @@ -900,7 +911,7 @@ static void arch_timer_of_configure_rate(u32 
> > > rate, struct device_node *np)
> > > ÂÂÂÂÂÂÂÂÂ arch_timer_rate = rate;
> > > ÂÂÂÂÂÂÂ /* Check the timer frequency. */
> > > -ÂÂÂ if (arch_timer_rate == 0)
> > > +ÂÂÂ if (validate_timer_rate())
> > > ÂÂÂÂÂÂÂÂÂ pr_warn("frequency not available\n");
> > > Â }
> > > Â @@ -1594,9 +1605,10 @@ static int __init 
> > > arch_timer_acpi_init(struct acpi_table_header *table)
> > > ÂÂÂÂÂÂ * CNTFRQ value. This *must* be correct.
> > > ÂÂÂÂÂÂ */
> > > ÂÂÂÂÂ arch_timer_rate = arch_timer_get_cntfrq();
> > > -ÂÂÂ if (!arch_timer_rate) {
> > > +ÂÂÂ ret = validate_timer_rate();
> > > +ÂÂÂ if (ret) {
> > > ÂÂÂÂÂÂÂÂÂ pr_err(FW_BUG "frequency not available.\n");
> > > -ÂÂÂÂÂÂÂ return -EINVAL;
> > > +ÂÂÂÂÂÂÂ return ret;
> > > ÂÂÂÂÂ }
> > > ÂÂÂÂÂÂÂ arch_timer_uses_ppi = arch_timer_select_ppi();
> >
> > Lastly, this is arch timer.
> > To increase chances of getting merge soon, I would recommend to drop
> > the patch from this series.
>
> And? It seems to address a potential issue where the time frequency
> is out of spec, and makes sure we don't end up with additional problems
> in the AMU code.

This patch just prints warning, does not change anything in booting or
in any code related to AMU.

> On its own, it is perfectly sensible and could be merged as part of this
> series with my
>
> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
>
>  ÂÂÂÂÂÂÂ M.

Regards,
Lukasz