Messages in this thread | | | Subject | Re: [PATCH v3 7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate | From | Lukasz Luba <> | Date | Wed, 12 Feb 2020 11:43:12 +0000 |
| |
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 <valentin.schneider@arm.com> >>>>> >>>>> 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@arm.com> >>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>> Cc: Marc Zyngier <maz@kernel.org> >>>>> --- >>>>> 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
| |