linux-kernel - Re: [PATCH 7/7] clocksource/arm_arch_timer: Use arch_timer_read

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: <bbe9b8c1-132f-bbfa-e3d0-ad10c4165ad7@arm.com>
Date:   Tue, 30 Apr 2019 16:27:43 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     Russell King <linux@....linux.org.uk>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@....com>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Valentin Schneider <valentin.schneider@....com>
Subject: Re: [PATCH 7/7] clocksource/arm_arch_timer: Use
 arch_timer_read_counter to access stable counters

On 15/04/2019 13:16, Daniel Lezcano wrote:
> On 08/04/2019 17:49, Marc Zyngier wrote:
>> Instead of always going via arch_counter_get_cntvct_stable to
>> access the counter workaround, let's have arch_timer_read_counter
>> to point to the right method.
>>
>> For that, we need to track whether any CPU in the system has a
>> workaround for the counter. This is done by having an atomic
>> variable tracking this.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
>> ---
> 
> [ ... ]
> 
>> +
>>  /*
>>   * Default to cp15 based access because arm64 uses this function for
>>   * sched_clock() before DT is probed and the cp15 method is guaranteed
>> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
>>  DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
>>  EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>>  
>> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
> 
> Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?

I don't think *_ONCE says anything about the atomicity of the access. It
only instruct the compiler that this should only be accessed once, and
not reloaded/rewritten. In this case, WRITE_ONCE() would work just as
well, but I feel that setting the expectations do matter.

I also had a vague idea to use this as a refcount to drop the
workarounds as CPUs get hotplugged off, in which case we'd need the RMW
operations to be atomic.

Anyway, I'm not hell bent on this. If you fundamentally disagree with me
I'll change it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...