linux-kernel - Re: [PATCH v8 4/9] clocksource/drivers/arm_arch

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] [day] [month] [year] [list]

Message-ID: <20160725224736.GT1041@n2100.armlinux.org.uk>
Date:	Mon, 25 Jul 2016 23:47:36 +0100
From:	Russell King - ARM Linux <linux@...linux.org.uk>
To:	Fu Wei <fu.wei@...aro.org>
Cc:	Will Deacon <will.deacon@....com>,
	Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	rruigrok@...eaurora.org, Wim Van Sebroeck <wim@...ana.be>,
	wei@...hat.com, Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Al Stone <al.stone@...aro.org>,
	Timur Tabi <timur@...eaurora.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Len Brown <lenb@...nel.org>, harba@...eaurora.org,
	linux-watchdog@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Marc Zyngier <marc.zyngier@....com>,
	Jon Masters <jcm@...hat.com>,
	Christopher Covington <cov@...eaurora.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-arm-kernel@...ts.infradead.org,
	G Gregory <graeme.gregory@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Leo Duran <leo.duran@....com>,
	Hanjun Guo <hanjun.guo@...aro.org>,
	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
	Sudeep Holla <sudeep.holla@....com>
Subject: Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to
 get 64-bit CNTVCT

On Mon, Jul 25, 2016 at 11:50:19PM +0800, Fu Wei wrote:
> Hi Will,
> 
> On 25 July 2016 at 17:02, Will Deacon <will.deacon@....com> wrote:
> > On Wed, Jul 20, 2016 at 02:17:59AM +0800, fu.wei@...aro.org wrote:
> >> From: Fu Wei <fu.wei@...aro.org>
> >>
> >> This patch simplify arch_counter_get_cntvct_mem function by
> >> using readq to get 64-bit CNTVCT value instead of readl_relaxed.
> >>
> >> Signed-off-by: Fu Wei <fu.wei@...aro.org>
> >> ---
> >>  drivers/clocksource/arm_arch_timer.c | 10 +---------
> >>  1 file changed, 1 insertion(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >> index e6fd42d..483d2f9 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -418,15 +418,7 @@ u32 arch_timer_get_rate(void)
> >>
> >>  static u64 arch_counter_get_cntvct_mem(void)
> >>  {
> >> -     u32 vct_lo, vct_hi, tmp_hi;
> >> -
> >> -     do {
> >> -             vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> >> -             vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
> >> -             tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> >> -     } while (vct_hi != tmp_hi);
> >> -
> >> -     return ((u64) vct_hi << 32) | vct_lo;
> >> +     return readq(arch_counter_base + CNTVCT_LO);
> >
> 
> Sorry, right after posting v9, I got your comment,
> 
> > What's the benefit of doing this? If you use readq here, how can we
> 
> benefit:
> 1. simplify the code
> 2. from arch/arm64/include/asm/io.h, I guess readq is more efficient

And the harm is that it breaks the build on ARM, because ARM doesn't
provide readq (as it can't, as not all CPUs on ARM support 64-bit
reads) and it's not appropriate for architecture code to emulate it.
Consider carefully why the original code has that loop present -
only a device driver hows how to read a 64-bit register safely using
two 32-bit reads.  Such a loop may not be appropriate for some other
device.

So... as the 0-day builder detected a failure on ARM with this, NAK.

If you want to make this conditional on readq() being present, that'd
be acceptable, but you must support the case where readq() is not
provided.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.