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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 29 Mar 2017 11:41:07 +0800
From:   Fu Wei <fu.wei@...aro.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Mark Rutland <mark.rutland@....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 v22 02/11] clocksource: arm_arch_timer: separate out
 device-tree code and remove arch_timer_detect_rate

Hi Daniel,

Great thanks for your review, allow me to answer your question below:

On 28 March 2017 at 22:58, Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
> On Wed, Mar 22, 2017 at 12:31:13AM +0800, fu.wei@...aro.org wrote:
>> From: Fu Wei <fu.wei@...aro.org>
>>
>> Currently, the counter frequency detection call(arch_timer_detect_rate)
>> includes getting the frequency from the device-tree property, the per-cpu
>> arch-timer and the memory-mapped (MMIO) timer interfaces.
>> But reading device-tree property will be needed only when system boot with
>> device-tree, and reading from the per-cpu arch-timer and the memory-mapped
>> (MMIO) timer interfaces will be needed only when the system initializes
>> the relevant timer.
>>
>> This patch separates out device-tree code, keep them in device-tree init
>> function, and removes arch_timer_detect_rate founction, then uses the
>> arch_timer_get_cntfrq and arch_timer_mem_get_cntfrq directly.
>>
>> Signed-off-by: Fu Wei <fu.wei@...aro.org>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 58 +++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 843f923..29ca7d6 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -560,30 +560,6 @@ static u32 arch_timer_mem_get_cntfrq(void __iomem *cntbase)
>>       return readl_relaxed(cntbase + CNTFRQ);
>>  }
>>
>> -static void
>> -arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>> -{
>> -     /* Who has more than one independent system counter? */
>> -     if (arch_timer_rate)
>> -             return;
>> -
>> -     /*
>> -      * Try to determine the frequency from the device tree or CNTFRQ,
>> -      * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
>> -      */
>> -     if (!acpi_disabled ||
>> -         of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
>> -             if (cntbase)
>> -                     arch_timer_rate = arch_timer_mem_get_cntfrq(cntbase);
>> -             else
>> -                     arch_timer_rate = arch_timer_get_cntfrq();
>> -     }
>> -
>> -     /* Check the timer frequency. */
>> -     if (arch_timer_rate == 0)
>> -             pr_warn("frequency not available\n");
>> -}
>> -
>>  static void arch_timer_banner(unsigned type)
>>  {
>>       pr_info("%s%s%s timer(s) running at %lu.%02luMHz (%s%s%s).\n",
>> @@ -958,7 +934,17 @@ static int __init arch_timer_of_init(struct device_node *np)
>>       for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI; i++)
>>               arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
>>
>> -     arch_timer_detect_rate(NULL, np);
>> +     /*
>> +      * Try to determine the frequency from the device tree,
>> +      * if fail, get the frequency from the sysreg CNTFRQ.
>> +      */
>> +     if (!arch_timer_rate &&
>
> This variable is set only if "arm,armv7-timer" and "arm,armv7-timer-mem" are
> declared together in the DT, right ?
>
> Two declarations for a single variable ? Ignore the !arch_timer_rate.

In this function, we try to initialize per-CPU arm arch_timer by DT.
this "!arch_timer_rate" is for testing that if we have got system
counter frequency from the memory-mapped timer. If so, we just skip
getting the frequency from DT or sysreg cntfrq again.
This variable is set only if "arm,armv7-timer-mem" is initialized
earlier than "arm,armv7-timer", in another word, maybe the node of
"arm,armv7-timer-mem" is declared earlier than  "arm,armv7-timer-mem"
one in DT.

we do this check is for keeping the same init logic as before in the
DT, try to avoid any possibility of  breaking devices which boot by
DT.

>
>> +         of_property_read_u32(np, "clock-frequency", &arch_timer_rate))
>> +             arch_timer_rate = arch_timer_get_cntfrq();
>> +     if (!arch_timer_rate) {
>> +             pr_err(FW_BUG "frequency not available.\n");
>> +             return -EINVAL;
>> +     }
>
> Please, clarify this block, the conditions are unclear.

this "!arch_timer_rate" is for verifying that if the system counter
frequency we just got from DT or sysreg cntfrq is valid(non-zero).

So here, you can see I check arch_timer_rate twice, but they are for
different cases.

>
>>
>>       arch_timer_c3stop = !of_property_read_bool(np, "always-on");
>>
>> @@ -1069,7 +1055,19 @@ static int __init arch_timer_mem_init(struct device_node *np)
>>               goto out;
>>       }
>>
>> -     arch_timer_detect_rate(base, np);
>> +     /*
>> +      * Try to determine the frequency from the device tree,
>> +      * if fail, get the frequency from the CNTFRQ reg of MMIO timer.
>> +      */
>> +     if (!arch_timer_rate &&
>> +         of_property_read_u32(np, "clock-frequency", &arch_timer_rate))
>> +             arch_timer_rate = arch_timer_mem_get_cntfrq(base);
>> +     if (!arch_timer_rate) {
>> +             pr_err(FW_BUG "MMIO frequency not available.\n");
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>>       ret = arch_timer_mem_register(base, irq);
>>       if (ret)
>>               goto out;
>> @@ -1130,8 +1128,12 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>>               map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt,
>>               gtdt->non_secure_el2_flags);
>>
>> -     /* Get the frequency from CNTFRQ */
>> -     arch_timer_detect_rate(NULL, NULL);
>> +     /* Get the frequency from the sysreg CNTFRQ */
>> +     arch_timer_rate = arch_timer_get_cntfrq();
>> +     if (!arch_timer_rate) {
>> +             pr_err(FW_BUG "frequency not available.\n");
>> +             return -EINVAL;
>> +     }
>>
>>       arch_timer_uses_ppi = arch_timer_select_ppi();
>>       if (!arch_timer_ppi[arch_timer_uses_ppi]) {
>> --
>> 2.9.3
>>
>
> --
>
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

Powered by blists - more mailing lists