linux-kernel - Re: [PATCH v21 04/13] clocksource: arm_arch_timer: split arch_timer

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]

Message-ID: <20170317190539.GD15909@leverpostej>
Date:   Fri, 17 Mar 2017 19:05:42 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     fu.wei@...aro.org
Cc:     rjw@...ysocki.net, lenb@...nel.org, daniel.lezcano@...aro.org,
        tglx@...utronix.de, marc.zyngier@....com,
        lorenzo.pieralisi@....com, sudeep.holla@....com,
        hanjun.guo@...aro.org, linux-arm-kernel@...ts.infradead.org,
        linaro-acpi@...ts.linaro.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, rruigrok@...eaurora.org,
        harba@...eaurora.org, cov@...eaurora.org, timur@...eaurora.org,
        graeme.gregory@...aro.org, al.stone@...aro.org, jcm@...hat.com,
        wei@...hat.com, arnd@...db.de, catalin.marinas@....com,
        will.deacon@....com, Suravee.Suthikulpanit@....com,
        leo.duran@....com, wim@...ana.be, linux@...ck-us.net,
        linux-watchdog@...r.kernel.org, tn@...ihalf.com,
        christoffer.dall@...aro.org, julien.grall@....com
Subject: Re: [PATCH v21 04/13] clocksource: arm_arch_timer: split
 arch_timer_rate for different types of timer

On Tue, Feb 07, 2017 at 02:50:06AM +0800, fu.wei@...aro.org wrote:
> From: Fu Wei <fu.wei@...aro.org>
> 
> Currently, arch_timer_rate is used to store the frequency got from per-cpu
> arch-timer or the memory-mapped (MMIO) timers. But those values come from
> different registers which should all be initialized by firmware.
> 
> This patch remove arch_timer_rate, and use arch_timer_sysreg_freq and
> arch_timer_mmio_freq instead.
> 
> Signed-off-by: Fu Wei <fu.wei@...aro.org>

Thanks for attacking this. Generally, I do think this is the right thing
to do.

However...

> @@ -1070,10 +1077,9 @@ static int __init arch_timer_mem_init(struct device_node *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_get_mmio_freq(base);
> -	if (!arch_timer_rate) {
> +	if (of_property_read_u32(np, "clock-frequency", &arch_timer_mmio_freq))
> +		arch_timer_mmio_freq = arch_timer_get_mmio_freq(base);
> +	if (!arch_timer_mmio_freq) {
>  		pr_err(FW_BUG "frequency not available for MMIO timer.\n");
>  		ret = -EINVAL;
>  		goto out;

... unfortunately, I believe that this will break some DT platforms that
have been (unintentionally) relying on the way currently allow the
frequency to be probed from either the MMIO timer or the sysreg timer.

So while the above was my suggestion, it was not my best.

For the timebeing, let's leave the single arch_timer_rate, but ensure
that it doesn't get in the way fo the ACPI code, by making the ACPI
probe path:

* Probe the sysreg timers first, using the sysreg cntfrq().

* Probe the MMIO timers second, verifying that each MMIO cntfrq matches
  the already-probed sysreg cntfrq.

... which is what I believe you suggested previously. 

Thanks,
Mark.