[v3,7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate

Commit Message

Ionela Voinescu Feb. 11, 2020, 6:45 p.m. UTC

From: Valentin Schneider <[e-mail]>

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 <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Marc Zyngier <[e-mail]>
---
 drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Valentin Schneider Feb. 12, 2020, 9:30 a.m. UTC | #1

On 11/02/2020 18:45, Ionela Voinescu wrote:
> From: Valentin Schneider <[e-mail]>
> 
> 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.
> 

I'm being pedantic here (as usual), but I'd contrast this a bit more. The
activity monitor code checks by itself that the ratio doesn't end up being
0, which is why we don't slam the brakes if the arch timer freq is < 1MHz.

It's just a CNTFRQ sanity check that goes a bit beyond the 0 value check,
IMO.

> 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 <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>

ISTR something somewhere that says the first signoff should be that of the
author of the patch, and seeing as I just provided an untested diff that
ought to be you :)

Lukasz Luba Feb. 12, 2020, 10:01 a.m. UTC | #2

Hi Ionela, Valentin

On 2/11/20 6:45 PM, Ionela Voinescu wrote:
> From: Valentin Schneider <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>
> ---
>   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...

> +
> +	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.

Regards,
Lukasz

Marc Zyngier Feb. 12, 2020, 10:12 a.m. UTC | #3

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 <[e-mail]>
>> 
>> 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 <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: Marc Zyngier <[e-mail]>
>> ---
>>   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?

> 
>> +
>> +	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.

On its own, it is perfectly sensible and could be merged as part of this
series with my

Acked-by: Marc Zyngier <[e-mail]>

         M.

Ionela Voinescu Feb. 12, 2020, 10:32 a.m. UTC | #4

Hi Valentin,

On Wednesday 12 Feb 2020 at 09:30:34 (+0000), Valentin Schneider wrote:
> On 11/02/2020 18:45, Ionela Voinescu wrote:
> > From: Valentin Schneider <[e-mail]>
> > 
> > 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.
> > 
> 
> I'm being pedantic here (as usual), but I'd contrast this a bit more. The
> activity monitor code checks by itself that the ratio doesn't end up being
> 0, which is why we don't slam the brakes if the arch timer freq is < 1MHz.
> 
> It's just a CNTFRQ sanity check that goes a bit beyond the 0 value check,
> IMO.
> 

I agree, but this part was just given as an example of functionality
that relies on a reasonable arch timer rate. The AMU code checks for the
ratio not to be 0 so it does not end up breaking frequency invariance.
But if the ratio does end up being 0 due to the value of arch_time_rate,
we bypass the use of activity monitors which I'd argue it's incorrect
functionality by disabling a potential better source of information for
frequency invariance.

But I can rewrite this part for more clarity.

> > 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 <[e-mail]>
> > Cc: Mark Rutland <[e-mail]>
> > Cc: Marc Zyngier <[e-mail]>
> 
> ISTR something somewhere that says the first signoff should be that of the
> author of the patch, and seeing as I just provided an untested diff that
> ought to be you :)

Will do!

Thanks,
Ionela.

Ionela Voinescu Feb. 12, 2020, 10:54 a.m. UTC | #5

Hi guys,

On Wednesday 12 Feb 2020 at 10:12:32 (+0000), 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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Cc: Mark Rutland <[e-mail]>
> > > Cc: Marc Zyngier <[e-mail]>
> > > ---
> > >   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?
> 

Probably it could have been better emphasised in the commit message but,
yes, [1] specifies a typical range of 1-50Mhz. Therefore, taken
independently the role of this patch is to warn about an out of spec arch
timer rate. 
The link to AMU is here just as an example of a scenario where an out of
spec rate can affect functionality, but there is no dependency between
the threshold chosen here and AMU. AMU functionality only assumes the rate
recommended in the spec.

[1] https://static.docs.arm.com/ddi0487/ea/DDI0487E_a_armv8_arm.pdf

> > 
> > > +
> > > +	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.
> 
> On its own, it is perfectly sensible and could be merged as part of this
> series with my
> 
> Acked-by: Marc Zyngier <[e-mail]>
> 
>         M.

Thanks, Marc! I'll keep this patch here then with some changes in the commit
message for more clarity and the change in author.

Thank you all,
Ionela.

> -- 
> Jazz is not dead. It just smells funny...

Lukasz Luba Feb. 12, 2020, 10:55 a.m. UTC | #6

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Cc: Mark Rutland <[e-mail]>
>>> Cc: Marc Zyngier <[e-mail]>
>>> ---
>>>   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 <[e-mail]>
> 
>          M.

Regards,
Lukasz

Marc Zyngier Feb. 12, 2020, 11:10 a.m. UTC | #7

On 2020-02-12 10:55, Lukasz Luba wrote:
> 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 <[e-mail]>
>>>> 
>>>> 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 <[e-mail]>
>>>> Cc: Mark Rutland <[e-mail]>
>>>> Cc: Marc Zyngier <[e-mail]>
>>>> ---
>>>>   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.'

Hint: constant apparent frequency.

> 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.

You're completely missing the point, I'm afraid. Nobody has to know that
this is happening. For all intent and purposes, the counter has always
the same frequency, even if the HW does fewer ticks of larger 
increments.


[...]

>>> 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.

It seems to solve an issue with an assumption made in the AMU driver,
and would help debugging the problem on broken systems. Are you saying
that this is not the case and that the AMU code can perfectly cope with
the frequency being less than 1MHz?

         M.

Valentin Schneider Feb. 12, 2020, 11:12 a.m. UTC | #8

On 12/02/2020 10:55, Lukasz Luba wrote:
>> 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.
> 

For that one case the value reported by CNTFRQ shouldn't change - it's still
a timer that looks like is operating at 10MHz, but under the hood is doing
bigger increments at lower freq.

As I was trying to get to, this patch isn't validating the actual frequency
the timer operates on, rather that whatever is reported by CNTFRQ is
somewhat sane (which here means [1, 50]MHz, although we just check the
lower bound).

[...]

>> 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.
> 

Right, but it should still be worth having - at least it shows up in
dmesg, and when someone reports something fishy we get a hint that we can
blame the hardware.

>>
>> On its own, it is perfectly sensible and could be merged as part of this
>> series with my
>>
>> Acked-by: Marc Zyngier <[e-mail]>
>>
>>          M.
> 
> Regards,
> Lukasz

Lukasz Luba Feb. 12, 2020, 11:43 a.m. UTC | #9

On 2/12/20 11:10 AM, Marc Zyngier wrote:
> On 2020-02-12 10:55, Lukasz Luba wrote:
>> 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 <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Cc: Mark Rutland <[e-mail]>
>>>>> Cc: Marc Zyngier <[e-mail]>
>>>>> ---
>>>>>   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.'
> 
> Hint: constant apparent frequency.
> 
>> 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.
> 
> You're completely missing the point, I'm afraid. Nobody has to know that
> this is happening. For all intent and purposes, the counter has always
> the same frequency, even if the HW does fewer ticks of larger increments.

Fair enough. As I said I don't know details of that code.

> 
> 
> [...]
> 
>>>> 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.
> 
> It seems to solve an issue with an assumption made in the AMU driver,
> and would help debugging the problem on broken systems. Are you saying
> that this is not the case and that the AMU code can perfectly cope with
> the frequency being less than 1MHz?

What I was saying is that patch 6/7 has the code which checks the rate
and reacts, so it does not need this patch. In case of helping with
debugging, the patch 6/7 also prints error
"System timer frequency too low" and bails out.
The commit message could have better emphasize it.

Regards,
Lukasz

Thomas Gleixner Feb. 14, 2020, 12:35 a.m. UTC | #10

Ionela Voinescu <[e-mail]> writes:

> From: Valentin Schneider <[e-mail]>
>
> 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 <[e-mail]>

So this patch is from Valentin. Where is his Signed-off-by?

>  
> +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);

This does not make sense to me. If the rate is out of bounds then why
warn an just continue instead of making it fail?

Thanks,

        tglx

Ionela Voinescu Feb. 14, 2020, 3:45 p.m. UTC | #11

Hi Thomas,

On Friday 14 Feb 2020 at 01:35:58 (+0100), Thomas Gleixner wrote:
> Ionela Voinescu <[e-mail]> writes:
> 
> > From: Valentin Schneider <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> So this patch is from Valentin. Where is his Signed-off-by?
> 

Yes, sorry about this. This was based on a diff that Valentin provided
in v2. I'll change the author as agreed at:
https://lore.kernel.org/lkml/[e-mail]/

> >  
> > +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);
> 
> This does not make sense to me. If the rate is out of bounds then why
> warn an just continue instead of making it fail?
> 

Because it's not a hard restriction, it's just atypical for the rate to
be below 1Mhz. The spec only mentions a typical range of 1 to 50MHz and
the warning is only here to flag a potentially problematic rate, below
what is assumed typical in the spec.

In [1], where I'm actually relying on arch_timer_rate being higher than
than 1/SCHED_CAPACITY_SCALE² of the maximum frequency, I am making it
fail, as, for that scenario, it is a hard restriction.


+	 * We use a factor of 2 * SCHED_CAPACITY_SHIFT -> SCHED_CAPACITY_SCALE²
+	 * in order to ensure a good resolution for arch_max_freq_scale for
+	 * very low arch timer frequencies (up to the KHz range which should be
+	 * unlikely).
+	 */
+	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
+	ratio = div64_u64(ratio, max_freq_hz);
+	if (!ratio) {
+		pr_err("System timer frequency too low.\n");
+		return -EINVAL;
+	}
+

[1] https://lore.kernel.org/lkml/[e-mail]/

Thanks,
Ionela.

> Thanks,
> 
>         tglx

Ionela Voinescu Feb. 14, 2020, 3:57 p.m. UTC | #12

On Friday 14 Feb 2020 at 15:45:25 (+0000), Ionela Voinescu wrote:
> Hi Thomas,
> 
> On Friday 14 Feb 2020 at 01:35:58 (+0100), Thomas Gleixner wrote:
> > Ionela Voinescu <[e-mail]> writes:
> > 
> > > From: Valentin Schneider <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > 
> > So this patch is from Valentin. Where is his Signed-off-by?
> > 
> 
> Yes, sorry about this. This was based on a diff that Valentin provided
> in v2. I'll change the author as agreed at:
> https://lore.kernel.org/lkml/[e-mail]/
> 
> > >  
> > > +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);
> > 
> > This does not make sense to me. If the rate is out of bounds then why
> > warn an just continue instead of making it fail?
> > 
> 
> Because it's not a hard restriction, it's just atypical for the rate to
> be below 1Mhz. The spec only mentions a typical range of 1 to 50MHz and
> the warning is only here to flag a potentially problematic rate, below
> what is assumed typical in the spec.
> 
> In [1], where I'm actually relying on arch_timer_rate being higher than
> than 1/SCHED_CAPACITY_SCALE² of the maximum frequency, I am making it
> fail, as, for that scenario, it is a hard restriction.
> 
> 
> +	 * We use a factor of 2 * SCHED_CAPACITY_SHIFT -> SCHED_CAPACITY_SCALE²
> +	 * in order to ensure a good resolution for arch_max_freq_scale for
> +	 * very low arch timer frequencies (up to the KHz range which should be
> +	 * unlikely).
> +	 */
> +	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
> +	ratio = div64_u64(ratio, max_freq_hz);
> +	if (!ratio) {
> +		pr_err("System timer frequency too low.\n");
> +		return -EINVAL;
> +	}
> +
> 
> [1] https://lore.kernel.org/lkml/[e-mail]/

I've mistakenly referenced a bad link ^

It was supposed to be:

[1] https://lore.kernel.org/lkml/[e-mail]/

Thanks,
Ionela.

Patch

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