| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADyBb7tMaOJ9D-AYnaNP1OX8kkst7DCkVT0giB2HqdrjSpgX3A@mail.gmail.com>
Date: Wed, 18 Jan 2017 12:27:37 +0800
From: Fu Wei <fu.wei@...aro.org>
To: Mark Rutland <mark.rutland@....com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Len Brown <lenb@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <marc.zyngier@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Sudeep Holla <sudeep.holla@....com>,
Hanjun Guo <hanjun.guo@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
rruigrok@...eaurora.org, "Abdulhamid, Harb" <harba@...eaurora.org>,
Christopher Covington <cov@...eaurora.org>,
Timur Tabi <timur@...eaurora.org>,
G Gregory <graeme.gregory@...aro.org>,
Al Stone <al.stone@...aro.org>, Jon Masters <jcm@...hat.com>,
Wei Huang <wei@...hat.com>, Arnd Bergmann <arnd@...db.de>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
Leo Duran <leo.duran@....com>,
Wim Van Sebroeck <wim@...ana.be>,
Guenter Roeck <linux@...ck-us.net>,
linux-watchdog@...r.kernel.org, Tomasz Nowicki <tn@...ihalf.com>,
Christoffer Dall <christoffer.dall@...aro.org>,
Julien Grall <julien.grall@....com>
Subject: Re: [PATCH v19 06/15] clocksource/drivers/arm_arch_timer: Rework
counter frequency detection.
Hi Mark,
On 17 January 2017 at 01:50, Mark Rutland <mark.rutland@....com> wrote:
> On Wed, Dec 21, 2016 at 02:45:54PM +0800, fu.wei@...aro.org wrote:
>> From: Fu Wei <fu.wei@...aro.org>
>>
>> Currently, the counter frequency detection call(arch_timer_detect_rate)
>> combines all the ways to get counter frequency: device-tree property,
>> system coprocessor register, MMIO timer. But in the most of use cases,
>> we don't need all the ways to try:
>> For example, reading device-tree property will be needed only when
>> system boot with device-tree, getting frequency from MMIO timer register
>> will beneeded only when we init MMIO timer.
>>
>> This patch separates paths to determine frequency:
>> Separate out device-tree code, keep them in device-tree init function.
>
> Splitting these out makes sense to me.
OK , will do
>
>> Separate out the MMIO frequency and the sysreg frequency detection call,
>> and use the appropriate one for the counter.
>
>> Signed-off-by: Fu Wei <fu.wei@...aro.org>
>> Tested-by: Xiongfeng Wang <wangxiongfeng2@...wei.com>
>> ---
>> drivers/clocksource/arm_arch_timer.c | 49 +++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index c7b4482..9a1f138 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -488,27 +488,31 @@ static int arch_timer_starting_cpu(unsigned int cpu)
>> return 0;
>> }
>>
>> -static void
>> -arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>> +static void arch_timer_detect_rate(void)
>> {
>> - /* Who has more than one independent system counter? */
>> - if (arch_timer_rate)
>> - return;
>> + /*
>> + * Try to get the timer frequency from
>> + * cntfrq_el0(system coprocessor register).
>> + */
>> + if (!arch_timer_rate)
>> + arch_timer_rate = arch_timer_get_cntfrq();
>> +
>> + /* Check the timer frequency. */
>> + if (!arch_timer_rate)
>> + pr_warn("frequency not available\n");
>> +}
>>
>> +static void arch_timer_mem_detect_rate(void __iomem *cntbase)
>> +{
>> /*
>> - * Try to determine the frequency from the device tree or CNTFRQ,
>> - * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
>> + * Try to determine the frequency from
>> + * CNTFRQ in memory-mapped timer.
>> */
>> - if (!acpi_disabled ||
>> - of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
>> - if (cntbase)
>> - arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>> - else
>> - arch_timer_rate = arch_timer_get_cntfrq();
>> - }
>> + if (!arch_timer_rate)
>> + arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>>
>> /* Check the timer frequency. */
>> - if (arch_timer_rate == 0)
>> + if (!arch_timer_rate)
I think you mean this one, this is for keeping consistency with
arch_timer_detect_rate.
>> pr_warn("frequency not available\n");
>> }
>
> There's a subtle change in behaviour here. Previously for ACPI we'd only
> ever use the sysreg CNTFRQ value for arch_timer_rate, whereas now we
> might use the MMIO timer rate. Maybe that's not a big deal, but I will
> need to think.
>
> Generally, the logic to determine the rate is fairly gnarly regardless.
>
> It would be nice if we could split the MMIO and sysreg rates entirely,
Yes, I am doing this way,
For sysreg rates,
static void arch_timer_detect_rate(void)
{
/*
* Try to get the timer frequency from
* cntfrq_el0(system coprocessor register).
*/
if (!arch_timer_rate)
arch_timer_rate = arch_timer_get_cntfrq();
/* Check the timer frequency. */
if (!arch_timer_rate)
pr_warn("frequency not available\n");
}
For MMIO timer,
static void arch_timer_mem_detect_rate(void __iomem *cntbase)
{
/*
* Try to determine the frequency from
* CNTFRQ in memory-mapped timer.
*/
if (!arch_timer_rate)
arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
/* Check the timer frequency. */
if (!arch_timer_rate)
pr_warn("frequency not available\n");
}
in arch_time_*_init, only call arch_timer_detect_rate,
in arch_timer_mem_init, only call arch_timer_mem_detect_rate.
But you are right, this is fairly gnarly regardless.
> and kill the implicit relationship between the two, or at least make one
> canonical and warn if the two differ.
So I think maybe we can do this:
static void __arch_timer_determine_rate(u32 rate)
{
/* Check the timer frequency. */
if (!arch_timer_rate)
if (rate)
arch_timer_rate = rate;
else
pr_warn("frequency not available\n");
else if (arch_timer_rate != rate)
pr_warn("got different frequency, keep original.\n");
}
static void arch_timer_detect_rate(void)
{
/*
* Try to get the timer frequency from
* cntfrq_el0(system coprocessor register).
*/
__arch_timer_determine_rate(arch_timer_get_cntfrq());
}
static void arch_timer_mem_detect_rate(void __iomem *cntbase)
{
/*
* Try to get the timer frequency from
* CNTFRQ in the MMIO timer.
*/
__arch_timer_determine_rate(readl_relaxed(cntbase + CNTFRQ));
}
any thought?
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Powered by blists - more mailing lists